-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Wednesday, April 27, 2011 8:26 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang;
Abhishek Kane (Mindtree Consulting PVT LTD)
Subject: Re: [PATCH 12/25] Staging: hv: Cleanup error handling in
vmbus_child_device_register()
On Wed, Apr 27, 2011 at 02:11:48AM +, KY Srinivasan wrote:
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Tuesday, April 26, 2011 6:51 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; 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 k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Abhishek Kane v-abk...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
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.
I did not say that device_register would return the probe_error. On the
contrary, the code I added explicitly ensures that proper cleanup is done
for the failure of device_register and if the probe function were
called on the same context executing the device_register call,
the failure of the probe call as well (looking at the stack trace while in
the probe function, this appeared to be the case currently).
If you look at the existing code; the current code did not deal with the
failure of
device_register() - it over-writes the return value of device_register with
that of the probe error. This patch fixed that problem.
The current code dealt with the probe error by spinning up a work item to
deal with the probe failure. This work item would try to unregister the device
that was not fully registered and this caused a problem. I tried
