On Wed, May 16, 2012 at 10:56:44AM +0800, Lan Tianyu wrote:
> On 2012年05月15日 23:27, Greg KH wrote:
> >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:
> >>>>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?
> Ok. I will remove them.
>
> >>>> - 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?
> Sorry. That's not really I want. If make it an iterator, how to get one hub's child?
I don't know, but that's not how you were using this function, so how
was I supposed to know your future use of it :)
All of the callers of this code was using it to walk all devices
attached to a hub, do you want to do something else instead? If so,
then please show that code (hint, we can't read your mind, and only
accept code for what is needed right now, not what might be needed in
the future by someone else.)
> As you know, I will also work on power off mechanism. In some places, I need
> to get one port's child or check whether there is a device on the port. So
> an iterator doesn't work.
Ok, that would be good to have known, but are you really going to make
this same call to get that information? Aren't you already be getting
the callback into the struct device of the hub port and you will already
have this information at that point in time, not needing this function
call at all? (see what I mean about trying to plan ahead, it doesn't
always work...)
> An more question: If I make an iterator, different places have different
> process way. That means to add an callback param?
Of course.
> usb_hub_children_iterator(hdev, callback)
> {
> struct usb_hub *hub = hdev_to_hub(hdev);
> struct usb_device *child;
> int port1;
>
> for(port1 = 1; port1 < hdev->maxchild; port1++) {
> child = usb_get_dev(hub->port_data[port1 - 1].child);
>
> if (child) {
> usb_lock_device(child);
> callback(child);
> usb_unlock_device(child);
> usb_put_dev(child);
> }
> }
> }
> right?
>
> Just from my opinion, get one child maybe more flexible.
Yes, it might be, but do you need it to be?
Revisit this after you rework how you are creating and storing those
ports, then I think you will know what you need to do.
good luck,
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
