Re: [PATCH 10/14] scsi: osd: fix device_register() error handling

[Greg KH]

On Mon, Sep 20, 2010 at 08:10:29AM -0700, Greg KH wrote:
> On Mon, Sep 20, 2010 at 06:58:17AM -0500, James Bottomley wrote:
> > On Sun, 2010-09-19 at 16:26 +0200, Dan Carpenter wrote:
> > > On Sun, Sep 19, 2010 at 04:55:07PM +0400, Vasiliy Kulikov wrote:
> > > > If device_register() fails then call put_device().
> > > > See comment to device_register.
> > > > 
> > > > Signed-off-by: Vasiliy Kulikov <segooon@xxxxxxxxx>
> > > > ---
> > > >  compile tested.
> > > > 
> > > >  drivers/scsi/osd/osd_uld.c |    4 +++-
> > > >  1 files changed, 3 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> > > > index cefb2c0..3e0edc2 100644
> > > > --- a/drivers/scsi/osd/osd_uld.c
> > > > +++ b/drivers/scsi/osd/osd_uld.c
> > > > @@ -474,7 +474,7 @@ static int osd_probe(struct device *dev)
> > > >  	error = device_register(&oud->class_dev);
> > > >  	if (error) {
> > > >  		OSD_ERR("device_register failed => %d\n", error);
> > > > -		goto err_put_cdev;
> > > > +		goto err_put_device;
> > > >  	}
> > > >  
> > > >  	get_device(&oud->class_dev);
> > > > @@ -482,6 +482,8 @@ static int osd_probe(struct device *dev)
> > > >  	OSD_INFO("osd_probe %s\n", disk->disk_name);
> > > >  	return 0;
> > > >  
> > > 
> > > Hm...  So if device_register() fails then we should always call
> > > device_put()?  It seems like a lot of existing code does that but I
> > > hadn't realized until now that that is how it works.
> > 
> > Heh, it wasn't a bug when most of the code was written.  It became a bug
> > when dev_set_name() was added because now the storage allocated for the
> > name has to be freed with a put.  Previous to this, the advice was just
> > to free the device if device_register() failed.

That was a long time ago.  When the driver core changed, all callers
were audited from what I recall.

> > > Why can't the device_put() just be added inside the device_register() so
> > > the unwinding works automatically?
> > 
> > Since Greg and Kay didn't actually alter any of the device_register()
> > failure paths, this does sound to be the better course of action ... of
> > course, every device_register() introduced after the dev_set_name()
> > change may call put_device() on the cleanup path ... someone needs to
> > check.
> 
> Yes, this patch series should not be needed at all.  If there's a
> problem with the driver core here, it should be fixed, not forcing the
> issue to all of the individual callers.

Nope, I'm wrong, sorry, this is correct.  We can't just free the device
ourselves in the driver core because other parts that the device might
be embedded in need to be cleaned up before it can go away.

So this patch series is correct, thanks.

greg k-h
--
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/