| Message ID | 20230926193708.22405-1-quic_kriskura@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] usb: gadget: udc: Handle gadget_connect failure during bind operation | expand |
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@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(-) > > 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 >
On 9/27/2023 1:24 AM, Alan Stern wrote: > 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@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(-) >> >> 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. > Hi Alan, Thanks for the review. Will push v5 addressing the changes. Regards, Krishna,
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,
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. Alan Stern
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(-)