Re: [PATCH ] dvb-usb provide exit for any structure inside priv.

[Malcolm Priestley <tvboxspy@gmail.com>]

On Sat, 2011-05-21 at 10:14 -0300, Mauro Carvalho Chehab wrote: 
> Em 16-05-2011 19:07, Malcolm Priestley escreveu:
> > Currently priv is freed from memory by dvb-usb on any error or exit.
> >  If any buffer has been allocated in the priv structure,
> >  freeing it is tricky.  While freeing it on device disconnect
> >  is fairly easy, on error it is almost impossible because it
> >  has been removed from memory by dvb-usb.
> > 
> > This patch provides an exit from the priv.
> > 
> > Signed-off-by: Malcolm Priestley <tvboxspy@gmail.com>
> > ---
> >  drivers/media/dvb/dvb-usb/dvb-usb-init.c |    2 ++
> >  drivers/media/dvb/dvb-usb/dvb-usb.h      |    1 +
> >  2 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-init.c b/drivers/media/dvb/dvb-usb/dvb-usb-init.c
> > index 2e3ea0f..217b948 100644
> > --- a/drivers/media/dvb/dvb-usb/dvb-usb-init.c
> > +++ b/drivers/media/dvb/dvb-usb/dvb-usb-init.c
> > @@ -118,6 +118,8 @@ static int dvb_usb_exit(struct dvb_usb_device *d)
> >  	dvb_usb_i2c_exit(d);
> >  	deb_info("state should be zero now: %x\n", d->state);
> >  	d->state = DVB_USB_STATE_INIT;
> > +	if (d->props.priv_exit)
> > +		d->props.priv_exit(d);
> >  	kfree(d->priv);
> >  	kfree(d);
> >  	return 0;
> 
> Hi Malcolm,
> 
> I had to read this patch a few times in order to better understand what "priv_exit"
> actually means, and to look into your next changeset. IMO, it is bad named. 
> 
> It is even more confused if we take a look on your next patch, where you're adding a
> "priv_exit" callback like:
> 
> static int lme2510_priv_exit(struct dvb_usb_device *d)
> +{
> +	struct lme2510_state *st = d->priv;
> +
> +	if (st->usb_buffer != NULL) {
> +		st->i2c_talk_onoff = 1;
> +		st->signal_lock = 0;
> +		st->signal_level = 0;
> +		st->signal_sn = 0;
> +		kfree(st->usb_buffer);
> +	}
> +
> +	if (st->lme_urb != NULL) {
> +		usb_kill_urb(st->lme_urb);
> +		usb_free_coherent(d->udev, 5000, st->buffer,
> +				  st->lme_urb->transfer_dma);
> +		info("Interrupt Service Stopped");
> +		rc_unregister_device(d->rc_dev);
> +		info("Remote Stopped");
> +	}
> +
> +	return 0;
> +}
> 
> At the above code, you're not doing anything to release the "priv", but, instead,
> all you're doing is to stop the pending USB operations and un-registering the
> remote controller.

Naming of this is difficult, perhaps priv_pre_disconnect or
priv_pre_exit?

> The complete dvb_usb_exit() is:
> 
> static int dvb_usb_exit(struct dvb_usb_device *d)
> {
> 	deb_info("state before exiting everything: %x\n", d->state);
> 	dvb_usb_remote_exit(d);
> 	dvb_usb_adapter_exit(d);
> 	dvb_usb_i2c_exit(d);
> 	deb_info("state should be zero now: %x\n", d->state);
> 	d->state = DVB_USB_STATE_INIT;
> 	kfree(d->priv);
> 	kfree(d);
> 	return 0;
> }
> 
> The remote controller is already unregistered at dvb_usb_remote_exit():
> 
> int dvb_usb_remote_exit(struct dvb_usb_device *d)
> {
> 	if (d->state & DVB_USB_STATE_REMOTE) {
> 		cancel_delayed_work_sync(&d->rc_query_work);
> 		if (d->props.rc.mode == DVB_RC_LEGACY)
> 			input_unregister_device(d->input_dev);
> 		else
> 			rc_unregister_device(d->rc_dev);
> 	}
> 	d->state &= ~DVB_USB_STATE_REMOTE;
> 	return 0;
> }
> 
> So, the rc_unregister_device() above is wrong.
> 

Currently, the remote control is integrated into the main body of the
driver and not in dvb-usb-remote.

The current connection path of remote is;
Start Remote -> Start Interrupt ...... Stop Interrupt -> Stop Remote.

The patch to move to dvb-usb-remote is nearly complete.

> I agree that stopping the current undergoing transfers is needed. Yet, as there is a
> "usb_urb_kill" function defined, I suspect that other dvb-usb drivers use a different
> logic to stop URB transfers. If not, then the same bug is present also on the other
> drivers ;)
> 

Namely, any driver who is not disconnecting calling dvb_usb_device_exit
directly.

Regards


Malcolm