Re: [Cocci] [RFC] drop owner assignment from platform_drivers

[Arnd Bergmann]

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

[Arnd Bergmann]

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

[Arnd Bergmann]

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