Hi Mauro,
Thanks for your reply.
On Thu, 08 Mar 2012 08:21:20 -0300, Mauro Carvalho Chehab wrote:
> Em 12-02-2012 08:19, Jean Delvare escreveu:
> > Reported by kmemleak.
> >
> > Signed-off-by: Jean Delvare <khali@xxxxxxxxxxxx>
> > Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxx>
> > Cc: Devin Heitmueller <dheitmueller@xxxxxxxxxxxxxx>
> > ---
> > I am not familiar with the usb API, are we also supposed to call
> > usb_kill_urb() in the error case maybe?
> >
> > drivers/media/dvb/dvb-usb/dib0700_core.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > --- linux-3.3-rc3.orig/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-01-20 14:06:38.000000000 +0100
> > +++ linux-3.3-rc3/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-02-12 00:32:19.005334036 +0100
> > @@ -787,6 +787,8 @@ int dib0700_rc_setup(struct dvb_usb_devi
> > if (ret)
> > err("rc submit urb failed\n");
> >
> > + usb_free_urb(purb);
> > +
> > return ret;
> > }
>
> This patch doesn't sound right on my eyes, as you're freeing
> an URB that you've just submitted _before_ having it handled
> by the dib0700_rc_urb_completion() callback.
Oops, you're totally right. I don't know a thing about USB as you can
see :(
> Btw, it seems that there's a bug at the fist if there:
>
> static void dib0700_rc_urb_completion(struct urb *purb)
> {
> struct dvb_usb_device *d = purb->context;
> struct dib0700_rc_response *poll_reply;
> u32 uninitialized_var(keycode);
> u8 toggle;
>
> deb_info("%s()\n", __func__);
> if (d == NULL)
> return;
>
> if (d->rc_dev == NULL) {
> /* This will occur if disable_rc_polling=1 */
> usb_free_urb(purb);
> return;
> }
>
> ...
>
> it should be, instead:
>
> if (!d || !d->rc_dev) {
> /* This will occur if disable_rc_polling=1 */
> usb_free_urb(purb);
> return;
> }
"!d" can't actually happen, so it doesn't matter. d is passed by
dib0700_rc_setup() when calling usb_fill_bulk_urb(), and
dib0700_rc_setup() starts with dereferencing d, if it was NULL we'd
crash right away. Hence d is never NULL in dib0700_rc_urb_completion().
So this "if (d == NULL)" is just paranoia and might as well be removed.
> That's said, clearly there's no condition to stop the DVB IR
> handling.
Indeed, as I read the code, unless disable_rc_polling=1 or a fatal
error occurs, dib0700_rc_urb_completion will loop over and over
endlessly. I guess it's what "RC polling" is all about. No surprise why
my DVB-T card sucks so much power...
> Probably, the right thing to do there is to add a function like:
>
> int dib0700_disconnect(...)
> {
> usb_unlink_urb(urb);
> usb_free_urb(urb);
>
> dvb_usb_device_exit(...);
> }
>
> and use such function for the usb_driver disconnect handling:
>
> static struct usb_driver dib0700_driver = {
> .name = "dvb_usb_dib0700",
> .probe = dib0700_probe,
> .disconnect = dib0700_disconnect,
> .id_table = dib0700_usb_id_table,
> };
This would avoid a memory leak on module removal, right? Sure, we can
do that, but what surprises me is that I don't remember removing the
module when kmemleak reported the leak to me. Oh well, kmemleak is
pretty new, maybe that was a false positive after all.
But is it OK to free the same URB twice? Your code above does it
unconditionally, while it may have been freed already
(disable_rc_polling=1 or a fatal error occurred). As it seems that
usb_unlink_urb() will call the completion callback with an error
status, and dib0700_rc_urb_completion() will free the URB when that
happens, I suppose it is sufficient to call usb_unlink_urb() in the
diconnect function?
Thanks,
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
