Re: USB Storage Device Not Working

On Mon, Nov 12, 2007 at 03:29:57PM -0500, Alan Stern wrote:
> On Mon, 12 Nov 2007, balajirrao wrote:
> 
Thank you for replying.
> > > 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.
> 
Oh.. I thought that may be it should go to hc_driver. But because you
told me, i put it in usb_hcd itself. I shall put it in hc_driver itself.
> > 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.
Right. But not with this patch applied, as the method to force a device
to work under companion is not the way the kernel does it for  full
speed devices plugged into hi speed ports.

I think i need to change the way the device is handed over to the
companion. It is implemented in a wrong manner in this patch. What it
does is, it sets the bit in companion_ports, and the port number is
shown up in the companion list. And this is the cause for the problem
mentioned above.

> 
> > 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.
> 
Right. I understand it now.
> > 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.
> 
I think the hardware takes of this if the device wont work under high
speed. 
> 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.
>
But suppose, a full speed device is plugged into a external hub. Wont
ehci_hcd then hand it over to the companion controller ? 
Ok. Then I need to find out how to indentify a root hub.
> > +}
> > +
> >  /* 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.
> 
Sorry. This was not intended.
> >  
> >  	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.
> 
OK. I shall do it.
> >  	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.
> 
Sorry, by mistake.
> >  #	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
> 
thank you,

regards,
balaji rao

-------------------------------------------------------------------------
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