Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230406062549.2461917-1-badhri@google.com> <2023040639-lair-risotto-4693@gregkh>
In-Reply-To: <2023040639-lair-risotto-4693@gregkh>
From:   Badhri Jagan Sridharan <badhri@google.com>
Date:   Thu, 6 Apr 2023 20:10:04 -0700
Message-ID: <CAPTae5LEswD6A34YK3Zkj3AaG0DK-92tCmg40mBZx6EmkWOJMw@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] usb: gadget: udc: core: Invoke usb_gadget_connect
 only when started
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     stern@rowland.harvard.edu, colin.i.king@gmail.com,
        xuetao09@huawei.com, quic_eserrao@quicinc.com,
        water.zhangjiantao@huawei.com, peter.chen@freescale.com,
        balbi@ti.com, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Apr 5, 2023 at 11:37=E2=80=AFPM Greg KH <gregkh@linuxfoundation.org=
> 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 co=
de
> > paths.
> >
> > Adding an additional check to make sure udc is started(udc->started)
> > before pullup callback is invoked.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler")
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> > * 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/cor=
e.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 tr=
ue
> >   * @started: the UDC's started state. True if the UDC had started.
> > + * @connect_lock: protects udc->vbus, udc->started, gadget->connect, g=
adget->deactivate related
> > + * functions. usb_gadget_connect_locked, usb_gadget_disconnect_locked,
> > + * usb_udc_connect_control_locked, usb_gadget_udc_start_locked, usb_ga=
dget_udc_stop_locked are
> > + * called with this lock held.
> >   *
> >   * This represents the internal data structure which is used by the UD=
C-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 sessio=
n
> > - * 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