RE: [PATCH 12/25] Staging: hv: Cleanup error handling invmbus_child_device_register()

[KY Srinivasan]

> -----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. 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.
> 
> >
> > -	if (ret)
> > +	if (ret) {
> >  		pr_err("Unable to register child device\n");
> > +		device_unregister(&child_device_obj->device);
> > +	}
> >  	else
> 
> 	} else
> is the preferred way.
> 
> Care to send a fixup patch to remove the probe_error field and fix this
> formatting error up?

I will fix up the formatting issue.

Regards,

K. Y
--
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/