On Wed, Jun 7, 2023 at 11:26 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxx/. > > commit 16737e78d190 ("usb: gadget: udc: core: Offload usb_udc_vbus_handler processing") > > addresses the crash reported. > > > > Cc: stable@xxxxxxxxxxxxxxx > > Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler") > > Signed-off-by: Badhri Jagan Sridharan <badhri@xxxxxxxxxx> > > --- > > 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
