Re: [RFC PATCH 1/4] usb: add struct usb_hub_port to store port related members.

[Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>]

On Tue, 20 Mar 2012, Lan Tianyu wrote:

> On 2012年03月20日 00:04, Alan Stern wrote:
> > On Mon, 19 Mar 2012, Lan Tianyu wrote:
> >
> >> Add struct usb_hub_port pointer port_data in the struct usb_hub and allocate
> >> struct usb_hub_port perspectively for each ports to store private data.
> >
> > You might as well add the child device pointer into your new data
> > structure.  This will require changes to at least three other files in
> > addition to hub.c:
> hi alan:
> 	Great thanks for your review.
> 	But I still confuse about "You might as well add the child device pointer
> into your new data structure." Could you help me to make it clear? :)
> 	Do you mean add struct usb_device pointer toward the device attached to the 
> port in the
> struct usb_hub_port? If yes,Why?

Yes, that's what I mean.  It's a natural thing to do; the child device 
is directly associated with the port.  It's better than allocating a 
separate array for hdev->children.

> > 	devices.c, host/r8a66597-hcd.c,
> > 	drivers/staging/usbip/usbip_common.c
> >
> > Maybe some others; I didn't look through the entire kernel source.  It
> > also means you will have to export a function to get a pointer to the
> > child device, given the port number.
> 	If it was necessary, could I fill the child pointer in the 
> hub_port_connect_change()? just
> after a new usb device being created.
> 
> static void hub_port_connect_change(struct usb_hub *hub, int port1,
> 					u16 portstatus, u16 portchange) {
> 		...
> 		/* Run it through the hoops (find a driver, etc) */
> 		if (!status) {
> 			status = usb_new_device(udev);
> 			if (status) {
> 				spin_lock_irq(&device_state_lock);
> 				hdev->children[port1-1] = NULL;
> 				spin_unlock_irq(&device_state_lock);
> 			}
> 			/* like here?*/
> 		}
> 		...
> }

That's the right idea, but the wrong place.  The right place to fill 
in the pointer is a few lines higher, where the code already does

			hdev->children[port1-1] = udev;

while holding the device_state_lock.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html