| Message ID | 20230405093133.1858140-1-badhri@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/2] usb: gadget: udc: core: Invoke usb_gadget_connect only when started | expand |
On Wed, Apr 05, 2023 at 09:31:32AM +0000, Badhri Jagan Sridharan wrote: > usb_udc_connect_control does not check to see if the udc > has already been started. This causes gadget->ops->pullup > to be called through usb_gadget_connect when invoked > from usb_udc_vbus_handler even before usb_gadget_udc_start > is called. Guard this by checking for udc->started in > usb_udc_connect_control before invoking usb_gadget_connect. > > Guarding udc_connect_control, udc->started and udc->vbus > with its own mutex as usb_udc_connect_control_locked > can be simulataneously invoked from different code paths. You have a full 72 columns, please use them all :) > > Cc: stable@vger.kernel.org > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler") No blank line after cc: stable, and put the fixes above your signed-off-by line please. > --- > drivers/usb/gadget/udc/core.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > index 3dcbba739db6..890f92cb6344 100644 > --- a/drivers/usb/gadget/udc/core.c > +++ b/drivers/usb/gadget/udc/core.c > @@ -56,6 +56,8 @@ static LIST_HEAD(udc_list); > /* Protects udc_list, udc->driver, driver->is_bound, and related calls */ > static DEFINE_MUTEX(udc_lock); > > +/* Protects udc->vbus, udc-started and udc_connect_control_locked */ > +static DEFINE_MUTEX(udc_connect_control_lock); Why a global lock? Shouldn't this be a per-device lock? > /* ------------------------------------------------------------------------- */ > > /** > @@ -1078,9 +1080,10 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state); > > /* ------------------------------------------------------------------------- */ > > -static void usb_udc_connect_control(struct usb_udc *udc) > +/* Acquire udc_connect_control_lock before calling this function. */ > +static void usb_udc_connect_control_locked(struct usb_udc *udc) > { > - if (udc->vbus) > + if (udc->vbus && udc->started) > usb_gadget_connect(udc->gadget); > else > usb_gadget_disconnect(udc->gadget); > @@ -1099,10 +1102,12 @@ void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status) > { > struct usb_udc *udc = gadget->udc; > > + mutex_lock(&udc_connect_control_lock); > if (udc) { > udc->vbus = status; > - usb_udc_connect_control(udc); > + usb_udc_connect_control_locked(udc); > } > + mutex_unlock(&udc_connect_control_lock); > } > EXPORT_SYMBOL_GPL(usb_udc_vbus_handler); > > @@ -1140,14 +1145,18 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) > { > int ret; > > + mutex_lock(&udc_connect_control_lock); > if (udc->started) { > dev_err(&udc->dev, "UDC had already started\n"); > + mutex_unlock(&udc_connect_control_lock); > return -EBUSY; > } > > ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver); > if (!ret) > udc->started = true; > + usb_udc_connect_control_locked(udc); > + mutex_unlock(&udc_connect_control_lock); > > return ret; > } > @@ -1165,13 +1174,17 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) > */ > static inline void usb_gadget_udc_stop(struct usb_udc *udc) > { > + mutex_lock(&udc_connect_control_lock); > if (!udc->started) { > dev_err(&udc->dev, "UDC had already stopped\n"); > + mutex_unlock(&udc_connect_control_lock); > return; > } > > udc->gadget->ops->udc_stop(udc->gadget); > udc->started = false; > + usb_udc_connect_control_locked(udc); > + mutex_unlock(&udc_connect_control_lock); > } > > /** > @@ -1527,7 +1540,6 @@ static int gadget_bind_driver(struct device *dev) > if (ret) > goto err_start; > usb_gadget_enable_async_callbacks(udc); > - usb_udc_connect_control(udc); Why drop this call here? thanks, greg k-h
On Wed, Apr 05, 2023 at 09:31:32AM +0000, Badhri Jagan Sridharan wrote: > usb_udc_connect_control does not check to see if the udc > has already been started. This causes gadget->ops->pullup > to be called through usb_gadget_connect when invoked > from usb_udc_vbus_handler even before usb_gadget_udc_start > is called. Guard this by checking for udc->started in > usb_udc_connect_control before invoking usb_gadget_connect. > > Guarding udc_connect_control, udc->started and udc->vbus > with its own mutex as usb_udc_connect_control_locked > can be simulataneously invoked from different code paths. > > Cc: stable@vger.kernel.org > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler") There's a problem with this patch. > --- > drivers/usb/gadget/udc/core.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > index 3dcbba739db6..890f92cb6344 100644 > --- a/drivers/usb/gadget/udc/core.c > +++ b/drivers/usb/gadget/udc/core.c > @@ -1140,14 +1145,18 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) > { > int ret; > > + mutex_lock(&udc_connect_control_lock); > if (udc->started) { > dev_err(&udc->dev, "UDC had already started\n"); > + mutex_unlock(&udc_connect_control_lock); > return -EBUSY; > } > > ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver); > if (!ret) > udc->started = true; > + usb_udc_connect_control_locked(udc); > + mutex_unlock(&udc_connect_control_lock); You moved the connect_control call up here, into usb_gadget_udc_start(). > return ret; > } > @@ -1165,13 +1174,17 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) > */ > static inline void usb_gadget_udc_stop(struct usb_udc *udc) > { > + mutex_lock(&udc_connect_control_lock); > if (!udc->started) { > dev_err(&udc->dev, "UDC had already stopped\n"); > + mutex_unlock(&udc_connect_control_lock); > return; > } > > udc->gadget->ops->udc_stop(udc->gadget); > udc->started = false; > + usb_udc_connect_control_locked(udc); > + mutex_unlock(&udc_connect_control_lock); > } > > /** > @@ -1527,7 +1540,6 @@ static int gadget_bind_driver(struct device *dev) > if (ret) > goto err_start; > usb_gadget_enable_async_callbacks(udc); > - usb_udc_connect_control(udc); This is where it used to be. The problem is that in the gadget_bind_driver pathway, usb_gadget_enable_async_callbacks() has to run _before_ the gadget connects. Maybe you can fix this by leaving the function call in its original location and protecting it with the new mutex? There may be a similar problem with disconnecting and the gadget_unbind_driver pathway (usb_gadget_disable_async_callbacks() has to run _after_ the disconnect occurs). I haven't tried to follow the patch in enough detail to see whether that's an issue. Alan Stern > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > return 0; > > base-commit: d629c0e221cd99198b843d8351a0a9bfec6c0423 > -- > 2.40.0.348.gf938b09366-goog >
> No blank line after cc: stable, and put the fixes above your signed-off-by line please. Fixed all commit message related concerns. > Why a global lock? Shouldn't this be a per-device lock? Ack ! Addressed this in V2. On Wed, Apr 5, 2023 at 6:29 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Wed, Apr 05, 2023 at 09:31:32AM +0000, Badhri Jagan Sridharan wrote: > > usb_udc_connect_control does not check to see if the udc > > has already been started. This causes gadget->ops->pullup > > to be called through usb_gadget_connect when invoked > > from usb_udc_vbus_handler even before usb_gadget_udc_start > > is called. Guard this by checking for udc->started in > > usb_udc_connect_control before invoking usb_gadget_connect. > > > > Guarding udc_connect_control, udc->started and udc->vbus > > with its own mutex as usb_udc_connect_control_locked > > can be simulataneously invoked from different code paths. > > > > Cc: stable@vger.kernel.org > > > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > > Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler") > > There's a problem with this patch. > > > --- > > drivers/usb/gadget/udc/core.c | 20 ++++++++++++++++---- > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > > index 3dcbba739db6..890f92cb6344 100644 > > --- a/drivers/usb/gadget/udc/core.c > > +++ b/drivers/usb/gadget/udc/core.c > > > @@ -1140,14 +1145,18 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) > > { > > int ret; > > > > + mutex_lock(&udc_connect_control_lock); > > if (udc->started) { > > dev_err(&udc->dev, "UDC had already started\n"); > > + mutex_unlock(&udc_connect_control_lock); > > return -EBUSY; > > } > > > > ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver); > > if (!ret) > > udc->started = true; > > + usb_udc_connect_control_locked(udc); > > + mutex_unlock(&udc_connect_control_lock); > > You moved the connect_control call up here, into usb_gadget_udc_start(). Have moved it back into gadget_bind_driver. > > > return ret; > > } > > @@ -1165,13 +1174,17 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) > > */ > > static inline void usb_gadget_udc_stop(struct usb_udc *udc) > > { > > + mutex_lock(&udc_connect_control_lock); > > if (!udc->started) { > > dev_err(&udc->dev, "UDC had already stopped\n"); > > + mutex_unlock(&udc_connect_control_lock); > > return; > > } > > > > udc->gadget->ops->udc_stop(udc->gadget); > > udc->started = false; > > + usb_udc_connect_control_locked(udc); > > + mutex_unlock(&udc_connect_control_lock); > > } > > > > /** > > @@ -1527,7 +1540,6 @@ static int gadget_bind_driver(struct device *dev) > > if (ret) > > goto err_start; > > usb_gadget_enable_async_callbacks(udc); > > - usb_udc_connect_control(udc); > > This is where it used to be. > > The problem is that in the gadget_bind_driver pathway, > usb_gadget_enable_async_callbacks() has to run _before_ the gadget > connects. Maybe you can fix this by leaving the function call in its > original location and protecting it with the new mutex? > > There may be a similar problem with disconnecting and the > gadget_unbind_driver pathway (usb_gadget_disable_async_callbacks() has to > run _after_ the disconnect occurs). I haven't tried to follow the patch > in enough detail to see whether that's an issue. Thanks for explaining what's the expectation here. I have incorporated the feedback in v2. The new lock now additionally guards gadget->connect and gadget->deactivate as well. Guarding all with the new lock as they are related to one another. I have made sure that the gadget_bind_driver and gadget_unbind_driver sequence remains unaltered. > > Alan Stern > > > > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > > return 0; > > > > base-commit: d629c0e221cd99198b843d8351a0a9bfec6c0423 > > -- > > 2.40.0.348.gf938b09366-goog > >
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index 3dcbba739db6..890f92cb6344 100644 --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -56,6 +56,8 @@ static LIST_HEAD(udc_list); /* Protects udc_list, udc->driver, driver->is_bound, and related calls */ static DEFINE_MUTEX(udc_lock); +/* Protects udc->vbus, udc-started and udc_connect_control_locked */ +static DEFINE_MUTEX(udc_connect_control_lock); /* ------------------------------------------------------------------------- */ /** @@ -1078,9 +1080,10 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state); /* ------------------------------------------------------------------------- */ -static void usb_udc_connect_control(struct usb_udc *udc) +/* Acquire udc_connect_control_lock before calling this function. */ +static void usb_udc_connect_control_locked(struct usb_udc *udc) { - if (udc->vbus) + if (udc->vbus && udc->started) usb_gadget_connect(udc->gadget); else usb_gadget_disconnect(udc->gadget); @@ -1099,10 +1102,12 @@ void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status) { struct usb_udc *udc = gadget->udc; + mutex_lock(&udc_connect_control_lock); if (udc) { udc->vbus = status; - usb_udc_connect_control(udc); + usb_udc_connect_control_locked(udc); } + mutex_unlock(&udc_connect_control_lock); } EXPORT_SYMBOL_GPL(usb_udc_vbus_handler); @@ -1140,14 +1145,18 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) { int ret; + mutex_lock(&udc_connect_control_lock); if (udc->started) { dev_err(&udc->dev, "UDC had already started\n"); + mutex_unlock(&udc_connect_control_lock); return -EBUSY; } ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver); if (!ret) udc->started = true; + usb_udc_connect_control_locked(udc); + mutex_unlock(&udc_connect_control_lock); return ret; } @@ -1165,13 +1174,17 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) */ static inline void usb_gadget_udc_stop(struct usb_udc *udc) { + mutex_lock(&udc_connect_control_lock); if (!udc->started) { dev_err(&udc->dev, "UDC had already stopped\n"); + mutex_unlock(&udc_connect_control_lock); return; } udc->gadget->ops->udc_stop(udc->gadget); udc->started = false; + usb_udc_connect_control_locked(udc); + mutex_unlock(&udc_connect_control_lock); } /** @@ -1527,7 +1540,6 @@ static int gadget_bind_driver(struct device *dev) if (ret) goto err_start; usb_gadget_enable_async_callbacks(udc); - usb_udc_connect_control(udc); kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); return 0;
usb_udc_connect_control does not check to see if the udc has already been started. This causes gadget->ops->pullup to be called through usb_gadget_connect when invoked from usb_udc_vbus_handler even before usb_gadget_udc_start is called. Guard this by checking for udc->started in usb_udc_connect_control before invoking usb_gadget_connect. Guarding udc_connect_control, udc->started and udc->vbus with its own mutex as usb_udc_connect_control_locked can be simulataneously invoked from different code paths. Cc: stable@vger.kernel.org Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler") --- drivers/usb/gadget/udc/core.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) base-commit: d629c0e221cd99198b843d8351a0a9bfec6c0423