Re: [PATCH v3] em28xx: Only deallocate struct em28xx after finishing all extensions

["Frank Schäfer" <fschaefer.oss@googlemail.com>]

Am 07.03.2014 18:38, schrieb Mauro Carvalho Chehab:
>>> static int em28xx_usb_suspend(struct usb_interface *interface,
>>> > > diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
>>> > > index d4986bdfbdc3..6dbc71ba2820 100644
>>> > > --- a/drivers/media/usb/em28xx/em28xx-dvb.c
>>> > > +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
>>> > > @@ -1043,7 +1043,6 @@ static int em28xx_dvb_init(struct em28xx *dev)
>>> > >  	em28xx_info("Binding DVB extension\n");
>>> > >  
>>> > >  	dvb = kzalloc(sizeof(struct em28xx_dvb), GFP_KERNEL);
>>> > > -
>>> > >  	if (dvb == NULL) {
>>> > >  		em28xx_info("em28xx_dvb: memory allocation failed\n");
>>> > >  		return -ENOMEM;
>>> > > @@ -1521,6 +1520,9 @@ static int em28xx_dvb_init(struct em28xx *dev)
>>> > >  	dvb->adapter.mfe_shared = mfe_shared;
>>> > >  
>>> > >  	em28xx_info("DVB extension successfully initialized\n");
>>> > > +
>>> > > +	kref_get(&dev->ref);
>>> > > +
>> > 
>> > The fini() functions are always called, even if an error occured in init().
>> > So (in opposition to the open()/close() functions) kref_get() needs to
>> > be called at the beginning of the init() methods.
>> > 
>> > "dev->is_audio_only" and "!dev->board.has_dvb" is checked in both
>> > functions (init+fini), so the right place here is one line before or after
>> > 
>> >     em28xx_info("Binding DVB extension\n");
>> > 
>> > 
>> > Everything else looks good.
> I actually prefer to fix it the other way, at the code for kref_put()...
> see below
...
>> > 
>> > Regards,
>> > Frank
>> > 
>>> > >  ret:
>>> > >  	em28xx_set_mode(dev, EM28XX_SUSPEND);
>>> > >  	mutex_unlock(&dev->lock);
>>> > > @@ -1579,6 +1581,8 @@ static int em28xx_dvb_fini(struct em28xx *dev)
>>> > >  		dev->dvb = NULL;
> Putting the kref_put() here. This part of the code is only called if
> dev->dvb it not NULL, and this is only possible to happen if the
> DVB is properly initialized.
Yes, that works, too.

Regards,
Frank

>
>>> > >  	}
>>> > >  
>>> > > +	kref_put(&dev->ref, em28xx_free_device);
>>> > > +
>>> > >  	return 0;
>>> > >  }