Re: [PATCH] usb/gadget/pch_udc: Fix ether gadget connect/disconnect issue

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 8106 bytes --]

Hi,

On Mon, Dec 26, 2011 at 01:04:43PM +0900, Tomoya MORINAGA wrote:
> ISSUE:
> After a USB cable is connect/disconnected, the system rarely freeze.
> CAUSE:
> Since the USB device controller cannot know to disconnect the USB cable, 
> when it is used without detecting VBUS by GPIO, the UDC driver does not 
> notify to USB Gadget.
> Since USB Gadget cannot know to disconnect, a false setting occurred 
> when the USB cable is connected/disconnect repeatedly.

you are doing much, much more then what's here. You really need to break
this patch up.

> Signed-off-by: Tomoya MORINAGA <tomoya.rohm@gmail.com>
> ---
>  drivers/usb/gadget/pch_udc.c |  115 ++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 105 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/gadget/pch_udc.c b/drivers/usb/gadget/pch_udc.c
> index 5048a0c..454028d 100644
> --- a/drivers/usb/gadget/pch_udc.c
> +++ b/drivers/usb/gadget/pch_udc.c
> @@ -311,6 +311,8 @@ struct pch_udc_ep {
>   * @registered:		driver regsitered with system
>   * @suspended:		driver in suspended state
>   * @connected:		gadget driver associated
> + * @pullup:		required pullup state
> + * @vbus_session:	required vbus_session state
>   * @set_cfg_not_acked:	pending acknowledgement 4 setup
>   * @waiting_zlp_ack:	pending acknowledgement 4 ZLP
>   * @data_requests:	DMA pool for data requests
> @@ -337,6 +339,8 @@ struct pch_udc_dev {
>  			registered:1,
>  			suspended:1,
>  			connected:1,
> +			pullup:1,
> +			vbus_session:1,
>  			set_cfg_not_acked:1,
>  			waiting_zlp_ack:1;
>  	struct pci_pool		*data_requests;
> @@ -554,6 +558,29 @@ static void pch_udc_clear_disconnect(struct pch_udc_dev *dev)
>  }
>  
>  /**
> + * pch_udc_reconnect() - This API initializes usb device controller,
> + *						and clear the disconnect status.
> + * @dev:		Reference to pch_udc_regs structure
> + */
> +static void pch_udc_init(struct pch_udc_dev *dev);
> +static void pch_udc_reconnect(struct pch_udc_dev *dev)
> +{
> +	pch_udc_init(dev);
> +
> +	/* enable device interrupts */
> +	/* pch_udc_enable_interrupts() */
> +	pch_udc_bit_clr(dev, UDC_DEVIRQMSK_ADDR,
> +			UDC_DEVINT_UR | UDC_DEVINT_ENUM);
> +
> +	/* Clear the disconnect */
> +	pch_udc_bit_set(dev, UDC_DEVCTL_ADDR, UDC_DEVCTL_RES);
> +	pch_udc_bit_clr(dev, UDC_DEVCTL_ADDR, UDC_DEVCTL_SD);
> +	mdelay(1);
> +	/* Resume USB signalling */
> +	pch_udc_bit_clr(dev, UDC_DEVCTL_ADDR, UDC_DEVCTL_RES);
> +}
> +
> +/**
>   * pch_udc_vbus_session() - set or clearr the disconnect status.
>   * @dev:	Reference to pch_udc_regs structure
>   * @is_active:	Parameter specifying the action
> @@ -563,10 +590,23 @@ static void pch_udc_clear_disconnect(struct pch_udc_dev *dev)
>  static inline void pch_udc_vbus_session(struct pch_udc_dev *dev,
>  					  int is_active)
>  {
> -	if (is_active)
> -		pch_udc_clear_disconnect(dev);
> -	else
> +	if (is_active) {
> +		if (dev->pullup == 1) {
> +			if (dev->vbus_session == 1)
> +				pch_udc_clear_disconnect(dev);
> +			else
> +				pch_udc_reconnect(dev);
> +		}
> +		dev->vbus_session = 1;
> +	} else {
> +		if (dev->driver && dev->driver->disconnect) {
> +			spin_unlock(&dev->lock);
> +			dev->driver->disconnect(&dev->gadget);
> +			spin_lock(&dev->lock);
> +		}
>  		pch_udc_set_disconnect(dev);
> +		dev->vbus_session = 0;
> +	}
>  }
>  
>  /**
> @@ -1126,7 +1166,23 @@ static int pch_udc_pcd_pullup(struct usb_gadget *gadget, int is_on)
>  	if (!gadget)
>  		return -EINVAL;
>  	dev = container_of(gadget, struct pch_udc_dev, gadget);
> -	pch_udc_vbus_session(dev, is_on);
> +	if (is_on) {
> +		if (dev->pullup == 1) {
> +			pch_udc_clear_disconnect(dev);
> +		} else {
> +			pch_udc_reconnect(dev);
> +			dev->pullup = 1;
> +		}
> +	} else {
> +		if (dev->driver && dev->driver->disconnect) {
> +			spin_unlock(&dev->lock);
> +			dev->driver->disconnect(&dev->gadget);
> +			spin_lock(&dev->lock);

this looks very wrong. ->pullup() should not call into the gadget
driver. Looking over your driver, it seems that one of the issues is
that you have a race between ->vbus_session() and ->pullup() because
they both call your pch_udc_vbus_session() without taking care of
locking those two.

> +		}
> +		pch_udc_set_disconnect(dev);
> +		dev->pullup = 0;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -2335,8 +2391,11 @@ static void pch_udc_svc_ur_interrupt(struct pch_udc_dev *dev)
>  		/* Complete request queue */
>  		empty_req_queue(ep);
>  	}
> -	if (dev->driver && dev->driver->disconnect)
> +	if (dev->driver && dev->driver->disconnect) {
> +		spin_unlock(&dev->lock);
>  		dev->driver->disconnect(&dev->gadget);
> +		spin_lock(&dev->lock);

this needs to be fixed, indeed. And maybe it's the only thing which fits
under $SUBJECT. Can you describe better the problem you're seeing ?
Maybe showing any WARN()/BUG() output which showed up ?

> @@ -2371,6 +2430,11 @@ static void pch_udc_svc_enum_interrupt(struct pch_udc_dev *dev)
>  	pch_udc_set_dma(dev, DMA_DIR_TX);
>  	pch_udc_set_dma(dev, DMA_DIR_RX);
>  	pch_udc_ep_set_rrdy(&(dev->ep[UDC_EP0OUT_IDX]));
> +
> +	/* enable device interrupts */
> +	pch_udc_enable_interrupts(dev, UDC_DEVINT_UR | UDC_DEVINT_US |
> +					UDC_DEVINT_ES | UDC_DEVINT_ENUM |
> +					UDC_DEVINT_SI | UDC_DEVINT_SC);

doesn't fit under $SUBJECT.

> @@ -2460,11 +2524,15 @@ static void pch_udc_svc_cfg_interrupt(struct pch_udc_dev *dev)
>  static void pch_udc_dev_isr(struct pch_udc_dev *dev, u32 dev_intr)
>  {
>  	/* USB Reset Interrupt */
> -	if (dev_intr & UDC_DEVINT_UR)
> +	if (dev_intr & UDC_DEVINT_UR) {
>  		pch_udc_svc_ur_interrupt(dev);
> +		dev_dbg(&dev->pdev->dev, "USB_RESET\n");

all these debugging message are _not_ part of $SUBJECT.

> @@ -2472,8 +2540,25 @@ static void pch_udc_dev_isr(struct pch_udc_dev *dev, u32 dev_intr)
>  	if (dev_intr & UDC_DEVINT_SC)
>  		pch_udc_svc_cfg_interrupt(dev);
>  	/* USB Suspend interrupt */
> -	if (dev_intr & UDC_DEVINT_US)
> +	if (dev_intr & UDC_DEVINT_US) {
> +		if (dev->gadget.speed != USB_SPEED_UNKNOWN

In fact, I would WARN() if your speed isn't known at this spot. This
would mean you have a big fat bug on your controller driver.

> +			&& dev->driver
> +			&& dev->driver->suspend) {
> +			spin_unlock(&dev->lock);
> +			dev->driver->suspend(&dev->gadget);
> +			spin_lock(&dev->lock);

Indeed, this is the right way, but not part of $SUBJECT.

> +		}
> +
> +		if ((dev->vbus_session == 0) && (dev->pullup == 1)) {
> +			if (dev->driver && dev->driver->disconnect) {
> +				spin_unlock(&dev->lock);
> +				dev->driver->disconnect(&dev->gadget);
> +				spin_lock(&dev->lock);

also correct, and looks part of $SUBJECT.

> @@ -2499,6 +2584,14 @@ static irqreturn_t pch_udc_isr(int irq, void *pdev)
>  	dev_intr = pch_udc_read_device_interrupts(dev);
>  	ep_intr = pch_udc_read_ep_interrupts(dev);
>  
> +	/* For a hot plug, this find that the controller is hung up. */
> +	if (dev_intr == ep_intr)
> +		if (dev_intr == pch_udc_readl(dev, UDC_DEVCFG_ADDR)) {
> +			dev_dbg(&dev->pdev->dev, "UDC: Hung up\n");
> +			/* The controller is reset */
> +			pch_udc_writel(dev, UDC_SRST, UDC_SRST_ADDR);
> +			return IRQ_HANDLED;
> +		}
>  	if (dev_intr)
>  		/* Clear device interrupts */
>  		pch_udc_write_device_interrupts(dev, dev_intr);
> @@ -2725,7 +2818,7 @@ static int pch_udc_start(struct usb_gadget_driver *driver,
>  	pch_udc_setup_ep0(dev);
>  
>  	/* clear SD */
> -	pch_udc_clear_disconnect(dev);
> +	pch_udc_pcd_pullup(&dev->gadget, 1);

this rings a bell, why do you need to change this ?

> @@ -2912,8 +3005,10 @@ static int pch_udc_probe(struct pci_dev *pdev,
>  	}
>  	pch_udc = dev;
>  	/* initialize the hardware */
> -	if (pch_udc_pcd_init(dev))
> +	if (pch_udc_pcd_init(dev)) {
> +		retval = -ENODEV;

not part of $SUBJECT but how about passing the return value from
pch_udc_pcd_init() to upper layer instead of changing it here ?

ret = pch_udc_pcd_init(dev);
if (ret) {
	....

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]