[Cocci] [RFC] drop owner assignment from platform_drivers
Hi, people found out that for platform_driver, we don't need to set the .owner field because this is done by the platform driver core. So far, so good. However, now I got patches removing the .owner field for this single i2c driver or for that one. To prevent getting thousands of patches fixing single drivers, I used coccinelle to remove all instances from the kernel. The SmPL looks like this, it doesn't blindly remove all THIS_MODULE, but checks if the platform_driver struct was really used by a call actually setting the .owner field: === @match1@ declarer name module_platform_driver; declarer name module_platform_driver_probe; identifier __driver; @@ ( module_platform_driver(__driver); | module_platform_driver_probe(__driver, ...); ) @fix1 depends on match1@ identifier match1.__driver; @@ static struct platform_driver __driver = { .driver = { - .owner = THIS_MODULE, } }; @match2@ identifier __driver; @@ ( platform_driver_register(&__driver) | platform_driver_probe(&__driver, ...) | platform_create_bundle(&__driver, ...) ) @fix2 depends on match2@ identifier match2.__driver; @@ static struct platform_driver __driver = { .driver = { - .owner = THIS_MODULE, } }; === I tried to group the changes. The current granularity is directory level. The resulting branch can be found here (it is based on linux-next of yesterday): git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git drop_platform_driver_owner This still results in 300 patches as this shortened pull request shows: === Wolfram Sang (299): ARM: common: drop owner assignment from platform_drivers ARM: mach-davinci: drop owner assignment from platform_drivers ARM: mach-imx: drop owner assignment from platform_drivers ... ASoC: txx9: drop owner assignment from platform_drivers ASoC: ux500: drop owner assignment from platform_drivers ALSA: sparc: drop owner assignment from platform_drivers arch/arm/common/sa.c | 1 - arch/arm/mach-davinci/cpuidle.c | 1 - arch/arm/mach-davinci/pm.c | 1 - ... sound/sparc/amd7930.c| 1 - sound/sparc/cs4231.c | 1 - sound/sparc/dbri.c | 1 - 1688 files changed, 1718 deletions(-) === I don't want to send all 300 patches to lkml. I still think, they should go via subsystems, though, and not via a single pull request. So, I am working on just sending smaller pieces of this huge series to the apropriate mailing lists (like arm, netdev...) as an independent series. Then, each subsystem can decide if the patches should be further folded. In my experience, this mileage varies from subsystem to subsystem. That's my plan for today. If you have comments, other suggestions or remarks, I'd like to hear them. Thanks, Wolfram signature.asc Description: Digital signature ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] [PATCH 1/1 linux-next] scripts/coccinelle/misc/boolinit.cocci: fix assignment warnings
Replace "Assignment of bool to 0/1" by "Assignment of 0/1 to bool" Suggested-by: Dan Mick Signed-off-by: Fabian Frederick --- scripts/coccinelle/misc/boolinit.cocci | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/coccinelle/misc/boolinit.cocci b/scripts/coccinelle/misc/boolinit.cocci index b9abed4..63c4562 100644 --- a/scripts/coccinelle/misc/boolinit.cocci +++ b/scripts/coccinelle/misc/boolinit.cocci @@ -145,13 +145,13 @@ cocci.print_main("WARNING: Comparison of bool to 0/1",p) p1 << r3.p1; @@ -cocci.print_main("WARNING: Assignment of bool to 0/1",p1) +cocci.print_main("WARNING: Assignment of 0/1 to bool",p1) @script:python depends on org@ p2 << r3.p2; @@ -cocci.print_main("ERROR: Assignment of bool to non-0/1 constant",p2) +cocci.print_main("ERROR: Assignment of non-0/1 constant to bool",p2) @script:python depends on report@ p << r1.p; @@ -169,10 +169,10 @@ coccilib.report.print_report(p[0],"WARNING: Comparison of bool to 0/1") p1 << r3.p1; @@ -coccilib.report.print_report(p1[0],"WARNING: Assignment of bool to 0/1") +coccilib.report.print_report(p1[0],"WARNING: Assignment of 0/1 to bool") @script:python depends on report@ p2 << r3.p2; @@ -coccilib.report.print_report(p2[0],"ERROR: Assignment of bool to non-0/1 constant") +coccilib.report.print_report(p2[0],"ERROR: Assignment of non-0/1 constant to bool") -- 1.9.3 ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] drop owner assignment from platform_drivers
> @match1@ > declarer name module_platform_driver; > declarer name module_platform_driver_probe; > identifier __driver; > @@ > ( > module_platform_driver(__driver); > | > module_platform_driver_probe(__driver, ...); > ) > > @fix1 depends on match1@ > identifier match1.__driver; > @@ > static struct platform_driver __driver = { > .driver = { > - .owner = THIS_MODULE, > } > }; > > @match2@ > identifier __driver; > @@ > ( > platform_driver_register(&__driver) > | > platform_driver_probe(&__driver, ...) > | > platform_create_bundle(&__driver, ...) > ) > > @fix2 depends on match2@ > identifier match2.__driver; > @@ > static struct platform_driver __driver = { > .driver = { > - .owner = THIS_MODULE, > } > }; The semantic patch looks fine. I'm a little surprised that it doesn't complain about the lack of a comma after .driver = { .owner = THIS_MODULE, } but it seems that it does not. If you think that it would be useful to have this in the Linux kernel, so people don't add the owner initializer back in the future, you can try coccinelle/tools/sgen/sgen (run make in the coccinelle/tools/sgen directory). That will guide you through the process of making a Linux-ready semantic patch. Feedback would be appreciated. julia ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH 1/1 linux-next] scripts/coccinelle/misc/boolinit.cocci: fix assignment warnings
Acked-by: Julia Lawall Thanks! On Fri, 10 Oct 2014, Fabian Frederick wrote: > Replace "Assignment of bool to 0/1" by "Assignment of 0/1 to bool" > > Suggested-by: Dan Mick > Signed-off-by: Fabian Frederick > --- > scripts/coccinelle/misc/boolinit.cocci | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/scripts/coccinelle/misc/boolinit.cocci > b/scripts/coccinelle/misc/boolinit.cocci > index b9abed4..63c4562 100644 > --- a/scripts/coccinelle/misc/boolinit.cocci > +++ b/scripts/coccinelle/misc/boolinit.cocci > @@ -145,13 +145,13 @@ cocci.print_main("WARNING: Comparison of bool to 0/1",p) > p1 << r3.p1; > @@ > > -cocci.print_main("WARNING: Assignment of bool to 0/1",p1) > +cocci.print_main("WARNING: Assignment of 0/1 to bool",p1) > > @script:python depends on org@ > p2 << r3.p2; > @@ > > -cocci.print_main("ERROR: Assignment of bool to non-0/1 constant",p2) > +cocci.print_main("ERROR: Assignment of non-0/1 constant to bool",p2) > > @script:python depends on report@ > p << r1.p; > @@ -169,10 +169,10 @@ coccilib.report.print_report(p[0],"WARNING: Comparison > of bool to 0/1") > p1 << r3.p1; > @@ > > -coccilib.report.print_report(p1[0],"WARNING: Assignment of bool to 0/1") > +coccilib.report.print_report(p1[0],"WARNING: Assignment of 0/1 to bool") > > @script:python depends on report@ > p2 << r3.p2; > @@ > > -coccilib.report.print_report(p2[0],"ERROR: Assignment of bool to non-0/1 > constant") > +coccilib.report.print_report(p2[0],"ERROR: Assignment of non-0/1 constant to > bool") > -- > 1.9.3 > > ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] drop owner assignment from platform_drivers
On Friday 10 October 2014 09:24:39 Wolfram Sang wrote: > people found out that for platform_driver, we don't need to set the > .owner field because this is done by the platform driver core. So far, > so good. However, now I got patches removing the .owner field for this > single i2c driver or for that one. To prevent getting thousands of > patches fixing single drivers, I used coccinelle to remove all instances > from the kernel. The SmPL looks like this, it doesn't blindly remove all > THIS_MODULE, but checks if the platform_driver struct was really used by > a call actually setting the .owner field: Is the intention just to save a few lines in the kernel source, or are there any additional upsides to doing this? While it looks like an obvious cleanup, it also seems to me that there is zero effect in terms of functionality, code size or enabling future changes. I'm all for adding your semantic patch to scripts/coccinelle so it gets picked up by anyone writing new drivers or doing code cleanup on their driver, but I'm unsure about the value of applying all your patches for the existing drivers. Arnd ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] drop owner assignment from platform_drivers
On Fri, Oct 10, 2014 at 09:24:39AM +0200, Wolfram Sang wrote: > people found out that for platform_driver, we don't need to set the > .owner field because this is done by the platform driver core. So far, > so good. However, now I got patches removing the .owner field for this > single i2c driver or for that one. To prevent getting thousands of > patches fixing single drivers, I used coccinelle to remove all instances > from the kernel. The SmPL looks like this, it doesn't blindly remove all > THIS_MODULE, but checks if the platform_driver struct was really used by > a call actually setting the .owner field: Is this correct? #define platform_driver_register(drv) \ __platform_driver_register(drv, THIS_MODULE) extern int __platform_driver_register(struct platform_driver *, struct module *); Fine for those which use platform_driver_register(), but: /* non-hotpluggable platform devices may use this so that probe() and * its support may live in __init sections, conserving runtime memory. */ extern int platform_driver_probe(struct platform_driver *driver, int (*probe)(struct platform_device *)); platform_driver_probe() doesn't seem to know which module called it. This is also true of platform_create_bundle: extern struct platform_device *platform_create_bundle( struct platform_driver *driver, int (*probe)(struct platform_device *), struct resource *res, unsigned int n_res, const void *data, size_t size); So, it's not as trivial as just "all platform driver's should not have a .owner field" - the real answer is far more complex than that. -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] drop owner assignment from platform_drivers
> The semantic patch looks fine. Wow, nothing to improve on the semantic patch? Now I am proud :) Thanks Julia for your support, as always! > If you think that it would be useful to have this in the Linux kernel, so > people don't add the owner initializer back in the future, you can try > > coccinelle/tools/sgen/sgen Will try later this weekend, thanks! signature.asc Description: Digital signature ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] drop owner assignment from platform_drivers
Hi Arnd, thanks for taking a look! On Fri, Oct 10, 2014 at 10:30:08AM +0200, Arnd Bergmann wrote: > On Friday 10 October 2014 09:24:39 Wolfram Sang wrote: > > people found out that for platform_driver, we don't need to set the > > .owner field because this is done by the platform driver core. So far, > > so good. However, now I got patches removing the .owner field for this > > single i2c driver or for that one. To prevent getting thousands of > > patches fixing single drivers, I used coccinelle to remove all instances > > from the kernel. The SmPL looks like this, it doesn't blindly remove all > > THIS_MODULE, but checks if the platform_driver struct was really used by > > a call actually setting the .owner field: > > Is the intention just to save a few lines in the kernel source, or are > there any additional upsides to doing this? As written above, I don't like getting patches removing this line for single drivers. I already got two and I am expecting more. So I'd prefer to do this on subsystem level. I will apply the I2C part, for sure. > While it looks like an obvious cleanup, it also seems to me that there > is zero effect in terms of functionality, code size or enabling future > changes. Well, the kernel image will compress better ;) And well, it is cleaner. Why should we set up something if it gets overwritten anyhow? > I'm all for adding your semantic patch to scripts/coccinelle so it gets > picked up by anyone writing new drivers or doing code cleanup on their > driver, but I'm unsure about the value of applying all your patches > for the existing drivers. I could try reducing the number of patches. Any other downsides? Thanks, Wolfram signature.asc Description: Digital signature ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] drop owner assignment from platform_drivers
On Fri, Oct 10, 2014 at 09:36:27AM +0100, Russell King - ARM Linux wrote: > On Fri, Oct 10, 2014 at 09:24:39AM +0200, Wolfram Sang wrote: > > people found out that for platform_driver, we don't need to set the > > .owner field because this is done by the platform driver core. So far, > > so good. However, now I got patches removing the .owner field for this > > single i2c driver or for that one. To prevent getting thousands of > > patches fixing single drivers, I used coccinelle to remove all instances > > from the kernel. The SmPL looks like this, it doesn't blindly remove all > > THIS_MODULE, but checks if the platform_driver struct was really used by > > a call actually setting the .owner field: > > Is this correct? > > #define platform_driver_register(drv) \ > __platform_driver_register(drv, THIS_MODULE) > extern int __platform_driver_register(struct platform_driver *, > struct module *); > > Fine for those which use platform_driver_register(), but: > > /* non-hotpluggable platform devices may use this so that probe() and > * its support may live in __init sections, conserving runtime memory. > */ > extern int platform_driver_probe(struct platform_driver *driver, > int (*probe)(struct platform_device *)); > > platform_driver_probe() doesn't seem to know which module called it. > This is also true of platform_create_bundle: > > extern struct platform_device *platform_create_bundle( > struct platform_driver *driver, int (*probe)(struct platform_device > *), > struct resource *res, unsigned int n_res, > const void *data, size_t size); > > So, it's not as trivial as just "all platform driver's should not have a > .owner field" - the real answer is far more complex than that. platform_create_bundle() calls platform_driver_probe(). platform_driver_probe() calls platform_driver_register(). platform_driver_register() modifies driver.owner. So, it is correct from the point of view that it doesn't make sense to set the .owner field if it gets overwritten anyhow. You got me wondering, though, that it could not be correct to call platform_driver_register() from the platform core instead of module init. I will check tomorrow. Still, this would be a bug independent of my series. Although I'd need to respin it if platform_driver_probe() needed a fix. Thanks, Wolfram signature.asc Description: Digital signature ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] drop owner assignment from platform_drivers
On Friday 10 October 2014 20:12:21 Wolfram Sang wrote: > Hi Arnd, > > thanks for taking a look! > > On Fri, Oct 10, 2014 at 10:30:08AM +0200, Arnd Bergmann wrote: > > On Friday 10 October 2014 09:24:39 Wolfram Sang wrote: > > > people found out that for platform_driver, we don't need to set the > > > .owner field because this is done by the platform driver core. So far, > > > so good. However, now I got patches removing the .owner field for this > > > single i2c driver or for that one. To prevent getting thousands of > > > patches fixing single drivers, I used coccinelle to remove all instances > > > from the kernel. The SmPL looks like this, it doesn't blindly remove all > > > THIS_MODULE, but checks if the platform_driver struct was really used by > > > a call actually setting the .owner field: > > > > Is the intention just to save a few lines in the kernel source, or are > > there any additional upsides to doing this? > > As written above, I don't like getting patches removing this line for > single drivers. I already got two and I am expecting more. So I'd prefer > to do this on subsystem level. I will apply the I2C part, for sure. Ah, right, that is certainly a good reason. Have you checked how many other patches like this got merged? If it's a common theme, then doing all at once sounds like a really good idea. > > While it looks like an obvious cleanup, it also seems to me that there > > is zero effect in terms of functionality, code size or enabling future > > changes. > > Well, the kernel image will compress better ;) Right, I hadn't thought of that. > And well, it is cleaner. > Why should we set up something if it gets overwritten anyhow? Of course we shouldn't. To take the analogy: just like it doesn't matter what the initial value of the .owner fields is after it gets overwritten by the probe call, the patch to remove the origonal initialization isn't very valuable after the code that has the useless initialization is already part of the kernel ;-) > > I'm all for adding your semantic patch to scripts/coccinelle so it gets > > picked up by anyone writing new drivers or doing code cleanup on their > > driver, but I'm unsure about the value of applying all your patches > > for the existing drivers. > > I could try reducing the number of patches. Any other downsides? Just to be clear: I don't really see any downsides to your patches, the problem is that the upsides relatively small, so it's unclear if all maintainers are better off applying the patches or not even knowing about them. Arnd ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] drop owner assignment from platform_drivers
On Friday 10 October 2014 20:26:05 Wolfram Sang wrote: > > You got me wondering, though, that it could not be correct to call > platform_driver_register() from the platform core instead of module > init. I will check tomorrow. Still, this would be a bug independent of > my series. Although I'd need to respin it if platform_driver_probe() > needed a fix. Right, this seems to be a preexisting bug. platform_create_bundle and platform_driver_probe will both overwrite the .owner field with NULL since they live in builtin code. They need to be replaced with __platform_driver_probe and __platform_driver_register that both take an extra owner argument passed down from the caller in the driver module. Arnd ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] drop owner assignment from platform_drivers
On Fri, Oct 10, 2014 at 08:26:05PM +0200, Wolfram Sang wrote: > platform_create_bundle() calls platform_driver_probe(). > platform_driver_probe() calls platform_driver_register(). > platform_driver_register() modifies driver.owner. > > So, it is correct from the point of view that it doesn't make sense to > set the .owner field if it gets overwritten anyhow. > > You got me wondering, though, that it could not be correct to call > platform_driver_register() from the platform core instead of module > init. I will check tomorrow. Still, this would be a bug independent of > my series. Although I'd need to respin it if platform_driver_probe() > needed a fix. This shows what the bad side-effect of people doing "cleanups" is. This bug was introduced by: commit 9447057eaff871dd7c63c808de761b8732407169 Author: Libo Chen Date: Sat May 25 12:40:50 2013 +0800 platform_device: use a macro instead of platform_driver_register I found a lot of mistakes using struct platform_driver without owner so I make a macro instead of the function platform_driver_register. It can set owner in it, then guys don`t care about module owner again. Signed-off-by: Libo Chen Signed-off-by: Greg Kroah-Hartman So, this patch subsituted one set of mistakes for another mistake... -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci