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;
Date:   Thu, 6 Apr 2023 08:37:07 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Badhri Jagan Sridharan <badhri@google.com>
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
Subject: Re: [PATCH v2 1/2] usb: gadget: udc: core: Invoke usb_gadget_connect
 only when started
Message-ID: <2023040639-lair-risotto-4693@gregkh>
References: <20230406062549.2461917-1-badhri@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230406062549.2461917-1-badhri@google.com>
Precedence: bulk

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@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/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?

thanks,

greg k-h