[RFC] drop owner assignment from platform_drivers

[Wolfram Sang]

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/sa1111.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

[Julia Lawall]

Post by Wolfram Sang
@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

[Wolfram Sang]

Post by Julia Lawall
The semantic patch looks fine.

Wow, nothing to improve on the semantic patch? Now I am proud :) Thanks
Julia for your support, as always!

Post by Julia Lawall
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!

[Arnd Bergmann]

Post by Wolfram Sang
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

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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Wolfram Sang]

Hi Arnd,

thanks for taking a look!

Post by Arnd Bergmann

Post by Wolfram Sang
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

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.

Post by Arnd Bergmann
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?

Post by Arnd Bergmann
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

[Arnd Bergmann]

Post by Wolfram Sang
Hi Arnd,
thanks for taking a look!

Post by Arnd Bergmann

Post by Wolfram Sang
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

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.

Post by Wolfram Sang

Post by Arnd Bergmann
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.

Post by Wolfram Sang
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 ;-)

Post by Wolfram Sang

Post by Arnd Bergmann
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

[Russell King - ARM Linux]

Post by Wolfram Sang
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

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.

[Wolfram Sang]

Post by Russell King - ARM Linux

Post by Wolfram Sang
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

Is this correct?
#define platform_driver_register(drv) \
__platform_driver_register(drv, THIS_MODULE)
extern int __platform_driver_register(struct platform_driver *,
struct module *);
/* 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.
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

[Arnd Bergmann]

Post by Wolfram Sang
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

[Wolfram Sang]

Post by Arnd Bergmann

Post by Wolfram Sang
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.

Yeah, that would be one solution. However, my personal favourite would
meanwhile be to revert the commit that Russell mentioned. I think it is
cleaner to have the owner explicitly set in the module rather than
hidden away by a function call. However, grepping through include/linux,
there are a few subsystems hiding it this way. So, it is a pattern
somewhow. Oh well...

[Russell King - ARM Linux]

Post by Wolfram Sang

Post by Arnd Bergmann

Post by Wolfram Sang
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.

Yeah, that would be one solution. However, my personal favourite would
meanwhile be to revert the commit that Russell mentioned. I think it is
cleaner to have the owner explicitly set in the module rather than
hidden away by a function call. However, grepping through include/linux,
there are a few subsystems hiding it this way. So, it is a pattern
somewhow. Oh well...

It really /ought/ to be consistent, because inconsistencies like that
will be a never-ending source of subtle mistakes.

Imagine what it would be like if the kernel was a complete mess of
functions with return type "int" where there was no predominant
pattern of returning negative errno numbers - where it was random
whether int-returning functions returned zero for failure, others
returned zero for success. We would have to look up every single
function to check it's return style, and it would be a bigger problem
when reviewing code.

There is a lot of value for saving time and reducing errors to have a
consistent, simple and obvious methodology.

(That's not to say that it should be enforced draconian style - but
there'd better be a good reason to be different, rather than "I think
it's better this way" or "my personal style is different".)

[Greg KH]

Post by Wolfram Sang

Post by Arnd Bergmann

Post by Wolfram Sang
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.

Yeah, that would be one solution. However, my personal favourite would
meanwhile be to revert the commit that Russell mentioned. I think it is
cleaner to have the owner explicitly set in the module rather than
hidden away by a function call. However, grepping through include/linux,
there are a few subsystems hiding it this way. So, it is a pattern
somewhow. Oh well...

The pattern is to not have to manually set MODULE_OWNER, and have the
pre-processor do it for you, otherwise you will forget or get it wrong.

That is why I accepted this patch to the platform driver interface, as
it is in line with many other bus driver apis (pci, usb, etc.).

I missed the one code path you pointed out, and that should be fixed,
but that doesn't mean that the original patch should be reverted, as it
is the way we want things to be, let's just fix up the bug and move on.

And again, may I just say how much I hate the platform driver code, one
of these days I'm going to lock myself in a room for a week and figure
out a way to just delete that stuff...

greg k-h

[Wolfram Sang]

Post by Greg KH
I missed the one code path you pointed out, and that should be fixed,
but that doesn't mean that the original patch should be reverted, as it
is the way we want things to be, let's just fix up the bug and move on.

OK, that is a clear statement.

So, what is your opinion on the original cleanup series removing
unnecessary '.owner = THIS_MODULE' lines in drivers? Helpful? Noise?

Thanks,

Wolfram

[Greg KH]

Post by Wolfram Sang

Post by Greg KH
I missed the one code path you pointed out, and that should be fixed,
but that doesn't mean that the original patch should be reverted, as it
is the way we want things to be, let's just fix up the bug and move on.

OK, that is a clear statement.
So, what is your opinion on the original cleanup series removing
unnecessary '.owner = THIS_MODULE' lines in drivers? Helpful? Noise?

Helpful, please do it. I can take it all through my driver-core tree if
you want, that might make things easier for others.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Wolfram Sang]

Post by Greg KH

Post by Wolfram Sang
So, what is your opinion on the original cleanup series removing
unnecessary '.owner = THIS_MODULE' lines in drivers? Helpful? Noise?

Helpful, please do it. I can take it all through my driver-core tree if
you want, that might make things easier for others.

Thanks, that might make sense this time.

So, I'll prepare the bugfixes, add the semantic patch, add this all to
my series and respin. It might take a few days because of ELCE in
DÃŒsseldorf, but I'll certainly do it.

[Russell King - ARM Linux]

Post by Wolfram Sang
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 <***@huawei.com>
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 <***@huawei.com>
Signed-off-by: Greg Kroah-Hartman <***@linuxfoundation.org>

So, this patch subsituted one set of mistakes for another mistake...