On Wed, Apr 5, 2023 at 11:37 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Apr 06, 2023 at 06:25:48AM +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->vbus, udc->started, gadget->connect, gadget->deactivate
> > related functions with connect_lock. 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 as they can be simulataneously invoked from different code
> > paths.
> >
> > Adding an additional check to make sure udc is started(udc->started)
> > before pullup callback is invoked.
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler")
> > Signed-off-by: Badhri Jagan Sridharan <badhri@xxxxxxxxxx>
> > ---
> > * Fixed commit message comments.
> > * Renamed udc_connect_control_lock to connect_lock and made it per
> > device.
> > * udc->vbus, udc->started, gadget->connect, gadget->deactivate are all
> > now guarded by connect_lock.
> > * Code now checks for udc->started to be set before invoking pullup
> > callback.
> > ---
> > drivers/usb/gadget/udc/core.c | 140 +++++++++++++++++++++++-----------
> > 1 file changed, 96 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index 3dcbba739db6..41d3a1998cff 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -37,6 +37,10 @@ static 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.
> > + * @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.
> > *
> > * This represents the internal data structure which is used by the UDC-class
> > * to hold information about udc driver and gadget together.
> > @@ -48,6 +52,7 @@ struct usb_udc {
> > struct list_head list;
> > bool vbus;
> > bool started;
> > + struct mutex connect_lock;
> > };
> >
> > static struct class *udc_class;
> > @@ -687,17 +692,8 @@ 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.
> > - */
> > -int usb_gadget_connect(struct usb_gadget *gadget)
> > +/* Internal version of usb_gadget_connect needs to be called with udc_connect_control_lock held. */
>
> Shouldn't you just use the __must_hold() marking here to document this
> so that the tools can properly check and validate it as well?
Sure ! Have fixed it in v3.
I also made these functions static in v4.
Thanks,
Badhri
>
> thanks,
>
> greg k-h
