On Mon, May 14, 2012 at 11:25:29PM +0800, Lan Tianyu wrote:
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
> drivers/usb/core/hub.c | 14 ++++++++++++++
> 1 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 6bf7124..6f909db 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -4236,6 +4236,20 @@ void usb_queue_reset_device(struct usb_interface *iface)
> }
> EXPORT_SYMBOL_GPL(usb_queue_reset_device);
>
> +/**
> + * usb_get_hub_child_device - Get the pointer of child device
> + * attached to the port which is specified by @port1.
> + *
> + * @hdev: USB device belonging to the usb hub
> + * @port1: port num to indicate which port the child device
> + * is attached to.
> + *
> + * USB drivers call this function to get hub's child device
> + * pointer.
> + *
> + * Return NULL if input param is invalid and
> + * child's usb_device pointer if non-NULL.
> + */
> struct usb_device *usb_get_hub_child_device(struct usb_device *hdev,
The more I look at this function, the worse I like it, here's why:
- you don't increment the reference count of the device, what is to
keep it from going away from when you find it and the caller uses
it?
- why all of the checking of the arguments, how, and who, would ever
mess up calling this function incorrectly?
- Whenever you are using this function, it's in a loop, which
indicates that what you really want is a function to iterate over
the child devices of a hub, right? So that is the function you need
to write, which will handle the proper locking and incrementing of
device reference counts.
So, care to just delete the function and do this correctly instead? As
it is, it seems very fragile.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
