In the event, gadget_connect call (which invokes pullup) fails,
propagate the error to udc bind operation which inturn sends the
error to configfs. The userspace can then retry enumeartion if
it chooses to.
Signed-off-by: Krishna Kurapati <[email protected]>
---
Changes in v4: Fixed mutex locking imbalance during connect_control
failure
Link to v3: https://lore.kernel.org/all/[email protected]/
drivers/usb/gadget/udc/core.c | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 7d49d8a0b00c..53af25a333a1 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1125,12 +1125,16 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state);
/* ------------------------------------------------------------------------- */
/* Acquire connect_lock before calling this function. */
-static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc->connect_lock)
+static int usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc->connect_lock)
{
+ int ret;
+
if (udc->vbus)
- usb_gadget_connect_locked(udc->gadget);
+ ret = usb_gadget_connect_locked(udc->gadget);
else
- usb_gadget_disconnect_locked(udc->gadget);
+ ret = usb_gadget_disconnect_locked(udc->gadget);
+
+ return ret;
}
static void vbus_event_work(struct work_struct *work)
@@ -1604,12 +1608,23 @@ static int gadget_bind_driver(struct device *dev)
}
usb_gadget_enable_async_callbacks(udc);
udc->allow_connect = true;
- usb_udc_connect_control_locked(udc);
+ ret = usb_udc_connect_control_locked(udc);
+ if (ret) {
+ mutex_unlock(&udc->connect_lock);
+ goto err_connect_control;
+ }
+
mutex_unlock(&udc->connect_lock);
kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
return 0;
+ err_connect_control:
+ usb_gadget_disable_async_callbacks(udc);
+ if (gadget->irq)
+ synchronize_irq(gadget->irq);
+ usb_gadget_udc_stop_locked(udc);
+
err_start:
driver->unbind(udc->gadget);
--
2.42.0
On 9/27/2023 1:36 AM, Krishna Kurapati PSSNV wrote:
>>> drivers/usb/gadget/udc/core.c | 23 +++++++++++++++++++----
>>> 1 file changed, 19 insertions(+), 4 deletions(-)
>>>
>>> static void vbus_event_work(struct work_struct *work)
>>> @@ -1604,12 +1608,23 @@ static int gadget_bind_driver(struct device
>>> *dev)
>>> }
>>> usb_gadget_enable_async_callbacks(udc);
>>> udc->allow_connect = true;
>>> - usb_udc_connect_control_locked(udc);
>>> + ret = usb_udc_connect_control_locked(udc);
>>> + if (ret) {
>>> + mutex_unlock(&udc->connect_lock);
>>> + goto err_connect_control;
>>> + }
>>> +
>>> mutex_unlock(&udc->connect_lock);
>>> kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>>> return 0;
>>> + err_connect_control:
>>> + usb_gadget_disable_async_callbacks(udc);
>>> + if (gadget->irq)
>>> + synchronize_irq(gadget->irq);
>>> + usb_gadget_udc_stop_locked(udc);
>>
>> Not good -- usb_gadget_udc_stop_locked() expects you to be holding
>> udc->connect_lock, but you just dropped the lock! Also, you never set
>> udc->allow_connect back to false.
>>
>> You should move the mutex_unlock() call from inside the "if" statement
>> to down here, and add a line for udc->allow_connect.
>>
>
> Hi Alan,
>
> Thanks for the review. Will push v5 addressing the changes.
>
>
Hi Alan,
I tried out the following diff:
- usb_udc_connect_control_locked(udc);
+ ret = usb_udc_connect_control_locked(udc);
+ if (ret)
+ goto err_connect_control;
+
mutex_unlock(&udc->connect_lock);
kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
return 0;
+ err_connect_control:
+ udc->allow_connect = false;
+ usb_gadget_disable_async_callbacks(udc);
+ if (gadget->irq)
+ synchronize_irq(gadget->irq);
+ usb_gadget_udc_stop_locked(udc);
+ mutex_unlock(&udc->connect_lock);
+
If I clear UDC and fail dwc3 soft reset on purpose, I see UDC_store failing:
#echo a600000.usb > /sys/kernel/config/usb_gadget/g1/UDC
[ 127.394087] dwc3 a600000.usb: request 000000003f43f907 was not queued
to ep0out
[ 127.401637] udc a600000.usb: failed to start g1: -110
[ 127.406841] configfs-gadget.g1: probe of gadget.0 failed with error -110
[ 127.413809] UDC core: g1: couldn't find an available UDC or it's busy
The same output came when I tested v4 as well. Every time soft_reset
would fail when I try to write to UDC, UDC_store fails and above log
will come up. Can you help confirm if the diff above is proper as I
don't see any diff in the logs in v4 and about to push v5.
Regards,
Krishna,