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: <20230531040203.19295-1-badhri@google.com> <20230531040203.19295-2-badhri@google.com>
 <618e4f17-2799-4838-a21c-184c9303bef6@rowland.harvard.edu>
In-Reply-To: <618e4f17-2799-4838-a21c-184c9303bef6@rowland.harvard.edu>
From:   Badhri Jagan Sridharan <badhri@google.com>
Date:   Wed, 31 May 2023 15:40:26 -0700
Message-ID: <CAPTae5KrTBa-=JuBDD8iVCx1+Hqd14yWOiCRkAg0+a75Q9QcXA@mail.gmail.com>
Subject: Re: [PATCH v5 2/3] usb: gadget: udc: core: Invoke usb_gadget_connect
 only when started
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     gregkh@linuxfoundation.org, colin.i.king@gmail.com,
        xuetao09@huawei.com, quic_eserrao@quicinc.com,
        water.zhangjiantao@huawei.com, francesco@dolcini.it,
        alistair@alistair23.me, stephan@gerhold.net, bagasdotme@gmail.com,
        luca@z3ntu.xyz, 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, May 31, 2023 at 10:55=E2=80=AFAM Alan Stern <stern@rowland.harvard.=
edu> wrote:
>
> On Wed, May 31, 2023 at 04:02:02AM +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().
>
> After a merged version of patches 1/3 and 3/3 have been applied, it
> seems like most of this will not be needed any more.  Maybe not any of
> it.

Without the connect_lock introduced in this patch, wouldn't the
usb_gadget_connect()/
usb_gadget_disconnect() through soft_connect_store() race against
usb_gadget_connect()/ usb_gadget_disconnect() through
usb_udc_connect_control() ?

On a side note, I am working on merging  patches 1/3 and 3/3.

Thanks,
Badhri

>
> usb_udc_connect_control() gets called from only two places.  One of them
> is in gadget_bind_driver(), where we know that the UDC has been started
> and connecting is allowed.  The other place is the vbus work routine
> queued by usb_udc_vbus_handler().  If that place checks the new
> allow_connect flag before calling usb_gadget_connect(), nothing more
> will be needed.  You just have to make sure that the allow_connect flag
> is set in gadget_bind_driver between the start and connect_control
> calls, and it is cleared in gadget_unbind_driver before the
> cancel_work_sync call.
>
> It's possible that a new mutex will be needed to synchronize accesses to
> the allow_connect flag.  That's something you will have to study and
> decide on.  But if you can avoid adding one, that would be best.
>
> > 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.
>
> It's a general principle of kernel programming that locks protect data,
> not code.  So if this patch were to be accepted, you would have to
> change this description to say that connect_lock guards various flags,
> not various function calls.
>
> Alan Stern