On Wed, Apr 27, 2011 at 02:11:48AM +0000, KY Srinivasan wrote: > > > > -----Original Message----- > > From: Greg KH [mailto:greg@xxxxxxxxx] > > Sent: Tuesday, April 26, 2011 6:51 PM > > To: KY Srinivasan > > Cc: gregkh@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > devel@xxxxxxxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxx; Haiyang Zhang; > > Abhishek Kane (Mindtree Consulting PVT LTD) > > Subject: Re: [PATCH 12/25] Staging: hv: Cleanup error handling in > > vmbus_child_device_register() > > > > On Tue, Apr 26, 2011 at 09:20:29AM -0700, K. Y. Srinivasan wrote: > > > Cleanup error handling in vmbus_child_device_register(). > > > > > > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx> > > > Signed-off-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx> > > > Signed-off-by: Abhishek Kane <v-abkane@xxxxxxxxxxxxx> > > > Signed-off-by: Hank Janssen <hjanssen@xxxxxxxxxxxxx> > > > --- > > > drivers/staging/hv/vmbus_drv.c | 7 ++++++- > > > 1 files changed, 6 insertions(+), 1 deletions(-) > > > > > > diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c > > > index d597dd4..4d569ad 100644 > > > --- a/drivers/staging/hv/vmbus_drv.c > > > +++ b/drivers/staging/hv/vmbus_drv.c > > > @@ -720,11 +720,16 @@ int vmbus_child_device_register(struct hv_device > > *child_device_obj) > > > */ > > > ret = device_register(&child_device_obj->device); > > > > > > + if (ret) > > > + return ret; > > > + > > > /* vmbus_probe() error does not get propergate to device_register(). */ > > > ret = child_device_obj->probe_error; > > > > Wait, why not? Why is the probe_error have to be saved off like this? > > That seems like something is wrong here, this patch should not be > > needed. > > > > Well, you should check the return value of device_register, that is > > needed, but this seems broken somehow. > > The current code had comments claiming that the probe error was not > correctly propagated. Looking at the kernel side of the code, it was not clear > if device_register() could succeed while the probe might fail. Of course it can, device_register() has nothing to do with the probe callback of the device itself. To think otherwise is to not understand the driver model and assume things that you should never be caring about. Think about it, if you register a device, you don't know at that point in time if a driver is currently loaded for it, and that it will be bound to that device. Nor do you care, as any needed notifications for new drivers will be sent to userspace, and they will be loaded at some random time in the future. So a probe() call might never be called for this device until some other time, running on some other processor, in some other thread. Drivers are allowed to return errors from their probe functions for valid reasons (i.e. this driver shouldn't bind to this device for a variety of good reasons.) No one cares about this, as the driver core handles it properly and will pass on to the next driver in the list that might be able to be bound to this device. So why do you care about the return value of the probe() call? It gets properly handled already by the driver core, why would your bus ever care about it? (Hint, no other bus does, as it makes no sense.) > In any event, if you can guarantee that device_register() can return > any probe related errors, I agree with you that saving the probe error > is an overkill. The current code saved the probe error and with new > check I added with regards to the return value of device_register, > there is no correctness issue with this patch. As explained above, no, it will not return a probe error, as that makes no sense. If the code is wanting to rely on this, it is broken and must be fixed. thanks, greg k-h _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
