Re: [PATCH 1/1] usb: dwc2: gadget: don't try to disable ep0 in dwc2_hsotg_suspend

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Minas,

On 12/14/21 7:22 AM, Minas Harutyunyan wrote:
Hi Amelie,

On 12/7/2021 5:01 PM, Amelie Delaunay wrote:
Calling dwc2_hsotg_ep_disable on ep0 (in/out) will lead to the following
logs before returning -EINVAL:
dwc2 49000000.usb-otg: dwc2_hsotg_ep_disable: called for ep0
dwc2 49000000.usb-otg: dwc2_hsotg_ep_disable: called for ep0


This messages printed for EP0 OUT which can't be disabled, but EP0 IN
can and should be disabled. Your patch exclude EP0 IN from disabling flow.


Thanks for the review. I wonder why then in dwc2_hsotg_udc_stop and dwc2_hsotg_core_init_disconnected EP0 IN is not disabled (loop starts from EP1) ?

Due to:
	/* Same dwc2_hsotg_ep is used in both directions for ep0 */
	hsotg->eps_out[0] = hsotg->eps_in[0];

the condition in dwc2_hsotg_ep_disable to display the error is always true for EP0, whatever the direction, so the log appears for EP0 IN & OUT:
	if (ep == &hsotg->eps_out[0]->ep) {
		dev_err(hsotg->dev, "%s: called for ep0\n", __func__);
		return -EINVAL;
	}

Should all loops need to be fixed to start loop from EP0 but exclude EP0 OUT from being disabled, so that EP0 IN can be disabled ? e.g. for dwc2_hsotg_suspend:
	

$ git diff drivers/usb/dwc2/gadget.c
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 11d85a6e0b0d..0c12219bfbf4 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -4231,7 +4231,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)

        dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);

-       if (ep == &hsotg->eps_out[0]->ep) {
+       if (ep == &hsotg->eps_out[0]->ep && !dir_in) {
                dev_err(hsotg->dev, "%s: called for ep0\n", __func__);
                return -EINVAL;
        }
@@ -5077,7 +5077,7 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
                for (ep = 0; ep < hsotg->num_of_eps; ep++) {
                        if (hsotg->eps_in[ep])

dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
-                       if (hsotg->eps_out[ep])
+                       if (ep > 0 && hsotg->eps_out[ep])

dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
                }
        }

Regards,
Amelie


Thanks,
Minas

To avoid these two logs while suspending, start disabling the endpoint
from the index 1, as done in dwc2_hsotg_udc_stop:

	/* all endpoints should be shutdown */
	for (ep = 1; ep < hsotg->num_of_eps; ep++) {
		if (hsotg->eps_in[ep])
			dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
		if (hsotg->eps_out[ep])
			dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
	}

Signed-off-by: Amelie Delaunay <amelie.delaunay@xxxxxxxxxxx>
---
   drivers/usb/dwc2/gadget.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index b884a83b26a6..ee31f9a328da 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -5086,7 +5086,7 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
   		hsotg->gadget.speed = USB_SPEED_UNKNOWN;
   		spin_unlock_irqrestore(&hsotg->lock, flags);
- for (ep = 0; ep < hsotg->num_of_eps; ep++) {
+		for (ep = 1; ep < hsotg->num_of_eps; ep++) {
   			if (hsotg->eps_in[ep])
   				dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
   			if (hsotg->eps_out[ep])




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux