diff mbox series

[8/9] iommu: arm-smmu: make it explicitly non-modular

Message ID 1543271498-28966-9-git-send-email-paul.gortmaker@windriver.com
State New, archived
Headers show
Series iommu: clean up/remove modular stuff from non-modules. | expand

Commit Message

Paul Gortmaker Nov. 26, 2018, 10:31 p.m. UTC
The Kconfig currently controlling compilation of this code is:

drivers/iommu/Kconfig:config ARM_SMMU
drivers/iommu/Kconfig:  bool "ARM Ltd. System MMU (SMMU) Support"

...meaning that it currently is not being built as a module by anyone.

Lets remove the modular code that is essentially orphaned, so that
when reading the driver there is no doubt it is builtin-only.

Since module_platform_driver() uses the same init level priority as
builtin_platform_driver() the init ordering remains unchanged with
this commit.

We explicitly disallow a driver unbind, since that doesn't have a
sensible use case anyway, but unlike most drivers, we can't delete the
function tied to the ".remove" field.  This is because as of commit
7aa8619a66ae ("iommu/arm-smmu-v3: Implement shutdown method") the
.remove function was given a one line wrapper and re-used to provide a
.shutdown service.  So we delete the wrapper and re-name the function
from remove to shutdown.

We add a moduleparam.h include since the file does actually declare
some module parameters, and leaving them as such is the easiest way
currently to remain backwards compatible with existing use cases.

We also delete the MODULE_LICENSE tag etc. since all that information
is already contained at the top of the file in the comments.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Nate Watterson <nwatters@codeaurora.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: iommu@lists.linux-foundation.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/iommu/arm-smmu.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

Comments

Robin Murphy Nov. 28, 2018, 12:42 p.m. UTC | #1
Hi Paul,

On 26/11/2018 22:31, Paul Gortmaker wrote:
> The Kconfig currently controlling compilation of this code is:
> 
> drivers/iommu/Kconfig:config ARM_SMMU
> drivers/iommu/Kconfig:  bool "ARM Ltd. System MMU (SMMU) Support"
> 
> ...meaning that it currently is not being built as a module by anyone.
> 
> Lets remove the modular code that is essentially orphaned, so that
> when reading the driver there is no doubt it is builtin-only.
> 
> Since module_platform_driver() uses the same init level priority as
> builtin_platform_driver() the init ordering remains unchanged with
> this commit.
> 
> We explicitly disallow a driver unbind, since that doesn't have a
> sensible use case anyway, but unlike most drivers, we can't delete the
> function tied to the ".remove" field.  This is because as of commit
> 7aa8619a66ae ("iommu/arm-smmu-v3: Implement shutdown method") the
> .remove function was given a one line wrapper and re-used to provide a
> .shutdown service.  So we delete the wrapper and re-name the function
> from remove to shutdown.
> 
> We add a moduleparam.h include since the file does actually declare
> some module parameters, and leaving them as such is the easiest way
> currently to remain backwards compatible with existing use cases.
> 
> We also delete the MODULE_LICENSE tag etc. since all that information
> is already contained at the top of the file in the comments.
> 
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Nate Watterson <nwatters@codeaurora.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: iommu@lists.linux-foundation.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>   drivers/iommu/arm-smmu.c | 32 +++++++++++++-------------------
>   1 file changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 5a28ae892504..4a2e143fdf52 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -41,7 +41,8 @@
>   #include <linux/io-64-nonatomic-hi-lo.h>
>   #include <linux/iommu.h>
>   #include <linux/iopoll.h>
> -#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/moduleparam.h>
>   #include <linux/of.h>
>   #include <linux/of_address.h>
>   #include <linux/of_device.h>
> @@ -101,6 +102,10 @@
>   #define MSI_IOVA_LENGTH			0x100000
>   
>   static int force_stage;
> +/*
> + * not really modular, but the easiest way to keep compat with existing
> + * bootargs behaviour is to continue using module_param() here.
> + */

Is it worth introducing builtin_param() and friends for this sort of 
thing, to echo the *_platform_driver() helpers? It seems like that could 
be justifiable under the motivation described in the cover letter.

Otherwise, the changes look reasonable. I still hold out hope that one 
day we'll be able to make IOMMU drivers modular (it can work with 
minimal hacks today, but it's far from robust in general), but for now I 
agree this makes sense (and it'll be easy enough to revert for playing 
with further hacks). With the title fixed up as Joerg asked,

Acked-by: Robin Murphy <robin.murphy@arm.com>

>   module_param(force_stage, int, S_IRUGO);
>   MODULE_PARM_DESC(force_stage,
>   	"Force SMMU mappings to be installed at a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation.");
> @@ -1964,7 +1969,6 @@ static const struct of_device_id arm_smmu_of_match[] = {
>   	{ .compatible = "cavium,smmu-v2", .data = &cavium_smmuv2 },
>   	{ },
>   };
> -MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>   
>   #ifdef CONFIG_ACPI
>   static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
> @@ -2224,24 +2228,18 @@ static int arm_smmu_legacy_bus_init(void)
>   }
>   device_initcall_sync(arm_smmu_legacy_bus_init);
>   
> -static int arm_smmu_device_remove(struct platform_device *pdev)
> +static void arm_smmu_device_shutdown(struct platform_device *pdev)
>   {
>   	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
>   
>   	if (!smmu)
> -		return -ENODEV;
> +		return;
>   
>   	if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
>   		dev_err(&pdev->dev, "removing device with active domains!\n");
>   
>   	/* Turn the thing off */
>   	writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> -	return 0;
> -}
> -
> -static void arm_smmu_device_shutdown(struct platform_device *pdev)
> -{
> -	arm_smmu_device_remove(pdev);
>   }
>   
>   static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> @@ -2256,16 +2254,12 @@ static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
>   
>   static struct platform_driver arm_smmu_driver = {
>   	.driver	= {
> -		.name		= "arm-smmu",
> -		.of_match_table	= of_match_ptr(arm_smmu_of_match),
> -		.pm		= &arm_smmu_pm_ops,
> +		.name			= "arm-smmu",
> +		.of_match_table		= of_match_ptr(arm_smmu_of_match),
> +		.pm			= &arm_smmu_pm_ops,
> +		.suppress_bind_attrs	= true,
>   	},
>   	.probe	= arm_smmu_device_probe,
> -	.remove	= arm_smmu_device_remove,
>   	.shutdown = arm_smmu_device_shutdown,
>   };
> -module_platform_driver(arm_smmu_driver);
> -
> -MODULE_DESCRIPTION("IOMMU API for ARM architected SMMU implementations");
> -MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
> -MODULE_LICENSE("GPL v2");
> +builtin_platform_driver(arm_smmu_driver);
>
Paul Gortmaker Nov. 28, 2018, 3:24 p.m. UTC | #2
[Re: [PATCH 8/9] iommu: arm-smmu: make it explicitly non-modular] On 28/11/2018 (Wed 12:42) Robin Murphy wrote:

> Hi Paul,
> 
> On 26/11/2018 22:31, Paul Gortmaker wrote:

[...]

> >We add a moduleparam.h include since the file does actually declare
> >some module parameters, and leaving them as such is the easiest way
> >currently to remain backwards compatible with existing use cases.

[...]


> Is it worth introducing builtin_param() and friends for this sort of thing,
> to echo the *_platform_driver() helpers? It seems like that could be
> justifiable under the motivation described in the cover letter.

I've definitely gone back and looked at this a few times when coming
across the few corner cases like these, to remind myself why I didn't do
it already.

We'd not want to replicate all the module_param stuff as an instance of
builtin_param() because we already have setup() and setup_param() in
init.h -- however they don't do the file name in the param - hence the
reason it isn't a direct swap in replacement.

So, it would become some more complex refactoring of moduleparam.h into
say bootparam.h - to reduce code/macro duplication, while at the same
time being aware of existing setup_param stuff and making something like
a new setup_param_named() that is consistent with existing setup fcns.

And based on past experience, there will be reviewers who don't see the
value in the distinction and simply reply with two words "why bother?".

Not impossible, but not as simple as the builtin_platform_driver and
similar wrappers that I've already added to mainline.  You've made we
want to go have another look at it again, but in the meantime we can do
what I've done here, and circle around later to update the few instances
of moduleparam in non-modules once/if the refactoring I describe above
works out and is accepted in mainline.

> 
> Otherwise, the changes look reasonable. I still hold out hope that one day
> we'll be able to make IOMMU drivers modular (it can work with minimal hacks
> today, but it's far from robust in general), but for now I agree this makes
> sense (and it'll be easy enough to revert for playing with further hacks).

I totally agree - I've had similar discussions with the DMA maintainers,
and if being modular can be made to work and has a use case - great!

But it should be a conscious decision, since nobody writes a new driver
from scratch; they copy one that is "close" as a template and then go
from there.  Which leads to a good percentage of drivers having hints
of modular stuff when there is no intent of them ever being modular.

> With the title fixed up as Joerg asked,
> 
> Acked-by: Robin Murphy <robin.murphy@arm.com>

Thanks for the feedback/review.  Will re-send with updated titles before
the week is finished and a good chance for additional feedback has elapsed.

Paul.
--

> 
> >  module_param(force_stage, int, S_IRUGO);
> >  MODULE_PARM_DESC(force_stage,
> >  	"Force SMMU mappings to be installed at a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation.");
Robin Murphy Nov. 28, 2018, 5:28 p.m. UTC | #3
On 28/11/2018 15:24, Paul Gortmaker wrote:
> [Re: [PATCH 8/9] iommu: arm-smmu: make it explicitly non-modular] On 28/11/2018 (Wed 12:42) Robin Murphy wrote:
> 
>> Hi Paul,
>>
>> On 26/11/2018 22:31, Paul Gortmaker wrote:
> 
> [...]
> 
>>> We add a moduleparam.h include since the file does actually declare
>>> some module parameters, and leaving them as such is the easiest way
>>> currently to remain backwards compatible with existing use cases.
> 
> [...]
> 
> 
>> Is it worth introducing builtin_param() and friends for this sort of thing,
>> to echo the *_platform_driver() helpers? It seems like that could be
>> justifiable under the motivation described in the cover letter.
> 
> I've definitely gone back and looked at this a few times when coming
> across the few corner cases like these, to remind myself why I didn't do
> it already.
> 
> We'd not want to replicate all the module_param stuff as an instance of
> builtin_param() because we already have setup() and setup_param() in
> init.h -- however they don't do the file name in the param - hence the
> reason it isn't a direct swap in replacement.
> 
> So, it would become some more complex refactoring of moduleparam.h into
> say bootparam.h - to reduce code/macro duplication, while at the same
> time being aware of existing setup_param stuff and making something like
> a new setup_param_named() that is consistent with existing setup fcns.
> 
> And based on past experience, there will be reviewers who don't see the
> value in the distinction and simply reply with two words "why bother?".
> 
> Not impossible, but not as simple as the builtin_platform_driver and
> similar wrappers that I've already added to mainline.  You've made we
> want to go have another look at it again, but in the meantime we can do
> what I've done here, and circle around later to update the few instances
> of moduleparam in non-modules once/if the refactoring I describe above
> works out and is accepted in mainline.

Sure, I definitely agree with doing a first pass like this to sweep up 
all the cruft and audit the module_param users at the same time, then 
considering a robust refactoring once we've got a clear idea of how many 
users actually need it.

TBH, at this point I was thinking along the lines of a simple:

#ifndef MODULE
#define builtin_param(name, type, perm)			\
	module_param(name, type, perm)
#define builtin_param_named(name, name, type, perrm)	\
	module_param_named(name, name, type, perm)
#endif

still in moduleparam.h, purely so that the intent can be made really 
clear in driver code and it's more searchable than just comments. But 
yeah, even that would probably be objectionable to many.

>> Otherwise, the changes look reasonable. I still hold out hope that one day
>> we'll be able to make IOMMU drivers modular (it can work with minimal hacks
>> today, but it's far from robust in general), but for now I agree this makes
>> sense (and it'll be easy enough to revert for playing with further hacks).
> 
> I totally agree - I've had similar discussions with the DMA maintainers,
> and if being modular can be made to work and has a use case - great!
> 
> But it should be a conscious decision, since nobody writes a new driver
> from scratch; they copy one that is "close" as a template and then go
> from there.  Which leads to a good percentage of drivers having hints
> of modular stuff when there is no intent of them ever being modular.
> 
>> With the title fixed up as Joerg asked,
>>
>> Acked-by: Robin Murphy <robin.murphy@arm.com>
> 
> Thanks for the feedback/review.  Will re-send with updated titles before
> the week is finished and a good chance for additional feedback has elapsed.

Great! There's probably some more subtleties that could be tidied up 
when the driver is truly non-removable, but I can take a look at that 
myself once this patch has gone in.

Cheers,
Robin.
diff mbox series

Patch

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 5a28ae892504..4a2e143fdf52 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -41,7 +41,8 @@ 
 #include <linux/io-64-nonatomic-hi-lo.h>
 #include <linux/iommu.h>
 #include <linux/iopoll.h>
-#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/moduleparam.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
@@ -101,6 +102,10 @@ 
 #define MSI_IOVA_LENGTH			0x100000
 
 static int force_stage;
+/*
+ * not really modular, but the easiest way to keep compat with existing
+ * bootargs behaviour is to continue using module_param() here.
+ */
 module_param(force_stage, int, S_IRUGO);
 MODULE_PARM_DESC(force_stage,
 	"Force SMMU mappings to be installed at a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation.");
@@ -1964,7 +1969,6 @@  static const struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "cavium,smmu-v2", .data = &cavium_smmuv2 },
 	{ },
 };
-MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
 
 #ifdef CONFIG_ACPI
 static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
@@ -2224,24 +2228,18 @@  static int arm_smmu_legacy_bus_init(void)
 }
 device_initcall_sync(arm_smmu_legacy_bus_init);
 
-static int arm_smmu_device_remove(struct platform_device *pdev)
+static void arm_smmu_device_shutdown(struct platform_device *pdev)
 {
 	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
 
 	if (!smmu)
-		return -ENODEV;
+		return;
 
 	if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
 		dev_err(&pdev->dev, "removing device with active domains!\n");
 
 	/* Turn the thing off */
 	writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
-	return 0;
-}
-
-static void arm_smmu_device_shutdown(struct platform_device *pdev)
-{
-	arm_smmu_device_remove(pdev);
 }
 
 static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
@@ -2256,16 +2254,12 @@  static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
 
 static struct platform_driver arm_smmu_driver = {
 	.driver	= {
-		.name		= "arm-smmu",
-		.of_match_table	= of_match_ptr(arm_smmu_of_match),
-		.pm		= &arm_smmu_pm_ops,
+		.name			= "arm-smmu",
+		.of_match_table		= of_match_ptr(arm_smmu_of_match),
+		.pm			= &arm_smmu_pm_ops,
+		.suppress_bind_attrs	= true,
 	},
 	.probe	= arm_smmu_device_probe,
-	.remove	= arm_smmu_device_remove,
 	.shutdown = arm_smmu_device_shutdown,
 };
-module_platform_driver(arm_smmu_driver);
-
-MODULE_DESCRIPTION("IOMMU API for ARM architected SMMU implementations");
-MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
-MODULE_LICENSE("GPL v2");
+builtin_platform_driver(arm_smmu_driver);