Re: USB Storage Device Not Working

On Mon, 12 Nov 2007, balajirrao wrote:

> > You will have to add a new method pointer to struct usb_hcd, maybe call

I made a mistake here.  The new method pointer should be added to 
struct hc_driver, not struct usb_hcd.  In other words, it should be 
stored along with all the other method pointers.

> > it "relinquish".  In ehci-hub.c make the method routine check that its
> > argument really is attached to a port on the root hub and hand that
> > port over to the companion controller.  Then in hub.c if
> > hub_port_connect_change() fails, make it invoke this new method.
> > 
> I have done what you have told with a few additional changes. This is my
> first real patch. So, please tell me where i am wrong. I just made it
> work. Dont know whether its the right way to do it. Please tell.
> 
> Also, there is one more problem. When the device which is forced to work
> by the kernel to work under companion is removed, the port should be
> removed from the companion list. Otherwise, when another high speed
> device is plugged into the same port, it would work in full speed.

This happens automatically.  For example, if you plug in a full-speed 
device it gets handed to the companion; when the device is unplugged 
and you plug a high-speed device into the same port, the new device is 
not forced to run at full speed.

> So, my idea is to have a variable called forced_companion_ports, and a
> method called un_relinqush, which takes care of removing the forced from
> the companion list.

You don't need this.  And you don't need to add anything to the 
companion list either.  Just hand the port to the companion and leave 
the list alone.

> But i didnt know from where to call this function.
> I tried calling it in the hub_port_connect_change function, but i got a
> OOPS in which khubd was killed. So if i could know which port the
> disconnected device was connected to, it would be helpful i guess.
> 
> The patch is inlined below. Please comment. (Please tell where i am
> wrong. )
> 
> Index: linux-2.6.24-rc2-usb/drivers/usb/host/ehci-hcd.c
> ===================================================================
> --- linux-2.6.24-rc2-usb.orig/drivers/usb/host/ehci-hcd.c
> +++ linux-2.6.24-rc2-usb/drivers/usb/host/ehci-hcd.c
> @@ -433,6 +433,13 @@ static void ehci_stop (struct usb_hcd *h
>  		    ehci_readl(ehci, &ehci->regs->status));
>  }
>  
> +static void relinquish_port (struct usb_hcd *hcd, int port_num) {
> +	struct ehci_hcd *ehci;
> +
> +	ehci = hcd_to_ehci(hcd);
> + 	store_companion(ehci, port_num, PORT_OWNER);	

Don't steal an existing name for your new routine.  Make up a new name
for it, like set_port_companion().

Remember also that not all EHCI controllers have companions.  Some have
built-in Transaction Translators; I don't know how to force such
controllers to set a port to full speed (maybe it can't be done).  See
how the ehci_is_TDI() test is used.  Your code must work with both
sorts of controllers.

And don't forget that none of this should even be attempted if the port 
doesn't belong to the root hub.  There's no way to tell an external hub 
to force a port to full speed.  Your routine needs to test for this.

> +}
> +
>  /* one-time init, only for memory state */
>  static int ehci_init(struct usb_hcd *hcd)
>  {
> @@ -443,6 +450,8 @@ static int ehci_init(struct usb_hcd *hcd
>  
>  	spin_lock_init(&ehci->lock);
>  
> +	hcd->relinquish_port = relinquish_port;
> +

This field should be part of the hc_driver structure, not the usb_hcd
structure.  It should be initialized statically in ehci-pci.c, along
with the rest of ehci_pci_hc_driver.

>  	init_timer(&ehci->watchdog);
>  	ehci->watchdog.function = ehci_watchdog;
>  	ehci->watchdog.data = (unsigned long) ehci;
> Index: linux-2.6.24-rc2-usb/drivers/usb/host/ehci-hub.c
> ===================================================================
> --- linux-2.6.24-rc2-usb.orig/drivers/usb/host/ehci-hub.c
> +++ linux-2.6.24-rc2-usb/drivers/usb/host/ehci-hub.c
> @@ -316,24 +316,12 @@ static ssize_t show_companion(struct cla
>   * Syntax is "[-]portnum", where a leading '-' sign means
>   * return control of the port to the EHCI controller.
>   */
> -static ssize_t store_companion(struct class_device *class_dev,
> -		const char *buf, size_t count)
> -{
> -	struct ehci_hcd		*ehci;
> -	int			portnum, new_owner, try;
> +void store_companion(struct ehci_hcd *ehci, int portnum, int new_owner)

Change the new routine's name.

> +{			
>  	u32 __iomem		*status_reg;
>  	u32			port_status;
> -
> -	ehci = hcd_to_ehci(bus_to_hcd(class_get_devdata(class_dev)));
> -	new_owner = PORT_OWNER;		/* Owned by companion */
> -	if (sscanf(buf, "%d", &portnum) != 1)
> -		return -EINVAL;
> -	if (portnum < 0) {
> -		portnum = - portnum;
> -		new_owner = 0;		/* Owned by EHCI */
> -	}
> -	if (portnum <= 0 || portnum > HCS_N_PORTS(ehci->hcs_params))
> -		return -ENOENT;
> +	int			try;
> +	
>  	status_reg = &ehci->regs->port_status[--portnum];
>  	if (new_owner)
>  		set_bit(portnum, &ehci->companion_ports);
> @@ -341,10 +329,10 @@ static ssize_t store_companion(struct cl
>  		clear_bit(portnum, &ehci->companion_ports);
>  
>  	/*
> -	 * The controller won't set the OWNER bit if the port is
> -	 * enabled, so this loop will sometimes require at least two
> -	 * iterations: one to disable the port and one to set OWNER.
> -	 */
> +	* The controller won't set the OWNER bit if the port is
> +	* enabled, so this loop will sometimes require at least two
> +	* iterations: one to disable the port and one to set OWNER.
> +	*/

Don't change the spacing of existing comments, especially if the old 
spacing was correct and your new spacing is wrong.

>  
>  	for (try = 4; try > 0; --try) {
>  		spin_lock_irq(&ehci->lock);
> @@ -361,10 +349,29 @@ static ssize_t store_companion(struct cl
>  		spin_unlock_irq(&ehci->lock);
>  		if (try > 1)
>  			msleep(5);
> +	}	
> +}
> +
> +static ssize_t sysfs_store_companion(struct class_device *class_dev,
> +		const char *buf, size_t count) {

Don't change the names of existing routines.

> +	
> +	struct ehci_hcd		*ehci;
> +	int			portnum, new_owner;
> +		
> +	ehci = hcd_to_ehci(bus_to_hcd(class_get_devdata(class_dev)));
> +	new_owner = PORT_OWNER;		/* Owned by companion */
> +	if (sscanf(buf, "%d", &portnum) != 1)
> +		return -EINVAL;
> +	if (portnum < 0) {
> +		portnum = - portnum;
> +		new_owner = 0;		/* Owned by EHCI */
>  	}
> +	if (portnum <= 0 || portnum > HCS_N_PORTS(ehci->hcs_params))
> +		return -ENOENT;
> +	store_companion(ehci, portnum, new_owner);
>  	return count;
>  }
> -static CLASS_DEVICE_ATTR(companion, 0644, show_companion, store_companion);
> +static CLASS_DEVICE_ATTR(companion, 0644, show_companion, sysfs_store_companion);
>  
>  static inline void create_companion_file(struct ehci_hcd *ehci)
>  {
> Index: linux-2.6.24-rc2-usb/drivers/usb/core/hcd.h
> ===================================================================
> --- linux-2.6.24-rc2-usb.orig/drivers/usb/core/hcd.h
> +++ linux-2.6.24-rc2-usb/drivers/usb/core/hcd.h
> @@ -90,6 +90,8 @@ struct usb_hcd {
>  #define HCD_FLAG_HW_ACCESSIBLE	0x00000001
>  #define HCD_FLAG_SAW_IRQ	0x00000002
>  
> +	void 		(*relinquish_port)(struct usb_hcd *, int port_num);
> +

Put this in struct hc_driver, not struct usb_hcd.  And add a comment 
explaining what it's supposed to do.

>  	unsigned		rh_registered:1;/* is root hub registered? */
>  
>  	/* The next flag is a stopgap, to be removed when all the HCDs
> @@ -110,6 +112,7 @@ struct usb_hcd {
>  	struct dma_pool		*pool [HCD_BUFFER_POOLS];
>  
>  	int			state;
> +	

Don't add extra blank lines to existing code.

>  #	define	__ACTIVE		0x01
>  #	define	__SUSPEND		0x04
>  #	define	__TRANSIENT		0x80
> Index: linux-2.6.24-rc2-usb/drivers/usb/core/hub.c
> ===================================================================
> --- linux-2.6.24-rc2-usb.orig/drivers/usb/core/hub.c
> +++ linux-2.6.24-rc2-usb/drivers/usb/core/hub.c
> @@ -2484,6 +2484,7 @@ static void hub_port_connect_change(stru
>  	struct device *hub_dev = hub->intfdev;
>  	u16 wHubCharacteristics = le16_to_cpu(hub->descriptor->wHubCharacteristics);
>  	int status, i;
> +	struct usb_hcd *hcd;
>   
>  	dev_dbg (hub_dev,
>  		"port %d, status %04x, change %04x, %s\n",
> @@ -2644,6 +2645,9 @@ loop:
>  	}
>   
>  done:
> +	hcd = bus_to_hcd(hub->hdev->bus);
> +	if (hcd->relinquish_port)
> +		hcd->relinquish_port(hcd, port1);
>  	hub_port_disable(hub, port1, 1);
>  }

This should be hcd->driver->relinquish_port, not hcd->relinquish_port.

Alan Stern


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel