On Mon, May 29, 2023 at 6:08 PM Alan Stern stern@rowland.harvard.edu wrote:
On Mon, May 29, 2023 at 11:48:16PM +0000, Badhri Jagan Sridharan wrote:
chipidea udc calls usb_udc_vbus_handler from udc_start gadget ops causing a deadlock. Avoid this by offloading usb_udc_vbus_handler processing.
This is not a good explanation. In particular, it doesn't explain why moving the processing to a workqueue is the proper solution.
You should describe the issue I raised earlier, namely, that usb_udc_vbus_handler() has to run in interrupt context but it calls usb_udc_connect_control(), which has to run in process context. And explain _why_ these routines have to run in those contexts.
drivers/usb/gadget/udc/core.c | 269 ++++++++++++++++------------------ 1 file changed, 123 insertions(+), 146 deletions(-)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index 4641153e9706..26380e621e9f 100644 --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -38,9 +38,10 @@ static const struct bus_type gadget_bus_type;
- for udcs who do not care about vbus status, this value is always true
- @started: the UDC's started state. True if the UDC had started.
- @connect_lock: protects udc->vbus, udc->started, gadget->connect, gadget->deactivate related
- functions. usb_gadget_connect_locked, usb_gadget_disconnect_locked,
- usb_udc_connect_control_locked, usb_gadget_udc_start_locked, usb_gadget_udc_stop_locked are
- called with this lock held.
- functions. usb_gadget_pullup_update_locked called with this lock held.
- @vbus_events: list head for processing vbus updates on usb_udc_vbus_handler.
- @vbus_events_lock: protects vbus_events list
- @vbus_work: work item that invokes usb_gadget_pullup_update_locked.
- This represents the internal data structure which is used by the UDC-class
- to hold information about udc driver and gadget together.
@@ -53,6 +54,20 @@ struct usb_udc { bool vbus; bool started; struct mutex connect_lock;
struct list_head vbus_events;spinlock_t vbus_events_lock;struct work_struct vbus_work;Do you really need three new fields here? Isn't vbus_work sufficient?
Ack. Just the vbus_work is sufficient as vbus can be updated to the latest value. Addressing in v5.
bool unbinding;Do not include this in the same patch. The unbinding flag does something different from the vbus_work structure, so it belongs in a different patch.
Sure, uploaded as a separate patch in v5. However, I named it allow_start instead as I believe that UDC should neither be started nor pulled up when unbound. Let me know your thoughts in v5 !
+};
+/**
- struct vbus_event - used to notify vbus updates posted through usb_udc_vbus_handler.
- @vbus_on: true when vbus is on. false other wise.
- @node: list node for maintaining a list of pending updates to be processed.
- */
+struct vbus_event {
bool vbus_on;struct list_head node;};
Why do we need this? Why can't the work routine simply set the pullup according to the current setting of vbus and the other flags? That's what usb_udc_vbus_handler() does now.
Ack. Dropping vbus_event and related fields.
static struct class *udc_class; @@ -693,40 +708,46 @@ int usb_gadget_vbus_disconnect(struct usb_gadget *gadget) EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect);
/* Internal version of usb_gadget_connect needs to be called with connect_lock held. */ -static int usb_gadget_connect_locked(struct usb_gadget *gadget) +static int usb_gadget_pullup_update_locked(struct usb_gadget *gadget) __must_hold(&gadget->udc->connect_lock) { int ret = 0;
bool connect = !gadget->deactivated && gadget->udc->started && gadget->udc->vbus &&!gadget->udc->unbinding;Since you are wrapping this line anyway, you might as well wrap it before column 76.
if (!gadget->ops->pullup) { ret = -EOPNOTSUPP; goto out; }
if (gadget->connected)goto out;if (gadget->deactivated || !gadget->udc->started) {/** If gadget is deactivated we only save new state.* Gadget will be connected automatically after activation.** udc first needs to be started before gadget can be pulled up.*/gadget->connected = true;goto out;
if (connect != gadget->connected) {ret = gadget->ops->pullup(gadget, connect);if (!ret)gadget->connected = connect;mutex_lock(&udc_lock);if (!connect)gadget->udc->driver->disconnect(gadget);mutex_unlock(&udc_lock); }What will happen if the gadget isn't deactivated, but it is started and VBUS power is prevent and the driver isn't unbinding, but the gadget driver decides to call usb_gadget_disconnect()?
Simplified as you recommended to directly call usb_udc_connect_control() from the work item. So, this is no longer an issue.
ret = gadget->ops->pullup(gadget, 1);if (!ret)gadget->connected = 1;out: trace_usb_gadget_connect(gadget, ret);
return ret;}
+static int usb_gadget_set_vbus(struct usb_gadget *gadget, bool vbus) +{
int ret;mutex_lock(&gadget->udc->connect_lock);gadget->udc->vbus = vbus;Why does this have to be here? What's wrong with setting vbus in interrupt context?
ret = usb_gadget_pullup_update_locked(gadget);mutex_unlock(&gadget->udc->connect_lock);Sorry, but at this point I'm getting tired of pointing out all the problems in this patch, so I'm going to stop here.
How about instead doing something really simple, like just make usb_udc_vbus_handler() queue up a work routine that calls usb_udc_connect_control()? Just a minimal change that will be really easy to review.
Ack. v5 now does this.
Thanks for all the feedback, Badhri
Alan Stern