On Tue, May 15, 2012 at 04:19:43PM +0800, Lan Tianyu wrote:
> On 2012年05月15日 00:23, Greg KH wrote:
> >On Mon, May 14, 2012 at 09:18:38AM -0700, Greg KH wrote:
> >>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?
> Do you mean the check is redundant?
>
> + if (!hub || port1 > hdev->maxchild || port1 < 1)
> + return NULL;
Yes, in the callers, who ever got any of those parameters wrong?
> >> - 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.
> >>
> ok. How about this?
> +
> +struct usb_device *usb_get_hub_child_device(struct usb_device *hdev,
> + int port1)
> +{
> + struct usb_hub *hub = hdev_to_hub(hdev);
> +
> + if (!hub || port1 > hdev->maxchild || port1 < 1)
> + return NULL;
> + get_device(hub->port_data[port1 - 1].child->dev]);
> + return hub->port_data[port1 - 1].child;
> +}
> +EXPORT_SYMBOL_GPL(usb_get_hub_child_device);
Ick, no, again, make it an iterator, as that's what you really want,
right?
And even if the function was the correct way, your "fix" was messy, use
the correct get/put functions (hint, one that starts with usb_).
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
