Messages in this thread | | | Date | Mon, 19 Jun 2023 18:57:47 +0530 | Subject | Re: [PATCH v3] usb: dwc3: gadget: Propagate core init errors to UDC during pullup | From | Krishna Kurapati PSSNV <> |
| |
On 6/19/2023 6:20 PM, Krishna Kurapati PSSNV wrote: > > > On 6/19/2023 12:36 PM, Johan Hovold wrote: >> On Sun, Jun 18, 2023 at 05:39:49PM +0530, Krishna Kurapati wrote: >>> In scenarios where pullup relies on resume (get sync) to initialize >>> the controller and set the run stop bit, then core_init is followed by >>> gadget_resume which will eventually set run stop bit. >>> >>> But in cases where the core_init fails, the return value is not sent >>> back to udc appropriately. So according to UDC the controller has >>> started but in reality we never set the run stop bit. >>> >>> On systems like Android, there are uevents sent to HAL depending on >>> whether the configfs_bind / configfs_disconnect were invoked. In the >>> above mentioned scnenario, if the core init fails, the run stop won't >>> be set and the cable plug-out won't result in generation of any >>> disconnect event and userspace would never get any uevent regarding >>> cable plug out and we never call pullup(0) again. Furthermore none of >>> the next Plug-In/Plug-Out's would be known to configfs. >>> >>> Return back the appropriate result to UDC to let the userspace/ >>> configfs know that the pullup failed so they can take appropriate >>> action. >>> >>> Fixes: 77adb8bdf422 ("usb: dwc3: gadget: Allow runtime suspend if UDC >>> unbinded") >>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>> Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>> --- >>> Changes in v3: Added changelog mising in v2 >>> Changes in v2: Added Fixes tag >>> >>> drivers/usb/dwc3/gadget.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index 578804dc29ca..27cb671e18e3 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -2747,7 +2747,9 @@ static int dwc3_gadget_pullup(struct usb_gadget >>> *g, int is_on) >>> ret = pm_runtime_get_sync(dwc->dev); >>> if (!ret || ret < 0) { >>> pm_runtime_put(dwc->dev); >>> - return 0; >>> + if (ret < 0) >>> + pm_runtime_set_suspended(dwc->dev); >> >> This bit is broken and is also not mentioned or explained in the commit >> message. What are you trying to achieve here? >> >> You cannot set the state like this after runtime PM is enabled and the >> above call will always fail. >> > Hi Johan, > > Apologies, I didn't understand the comment fully. > > The reason why I an returning ret is because, when the first get_sync > fails because of core_init failure and we return 0 instead of ret, the > UDC thinks that controller has started successfully but we never set the > run stop bit. So when we plug out the cable, the disconnect event won't > be generated and we never send on systems like android the user space > will never clear the UDC upon disconnect. Its a sort of mismatch between > controller and udc. > Typo here:
So when we plug out the cable, the disconnect event won't be generated and on systems like android the user space will never clear the UDC upon disconnect.
> Also once the first get_sync fails, the dwc->dev->power.runtime_error > flag is set and successive calls to get_sync always return -EINVAL. In > this situation even if UDC/configfs retry pullup, resume_common will > never be called and we never actually start the controller or resume > dwc->dev. > > By calling set_suspended, I am trying to clear the runtime_error flag so > that the next retry to pullup will call resume_common and retry > core_init and set run_stop. > > Regards, > Krishna, > >>> + return ret; >>> } >>> if (dwc->pullups_connected == is_on) { >> >> Johan
| |