From: Julia Lawall <[email protected]>
The return value of the remove function of a driver structure, and thus of
a platform_driver structure, is ultimately ignored, and is thus
unnecessary. The goal of this patch is to make it possible to convert the
platform_driver functions stored in the remove field such that they return
void. This patch introduces a temporary field remove_new with return type
void into the platform_driver structure, and updates the only place that
the remove function is called to call the function in the remove_new field,
if one is available. The subsequent patches update some drivers to use the
remove_new field.
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/base/platform.c | 16 ++++++++++++++--
include/linux/platform_device.h | 1 +
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 4b8cc6a..4800110 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -50,6 +50,7 @@ extern void platform_device_put(struct platform_device *pdev);
struct platform_driver {
int (*probe)(struct platform_device *);
int (*remove)(struct platform_device *);
+ void (*remove_new)(struct platform_device *);
void (*shutdown)(struct platform_device *);
int (*suspend)(struct platform_device *, pm_message_t state);
int (*suspend_late)(struct platform_device *, pm_message_t state);
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index dfcbfe5..2d7b26a 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -459,7 +459,12 @@ static int platform_drv_remove(struct device *_dev)
struct platform_driver *drv = to_platform_driver(_dev->driver);
struct platform_device *dev = to_platform_device(_dev);
- return drv->remove(dev);
+ if (drv->remove)
+ drv->remove(dev);
+ else
+ drv->remove_new(dev);
+
+ return 0;
}
static void platform_drv_shutdown(struct device *_dev)
@@ -492,10 +497,17 @@ static int platform_drv_resume(struct device *_dev)
*/
int platform_driver_register(struct platform_driver *drv)
{
+ if (drv->remove && drv->remove_new) {
+ printk(KERN_ERR "only one of remove() and "
+ "remove_new() callbacks can be defined, "
+ "aborting...\n");
+ return -EINVAL;
+ }
+
drv->driver.bus = &platform_bus_type;
if (drv->probe)
drv->driver.probe = platform_drv_probe;
- if (drv->remove)
+ if (drv->remove || drv->remove_new)
drv->driver.remove = platform_drv_remove;
if (drv->shutdown)
drv->driver.shutdown = platform_drv_shutdown;
On Wed, 10 Dec 2008 17:26:26 +0100 (CET)
Julia Lawall <[email protected]> wrote:
> From: Julia Lawall <[email protected]>
>
> The return value of the remove function of a driver structure, and thus of
> a platform_driver structure, is ultimately ignored
Currently
> and is thus unnecessary.
Really - capturing those kind of failures is useful information which is
currently discarded but could later be used. Removing that information
makes it very hard to go and put back into all the drivers.
Alan
On Wed, Dec 10, 2008 at 11:26, Julia Lawall wrote:
> The return value of the remove function of a driver structure, and thus of
> a platform_driver structure, is ultimately ignored, and is thus
> unnecessary. The goal of this patch is to make it possible to convert the
> platform_driver functions stored in the remove field such that they return
> void. This patch introduces a temporary field remove_new with return type
> void into the platform_driver structure, and updates the only place that
> the remove function is called to call the function in the remove_new field,
> if one is available. The subsequent patches update some drivers to use the
> remove_new field.
why bother with remove -> remove_new convention ? you'll get a
warning in C about the assignment, but you wont get a build failure,
nor should you get a runtime failure ... and if your ultimate goal is
to drop the return value, then this would be better as you'd get
warnings for everything that needs converting. plus, once .remove is
gone, you're going to have to post another series of patches to
convert .remove_new back to .remove ...
-mike
> On Wed, 10 Dec 2008 17:26:26 +0100 (CET)
> Julia Lawall <[email protected]> wrote:
>
>> From: Julia Lawall <[email protected]>
>>
>> The return value of the remove function of a driver structure, and thus
>> of
>> a platform_driver structure, is ultimately ignored
>
> Currently
Are there really any plans about actually using the return value?
>
>> and is thus unnecessary.
>
> Really - capturing those kind of failures is useful information which is
> currently discarded but could later be used. Removing that information
> makes it very hard to go and put back into all the drivers.
The overwhelming majority of platform drivers do not return anything
meaningful from their remove() callbacks. So, it looked like void is
appropriate here in the same manner as, for example, the module exit
functions are void.
Let me please shed some light on the history of this patchset. It all
began when I sent a benign one-liner to shut up an annoying compiler
warning:
http://marc.info/?l=linux-scsi&m=122714044609103&w=2
It was immediately noticed that the return value of the remove() callback
is actually ignored in the drivers/base/ code:
http://marc.info/?l=linux-scsi&m=122832419623402&w=2
So, the idea of converting the drivers from int (*remove)() to void
(*remove)() was born.
Then, Julia decided to use her semantic patch tool, which looked very
appropriate for this kind of change: conversion of int-returning remove()
callbacks to void callbacks. We asked Greg Kroah-Hartman for his opinion
about the exact way to proceed here, and this is what he suggested
off-list:
<<<
You can do a "migration" type change:
- create a new callback function pointer that returns void
- start moving code over to the new callback
- when finished, delete the old callback
- create a new callback with the same old name, that returns
void
- move the code to use the new name.
Yeah, it's a pain, but it can be done in an incremental way.
thanks,
greg k-h
<<<
The patchset is therefore the first step in the direction of the API cleanup.
So, maybe this background info would help the reviewers somehow.
Thanks,
Dmitri Vorobiev
On Wed, Dec 10, 2008 at 08:03:56PM +0200, Vorobiev Dmitri wrote:
> > On Wed, 10 Dec 2008 17:26:26 +0100 (CET)
> > Julia Lawall <[email protected]> wrote:
> >
> >> From: Julia Lawall <[email protected]>
> >>
> >> The return value of the remove function of a driver structure, and thus
> >> of
> >> a platform_driver structure, is ultimately ignored
> >
> > Currently
>
> Are there really any plans about actually using the return value?
It's often used by the drivers, but currently not handled by
the subsystem. For example, _remove() callback might return -EBUSY
or -EAGAIN, which means that whoever called the _remove() should
try later.
For example see drivers/mfd/asic3.c. The driver registers GPIO
chips, on _remove() it *tries* to unregister these chips, but
it could fail (when provided GPIOs are in use by somebody -- it
might be in-kernel users, or sysfs users).
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
> On Wed, Dec 10, 2008 at 08:03:56PM +0200, Vorobiev Dmitri wrote:
>> > On Wed, 10 Dec 2008 17:26:26 +0100 (CET)
>> > Julia Lawall <[email protected]> wrote:
>> >
>> >> From: Julia Lawall <[email protected]>
>> >>
>> >> The return value of the remove function of a driver structure, and
>> thus
>> >> of
>> >> a platform_driver structure, is ultimately ignored
>> >
>> > Currently
>>
>> Are there really any plans about actually using the return value?
>
> It's often used by the drivers, but currently not handled by
> the subsystem. For example, _remove() callback might return -EBUSY
> or -EAGAIN, which means that whoever called the _remove() should
> try later.
Sure, it's easy to find drivers, which that return a non-dummy value from
the remove() callback thinking that someone up there will take care of the
error.
The point is, however, that
1) the SGI Indy SCSI controller driver doesn't compile cleanly [2], and
SCSI maintainers do not apply the patch because, in principle, the
(*remove)() callback should not return int since the return value is
discarded [1];
2) the changes in the platform driver framework are, as it seems, not
acceptable because the non-dummy return values can, in principle, be used
for error checking.
All that I actually care about is to get rid of this:
<<<
CC [M] drivers/scsi/sgiwd93.o
drivers/scsi/sgiwd93.c:314: warning: initialization from incompatible
pointer type
<<<
I don't care whether the compilation warning is gone because [2] is
applied, or the entire platform driver framework is combed though to
change int to void.
Thanks,
Dmitri
[1] http://kerneltrap.org/mailarchive/linux-scsi/2008/12/3/4300474
[2] http://kerneltrap.org/mailarchive/linux-scsi/2008/11/20/4169644
> On Wed, Dec 10, 2008 at 11:26, Julia Lawall wrote:
>> The return value of the remove function of a driver structure, and thus
>> of
>> a platform_driver structure, is ultimately ignored, and is thus
>> unnecessary. The goal of this patch is to make it possible to convert
>> the
>> platform_driver functions stored in the remove field such that they
>> return
>> void. This patch introduces a temporary field remove_new with return
>> type
>> void into the platform_driver structure, and updates the only place that
>> the remove function is called to call the function in the remove_new
>> field,
>> if one is available. The subsequent patches update some drivers to use
>> the
>> remove_new field.
>
> why bother with remove -> remove_new convention ?
Please see this email for the background:
http://lkml.org/lkml/2008/12/10/231
> you'll get a
> warning in C about the assignment, but you wont get a build failure,
...unless you compile with -Werror, which frequently the case.
Dmitri
On Wed, Dec 10, 2008 at 17:18, Vorobiev Dmitri wrote:
>> On Wed, Dec 10, 2008 at 11:26, Julia Lawall wrote:
>>> The return value of the remove function of a driver structure, and thus
>>> of
>>> a platform_driver structure, is ultimately ignored, and is thus
>>> unnecessary. The goal of this patch is to make it possible to convert
>>> the
>>> platform_driver functions stored in the remove field such that they
>>> return
>>> void. This patch introduces a temporary field remove_new with return
>>> type
>>> void into the platform_driver structure, and updates the only place that
>>> the remove function is called to call the function in the remove_new
>>> field,
>>> if one is available. The subsequent patches update some drivers to use
>>> the
>>> remove_new field.
>>
>> why bother with remove -> remove_new convention ?
>
> Please see this email for the background:
>
> http://lkml.org/lkml/2008/12/10/231
>
>> you'll get a
>> warning in C about the assignment, but you wont get a build failure,
>
> ...unless you compile with -Werror, which frequently the case.
anyone crazy enough to build with -Werror is crazy enough to send in a fix ;)
-mike
On Thu, Dec 11, 2008 at 12:06:34AM +0200, Vorobiev Dmitri wrote:
> > On Wed, Dec 10, 2008 at 08:03:56PM +0200, Vorobiev Dmitri wrote:
> >> > On Wed, 10 Dec 2008 17:26:26 +0100 (CET)
> >> > Julia Lawall <[email protected]> wrote:
> >> >
> >> >> From: Julia Lawall <[email protected]>
> >> >>
> >> >> The return value of the remove function of a driver structure, and
> >> thus
> >> >> of
> >> >> a platform_driver structure, is ultimately ignored
> >> >
> >> > Currently
> >>
> >> Are there really any plans about actually using the return value?
> >
> > It's often used by the drivers, but currently not handled by
> > the subsystem. For example, _remove() callback might return -EBUSY
> > or -EAGAIN, which means that whoever called the _remove() should
> > try later.
>
> Sure, it's easy to find drivers, which that return a non-dummy value from
> the remove() callback thinking that someone up there will take care of the
> error.
>
> The point is, however, that
[...]
> SCSI maintainers do not apply the patch
This sometimes happens. You can try to repost your original patch, and
in the commit message briefly describe that return type is unlikely to
change, and give a pointer to this discussion.
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Wed, Dec 10, 2008 at 05:37:42PM -0500, Mike Frysinger wrote:
> On Wed, Dec 10, 2008 at 17:18, Vorobiev Dmitri wrote:
> >> On Wed, Dec 10, 2008 at 11:26, Julia Lawall wrote:
> >>> The return value of the remove function of a driver structure, and thus
> >>> of
> >>> a platform_driver structure, is ultimately ignored, and is thus
> >>> unnecessary. The goal of this patch is to make it possible to convert
> >>> the
> >>> platform_driver functions stored in the remove field such that they
> >>> return
> >>> void. This patch introduces a temporary field remove_new with return
> >>> type
> >>> void into the platform_driver structure, and updates the only place that
> >>> the remove function is called to call the function in the remove_new
> >>> field,
> >>> if one is available. The subsequent patches update some drivers to use
> >>> the
> >>> remove_new field.
> >>
> >> why bother with remove -> remove_new convention ?
> >
> > Please see this email for the background:
> >
> > http://lkml.org/lkml/2008/12/10/231
> >
> >> you'll get a
> >> warning in C about the assignment, but you wont get a build failure,
> >
> > ...unless you compile with -Werror, which frequently the case.
>
> anyone crazy enough to build with -Werror is crazy enough to send in a fix ;)
Hm, have you noted that some arches have that flag enabled in their
build?
And it's not ok to add a couple of hundred build warnings to the system,
sorry.
thanks,
greg k-h
Greg KH wrote:
> On Wed, Dec 10, 2008 at 05:37:42PM -0500, Mike Frysinger wrote:
>> On Wed, Dec 10, 2008 at 17:18, Vorobiev Dmitri wrote:
>>>> On Wed, Dec 10, 2008 at 11:26, Julia Lawall wrote:
>>>>> The return value of the remove function of a driver structure, and thus
>>>>> of
>>>>> a platform_driver structure, is ultimately ignored, and is thus
>>>>> unnecessary. The goal of this patch is to make it possible to convert
>>>>> the
>>>>> platform_driver functions stored in the remove field such that they
>>>>> return
>>>>> void. This patch introduces a temporary field remove_new with return
>>>>> type
>>>>> void into the platform_driver structure, and updates the only place that
>>>>> the remove function is called to call the function in the remove_new
>>>>> field,
>>>>> if one is available. The subsequent patches update some drivers to use
>>>>> the
>>>>> remove_new field.
>>>> why bother with remove -> remove_new convention ?
>>> Please see this email for the background:
>>>
>>> http://lkml.org/lkml/2008/12/10/231
>>>
>>>> you'll get a
>>>> warning in C about the assignment, but you wont get a build failure,
>>> ...unless you compile with -Werror, which frequently the case.
>> anyone crazy enough to build with -Werror is crazy enough to send in a fix ;)
>
> Hm, have you noted that some arches have that flag enabled in their
> build?
>
> And it's not ok to add a couple of hundred build warnings to the system,
> sorry.
Still, what about the whole series? What do you think about int->void migration for the remove() callback?
Thanks,
Dmitri
>
> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Fri, Dec 12, 2008 at 00:17, Greg KH wrote:
> On Wed, Dec 10, 2008 at 05:37:42PM -0500, Mike Frysinger wrote:
>> On Wed, Dec 10, 2008 at 17:18, Vorobiev Dmitri wrote:
>> >> On Wed, Dec 10, 2008 at 11:26, Julia Lawall wrote:
>> >> you'll get a
>> >> warning in C about the assignment, but you wont get a build failure,
>> >
>> > ...unless you compile with -Werror, which frequently the case.
>>
>> anyone crazy enough to build with -Werror is crazy enough to send in a fix ;)
>
> Hm, have you noted that some arches have that flag enabled in their
> build?
i dont see anyone building things beyond a select dir or two (none of
which contain platform drivers) with -Werror. where are you seeing
this ?
besides, while using -Werror in development may make sense, sticking
it into the release for end users is just plain mean. gcc is so
unreliable with warnings across versions.
-mike
On Fri, Dec 12, 2008 at 04:00:02PM -0500, Mike Frysinger wrote:
> On Fri, Dec 12, 2008 at 00:17, Greg KH wrote:
> > On Wed, Dec 10, 2008 at 05:37:42PM -0500, Mike Frysinger wrote:
> >> On Wed, Dec 10, 2008 at 17:18, Vorobiev Dmitri wrote:
> >> >> On Wed, Dec 10, 2008 at 11:26, Julia Lawall wrote:
> >> >> you'll get a
> >> >> warning in C about the assignment, but you wont get a build failure,
> >> >
> >> > ...unless you compile with -Werror, which frequently the case.
> >>
> >> anyone crazy enough to build with -Werror is crazy enough to send in a fix ;)
> >
> > Hm, have you noted that some arches have that flag enabled in their
> > build?
>
> i dont see anyone building things beyond a select dir or two (none of
> which contain platform drivers) with -Werror. where are you seeing
> this ?
Sparc builds add this from what I remember, I've broken their build at
times by adding unnecessary errors and got an inbox full of emails about
it :)
thanks,
greg k-h
On Fri, Dec 12, 2008 at 01:00:08PM +0200, Dmitri Vorobiev wrote:
> Greg KH wrote:
> > On Wed, Dec 10, 2008 at 05:37:42PM -0500, Mike Frysinger wrote:
> >> On Wed, Dec 10, 2008 at 17:18, Vorobiev Dmitri wrote:
> >>>> On Wed, Dec 10, 2008 at 11:26, Julia Lawall wrote:
> >>>>> The return value of the remove function of a driver structure, and thus
> >>>>> of
> >>>>> a platform_driver structure, is ultimately ignored, and is thus
> >>>>> unnecessary. The goal of this patch is to make it possible to convert
> >>>>> the
> >>>>> platform_driver functions stored in the remove field such that they
> >>>>> return
> >>>>> void. This patch introduces a temporary field remove_new with return
> >>>>> type
> >>>>> void into the platform_driver structure, and updates the only place that
> >>>>> the remove function is called to call the function in the remove_new
> >>>>> field,
> >>>>> if one is available. The subsequent patches update some drivers to use
> >>>>> the
> >>>>> remove_new field.
> >>>> why bother with remove -> remove_new convention ?
> >>> Please see this email for the background:
> >>>
> >>> http://lkml.org/lkml/2008/12/10/231
> >>>
> >>>> you'll get a
> >>>> warning in C about the assignment, but you wont get a build failure,
> >>> ...unless you compile with -Werror, which frequently the case.
> >> anyone crazy enough to build with -Werror is crazy enough to send in a fix ;)
> >
> > Hm, have you noted that some arches have that flag enabled in their
> > build?
> >
> > And it's not ok to add a couple of hundred build warnings to the system,
> > sorry.
>
> Still, what about the whole series? What do you think about int->void
> migration for the remove() callback?
In thinking about it some more, I don't really see the point. We should
probably just do something about the return value, as that would be
better, and easier to do.
thanks,
greg k-h