| Message ID | 20230531040203.19295-3-badhri@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5,1/3] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing | expand |
On Wed, May 31, 2023 at 04:02:03AM +0000, Badhri Jagan Sridharan wrote: > UDC should neither be started nor pulled up unless the gadget driver is > bound. The new flag "allow_start" is now set by gadget_bind_driver() > and cleared by gadget_unbind_driver(). usb_gadget_udc_start_locked() > now checks whether allow_start is set before starting the UDC by > invoking the ->udc_start() callback. "allow_start" isn't quite the right name either, because there is a short time interval during which the UDC is started but we don't want to allow it to connect to the host (this occurs in gadget_unbind_driver() between the disconnect call and the usb_gadge_udc_stop call). A more accurate name would be "allow_connect" or "allow_pullup". > Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets") > Cc: stable <stable@kernel.org> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > --- > v5 is the first version in this series. > --- > drivers/usb/gadget/udc/core.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > index 6ffe5fda8bb7..ac9d6186815d 100644 > --- a/drivers/usb/gadget/udc/core.c > +++ b/drivers/usb/gadget/udc/core.c > @@ -37,6 +37,8 @@ static const struct bus_type gadget_bus_type; > * @vbus: for udcs who care about vbus status, this value is real vbus status; > * 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. > + * @allow_start: Indicates whether UDC is allowed to start. Set/cleared by gadget_(un)bind_driver() > + * after gadget driver is bound or unbound. > * @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 As before, wrap the comments around column 76. > @@ -52,6 +54,7 @@ struct usb_udc { > struct list_head list; > bool vbus; > bool started; > + bool allow_start; > struct work_struct vbus_work; > struct mutex connect_lock; > }; > @@ -1204,6 +1207,9 @@ static inline int usb_gadget_udc_start_locked(struct usb_udc *udc) > if (udc->started) { > dev_err(&udc->dev, "UDC had already started\n"); > return -EBUSY; > + } else if (!udc->allow_start) { > + dev_err(&udc->dev, "UDC not allowed to start. Is gadget driver bound ?\n"); > + return -EIO; > } This isn't the right test or the right place. We want to prevent the UDC from connecting to the host, not prevent it from starting. > > ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver); > @@ -1590,6 +1596,7 @@ static int gadget_bind_driver(struct device *dev) > goto err_bind; > > mutex_lock(&udc->connect_lock); > + udc->allow_start = true; > ret = usb_gadget_udc_start_locked(udc); > if (ret) { > mutex_unlock(&udc->connect_lock); > @@ -1630,6 +1637,7 @@ static void gadget_unbind_driver(struct device *dev) > > cancel_work_sync(&udc->vbus_work); > mutex_lock(&udc->connect_lock); > + udc->allow_start = false; > usb_gadget_disconnect_locked(gadget); > usb_gadget_disable_async_callbacks(udc); > if (gadget->irq) Here is where the problem about the vbus work item getting requeued can be fixed. Clear the allow_connect flag before the call to cancel_work_sync(). That way, even if the vbus work item gets queued again after it is cancelled, when it does run it won't do anything. Although, come to think of it, there is still the problem of making sure that the work item doesn't run after the udc has been deallocated. It looks like you will also need to cancel the work item at the end of usb_del_gadget(). At that point the UDC has already been stopped, so it won't call usb_hcd_vbus_handler() again. Alan Stern
On Wed, May 31, 2023 at 10:40 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Wed, May 31, 2023 at 04:02:03AM +0000, Badhri Jagan Sridharan wrote: > > UDC should neither be started nor pulled up unless the gadget driver is > > bound. The new flag "allow_start" is now set by gadget_bind_driver() > > and cleared by gadget_unbind_driver(). usb_gadget_udc_start_locked() > > now checks whether allow_start is set before starting the UDC by > > invoking the ->udc_start() callback. > > "allow_start" isn't quite the right name either, because there is a > short time interval during which the UDC is started but we don't want > to allow it to connect to the host (this occurs in > gadget_unbind_driver() between the disconnect call and the > usb_gadge_udc_stop call). A more accurate name would be "allow_connect" > or "allow_pullup". Sure, Have renamed the flag to "allow_connect". > > > Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets") > > Cc: stable <stable@kernel.org> > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > > --- > > v5 is the first version in this series. > > --- > > drivers/usb/gadget/udc/core.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > > index 6ffe5fda8bb7..ac9d6186815d 100644 > > --- a/drivers/usb/gadget/udc/core.c > > +++ b/drivers/usb/gadget/udc/core.c > > @@ -37,6 +37,8 @@ static const struct bus_type gadget_bus_type; > > * @vbus: for udcs who care about vbus status, this value is real vbus status; > > * 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. > > + * @allow_start: Indicates whether UDC is allowed to start. Set/cleared by gadget_(un)bind_driver() > > + * after gadget driver is bound or unbound. > > * @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 > > As before, wrap the comments around column 76. Sounds good ! Have addressed this in v6. Wrapping comments at 76. > > > @@ -52,6 +54,7 @@ struct usb_udc { > > struct list_head list; > > bool vbus; > > bool started; > > + bool allow_start; > > struct work_struct vbus_work; > > struct mutex connect_lock; > > }; > > @@ -1204,6 +1207,9 @@ static inline int usb_gadget_udc_start_locked(struct usb_udc *udc) > > if (udc->started) { > > dev_err(&udc->dev, "UDC had already started\n"); > > return -EBUSY; > > + } else if (!udc->allow_start) { > > + dev_err(&udc->dev, "UDC not allowed to start. Is gadget driver bound ?\n"); > > + return -EIO; > > } > > This isn't the right test or the right place. We want to prevent the > UDC from connecting to the host, not prevent it from starting. > > > > > ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver); > > @@ -1590,6 +1596,7 @@ static int gadget_bind_driver(struct device *dev) > > goto err_bind; > > > > mutex_lock(&udc->connect_lock); > > + udc->allow_start = true; > > ret = usb_gadget_udc_start_locked(udc); > > if (ret) { > > mutex_unlock(&udc->connect_lock); > > @@ -1630,6 +1637,7 @@ static void gadget_unbind_driver(struct device *dev) > > > > cancel_work_sync(&udc->vbus_work); > > mutex_lock(&udc->connect_lock); > > + udc->allow_start = false; > > usb_gadget_disconnect_locked(gadget); > > usb_gadget_disable_async_callbacks(udc); > > if (gadget->irq) > > Here is where the problem about the vbus work item getting requeued can > be fixed. Clear the allow_connect flag before the call to > cancel_work_sync(). That way, even if the vbus work item gets queued > again after it is cancelled, when it does run it won't do anything. > > Although, come to think of it, there is still the problem of making sure > that the work item doesn't run after the udc has been deallocated. It > looks like you will also need to cancel the work item at the end of > usb_del_gadget(). At that point the UDC has already been stopped, so it > won't call usb_hcd_vbus_handler() again. Sure, Have made these changes in V6. Thanks a lot, Badhri > > Alan Stern
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index 6ffe5fda8bb7..ac9d6186815d 100644 --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -37,6 +37,8 @@ static const struct bus_type gadget_bus_type; * @vbus: for udcs who care about vbus status, this value is real vbus status; * 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. + * @allow_start: Indicates whether UDC is allowed to start. Set/cleared by gadget_(un)bind_driver() + * after gadget driver is bound or unbound. * @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 @@ -52,6 +54,7 @@ struct usb_udc { struct list_head list; bool vbus; bool started; + bool allow_start; struct work_struct vbus_work; struct mutex connect_lock; }; @@ -1204,6 +1207,9 @@ static inline int usb_gadget_udc_start_locked(struct usb_udc *udc) if (udc->started) { dev_err(&udc->dev, "UDC had already started\n"); return -EBUSY; + } else if (!udc->allow_start) { + dev_err(&udc->dev, "UDC not allowed to start. Is gadget driver bound ?\n"); + return -EIO; } ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver); @@ -1590,6 +1596,7 @@ static int gadget_bind_driver(struct device *dev) goto err_bind; mutex_lock(&udc->connect_lock); + udc->allow_start = true; ret = usb_gadget_udc_start_locked(udc); if (ret) { mutex_unlock(&udc->connect_lock); @@ -1630,6 +1637,7 @@ static void gadget_unbind_driver(struct device *dev) cancel_work_sync(&udc->vbus_work); mutex_lock(&udc->connect_lock); + udc->allow_start = false; usb_gadget_disconnect_locked(gadget); usb_gadget_disable_async_callbacks(udc); if (gadget->irq)
UDC should neither be started nor pulled up unless the gadget driver is bound. The new flag "allow_start" is now set by gadget_bind_driver() and cleared by gadget_unbind_driver(). usb_gadget_udc_start_locked() now checks whether allow_start is set before starting the UDC by invoking the ->udc_start() callback. Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets") Cc: stable <stable@kernel.org> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> --- v5 is the first version in this series. --- drivers/usb/gadget/udc/core.c | 8 ++++++++ 1 file changed, 8 insertions(+)