| Message ID | 20230926193708.22405-1-quic_kriskura@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] usb: gadget: udc: Handle gadget_connect failure during bind operation | expand |
On 9/27/2023 2:54 AM, Alan Stern wrote: > On Wed, Sep 27, 2023 at 01:54:34AM +0530, Krishna Kurapati PSSNV wrote: >> >> >> 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. > > Isn't that what you want? I thought the whole purpose of this patch was > to make it so that configfs would realize when > usb_udc_connect_control_locked() had failed. So you should be happy > that the log shows a failure occurred. > >> 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. > > "Diff in the logs in v4"? What does that mean? A diff is a comparison > between two text files (often between before-and-after versions of a > source code file). Why would you expect a diff to show up in the logs? > > This revised patch looks okay to me. > Thanks for the confirmation. Will push v5. Regards, Krishna,
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);
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 <quic_kriskura@quicinc.com> --- Changes in v4: Fixed mutex locking imbalance during connect_control failure Link to v3: https://lore.kernel.org/all/20230510075252.31023-3-quic_kriskura@quicinc.com/ drivers/usb/gadget/udc/core.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)