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

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

[Tomoya MORINAGA]

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.

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);
+		}
+		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);
+	}
 }
 
 /**
@@ -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);
 }
 
 /**
@@ -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");
+	}
 	/* Enumeration Done Interrupt */
-	if (dev_intr & UDC_DEVINT_ENUM)
+	if (dev_intr & UDC_DEVINT_ENUM) {
 		pch_udc_svc_enum_interrupt(dev);
+		dev_dbg(&dev->pdev->dev, "USB_ENUM\n");
+	}
 	/* Set Interface Interrupt */
 	if (dev_intr & UDC_DEVINT_SI)
 		pch_udc_svc_intf_interrupt(dev);
@@ -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
+			&& dev->driver
+			&& dev->driver->suspend) {
+			spin_unlock(&dev->lock);
+			dev->driver->suspend(&dev->gadget);
+			spin_lock(&dev->lock);
+		}
+
+		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);
+			}
+			pch_udc_reconnect(dev);
+		}
 		dev_dbg(&dev->pdev->dev, "USB_SUSPEND\n");
+	}
 	/* Clear the SOF interrupt, if enabled */
 	if (dev_intr & UDC_DEVINT_SOF)
 		dev_dbg(&dev->pdev->dev, "SOF\n");
@@ -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);
 
 	dev->connected = 1;
 	return 0;
@@ -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;
 		goto finished;
+	}
 	if (request_irq(pdev->irq, pch_udc_isr, IRQF_SHARED, KBUILD_MODNAME,
 			dev)) {
 		dev_err(&pdev->dev, "%s: request_irq(%d) fail\n", __func__,
-- 
1.7.4.4

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

[Felipe Balbi]

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

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

[Tomoya MORINAGA]

Hi, Balbi

Please see my comment inline.
BTW, I've divided previous patch into six patches.
After this mail, I'll post the patch series soon.

2012/1/9 Felipe Balbi <balbi@ti.com>:
> > @@ -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.

I was not careful about those two conflict. I make the condition simple.
Please see my new patch, and please give me your advice.
Please refer to new 4th patch.

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

After a USB cable is connect/disconnected, the system rarely freezes.
I do not have conclusive evidence that this is a root cause.
It was pointed out that this should call spin_unlock() and spin_lock()
by one user.
Please refer to new 1st patch.

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

This is separated from this $SUBJECT.
Please refer to new 5th patch.

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

This is separated from this $SUBJECT.
Please refer to new 6th patch.

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

It was not necessary to check speed in this spot.
gadget.speed = USB_SPEED_UNKNOWN is removed.
Please refer to new 3rd patch.


> > +                     && 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.

This is separated from this $SUBJECT.
Please refer to new 3rd patch.

> > +             }
> > +
> > +             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.

Please refer to new 4th patch.

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

I wanted to set as 1 to dev->pullup.
However, this is not needed anymore.


> > @@ -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) {
>        ....
>

This is separated from this $ SUBJECT.
Your description is better.
However, I followed a similar description in this function.
Please refer to new 2nd patch.

thanks,
tomoya