| Message ID | 20230601031028.544244-2-badhri@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v6,1/2] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing | expand |
This patch looks basically okay. Most of the comments below are concerned only with style or presentation. On Thu, Jun 01, 2023 at 03:10:28AM +0000, Badhri Jagan Sridharan wrote: > usb_udc_connect_control(), soft_connect_store() and > usb_gadget_deactivate() can potentialy race against each other to invoke "potentially" has two 'l's. > usb_gadget_connect()/usb_gadget_disconnect(). To prevent this, guarding s/guarding/guard/ > udc->vbus, udc->started, gadget->allow_connect, gadget->deactivate with Insert "and" before "gadget->deactivate". Also, I don't think the patch guards udc->vbus. After all, that flag can be changed in usb_udc_vbus_handler() without the protection of the mutex. > connect_lock so that ->pullup() is only invoked when gadget driver is > bound, not deactivated and started. "when the gadget", not "when gadget driver". The driver isn't what gets deactivated or started; the gadget is. This would be easier to understand if the last two items were switched: bound, started, and not deactivated. Also, it would help readers if there were some additional text to help separate the end of this sentence from the start of the next one. For example, you might insert "The routines" at the beginning of the next sentence. > 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. > > This commit was reverted due to the crash reported in An earlier version of this commit... > https://lore.kernel.org/all/ZF4BvgsOyoKxdPFF@francesco-nb.int.toradex.com/. > commit 16737e78d190 ("usb: gadget: udc: core: Offload usb_udc_vbus_handler processing") > addresses the crash reported. > > Cc: stable@vger.kernel.org > Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler") > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > --- > v5 is the first version in this series. > Changes since v5: > ** Fixed up commit message > ** Wrapped comments at 76. > --- > drivers/usb/gadget/udc/core.c | 151 ++++++++++++++++++++++++---------- > 1 file changed, 109 insertions(+), 42 deletions(-) > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > index d2e4f78c53e3..b53f74fcad60 100644 > --- a/drivers/usb/gadget/udc/core.c > +++ b/drivers/usb/gadget/udc/core.c > @@ -40,6 +40,11 @@ static const struct bus_type gadget_bus_type; > * @allow_connect: Indicates whether UDC is allowed to be pulled up. > * 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. usb_gadget_connect_locked, Again, separate the two sentences with some extra text. Otherwise the period looks too much like a comma for people to see it easily. > + * 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. > * > * This represents the internal data structure which is used by the UDC-class > * to hold information about udc driver and gadget together. > @@ -53,6 +58,7 @@ struct usb_udc { > bool started; > bool allow_connect; > struct work_struct vbus_work; > + struct mutex connect_lock; > }; > > static struct class *udc_class; > @@ -692,17 +698,12 @@ int usb_gadget_vbus_disconnect(struct usb_gadget *gadget) > } > EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect); > > -/** > - * usb_gadget_connect - software-controlled connect to USB host > - * @gadget:the peripheral being connected > - * > - * Enables the D+ (or potentially D-) pullup. The host will start > - * enumerating this gadget when the pullup is active and a VBUS session > - * is active (the link is powered). > - * > - * Returns zero on success, else negative errno. > +/* > + * Internal version of usb_gadget_connect needs to be called with > + * connect_lock held. I'm not sure you need to say this; it's pretty obvious from the __must_hold() annotation a few lines later. Consider removing this paragraph and the similar paragraphs in the other new internal routines. > */ > -int usb_gadget_connect(struct usb_gadget *gadget) > +static int usb_gadget_connect_locked(struct usb_gadget *gadget) > + __must_hold(&gadget->udc->connect_lock) > { > int ret = 0; > > @@ -711,10 +712,12 @@ int usb_gadget_connect(struct usb_gadget *gadget) > goto out; > } > > - if (gadget->deactivated || !gadget->udc->allow_connect) { > + if (gadget->deactivated || !gadget->udc->allow_connect || !gadget->udc->started) { > /* > * If gadget is deactivated we only save new state. > * Gadget will be connected automatically after activation. This comment is now inaccurate. Change it to say: * If the gadget isn't usable (because it is deactivated, * unbound, or not yet started), we only save the new state. * The gadget will be connected automatically when it is * activated/bound/started. > + * > + * udc first needs to be started before gadget can be pulled up. And then this sentence won't be needed. > */ > gadget->connected = true; > goto out; > @@ -729,22 +732,35 @@ int usb_gadget_connect(struct usb_gadget *gadget) > > return ret; > } > -EXPORT_SYMBOL_GPL(usb_gadget_connect); > > /** > - * usb_gadget_disconnect - software-controlled disconnect from USB host > - * @gadget:the peripheral being disconnected > - * > - * Disables the D+ (or potentially D-) pullup, which the host may see > - * as a disconnect (when a VBUS session is active). Not all systems > - * support software pullup controls. > + * usb_gadget_connect - software-controlled connect to USB host > + * @gadget:the peripheral being connected > * > - * Following a successful disconnect, invoke the ->disconnect() callback > - * for the current gadget driver so that UDC drivers don't need to. > + * Enables the D+ (or potentially D-) pullup. The host will start > + * enumerating this gadget when the pullup is active and a VBUS session > + * is active (the link is powered). > * > * Returns zero on success, else negative errno. > */ > -int usb_gadget_disconnect(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; > +} > +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; > > @@ -756,10 +772,12 @@ int usb_gadget_disconnect(struct usb_gadget *gadget) > if (!gadget->connected) > goto out; > > - if (gadget->deactivated) { > + if (gadget->deactivated || !gadget->udc->started) { Do you really need to add this extra test? After all, if the gadget isn't started then how could the previous test of gadget->connected possibly succeed? In fact, I suspect this entire section of code was always useless, since the gadget couldn't be connected now if it was already deactivated. > /* > * 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. No need to mention this. > */ > gadget->connected = false; > goto out; > @@ -779,6 +797,30 @@ int usb_gadget_disconnect(struct usb_gadget *gadget) > > return ret; > } > + > +/** > + * usb_gadget_disconnect - software-controlled disconnect from USB host > + * @gadget:the peripheral being disconnected > + * > + * Disables the D+ (or potentially D-) pullup, which the host may see > + * as a disconnect (when a VBUS session is active). Not all systems > + * support software pullup controls. > + * > + * Following a successful disconnect, invoke the ->disconnect() callback > + * for the current gadget driver so that UDC drivers don't need to. > + * > + * Returns zero on success, else negative errno. > + */ > +int usb_gadget_disconnect(struct usb_gadget *gadget) > +{ > + int ret; > + > + mutex_lock(&gadget->udc->connect_lock); > + ret = usb_gadget_disconnect_locked(gadget); > + mutex_unlock(&gadget->udc->connect_lock); > + > + return ret; > +} > EXPORT_SYMBOL_GPL(usb_gadget_disconnect); > > /** > @@ -799,10 +841,11 @@ int usb_gadget_deactivate(struct usb_gadget *gadget) > if (gadget->deactivated) > goto out; > > + mutex_lock(&gadget->udc->connect_lock); The mutex should be acquired _before_ the test of gadget->deactivated. Otherwise the the state could change between the time of the test and the time of the mutex_lock(). > if (gadget->connected) { > - ret = usb_gadget_disconnect(gadget); > + ret = usb_gadget_disconnect_locked(gadget); > if (ret) > - goto out; > + goto unlock; > > /* > * If gadget was being connected before deactivation, we want > @@ -812,6 +855,8 @@ int usb_gadget_deactivate(struct usb_gadget *gadget) > } > gadget->deactivated = true; > > +unlock: > + mutex_unlock(&gadget->udc->connect_lock); > out: > trace_usb_gadget_deactivate(gadget, ret); > > @@ -835,6 +880,7 @@ int usb_gadget_activate(struct usb_gadget *gadget) > if (!gadget->deactivated) > goto out; > > + mutex_lock(&gadget->udc->connect_lock); > gadget->deactivated = false; Again, lock the mutex before testing the flag. > > /* > @@ -842,7 +888,8 @@ int usb_gadget_activate(struct usb_gadget *gadget) > * while it was being deactivated, we call usb_gadget_connect(). > */ > if (gadget->connected) > - ret = usb_gadget_connect(gadget); > + ret = usb_gadget_connect_locked(gadget); > + mutex_unlock(&gadget->udc->connect_lock); > > out: > trace_usb_gadget_activate(gadget, ret); > @@ -1083,19 +1130,22 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state); > > /* ------------------------------------------------------------------------- */ > > -static void usb_udc_connect_control(struct usb_udc *udc) > +/* Acquire connect_lock before calling this function. */ > +static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc->connect_lock) > { > if (udc->vbus) > - usb_gadget_connect(udc->gadget); > + usb_gadget_connect_locked(udc->gadget); > else > - usb_gadget_disconnect(udc->gadget); > + usb_gadget_disconnect_locked(udc->gadget); > } > > static void vbus_event_work(struct work_struct *work) > { > struct usb_udc *udc = container_of(work, struct usb_udc, vbus_work); > > - usb_udc_connect_control(udc); > + mutex_lock(&udc->connect_lock); > + usb_udc_connect_control_locked(udc); > + mutex_unlock(&udc->connect_lock); > } > > /** > @@ -1144,7 +1194,7 @@ void usb_gadget_udc_reset(struct usb_gadget *gadget, > EXPORT_SYMBOL_GPL(usb_gadget_udc_reset); > > /** > - * usb_gadget_udc_start - tells usb device controller to start up > + * usb_gadget_udc_start_locked - 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 > @@ -1155,8 +1205,11 @@ 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(struct usb_udc *udc) > +static inline int usb_gadget_udc_start_locked(struct usb_udc *udc) > + __must_hold(&udc->connect_lock) > { > int ret; > > @@ -1173,7 +1226,7 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) > } > > /** > - * usb_gadget_udc_stop - tells usb device controller we don't need it anymore > + * usb_gadget_udc_stop_locked - 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 > @@ -1182,8 +1235,11 @@ static inline int usb_gadget_udc_start(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(struct usb_udc *udc) > +static inline void usb_gadget_udc_stop_locked(struct usb_udc *udc) > + __must_hold(&udc->connect_lock) > { > if (!udc->started) { > dev_err(&udc->dev, "UDC had already stopped\n"); > @@ -1342,6 +1398,7 @@ int usb_add_gadget(struct usb_gadget *gadget) > > udc->gadget = gadget; > gadget->udc = udc; > + mutex_init(&udc->connect_lock); > > udc->started = false; > > @@ -1545,12 +1602,16 @@ static int gadget_bind_driver(struct device *dev) > if (ret) > goto err_bind; > > - ret = usb_gadget_udc_start(udc); > - if (ret) > + mutex_lock(&udc->connect_lock); > + ret = usb_gadget_udc_start_locked(udc); > + if (ret) { > + mutex_unlock(&udc->connect_lock); > goto err_start; > + } > usb_gadget_enable_async_callbacks(udc); > udc->allow_connect = true; > - usb_udc_connect_control(udc); > + usb_udc_connect_control_locked(udc); > + mutex_unlock(&udc->connect_lock); > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > return 0; > @@ -1583,12 +1644,14 @@ static void gadget_unbind_driver(struct device *dev) > > udc->allow_connect = false; > cancel_work_sync(&udc->vbus_work); > - usb_gadget_disconnect(gadget); > + mutex_lock(&udc->connect_lock); > + usb_gadget_disconnect_locked(gadget); > usb_gadget_disable_async_callbacks(udc); > if (gadget->irq) > synchronize_irq(gadget->irq); > udc->driver->unbind(gadget); > - usb_gadget_udc_stop(udc); > + usb_gadget_udc_stop_locked(udc); > + mutex_unlock(&udc->connect_lock); > > mutex_lock(&udc_lock); > driver->is_bound = false; In both these routines, you are careful not to hold the udc->connect_lock and the udc_lock at the same time. Good. > @@ -1674,11 +1737,15 @@ static ssize_t soft_connect_store(struct device *dev, > } > > if (sysfs_streq(buf, "connect")) { > - usb_gadget_udc_start(udc); > - usb_gadget_connect(udc->gadget); > + mutex_lock(&udc->connect_lock); > + usb_gadget_udc_start_locked(udc); > + usb_gadget_connect_locked(udc->gadget); Interesting change of behavior here. Before this patch the connect operation would succeed, even if no gadget driver was bound. Now it won't, which is how it ought to behave. > + mutex_unlock(&udc->connect_lock); > } else if (sysfs_streq(buf, "disconnect")) { > - usb_gadget_disconnect(udc->gadget); > - usb_gadget_udc_stop(udc); > + mutex_lock(&udc->connect_lock); > + usb_gadget_disconnect_locked(udc->gadget); > + usb_gadget_udc_stop_locked(udc); > + mutex_unlock(&udc->connect_lock); > } else { > dev_err(dev, "unsupported command '%s'\n", buf); > ret = -EINVAL; Alan Stern
On Wed, Jun 7, 2023 at 11:26 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > This patch looks basically okay. Most of the comments below are > concerned only with style or presentation. > > On Thu, Jun 01, 2023 at 03:10:28AM +0000, Badhri Jagan Sridharan wrote: > > usb_udc_connect_control(), soft_connect_store() and > > usb_gadget_deactivate() can potentialy race against each other to invoke > > "potentially" has two 'l's. > > > usb_gadget_connect()/usb_gadget_disconnect(). To prevent this, guarding > > s/guarding/guard/ > > > udc->vbus, udc->started, gadget->allow_connect, gadget->deactivate with > > Insert "and" before "gadget->deactivate". > > Also, I don't think the patch guards udc->vbus. After all, that flag > can be changed in usb_udc_vbus_handler() without the protection of the > mutex. > > > connect_lock so that ->pullup() is only invoked when gadget driver is > > bound, not deactivated and started. > > "when the gadget", not "when gadget driver". The driver isn't what gets > deactivated or started; the gadget is. > > This would be easier to understand if the last two items were switched: > > bound, started, and not deactivated. > > Also, it would help readers if there were some additional text to help > separate the end of this sentence from the start of the next one. For > example, you might insert "The routines" at the beginning of the next > sentence. > > > 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. > > > > This commit was reverted due to the crash reported in > > An earlier version of this commit... > > > https://lore.kernel.org/all/ZF4BvgsOyoKxdPFF@francesco-nb.int.toradex.com/. > > commit 16737e78d190 ("usb: gadget: udc: core: Offload usb_udc_vbus_handler processing") > > addresses the crash reported. > > > > Cc: stable@vger.kernel.org > > Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler") > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > > --- > > v5 is the first version in this series. > > Changes since v5: > > ** Fixed up commit message > > ** Wrapped comments at 76. > > --- > > drivers/usb/gadget/udc/core.c | 151 ++++++++++++++++++++++++---------- > > 1 file changed, 109 insertions(+), 42 deletions(-) > > > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > > index d2e4f78c53e3..b53f74fcad60 100644 > > --- a/drivers/usb/gadget/udc/core.c > > +++ b/drivers/usb/gadget/udc/core.c > > @@ -40,6 +40,11 @@ static const struct bus_type gadget_bus_type; > > * @allow_connect: Indicates whether UDC is allowed to be pulled up. > > * 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. usb_gadget_connect_locked, > > Again, separate the two sentences with some extra text. Otherwise the > period looks too much like a comma for people to see it easily. > > > + * 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. > > * > > * This represents the internal data structure which is used by the UDC-class > > * to hold information about udc driver and gadget together. > > @@ -53,6 +58,7 @@ struct usb_udc { > > bool started; > > bool allow_connect; > > struct work_struct vbus_work; > > + struct mutex connect_lock; > > }; > > > > static struct class *udc_class; > > @@ -692,17 +698,12 @@ int usb_gadget_vbus_disconnect(struct usb_gadget *gadget) > > } > > EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect); > > > > -/** > > - * usb_gadget_connect - software-controlled connect to USB host > > - * @gadget:the peripheral being connected > > - * > > - * Enables the D+ (or potentially D-) pullup. The host will start > > - * enumerating this gadget when the pullup is active and a VBUS session > > - * is active (the link is powered). > > - * > > - * Returns zero on success, else negative errno. > > +/* > > + * Internal version of usb_gadget_connect needs to be called with > > + * connect_lock held. > > I'm not sure you need to say this; it's pretty obvious from the > __must_hold() annotation a few lines later. Consider removing this > paragraph and the similar paragraphs in the other new internal routines. > > > */ > > -int usb_gadget_connect(struct usb_gadget *gadget) > > +static int usb_gadget_connect_locked(struct usb_gadget *gadget) > > + __must_hold(&gadget->udc->connect_lock) > > { > > int ret = 0; > > > > @@ -711,10 +712,12 @@ int usb_gadget_connect(struct usb_gadget *gadget) > > goto out; > > } > > > > - if (gadget->deactivated || !gadget->udc->allow_connect) { > > + if (gadget->deactivated || !gadget->udc->allow_connect || !gadget->udc->started) { > > /* > > * If gadget is deactivated we only save new state. > > * Gadget will be connected automatically after activation. > > This comment is now inaccurate. Change it to say: > > * If the gadget isn't usable (because it is deactivated, > * unbound, or not yet started), we only save the new state. > * The gadget will be connected automatically when it is > * activated/bound/started. > > > + * > > + * udc first needs to be started before gadget can be pulled up. > > And then this sentence won't be needed. > > > */ > > gadget->connected = true; > > goto out; > > @@ -729,22 +732,35 @@ int usb_gadget_connect(struct usb_gadget *gadget) > > > > return ret; > > } > > -EXPORT_SYMBOL_GPL(usb_gadget_connect); > > > > /** > > - * usb_gadget_disconnect - software-controlled disconnect from USB host > > - * @gadget:the peripheral being disconnected > > - * > > - * Disables the D+ (or potentially D-) pullup, which the host may see > > - * as a disconnect (when a VBUS session is active). Not all systems > > - * support software pullup controls. > > + * usb_gadget_connect - software-controlled connect to USB host > > + * @gadget:the peripheral being connected > > * > > - * Following a successful disconnect, invoke the ->disconnect() callback > > - * for the current gadget driver so that UDC drivers don't need to. > > + * Enables the D+ (or potentially D-) pullup. The host will start > > + * enumerating this gadget when the pullup is active and a VBUS session > > + * is active (the link is powered). > > * > > * Returns zero on success, else negative errno. > > */ > > -int usb_gadget_disconnect(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; > > +} > > +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; > > > > @@ -756,10 +772,12 @@ int usb_gadget_disconnect(struct usb_gadget *gadget) > > if (!gadget->connected) > > goto out; > > > > - if (gadget->deactivated) { > > + if (gadget->deactivated || !gadget->udc->started) { > > Do you really need to add this extra test? After all, if the gadget > isn't started then how could the previous test of gadget->connected > possibly succeed? > > In fact, I suspect this entire section of code was always useless, since > the gadget couldn't be connected now if it was already deactivated. > Thanks Alan ! Will fix all other comments in v7 but not sure about this one. Although the ->pullup() function will not be called, -> connected flag could actually be set when the gadget is not started. - if (gadget->deactivated || !gadget->udc->allow_connect) { + if (gadget->deactivated || !gadget->udc->allow_connect || !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; This could happen, for instance, when usb_udc_vbus_handler() is invoked after soft_connect_store() disconnects the gadget. Same applies to when usb_gadget_connect() is called after the gadget has been deactivated through usb_gadget_deactivate(). This implies that the checks should be there, right ? > > /* > > * 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. > > No need to mention this. > > > */ > > gadget->connected = false; > > goto out; > > @@ -779,6 +797,30 @@ int usb_gadget_disconnect(struct usb_gadget *gadget) > > > > return ret; > > } > > + > > +/** > > + * usb_gadget_disconnect - software-controlled disconnect from USB host > > + * @gadget:the peripheral being disconnected > > + * > > + * Disables the D+ (or potentially D-) pullup, which the host may see > > + * as a disconnect (when a VBUS session is active). Not all systems > > + * support software pullup controls. > > + * > > + * Following a successful disconnect, invoke the ->disconnect() callback > > + * for the current gadget driver so that UDC drivers don't need to. > > + * > > + * Returns zero on success, else negative errno. > > + */ > > +int usb_gadget_disconnect(struct usb_gadget *gadget) > > +{ > > + int ret; > > + > > + mutex_lock(&gadget->udc->connect_lock); > > + ret = usb_gadget_disconnect_locked(gadget); > > + mutex_unlock(&gadget->udc->connect_lock); > > + > > + return ret; > > +} > > EXPORT_SYMBOL_GPL(usb_gadget_disconnect); > > > > /** > > @@ -799,10 +841,11 @@ int usb_gadget_deactivate(struct usb_gadget *gadget) > > if (gadget->deactivated) > > goto out; > > > > + mutex_lock(&gadget->udc->connect_lock); > > The mutex should be acquired _before_ the test of gadget->deactivated. > Otherwise the the state could change between the time of the test and > the time of the mutex_lock(). > > > if (gadget->connected) { > > - ret = usb_gadget_disconnect(gadget); > > + ret = usb_gadget_disconnect_locked(gadget); > > if (ret) > > - goto out; > > + goto unlock; > > > > /* > > * If gadget was being connected before deactivation, we want > > @@ -812,6 +855,8 @@ int usb_gadget_deactivate(struct usb_gadget *gadget) > > } > > gadget->deactivated = true; > > > > +unlock: > > + mutex_unlock(&gadget->udc->connect_lock); > > out: > > trace_usb_gadget_deactivate(gadget, ret); > > > > @@ -835,6 +880,7 @@ int usb_gadget_activate(struct usb_gadget *gadget) > > if (!gadget->deactivated) > > goto out; > > > > + mutex_lock(&gadget->udc->connect_lock); > > gadget->deactivated = false; > > Again, lock the mutex before testing the flag. > > > > > /* > > @@ -842,7 +888,8 @@ int usb_gadget_activate(struct usb_gadget *gadget) > > * while it was being deactivated, we call usb_gadget_connect(). > > */ > > if (gadget->connected) > > - ret = usb_gadget_connect(gadget); > > + ret = usb_gadget_connect_locked(gadget); > > + mutex_unlock(&gadget->udc->connect_lock); > > > > out: > > trace_usb_gadget_activate(gadget, ret); > > @@ -1083,19 +1130,22 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state); > > > > /* ------------------------------------------------------------------------- */ > > > > -static void usb_udc_connect_control(struct usb_udc *udc) > > +/* Acquire connect_lock before calling this function. */ > > +static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc->connect_lock) > > { > > if (udc->vbus) > > - usb_gadget_connect(udc->gadget); > > + usb_gadget_connect_locked(udc->gadget); > > else > > - usb_gadget_disconnect(udc->gadget); > > + usb_gadget_disconnect_locked(udc->gadget); > > } > > > > static void vbus_event_work(struct work_struct *work) > > { > > struct usb_udc *udc = container_of(work, struct usb_udc, vbus_work); > > > > - usb_udc_connect_control(udc); > > + mutex_lock(&udc->connect_lock); > > + usb_udc_connect_control_locked(udc); > > + mutex_unlock(&udc->connect_lock); > > } > > > > /** > > @@ -1144,7 +1194,7 @@ void usb_gadget_udc_reset(struct usb_gadget *gadget, > > EXPORT_SYMBOL_GPL(usb_gadget_udc_reset); > > > > /** > > - * usb_gadget_udc_start - tells usb device controller to start up > > + * usb_gadget_udc_start_locked - 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 > > @@ -1155,8 +1205,11 @@ 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(struct usb_udc *udc) > > +static inline int usb_gadget_udc_start_locked(struct usb_udc *udc) > > + __must_hold(&udc->connect_lock) > > { > > int ret; > > > > @@ -1173,7 +1226,7 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) > > } > > > > /** > > - * usb_gadget_udc_stop - tells usb device controller we don't need it anymore > > + * usb_gadget_udc_stop_locked - 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 > > @@ -1182,8 +1235,11 @@ static inline int usb_gadget_udc_start(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(struct usb_udc *udc) > > +static inline void usb_gadget_udc_stop_locked(struct usb_udc *udc) > > + __must_hold(&udc->connect_lock) > > { > > if (!udc->started) { > > dev_err(&udc->dev, "UDC had already stopped\n"); > > @@ -1342,6 +1398,7 @@ int usb_add_gadget(struct usb_gadget *gadget) > > > > udc->gadget = gadget; > > gadget->udc = udc; > > + mutex_init(&udc->connect_lock); > > > > udc->started = false; > > > > @@ -1545,12 +1602,16 @@ static int gadget_bind_driver(struct device *dev) > > if (ret) > > goto err_bind; > > > > - ret = usb_gadget_udc_start(udc); > > - if (ret) > > + mutex_lock(&udc->connect_lock); > > + ret = usb_gadget_udc_start_locked(udc); > > + if (ret) { > > + mutex_unlock(&udc->connect_lock); > > goto err_start; > > + } > > usb_gadget_enable_async_callbacks(udc); > > udc->allow_connect = true; > > - usb_udc_connect_control(udc); > > + usb_udc_connect_control_locked(udc); > > + mutex_unlock(&udc->connect_lock); > > > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > > return 0; > > @@ -1583,12 +1644,14 @@ static void gadget_unbind_driver(struct device *dev) > > > > udc->allow_connect = false; > > cancel_work_sync(&udc->vbus_work); > > - usb_gadget_disconnect(gadget); > > + mutex_lock(&udc->connect_lock); > > + usb_gadget_disconnect_locked(gadget); > > usb_gadget_disable_async_callbacks(udc); > > if (gadget->irq) > > synchronize_irq(gadget->irq); > > udc->driver->unbind(gadget); > > - usb_gadget_udc_stop(udc); > > + usb_gadget_udc_stop_locked(udc); > > + mutex_unlock(&udc->connect_lock); > > > > mutex_lock(&udc_lock); > > driver->is_bound = false; > > In both these routines, you are careful not to hold the > udc->connect_lock and the udc_lock at the same time. Good. > > > @@ -1674,11 +1737,15 @@ static ssize_t soft_connect_store(struct device *dev, > > } > > > > if (sysfs_streq(buf, "connect")) { > > - usb_gadget_udc_start(udc); > > - usb_gadget_connect(udc->gadget); > > + mutex_lock(&udc->connect_lock); > > + usb_gadget_udc_start_locked(udc); > > + usb_gadget_connect_locked(udc->gadget); > > Interesting change of behavior here. Before this patch the connect > operation would succeed, even if no gadget driver was bound. Now it > won't, which is how it ought to behave. > > > + mutex_unlock(&udc->connect_lock); > > } else if (sysfs_streq(buf, "disconnect")) { > > - usb_gadget_disconnect(udc->gadget); > > - usb_gadget_udc_stop(udc); > > + mutex_lock(&udc->connect_lock); > > + usb_gadget_disconnect_locked(udc->gadget); > > + usb_gadget_udc_stop_locked(udc); > > + mutex_unlock(&udc->connect_lock); > > } else { > > dev_err(dev, "unsupported command '%s'\n", buf); > > ret = -EINVAL; > > Alan Stern
On Wed, Jun 07, 2023 at 10:17:04PM -0700, Badhri Jagan Sridharan wrote: > On Wed, Jun 7, 2023 at 11:26 AM Alan Stern <stern@rowland.harvard.edu> > wrote: > > > @@ -756,10 +772,12 @@ int usb_gadget_disconnect(struct usb_gadget > > *gadget) > > > if (!gadget->connected) > > > goto out; > > > > > > - if (gadget->deactivated) { > > > + if (gadget->deactivated || !gadget->udc->started) { > > > > Do you really need to add this extra test? After all, if the gadget > > isn't started then how could the previous test of gadget->connected > > possibly succeed? > > > > In fact, I suspect this entire section of code was always useless, since > > the gadget couldn't be connected now if it was already deactivated. > > > > Thanks Alan ! Will fix all other comments in v7 but not sure about this one. > Although the ->pullup() function will not be called, > -> connected flag could actually be set when the gadget is not started. > > - if (gadget->deactivated || !gadget->udc->allow_connect) { > + if (gadget->deactivated || !gadget->udc->allow_connect || > !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; > > This could happen, for instance, when usb_udc_vbus_handler() is invoked > after soft_connect_store() disconnects the gadget. > Same applies to when usb_gadget_connect() is called after the gadget has > been deactivated through usb_gadget_deactivate(). > > This implies that the checks should be there, right ? Yes, you're right; the checks do need to be there. I had forgotten about these possible cases. Ignore that comment. Alan Stern
Thanks Alan ! Just sent out the v7 of the series after fixing all other comments. Regards, Badhri On Thu, Jun 8, 2023 at 8:27 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Wed, Jun 07, 2023 at 10:17:04PM -0700, Badhri Jagan Sridharan wrote: > > On Wed, Jun 7, 2023 at 11:26 AM Alan Stern <stern@rowland.harvard.edu> > > wrote: > > > > @@ -756,10 +772,12 @@ int usb_gadget_disconnect(struct usb_gadget > > > *gadget) > > > > if (!gadget->connected) > > > > goto out; > > > > > > > > - if (gadget->deactivated) { > > > > + if (gadget->deactivated || !gadget->udc->started) { > > > > > > Do you really need to add this extra test? After all, if the gadget > > > isn't started then how could the previous test of gadget->connected > > > possibly succeed? > > > > > > In fact, I suspect this entire section of code was always useless, since > > > the gadget couldn't be connected now if it was already deactivated. > > > > > > > Thanks Alan ! Will fix all other comments in v7 but not sure about this one. > > Although the ->pullup() function will not be called, > > -> connected flag could actually be set when the gadget is not started. > > > > - if (gadget->deactivated || !gadget->udc->allow_connect) { > > + if (gadget->deactivated || !gadget->udc->allow_connect || > > !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; > > > > This could happen, for instance, when usb_udc_vbus_handler() is invoked > > after soft_connect_store() disconnects the gadget. > > Same applies to when usb_gadget_connect() is called after the gadget has > > been deactivated through usb_gadget_deactivate(). > > > > This implies that the checks should be there, right ? > > Yes, you're right; the checks do need to be there. I had forgotten > about these possible cases. Ignore that comment. > > Alan Stern
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index d2e4f78c53e3..b53f74fcad60 100644 --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -40,6 +40,11 @@ static const struct bus_type gadget_bus_type; * @allow_connect: Indicates whether UDC is allowed to be pulled up. * 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. 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. * * This represents the internal data structure which is used by the UDC-class * to hold information about udc driver and gadget together. @@ -53,6 +58,7 @@ struct usb_udc { bool started; bool allow_connect; struct work_struct vbus_work; + struct mutex connect_lock; }; static struct class *udc_class; @@ -692,17 +698,12 @@ int usb_gadget_vbus_disconnect(struct usb_gadget *gadget) } EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect); -/** - * usb_gadget_connect - software-controlled connect to USB host - * @gadget:the peripheral being connected - * - * Enables the D+ (or potentially D-) pullup. The host will start - * enumerating this gadget when the pullup is active and a VBUS session - * is active (the link is powered). - * - * Returns zero on success, else negative errno. +/* + * Internal version of usb_gadget_connect needs to be called with + * connect_lock held. */ -int usb_gadget_connect(struct usb_gadget *gadget) +static int usb_gadget_connect_locked(struct usb_gadget *gadget) + __must_hold(&gadget->udc->connect_lock) { int ret = 0; @@ -711,10 +712,12 @@ int usb_gadget_connect(struct usb_gadget *gadget) goto out; } - if (gadget->deactivated || !gadget->udc->allow_connect) { + if (gadget->deactivated || !gadget->udc->allow_connect || !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; @@ -729,22 +732,35 @@ int usb_gadget_connect(struct usb_gadget *gadget) return ret; } -EXPORT_SYMBOL_GPL(usb_gadget_connect); /** - * usb_gadget_disconnect - software-controlled disconnect from USB host - * @gadget:the peripheral being disconnected - * - * Disables the D+ (or potentially D-) pullup, which the host may see - * as a disconnect (when a VBUS session is active). Not all systems - * support software pullup controls. + * usb_gadget_connect - software-controlled connect to USB host + * @gadget:the peripheral being connected * - * Following a successful disconnect, invoke the ->disconnect() callback - * for the current gadget driver so that UDC drivers don't need to. + * Enables the D+ (or potentially D-) pullup. The host will start + * enumerating this gadget when the pullup is active and a VBUS session + * is active (the link is powered). * * Returns zero on success, else negative errno. */ -int usb_gadget_disconnect(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; +} +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; @@ -756,10 +772,12 @@ int usb_gadget_disconnect(struct usb_gadget *gadget) if (!gadget->connected) goto out; - if (gadget->deactivated) { + 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; @@ -779,6 +797,30 @@ int usb_gadget_disconnect(struct usb_gadget *gadget) return ret; } + +/** + * usb_gadget_disconnect - software-controlled disconnect from USB host + * @gadget:the peripheral being disconnected + * + * Disables the D+ (or potentially D-) pullup, which the host may see + * as a disconnect (when a VBUS session is active). Not all systems + * support software pullup controls. + * + * Following a successful disconnect, invoke the ->disconnect() callback + * for the current gadget driver so that UDC drivers don't need to. + * + * Returns zero on success, else negative errno. + */ +int usb_gadget_disconnect(struct usb_gadget *gadget) +{ + int ret; + + mutex_lock(&gadget->udc->connect_lock); + ret = usb_gadget_disconnect_locked(gadget); + mutex_unlock(&gadget->udc->connect_lock); + + return ret; +} EXPORT_SYMBOL_GPL(usb_gadget_disconnect); /** @@ -799,10 +841,11 @@ int usb_gadget_deactivate(struct usb_gadget *gadget) if (gadget->deactivated) goto out; + mutex_lock(&gadget->udc->connect_lock); if (gadget->connected) { - ret = usb_gadget_disconnect(gadget); + ret = usb_gadget_disconnect_locked(gadget); if (ret) - goto out; + goto unlock; /* * If gadget was being connected before deactivation, we want @@ -812,6 +855,8 @@ int usb_gadget_deactivate(struct usb_gadget *gadget) } gadget->deactivated = true; +unlock: + mutex_unlock(&gadget->udc->connect_lock); out: trace_usb_gadget_deactivate(gadget, ret); @@ -835,6 +880,7 @@ int usb_gadget_activate(struct usb_gadget *gadget) if (!gadget->deactivated) goto out; + mutex_lock(&gadget->udc->connect_lock); gadget->deactivated = false; /* @@ -842,7 +888,8 @@ int usb_gadget_activate(struct usb_gadget *gadget) * while it was being deactivated, we call usb_gadget_connect(). */ if (gadget->connected) - ret = usb_gadget_connect(gadget); + ret = usb_gadget_connect_locked(gadget); + mutex_unlock(&gadget->udc->connect_lock); out: trace_usb_gadget_activate(gadget, ret); @@ -1083,19 +1130,22 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state); /* ------------------------------------------------------------------------- */ -static void usb_udc_connect_control(struct usb_udc *udc) +/* Acquire connect_lock before calling this function. */ +static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc->connect_lock) { if (udc->vbus) - usb_gadget_connect(udc->gadget); + usb_gadget_connect_locked(udc->gadget); else - usb_gadget_disconnect(udc->gadget); + usb_gadget_disconnect_locked(udc->gadget); } static void vbus_event_work(struct work_struct *work) { struct usb_udc *udc = container_of(work, struct usb_udc, vbus_work); - usb_udc_connect_control(udc); + mutex_lock(&udc->connect_lock); + usb_udc_connect_control_locked(udc); + mutex_unlock(&udc->connect_lock); } /** @@ -1144,7 +1194,7 @@ void usb_gadget_udc_reset(struct usb_gadget *gadget, EXPORT_SYMBOL_GPL(usb_gadget_udc_reset); /** - * usb_gadget_udc_start - tells usb device controller to start up + * usb_gadget_udc_start_locked - 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 @@ -1155,8 +1205,11 @@ 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(struct usb_udc *udc) +static inline int usb_gadget_udc_start_locked(struct usb_udc *udc) + __must_hold(&udc->connect_lock) { int ret; @@ -1173,7 +1226,7 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) } /** - * usb_gadget_udc_stop - tells usb device controller we don't need it anymore + * usb_gadget_udc_stop_locked - 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 @@ -1182,8 +1235,11 @@ static inline int usb_gadget_udc_start(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(struct usb_udc *udc) +static inline void usb_gadget_udc_stop_locked(struct usb_udc *udc) + __must_hold(&udc->connect_lock) { if (!udc->started) { dev_err(&udc->dev, "UDC had already stopped\n"); @@ -1342,6 +1398,7 @@ int usb_add_gadget(struct usb_gadget *gadget) udc->gadget = gadget; gadget->udc = udc; + mutex_init(&udc->connect_lock); udc->started = false; @@ -1545,12 +1602,16 @@ static int gadget_bind_driver(struct device *dev) if (ret) goto err_bind; - ret = usb_gadget_udc_start(udc); - if (ret) + mutex_lock(&udc->connect_lock); + ret = usb_gadget_udc_start_locked(udc); + if (ret) { + mutex_unlock(&udc->connect_lock); goto err_start; + } usb_gadget_enable_async_callbacks(udc); udc->allow_connect = true; - usb_udc_connect_control(udc); + usb_udc_connect_control_locked(udc); + mutex_unlock(&udc->connect_lock); kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); return 0; @@ -1583,12 +1644,14 @@ static void gadget_unbind_driver(struct device *dev) udc->allow_connect = false; cancel_work_sync(&udc->vbus_work); - usb_gadget_disconnect(gadget); + mutex_lock(&udc->connect_lock); + usb_gadget_disconnect_locked(gadget); usb_gadget_disable_async_callbacks(udc); if (gadget->irq) synchronize_irq(gadget->irq); udc->driver->unbind(gadget); - usb_gadget_udc_stop(udc); + usb_gadget_udc_stop_locked(udc); + mutex_unlock(&udc->connect_lock); mutex_lock(&udc_lock); driver->is_bound = false; @@ -1674,11 +1737,15 @@ static ssize_t soft_connect_store(struct device *dev, } if (sysfs_streq(buf, "connect")) { - usb_gadget_udc_start(udc); - usb_gadget_connect(udc->gadget); + mutex_lock(&udc->connect_lock); + usb_gadget_udc_start_locked(udc); + usb_gadget_connect_locked(udc->gadget); + mutex_unlock(&udc->connect_lock); } else if (sysfs_streq(buf, "disconnect")) { - usb_gadget_disconnect(udc->gadget); - usb_gadget_udc_stop(udc); + mutex_lock(&udc->connect_lock); + usb_gadget_disconnect_locked(udc->gadget); + usb_gadget_udc_stop_locked(udc); + mutex_unlock(&udc->connect_lock); } else { dev_err(dev, "unsupported command '%s'\n", buf); ret = -EINVAL;
usb_udc_connect_control(), soft_connect_store() and usb_gadget_deactivate() can potentialy race against each other to invoke usb_gadget_connect()/usb_gadget_disconnect(). To prevent this, guarding udc->vbus, udc->started, gadget->allow_connect, gadget->deactivate with connect_lock so that ->pullup() is only invoked when gadget driver is bound, not deactivated and started. 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. This commit was reverted due to the crash reported in https://lore.kernel.org/all/ZF4BvgsOyoKxdPFF@francesco-nb.int.toradex.com/. commit 16737e78d190 ("usb: gadget: udc: core: Offload usb_udc_vbus_handler processing") addresses the crash reported. Cc: stable@vger.kernel.org Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler") Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> --- v5 is the first version in this series. Changes since v5: ** Fixed up commit message ** Wrapped comments at 76. --- drivers/usb/gadget/udc/core.c | 151 ++++++++++++++++++++++++---------- 1 file changed, 109 insertions(+), 42 deletions(-)