| Message ID | 20230529233700.3706661-3-badhri@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/3] Revert "Revert "usb: gadget: udc: core: Invoke usb_gadget_connect only when started"" | expand |
Please disregard v3 as I missed a minor cleanup. Sending v4. Thanks, Badhri On Mon, May 29, 2023 at 4:37 PM Badhri Jagan Sridharan <badhri@google.com> 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. > > ============================================ > WARNING: possible recursive locking detected > 640-rc1-000-devel-00005-gcda3c69ebc14 #1 Not tainted > ------------------------------------------- > > CPU0 > ---- > lock(&udc->connect_lock); > lock(&udc->connect_lock); > > DEADLOCK > > stack backtrace: > CPU: 1 PID: 566 Comm: echo Not tainted 640-rc1-000-devel-00005-gcda3c69ebc14 #1 > Hardware name: Freescale iMX7 Dual (Device Tree) > unwind_backtrace from show_stack+0x10/0x14 > show_stack from dump_stack_lvl+0x70/0xb0 > dump_stack_lvl from __lock_acquire+0x924/0x22c4 > __lock_acquire from lock_acquire+0x100/0x370 > lock_acquire from __mutex_lock+0xa8/0xfb4 > __mutex_lock from mutex_lock_nested+0x1c/0x24 > mutex_lock_nested from usb_udc_vbus_handler+0x1c/0x60 > usb_udc_vbus_handler from ci_udc_start+0x74/0x9c > ci_udc_start from gadget_bind_driver+0x130/0x230 > gadget_bind_driver from really_probe+0xd8/0x3fc > really_probe from __driver_probe_device+0x94/0x1f0 > __driver_probe_device from driver_probe_device+0x2c/0xc4 > driver_probe_device from __driver_attach+0x114/0x1cc > __driver_attach from bus_for_each_dev+0x7c/0xcc > bus_for_each_dev from bus_add_driver+0xd4/0x200 > bus_add_driver from driver_register+0x7c/0x114 > driver_register from usb_gadget_register_driver_owner+0x40/0xe0 > usb_gadget_register_driver_owner from gadget_dev_desc_UDC_store+0xd4/0x110 > gadget_dev_desc_UDC_store from configfs_write_iter+0xac/0x118 > configfs_write_iter from vfs_write+0x1b4/0x40c > vfs_write from ksys_write+0x70/0xf8 > ksys_write from ret_fast_syscall+0x0/0x1c > > Fixes: 0db213ea8eed ("Revert "Revert "usb: gadget: udc: core: Prevent redundant calls to pullup""") > Cc: stable@vger.kernel.org > Reported-by: Stephan Gerhold <stephan@gerhold.net> > Closes: https://lore.kernel.org/all/ZF4bMptC3Lf2Hnee@gerhold.net/ > Reported-by: Francesco Dolcini <francesco.dolcini@toradex.com> > Closes: https://lore.kernel.org/all/ZF4BvgsOyoKxdPFF@francesco-nb.int.toradex.com/ > Reported-by: Alistair <alistair@alistair23.me> > Closes: https://lore.kernel.org/lkml/0cf8c588b701d7cf25ffe1a9217b81716e6a5c51.camel@alistair23.me/ > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > --- > Changes since v1: > - Address Alan Stern's comment on usb_udc_vbus_handler invocation from > atomic context: > * vbus_events_lock is now a spinlock and allocations in > * usb_udc_vbus_handler are atomic now. > > Changes since v2: > - Addressing Alan Stern's comments: > ** connect_lock is now held by callers of > * usb_gadget_pullup_update_locked() and gadget_(un)bind_driver() does > * notdirectly hold the lock. > > ** Both usb_gadget_(dis)connect() and usb_udc_vbus_handler() would > * set/clear udc->vbus and invoke usb_gadget_pullup_update_locked. > > ** Add "unbinding" to prevent new connections after the gadget is being > * unbound. > --- > drivers/usb/gadget/udc/core.c | 269 ++++++++++++++++------------------ > 1 file changed, 124 insertions(+), 145 deletions(-) > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > index 4641153e9706..2aa914a0a195 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; > + bool unbinding; > +}; > + > +/** > + * 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; > }; > > 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; > > 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); > } > > - 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; > + ret = usb_gadget_pullup_update_locked(gadget); > + mutex_unlock(&gadget->udc->connect_lock); > + > + return ret; > +} > + > /** > * usb_gadget_connect - software-controlled connect to USB host > * @gadget:the peripheral being connected > @@ -739,56 +760,10 @@ static int usb_gadget_connect_locked(struct usb_gadget *gadget) > */ > int usb_gadget_connect(struct usb_gadget *gadget) > { > - int ret; > - > - mutex_lock(&gadget->udc->connect_lock); > - ret = usb_gadget_connect_locked(gadget); > - mutex_unlock(&gadget->udc->connect_lock); > - > - return ret; > + return usb_gadget_set_vbus(gadget, true); > } > EXPORT_SYMBOL_GPL(usb_gadget_connect); > > -/* Internal version of usb_gadget_disconnect needs to be called with connect_lock held. */ > -static int usb_gadget_disconnect_locked(struct usb_gadget *gadget) > - __must_hold(&gadget->udc->connect_lock) > -{ > - int ret = 0; > - > - 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 stay disconnected after activation. > - * > - * udc should have been started before gadget being pulled down. > - */ > - gadget->connected = false; > - goto out; > - } > - > - ret = gadget->ops->pullup(gadget, 0); > - if (!ret) > - gadget->connected = 0; > - > - mutex_lock(&udc_lock); > - if (gadget->udc->driver) > - gadget->udc->driver->disconnect(gadget); > - mutex_unlock(&udc_lock); > - > -out: > - trace_usb_gadget_disconnect(gadget, ret); > - > - return ret; > -} > - > /** > * usb_gadget_disconnect - software-controlled disconnect from USB host > * @gadget:the peripheral being disconnected > @@ -803,16 +778,22 @@ static int usb_gadget_disconnect_locked(struct usb_gadget *gadget) > * Returns zero on success, else negative errno. > */ > int usb_gadget_disconnect(struct usb_gadget *gadget) > +{ > + return usb_gadget_set_vbus(gadget, false); > +} > +EXPORT_SYMBOL_GPL(usb_gadget_disconnect); > + > +static int usb_gadget_set_deactivate(struct usb_gadget *gadget, bool deactivate) > { > int ret; > > mutex_lock(&gadget->udc->connect_lock); > - ret = usb_gadget_disconnect_locked(gadget); > + gadget->deactivated = deactivate; > + ret = usb_gadget_pullup_update_locked(gadget); > mutex_unlock(&gadget->udc->connect_lock); > > return ret; > } > -EXPORT_SYMBOL_GPL(usb_gadget_disconnect); > > /** > * usb_gadget_deactivate - deactivate function which is not ready to work > @@ -829,26 +810,7 @@ int usb_gadget_deactivate(struct usb_gadget *gadget) > { > int ret = 0; > > - if (gadget->deactivated) > - goto out; > - > - mutex_lock(&gadget->udc->connect_lock); > - if (gadget->connected) { > - ret = usb_gadget_disconnect_locked(gadget); > - if (ret) > - goto unlock; > - > - /* > - * If gadget was being connected before deactivation, we want > - * to reconnect it in usb_gadget_activate(). > - */ > - gadget->connected = true; > - } > - gadget->deactivated = true; > - > -unlock: > - mutex_unlock(&gadget->udc->connect_lock); > -out: > + ret = usb_gadget_set_deactivate(gadget, true); > trace_usb_gadget_deactivate(gadget, ret); > > return ret; > @@ -868,21 +830,7 @@ int usb_gadget_activate(struct usb_gadget *gadget) > { > int ret = 0; > > - if (!gadget->deactivated) > - goto out; > - > - mutex_lock(&gadget->udc->connect_lock); > - gadget->deactivated = false; > - > - /* > - * If gadget has been connected before deactivation, or became connected > - * while it was being deactivated, we call usb_gadget_connect(). > - */ > - if (gadget->connected) > - ret = usb_gadget_connect_locked(gadget); > - mutex_unlock(&gadget->udc->connect_lock); > - > -out: > + ret = usb_gadget_set_deactivate(gadget, false); > trace_usb_gadget_activate(gadget, ret); > > return ret; > @@ -1121,13 +1069,28 @@ 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 void vbus_event_work(struct work_struct *work) > { > - if (udc->vbus && udc->started) > - usb_gadget_connect_locked(udc->gadget); > - else > - usb_gadget_disconnect_locked(udc->gadget); > + struct vbus_event *event, *n; > + struct usb_udc *udc = container_of(work, struct usb_udc, vbus_work); > + unsigned long flags; > + > + spin_lock_irqsave(&udc->vbus_events_lock, flags); > + list_for_each_entry_safe(event, n, &udc->vbus_events, node) { > + list_del(&event->node); > + /* OK to drop the lock here as it suffice to syncrhronize udc->vbus_events node > + * retrieval and deletion against usb_udc_vbus_handler. usb_udc_vbus_handler does > + * list_add_tail so n would be the same even if the lock is dropped. > + */ > + spin_unlock_irqrestore(&udc->vbus_events_lock, flags); > + mutex_lock(&udc->connect_lock); > + udc->vbus = event->vbus_on; > + usb_gadget_pullup_update_locked(udc->gadget); > + kfree(event); > + mutex_unlock(&udc->connect_lock); > + spin_lock_irqsave(&udc->vbus_events_lock, flags); > + } > + spin_unlock_irqrestore(&udc->vbus_events_lock, flags); > } > > /** > @@ -1141,14 +1104,26 @@ static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc > */ > void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status) > { > - struct usb_udc *udc = gadget->udc; > + struct usb_udc *udc; > + struct vbus_event *vbus_event; > + unsigned long flags; > > - mutex_lock(&udc->connect_lock); > - if (udc) { > - udc->vbus = status; > - usb_udc_connect_control_locked(udc); > + //mutex_lock(&udc_lock); > + if (!gadget || !gadget->udc) { > + //mutex_unlock(&udc_lock); > + return; > } > - mutex_unlock(&udc->connect_lock); > + udc = gadget->udc; > + > + vbus_event = kzalloc(sizeof(*vbus_event), GFP_ATOMIC); > + if (!vbus_event) > + return; > + > + spin_lock_irqsave(&udc->vbus_events_lock, flags); > + vbus_event->vbus_on = status; > + list_add_tail(&vbus_event->node, &udc->vbus_events); > + spin_unlock_irqrestore(&udc->vbus_events_lock, flags); > + schedule_work(&udc->vbus_work); > } > EXPORT_SYMBOL_GPL(usb_udc_vbus_handler); > > @@ -1170,7 +1145,7 @@ void usb_gadget_udc_reset(struct usb_gadget *gadget, > EXPORT_SYMBOL_GPL(usb_gadget_udc_reset); > > /** > - * usb_gadget_udc_start_locked - tells usb device controller to start up > + * usb_gadget_udc_start - tells usb device controller to start up > * @udc: The UDC to be started > * > * This call is issued by the UDC Class driver when it's about > @@ -1181,11 +1156,8 @@ EXPORT_SYMBOL_GPL(usb_gadget_udc_reset); > * necessary to have it powered on. > * > * Returns zero on success, else negative errno. > - * > - * Caller should acquire connect_lock before invoking this function. > */ > -static inline int usb_gadget_udc_start_locked(struct usb_udc *udc) > - __must_hold(&udc->connect_lock) > +static inline int usb_gadget_udc_start(struct usb_udc *udc) > { > int ret; > > @@ -1194,15 +1166,17 @@ static inline int usb_gadget_udc_start_locked(struct usb_udc *udc) > return -EBUSY; > } > > + mutex_lock(&udc->connect_lock); > ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver); > if (!ret) > udc->started = true; > + mutex_unlock(&udc->connect_lock); > > return ret; > } > > /** > - * usb_gadget_udc_stop_locked - tells usb device controller we don't need it anymore > + * usb_gadget_udc_stop - tells usb device controller we don't need it anymore > * @udc: The UDC to be stopped > * > * This call is issued by the UDC Class driver after calling > @@ -1211,19 +1185,18 @@ static inline int usb_gadget_udc_start_locked(struct usb_udc *udc) > * The details are implementation specific, but it can go as > * far as powering off UDC completely and disable its data > * line pullups. > - * > - * Caller should acquire connect lock before invoking this function. > */ > -static inline void usb_gadget_udc_stop_locked(struct usb_udc *udc) > - __must_hold(&udc->connect_lock) > +static inline void usb_gadget_udc_stop(struct usb_udc *udc) > { > if (!udc->started) { > dev_err(&udc->dev, "UDC had already stopped\n"); > return; > } > > + mutex_lock(&udc->connect_lock); > udc->gadget->ops->udc_stop(udc->gadget); > udc->started = false; > + mutex_unlock(&udc->connect_lock); > } > > /** > @@ -1362,6 +1335,7 @@ int usb_add_gadget(struct usb_gadget *gadget) > if (!udc) > goto error; > > + udc->unbinding = true; > device_initialize(&udc->dev); > udc->dev.release = usb_udc_release; > udc->dev.class = udc_class; > @@ -1375,6 +1349,9 @@ int usb_add_gadget(struct usb_gadget *gadget) > udc->gadget = gadget; > gadget->udc = udc; > mutex_init(&udc->connect_lock); > + INIT_LIST_HEAD(&udc->vbus_events); > + spin_lock_init(&udc->vbus_events_lock); > + INIT_WORK(&udc->vbus_work, vbus_event_work); > > udc->started = false; > > @@ -1474,6 +1451,17 @@ char *usb_get_gadget_udc_name(void) > } > EXPORT_SYMBOL_GPL(usb_get_gadget_udc_name); > > +static int usb_gadget_set_unbinding(struct usb_udc *udc, bool status) > +{ > + int ret; > + > + mutex_lock(&udc->connect_lock); > + udc->unbinding = status; > + ret = usb_gadget_pullup_update_locked(udc->gadget); > + mutex_unlock(&udc->connect_lock); > + > + return ret; > +} > /** > * usb_add_gadget_udc - adds a new gadget to the udc class driver list > * @parent: the parent device to this udc. Usually the controller > @@ -1576,15 +1564,11 @@ static int gadget_bind_driver(struct device *dev) > if (ret) > goto err_bind; > > - mutex_lock(&udc->connect_lock); > - ret = usb_gadget_udc_start_locked(udc); > - if (ret) { > - mutex_unlock(&udc->connect_lock); > + ret = usb_gadget_udc_start(udc); > + if (ret) > goto err_start; > - } > usb_gadget_enable_async_callbacks(udc); > - usb_udc_connect_control_locked(udc); > - mutex_unlock(&udc->connect_lock); > + usb_gadget_set_unbinding(udc, false); > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > return 0; > @@ -1615,14 +1599,13 @@ static void gadget_unbind_driver(struct device *dev) > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > > - mutex_lock(&udc->connect_lock); > - usb_gadget_disconnect_locked(gadget); > + usb_gadget_set_unbinding(udc, true); > + cancel_work_sync(&udc->vbus_work); > usb_gadget_disable_async_callbacks(udc); > if (gadget->irq) > synchronize_irq(gadget->irq); > udc->driver->unbind(gadget); > - usb_gadget_udc_stop_locked(udc); > - mutex_unlock(&udc->connect_lock); > + usb_gadget_udc_stop(udc); > > mutex_lock(&udc_lock); > driver->is_bound = false; > @@ -1708,15 +1691,11 @@ static ssize_t soft_connect_store(struct device *dev, > } > > if (sysfs_streq(buf, "connect")) { > - mutex_lock(&udc->connect_lock); > - usb_gadget_udc_start_locked(udc); > - usb_gadget_connect_locked(udc->gadget); > - mutex_unlock(&udc->connect_lock); > + usb_gadget_udc_start(udc); > + usb_udc_vbus_handler(udc->gadget, true); > } else if (sysfs_streq(buf, "disconnect")) { > - mutex_lock(&udc->connect_lock); > - usb_gadget_disconnect_locked(udc->gadget); > - usb_gadget_udc_stop_locked(udc); > - mutex_unlock(&udc->connect_lock); > + usb_udc_vbus_handler(udc->gadget, false); > + usb_gadget_udc_stop(udc); > } else { > dev_err(dev, "unsupported command '%s'\n", buf); > ret = -EINVAL; > -- > 2.41.0.rc0.172.g3f132b7071-goog >
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index 4641153e9706..2aa914a0a195 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; + bool unbinding; +}; + +/** + * 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; }; 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; 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); } - 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; + ret = usb_gadget_pullup_update_locked(gadget); + mutex_unlock(&gadget->udc->connect_lock); + + return ret; +} + /** * usb_gadget_connect - software-controlled connect to USB host * @gadget:the peripheral being connected @@ -739,56 +760,10 @@ static int usb_gadget_connect_locked(struct usb_gadget *gadget) */ int usb_gadget_connect(struct usb_gadget *gadget) { - int ret; - - mutex_lock(&gadget->udc->connect_lock); - ret = usb_gadget_connect_locked(gadget); - mutex_unlock(&gadget->udc->connect_lock); - - return ret; + return usb_gadget_set_vbus(gadget, true); } EXPORT_SYMBOL_GPL(usb_gadget_connect); -/* Internal version of usb_gadget_disconnect needs to be called with connect_lock held. */ -static int usb_gadget_disconnect_locked(struct usb_gadget *gadget) - __must_hold(&gadget->udc->connect_lock) -{ - int ret = 0; - - 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 stay disconnected after activation. - * - * udc should have been started before gadget being pulled down. - */ - gadget->connected = false; - goto out; - } - - ret = gadget->ops->pullup(gadget, 0); - if (!ret) - gadget->connected = 0; - - mutex_lock(&udc_lock); - if (gadget->udc->driver) - gadget->udc->driver->disconnect(gadget); - mutex_unlock(&udc_lock); - -out: - trace_usb_gadget_disconnect(gadget, ret); - - return ret; -} - /** * usb_gadget_disconnect - software-controlled disconnect from USB host * @gadget:the peripheral being disconnected @@ -803,16 +778,22 @@ static int usb_gadget_disconnect_locked(struct usb_gadget *gadget) * Returns zero on success, else negative errno. */ int usb_gadget_disconnect(struct usb_gadget *gadget) +{ + return usb_gadget_set_vbus(gadget, false); +} +EXPORT_SYMBOL_GPL(usb_gadget_disconnect); + +static int usb_gadget_set_deactivate(struct usb_gadget *gadget, bool deactivate) { int ret; mutex_lock(&gadget->udc->connect_lock); - ret = usb_gadget_disconnect_locked(gadget); + gadget->deactivated = deactivate; + ret = usb_gadget_pullup_update_locked(gadget); mutex_unlock(&gadget->udc->connect_lock); return ret; } -EXPORT_SYMBOL_GPL(usb_gadget_disconnect); /** * usb_gadget_deactivate - deactivate function which is not ready to work @@ -829,26 +810,7 @@ int usb_gadget_deactivate(struct usb_gadget *gadget) { int ret = 0; - if (gadget->deactivated) - goto out; - - mutex_lock(&gadget->udc->connect_lock); - if (gadget->connected) { - ret = usb_gadget_disconnect_locked(gadget); - if (ret) - goto unlock; - - /* - * If gadget was being connected before deactivation, we want - * to reconnect it in usb_gadget_activate(). - */ - gadget->connected = true; - } - gadget->deactivated = true; - -unlock: - mutex_unlock(&gadget->udc->connect_lock); -out: + ret = usb_gadget_set_deactivate(gadget, true); trace_usb_gadget_deactivate(gadget, ret); return ret; @@ -868,21 +830,7 @@ int usb_gadget_activate(struct usb_gadget *gadget) { int ret = 0; - if (!gadget->deactivated) - goto out; - - mutex_lock(&gadget->udc->connect_lock); - gadget->deactivated = false; - - /* - * If gadget has been connected before deactivation, or became connected - * while it was being deactivated, we call usb_gadget_connect(). - */ - if (gadget->connected) - ret = usb_gadget_connect_locked(gadget); - mutex_unlock(&gadget->udc->connect_lock); - -out: + ret = usb_gadget_set_deactivate(gadget, false); trace_usb_gadget_activate(gadget, ret); return ret; @@ -1121,13 +1069,28 @@ 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 void vbus_event_work(struct work_struct *work) { - if (udc->vbus && udc->started) - usb_gadget_connect_locked(udc->gadget); - else - usb_gadget_disconnect_locked(udc->gadget); + struct vbus_event *event, *n; + struct usb_udc *udc = container_of(work, struct usb_udc, vbus_work); + unsigned long flags; + + spin_lock_irqsave(&udc->vbus_events_lock, flags); + list_for_each_entry_safe(event, n, &udc->vbus_events, node) { + list_del(&event->node); + /* OK to drop the lock here as it suffice to syncrhronize udc->vbus_events node + * retrieval and deletion against usb_udc_vbus_handler. usb_udc_vbus_handler does + * list_add_tail so n would be the same even if the lock is dropped. + */ + spin_unlock_irqrestore(&udc->vbus_events_lock, flags); + mutex_lock(&udc->connect_lock); + udc->vbus = event->vbus_on; + usb_gadget_pullup_update_locked(udc->gadget); + kfree(event); + mutex_unlock(&udc->connect_lock); + spin_lock_irqsave(&udc->vbus_events_lock, flags); + } + spin_unlock_irqrestore(&udc->vbus_events_lock, flags); } /** @@ -1141,14 +1104,26 @@ static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc */ void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status) { - struct usb_udc *udc = gadget->udc; + struct usb_udc *udc; + struct vbus_event *vbus_event; + unsigned long flags; - mutex_lock(&udc->connect_lock); - if (udc) { - udc->vbus = status; - usb_udc_connect_control_locked(udc); + //mutex_lock(&udc_lock); + if (!gadget || !gadget->udc) { + //mutex_unlock(&udc_lock); + return; } - mutex_unlock(&udc->connect_lock); + udc = gadget->udc; + + vbus_event = kzalloc(sizeof(*vbus_event), GFP_ATOMIC); + if (!vbus_event) + return; + + spin_lock_irqsave(&udc->vbus_events_lock, flags); + vbus_event->vbus_on = status; + list_add_tail(&vbus_event->node, &udc->vbus_events); + spin_unlock_irqrestore(&udc->vbus_events_lock, flags); + schedule_work(&udc->vbus_work); } EXPORT_SYMBOL_GPL(usb_udc_vbus_handler); @@ -1170,7 +1145,7 @@ void usb_gadget_udc_reset(struct usb_gadget *gadget, EXPORT_SYMBOL_GPL(usb_gadget_udc_reset); /** - * usb_gadget_udc_start_locked - tells usb device controller to start up + * usb_gadget_udc_start - tells usb device controller to start up * @udc: The UDC to be started * * This call is issued by the UDC Class driver when it's about @@ -1181,11 +1156,8 @@ EXPORT_SYMBOL_GPL(usb_gadget_udc_reset); * necessary to have it powered on. * * Returns zero on success, else negative errno. - * - * Caller should acquire connect_lock before invoking this function. */ -static inline int usb_gadget_udc_start_locked(struct usb_udc *udc) - __must_hold(&udc->connect_lock) +static inline int usb_gadget_udc_start(struct usb_udc *udc) { int ret; @@ -1194,15 +1166,17 @@ static inline int usb_gadget_udc_start_locked(struct usb_udc *udc) return -EBUSY; } + mutex_lock(&udc->connect_lock); ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver); if (!ret) udc->started = true; + mutex_unlock(&udc->connect_lock); return ret; } /** - * usb_gadget_udc_stop_locked - tells usb device controller we don't need it anymore + * usb_gadget_udc_stop - tells usb device controller we don't need it anymore * @udc: The UDC to be stopped * * This call is issued by the UDC Class driver after calling @@ -1211,19 +1185,18 @@ static inline int usb_gadget_udc_start_locked(struct usb_udc *udc) * The details are implementation specific, but it can go as * far as powering off UDC completely and disable its data * line pullups. - * - * Caller should acquire connect lock before invoking this function. */ -static inline void usb_gadget_udc_stop_locked(struct usb_udc *udc) - __must_hold(&udc->connect_lock) +static inline void usb_gadget_udc_stop(struct usb_udc *udc) { if (!udc->started) { dev_err(&udc->dev, "UDC had already stopped\n"); return; } + mutex_lock(&udc->connect_lock); udc->gadget->ops->udc_stop(udc->gadget); udc->started = false; + mutex_unlock(&udc->connect_lock); } /** @@ -1362,6 +1335,7 @@ int usb_add_gadget(struct usb_gadget *gadget) if (!udc) goto error; + udc->unbinding = true; device_initialize(&udc->dev); udc->dev.release = usb_udc_release; udc->dev.class = udc_class; @@ -1375,6 +1349,9 @@ int usb_add_gadget(struct usb_gadget *gadget) udc->gadget = gadget; gadget->udc = udc; mutex_init(&udc->connect_lock); + INIT_LIST_HEAD(&udc->vbus_events); + spin_lock_init(&udc->vbus_events_lock); + INIT_WORK(&udc->vbus_work, vbus_event_work); udc->started = false; @@ -1474,6 +1451,17 @@ char *usb_get_gadget_udc_name(void) } EXPORT_SYMBOL_GPL(usb_get_gadget_udc_name); +static int usb_gadget_set_unbinding(struct usb_udc *udc, bool status) +{ + int ret; + + mutex_lock(&udc->connect_lock); + udc->unbinding = status; + ret = usb_gadget_pullup_update_locked(udc->gadget); + mutex_unlock(&udc->connect_lock); + + return ret; +} /** * usb_add_gadget_udc - adds a new gadget to the udc class driver list * @parent: the parent device to this udc. Usually the controller @@ -1576,15 +1564,11 @@ static int gadget_bind_driver(struct device *dev) if (ret) goto err_bind; - mutex_lock(&udc->connect_lock); - ret = usb_gadget_udc_start_locked(udc); - if (ret) { - mutex_unlock(&udc->connect_lock); + ret = usb_gadget_udc_start(udc); + if (ret) goto err_start; - } usb_gadget_enable_async_callbacks(udc); - usb_udc_connect_control_locked(udc); - mutex_unlock(&udc->connect_lock); + usb_gadget_set_unbinding(udc, false); kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); return 0; @@ -1615,14 +1599,13 @@ static void gadget_unbind_driver(struct device *dev) kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); - mutex_lock(&udc->connect_lock); - usb_gadget_disconnect_locked(gadget); + usb_gadget_set_unbinding(udc, true); + cancel_work_sync(&udc->vbus_work); usb_gadget_disable_async_callbacks(udc); if (gadget->irq) synchronize_irq(gadget->irq); udc->driver->unbind(gadget); - usb_gadget_udc_stop_locked(udc); - mutex_unlock(&udc->connect_lock); + usb_gadget_udc_stop(udc); mutex_lock(&udc_lock); driver->is_bound = false; @@ -1708,15 +1691,11 @@ static ssize_t soft_connect_store(struct device *dev, } if (sysfs_streq(buf, "connect")) { - mutex_lock(&udc->connect_lock); - usb_gadget_udc_start_locked(udc); - usb_gadget_connect_locked(udc->gadget); - mutex_unlock(&udc->connect_lock); + usb_gadget_udc_start(udc); + usb_udc_vbus_handler(udc->gadget, true); } else if (sysfs_streq(buf, "disconnect")) { - mutex_lock(&udc->connect_lock); - usb_gadget_disconnect_locked(udc->gadget); - usb_gadget_udc_stop_locked(udc); - mutex_unlock(&udc->connect_lock); + usb_udc_vbus_handler(udc->gadget, false); + usb_gadget_udc_stop(udc); } else { dev_err(dev, "unsupported command '%s'\n", buf); ret = -EINVAL;
chipidea udc calls usb_udc_vbus_handler from udc_start gadget ops causing a deadlock. Avoid this by offloading usb_udc_vbus_handler processing. ============================================ WARNING: possible recursive locking detected 640-rc1-000-devel-00005-gcda3c69ebc14 #1 Not tainted ------------------------------------------- CPU0 ---- lock(&udc->connect_lock); lock(&udc->connect_lock); DEADLOCK stack backtrace: CPU: 1 PID: 566 Comm: echo Not tainted 640-rc1-000-devel-00005-gcda3c69ebc14 #1 Hardware name: Freescale iMX7 Dual (Device Tree) unwind_backtrace from show_stack+0x10/0x14 show_stack from dump_stack_lvl+0x70/0xb0 dump_stack_lvl from __lock_acquire+0x924/0x22c4 __lock_acquire from lock_acquire+0x100/0x370 lock_acquire from __mutex_lock+0xa8/0xfb4 __mutex_lock from mutex_lock_nested+0x1c/0x24 mutex_lock_nested from usb_udc_vbus_handler+0x1c/0x60 usb_udc_vbus_handler from ci_udc_start+0x74/0x9c ci_udc_start from gadget_bind_driver+0x130/0x230 gadget_bind_driver from really_probe+0xd8/0x3fc really_probe from __driver_probe_device+0x94/0x1f0 __driver_probe_device from driver_probe_device+0x2c/0xc4 driver_probe_device from __driver_attach+0x114/0x1cc __driver_attach from bus_for_each_dev+0x7c/0xcc bus_for_each_dev from bus_add_driver+0xd4/0x200 bus_add_driver from driver_register+0x7c/0x114 driver_register from usb_gadget_register_driver_owner+0x40/0xe0 usb_gadget_register_driver_owner from gadget_dev_desc_UDC_store+0xd4/0x110 gadget_dev_desc_UDC_store from configfs_write_iter+0xac/0x118 configfs_write_iter from vfs_write+0x1b4/0x40c vfs_write from ksys_write+0x70/0xf8 ksys_write from ret_fast_syscall+0x0/0x1c Fixes: 0db213ea8eed ("Revert "Revert "usb: gadget: udc: core: Prevent redundant calls to pullup""") Cc: stable@vger.kernel.org Reported-by: Stephan Gerhold <stephan@gerhold.net> Closes: https://lore.kernel.org/all/ZF4bMptC3Lf2Hnee@gerhold.net/ Reported-by: Francesco Dolcini <francesco.dolcini@toradex.com> Closes: https://lore.kernel.org/all/ZF4BvgsOyoKxdPFF@francesco-nb.int.toradex.com/ Reported-by: Alistair <alistair@alistair23.me> Closes: https://lore.kernel.org/lkml/0cf8c588b701d7cf25ffe1a9217b81716e6a5c51.camel@alistair23.me/ Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> --- Changes since v1: - Address Alan Stern's comment on usb_udc_vbus_handler invocation from atomic context: * vbus_events_lock is now a spinlock and allocations in * usb_udc_vbus_handler are atomic now. Changes since v2: - Addressing Alan Stern's comments: ** connect_lock is now held by callers of * usb_gadget_pullup_update_locked() and gadget_(un)bind_driver() does * notdirectly hold the lock. ** Both usb_gadget_(dis)connect() and usb_udc_vbus_handler() would * set/clear udc->vbus and invoke usb_gadget_pullup_update_locked. ** Add "unbinding" to prevent new connections after the gadget is being * unbound. --- drivers/usb/gadget/udc/core.c | 269 ++++++++++++++++------------------ 1 file changed, 124 insertions(+), 145 deletions(-)