On 6/19/2023 8:36 PM, Johan Hovold wrote:
On Mon, Jun 19, 2023 at 06:20:43PM +0530, 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:
@@ -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.
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.
That bit is clear.
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.
Ok, but the controller is an error state after the resume failure. And
here you rely on user space to retry gadget activation in order to
eventually detect the disconnect event?
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.
Ok, thanks, that's the bit I was missing in the commit message.
First, I perhaps mistakingly thought pm_runtime_set_suspended() may only
be called with PM runtime disabled, but it appears it may indeed be
valid to call also after an error but with the caveat that the device
must then actually be in the suspended state.
The documentation and implementation is inconsistent here as the kernel
doc for pm_runtime_set_suspended() clearly states:
It is not valid to call this function for devices with runtime
PM enabled.
and it also looks like we'd end up with an active-child counter
imbalance if anyone actually tries to do so.
But either way, it also seems like the controller is not guaranteed to
be suspended here as pm_runtime_get_sync() may also fail after a
previous errors that have left the controller in the active state?
> Also, what kind of errors would cause core_init and resume to fail here?
Hi Johan,
As per the comment just above the get_sync during pullup, the
resume_common path is used to resume the controller and start peripheral
mode incase the dwc3 was in suspended state. So if we are entering
gadget_resume we are in suspended state the first time it is called.
Regarding the errors that might leave controller in active state, I have
faced issue in core init, and in that function there is a cleanup
happening in case something fails. So controller was actually not in
active state after cleanup was done. In resume common, if
core_init_for_resume is failing, we cleanup everything initialized up
until that point.
The scenario you mentioned would be applicable in case gadget_resume
fails. We are not having a return value check and I am not sure what
would be the side effect of not having that check there. Either ways,
since it was failing for core init, I went ahead and made this patch.
As per the reason for failure in core init, the following is what was
happening at customer's end:
1. Cable plug-in
2. get_sync calls resume common which inturn calls core_init
3. core soft reset fails in core init, we cleanup and return -110
4. After applying this patch, the -110 was propagated to UDC properly
5. We got a second call to pullup via connect_control and this time
reset was successful.
The behavior was similar to [1]. There as well, on all Gen-2 targets,
after the retry happens I see soft reset is passing but failing for
first attempt.
[1]:
https://lore.kernel.org/all/20230510075252.31023-2-quic_kriskura@xxxxxxxxxxx/
Regards,
Krishna,
If this is something that you see during normal operation then this
seems to suggest that something is wrong with the runtime pm
implementation.
Note that virtually all drivers treat resume failures as fatal errors
and do not implement any recovery from that.
In fact, the only other example of this kind of usage that I could find
is also for a Qualcomm driver...
Johan
