On Wed, Sep 27, 2023 at 01:07:08AM +0530, Krishna Kurapati wrote: > In the event, gadget_connect call (which invokes pullup) fails, s/event,/event/ > propagate the error to udc bind operation which inturn sends the s/inturn/in turn/ > error to configfs. The userspace can then retry enumeartion if s/enumeartion/enumeration/ > it chooses to. > > Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx> > --- > Changes in v4: Fixed mutex locking imbalance during connect_control > failure > Link to v3: https://lore.kernel.org/all/20230510075252.31023-3-quic_kriskura@xxxxxxxxxxx/ > > 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; You don't actually need the new variable ret. You can just do: if (udc->vbus) return usb_gadget_connect_locked(udc->gadget); else return usb_gadget_disconnect_locked(udc->gadget); > } > > 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. Alan Stern > + > err_start: > driver->unbind(udc->gadget); > > -- > 2.42.0 >
