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 <tvbox...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
