| Message ID | 1305583637.2481.3.camel@localhost (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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. 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. 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 ;) Patrick, Any comments? Thanks, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; diff --git a/drivers/media/dvb/dvb-usb/dvb-usb.h b/drivers/media/dvb/dvb-usb/dvb-usb.h index 76a8096..044c906 100644 --- a/drivers/media/dvb/dvb-usb/dvb-usb.h +++ b/drivers/media/dvb/dvb-usb/dvb-usb.h @@ -263,6 +263,7 @@ struct dvb_usb_device_properties { int no_reconnect; int size_of_priv; + int (*priv_exit) (struct dvb_usb_device *); int num_adapters; struct dvb_usb_adapter_properties adapter[MAX_NO_OF_ADAPTER_PER_DEVICE];
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(-)