Re: [22/44] driver core: let dev_set_drvdata return int instead of voidas it can fail

[Milton Miller]

On Fri, 20 May 2011 about 00:10:40 -0000, Greg Kroah-Hartman wrote:
> 
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> 
> Before commit
> 
> 	b402843 (Driver core: move dev_get/set_drvdata to drivers/base/dd.c)
> 
> calling dev_set_drvdata with dev=NULL was an unchecked error. After some
> discussion about what to return in this case removing the check (and so
> producing a null pointer exception) seems fine.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxx>
> 

I obviously didn't follow the discussions close enough and got
confused[1], but I don't like the anticipated fallout of pushing this
change though all the drivers.  Let alone see the arguments that not
being able to free the device when it fails is no reason to fail.

So, my first question is:  When is it legal for a driver to set driver
data before a device is registered?

I understand we can call device_initialize and then set driver and
then add it to the bus, but is there ever a reason to add driver data
before we call device_initialize?


And if for some obscure reason we need it early, could we instead
move it to the public portion of struct device?  Its not like its
core data list that needs special locking.

Or ... I was going to say move the private allocation to
device_initialize instead, but I see that too is void.  On the other
hand there are only 116 lines found by git grep device_initialize,
as oposed to 3543 with set_drvdata.






AHH!

I just found this quote:

https://lkml.org/lkml/2011/4/29/57

Uwe:
> Russell:
> > If the answer is no one, its pointless returning an error value in the
> > first place (which I think is what the original author already thought
> > about.)
> In the meantime I learned that dev->p is valid when the device is
> registered. As calling dev_set_drvdata on an unregisted device is not
> allowed maybe issuing a warning instead would be OK for me, too.


So why is this in the queue to be merged?

milton


[1] I scanned the commit message and saw "just dereference NULL"
and didn't notice we are still changing the return type.

So we loose the check on no-mmu or archs that allow map of
page 0.  Where is Russell's BUG_ON_MAPPABLE_NULL?


Actually I probably read https://lkml.org/lkml/2011/4/29/57 and
expected the patch in its current form to be dead.


Anyways, here is a patch for next:

Prove no driver calls dev_set_drvdata before device_add, so we
can make it a full BUG_ON(!dev || !dev->p);

Index: work.git/drivers/base/dd.c
===================================================================
--- work.git.orig/drivers/base/dd.c	2011-05-20 01:57:32.390257692 -0500
+++ work.git/drivers/base/dd.c	2011-05-20 02:16:56.654275127 -0500
@@ -413,6 +413,7 @@ int dev_set_drvdata(struct device *dev, 
 	int error;
 
 	if (!dev->p) {
+		WARN(1, "dev_set_drvdata called before device_add");
 		error = device_private_init(dev);
 		if (error)
 			return error;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/