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

[Lan Tianyu <tianyu.lan@intel.com>]

On 2012年03月20日 22:04, Alan Stern wrote:
> 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.
OK. That' mean that I should replace the hdev->children with 
port_data->child_device.
In the hub.c, the struct usb_hub_port can be visited and the child_device can be 
accessed
directly. Out of hub.c, I should provide function to return child pointer. Is 
that right?
>
>>> 	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
>
>

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