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

[Lan Tianyu <tianyu.lan@xxxxxxxxx>]

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?
> 	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?*/
		}
		...
}

Thanks again. Look forward for your reply.
> > @@ -1451,13 +1456,12 @@ void usb_hub_release_all_ports(struct usb_device *hdev, void *owner)
> >   	int n;
> >   	void **powner;
> >
> > -	n = find_port_owner(hdev, 1,&powner);
> > -	if (n == 0) {
> > -		for (; n<  hdev->maxchild; (++n, ++powner)) {
> > -			if (*powner == owner)
> > -				*powner = NULL;
> > -		}
> > +	for (n = 1; n<= hdev->maxchild; n++) {
> > +		find_port_owner(hdev, n,&powner);
> > +		if (*powner == owner)
> > +			*powner = NULL;
> >   	}
>
> This is wrong; find_port_owner() doesn't set powner to anything if an
> error occurs.
>
> You can just set the pointer field directly to NULL, instead of looking
> it up in find_port_owner().  I don't remember why it was written that
> way originally.
>
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html