Messages in this thread | | | Date | Mon, 6 Nov 2006 18:49:49 -0800 | From | Andrew Morton <> | Subject | Re: [PATCH 2.6.19-rc4] usb urb unlink/free clenup |
| |
On Mon, 6 Nov 2006 22:28:37 +0100 Mariusz Kozlowski <m.kozlowski@tuxland.pl> wrote:
> Hello, > > > In many places usb_(unlink,kill,free)_urb() are called this way: > > if (urb) > usb_something_urb(...); > > which is not needed because functions like usb_unlink_urb() and usb_free_urb() are defined this way: > > void usb_free_urb(struct urb *urb) > { > if (urb) > kref_put(&urb->kref, urb_destroy); > } > > int usb_unlink_urb(struct urb *urb) > { > if (!urb) > return -EINVAL; > ... > } > > We do not need to check for urb != NULL before we call them.
Seems reasonable.
Your patch had all its tabs replaced with spaces by your email client. I fixed that all up, but it was rather dull work and I'd prefer not to have to do it again.
It is also possible to do similar cleanup > for usb_kill_urb(). The thing is it does urb check at the begining but right before is > might_sleep(): > > void usb_kill_urb(struct urb *urb) > { > might_sleep(); > if (!(urb && urb->dev && urb->dev->bus)) > return; > ... > > which confuses me. I would like to know what to do about it. Can this be rewritten this way?: > > void usb_kill_urb(struct urb *urb) > { > if (!urb) > return; > might_sleep(); > if (!urb->dev || !urb->dev->bus)) > return; > ... > > Or maybe there is no need for this? >
I think it's OK as-is. Presumably it's rare for a caller to pass in a NULL urb.
It's possible that your proposed change will cause new (and incorrect) warnings to be emitted, but we can handle those if/when they come out.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |