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?
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.
An more question: If I make an iterator, different places have different
process way. That means to add an callback param?
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.
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_).
That's right. Thanks for reminder.
greg k-h
--
Best Regards
Tianyu Lan
linux kernel enabling team
--
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
