Re: [PATCH v4 1/2] usb: gadget: udc: core: Invoke usb_gadget_connect only when started

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2023-04-07 at 03:07 +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>

This patch causes a kernel hang when trying to boot with the
usb/chipidea/udc.c driver.

The call stack below causes the hang:

 - gadget_bind_driver(struct device *dev)
    - mutex_lock(&udc->connect_lock);
    - usb_gadget_udc_start_locked(struct usb_udc *udc)
        - udc->gadget->ops->udc_start(udc->gadget, udc->driver)

At which point we are calling ci_udc_start(..), but with the
connect_lock mutex locked.

ci_udc_start() then calls usb_udc_vbus_handler() which tries to lock
the connect_lock while it's already locked. Resulting in a kernel hang.

Reverting this patch fixes the hang.

Alistair

> ---
> Changes since v3:
> * Make internal gadget_connect/disconnect functions static
> Changes since v2:
> * Added __must_hold marking for connect_lock
> Changes since v1:
> * 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 | 148 ++++++++++++++++++++++++--------
> --
>  1 file changed, 104 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/core.c
> b/drivers/usb/gadget/udc/core.c
> index 3dcbba739db6..af92c2e8e10c 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,9 @@ 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
> connect_lock held. */
> +static int usb_gadget_connect_locked(struct usb_gadget *gadget)
> +       __must_hold(&gadget->udc->connect_lock)
>  {
>         int ret = 0;
>  
> @@ -706,10 +703,12 @@ int usb_gadget_connect(struct usb_gadget
> *gadget)
>                 goto out;
>         }
>  
> -       if (gadget->deactivated) {
> +       if (gadget->deactivated || !gadget->udc->started) {
>                 /*
>                  * If gadget is deactivated we only save new state.
>                  * Gadget will be connected automatically after
> activation.
> +                *
> +                * udc first needs to be started before gadget can be
> pulled up.
>                  */
>                 gadget->connected = true;
>                 goto out;
> @@ -724,22 +723,32 @@ int usb_gadget_connect(struct usb_gadget
> *gadget)
>  
>         return ret;
>  }
> -EXPORT_SYMBOL_GPL(usb_gadget_connect);
>  
>  /**
> - * usb_gadget_disconnect - software-controlled disconnect from USB
> host
> - * @gadget:the peripheral being disconnected
> - *
> - * Disables the D+ (or potentially D-) pullup, which the host may
> see
> - * as a disconnect (when a VBUS session is active).  Not all systems
> - * support software pullup controls.
> + * usb_gadget_connect - software-controlled connect to USB host
> + * @gadget:the peripheral being connected
>   *
> - * Following a successful disconnect, invoke the ->disconnect()
> callback
> - * for the current gadget driver so that UDC drivers don't need to.
> + * 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_disconnect(struct usb_gadget *gadget)
> +int usb_gadget_connect(struct usb_gadget *gadget)
> +{
> +       int ret;
> +
> +       mutex_lock(&gadget->udc->connect_lock);
> +       ret = usb_gadget_connect_locked(gadget);
> +       mutex_unlock(&gadget->udc->connect_lock);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_gadget_connect);
> +
> +/* Internal version of usb_gadget_disconnect needs to be called with
> connect_lock held. */
> +static int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
> +       __must_hold(&gadget->udc->connect_lock)
>  {
>         int ret = 0;
>  
> @@ -751,10 +760,12 @@ int usb_gadget_disconnect(struct usb_gadget
> *gadget)
>         if (!gadget->connected)
>                 goto out;
>  
> -       if (gadget->deactivated) {
> +       if (gadget->deactivated || !gadget->udc->started) {
>                 /*
>                  * If gadget is deactivated we only save new state.
>                  * Gadget will stay disconnected after activation.
> +                *
> +                * udc should have been started before gadget being
> pulled down.
>                  */
>                 gadget->connected = false;
>                 goto out;
> @@ -774,6 +785,30 @@ int usb_gadget_disconnect(struct usb_gadget
> *gadget)
>  
>         return ret;
>  }
> +
> +/**
> + * usb_gadget_disconnect - software-controlled disconnect from USB
> host
> + * @gadget:the peripheral being disconnected
> + *
> + * Disables the D+ (or potentially D-) pullup, which the host may
> see
> + * as a disconnect (when a VBUS session is active).  Not all systems
> + * support software pullup controls.
> + *
> + * Following a successful disconnect, invoke the ->disconnect()
> callback
> + * for the current gadget driver so that UDC drivers don't need to.
> + *
> + * Returns zero on success, else negative errno.
> + */
> +int usb_gadget_disconnect(struct usb_gadget *gadget)
> +{
> +       int ret;
> +
> +       mutex_lock(&gadget->udc->connect_lock);
> +       ret = usb_gadget_disconnect_locked(gadget);
> +       mutex_unlock(&gadget->udc->connect_lock);
> +
> +       return ret;
> +}
>  EXPORT_SYMBOL_GPL(usb_gadget_disconnect);
>  
>  /**
> @@ -794,10 +829,11 @@ int usb_gadget_deactivate(struct usb_gadget
> *gadget)
>         if (gadget->deactivated)
>                 goto out;
>  
> +       mutex_lock(&gadget->udc->connect_lock);
>         if (gadget->connected) {
> -               ret = usb_gadget_disconnect(gadget);
> +               ret = usb_gadget_disconnect_locked(gadget);
>                 if (ret)
> -                       goto out;
> +                       goto unlock;
>  
>                 /*
>                  * If gadget was being connected before deactivation,
> we want
> @@ -807,6 +843,8 @@ int usb_gadget_deactivate(struct usb_gadget
> *gadget)
>         }
>         gadget->deactivated = true;
>  
> +unlock:
> +       mutex_unlock(&gadget->udc->connect_lock);
>  out:
>         trace_usb_gadget_deactivate(gadget, ret);
>  
> @@ -830,6 +868,7 @@ int usb_gadget_activate(struct usb_gadget
> *gadget)
>         if (!gadget->deactivated)
>                 goto out;
>  
> +       mutex_lock(&gadget->udc->connect_lock);
>         gadget->deactivated = false;
>  
>         /*
> @@ -837,7 +876,8 @@ int usb_gadget_activate(struct usb_gadget
> *gadget)
>          * while it was being deactivated, we call
> usb_gadget_connect().
>          */
>         if (gadget->connected)
> -               ret = usb_gadget_connect(gadget);
> +               ret = usb_gadget_connect_locked(gadget);
> +       mutex_unlock(&gadget->udc->connect_lock);
>  
>  out:
>         trace_usb_gadget_activate(gadget, ret);
> @@ -1078,12 +1118,13 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state);
>  
>  /* -----------------------------------------------------------------
> -------- */
>  
> -static void usb_udc_connect_control(struct usb_udc *udc)
> +/* Acquire connect_lock before calling this function. */
> +static void usb_udc_connect_control_locked(struct usb_udc *udc)
> __must_hold(&udc->connect_lock)
>  {
> -       if (udc->vbus)
> -               usb_gadget_connect(udc->gadget);
> +       if (udc->vbus && udc->started)
> +               usb_gadget_connect_locked(udc->gadget);
>         else
> -               usb_gadget_disconnect(udc->gadget);
> +               usb_gadget_disconnect_locked(udc->gadget);
>  }
>  
>  /**
> @@ -1099,10 +1140,12 @@ void usb_udc_vbus_handler(struct usb_gadget
> *gadget, bool status)
>  {
>         struct usb_udc *udc = gadget->udc;
>  
> +       mutex_lock(&udc->connect_lock);
>         if (udc) {
>                 udc->vbus = status;
> -               usb_udc_connect_control(udc);
> +               usb_udc_connect_control_locked(udc);
>         }
> +       mutex_unlock(&udc->connect_lock);
>  }
>  EXPORT_SYMBOL_GPL(usb_udc_vbus_handler);
>  
> @@ -1124,7 +1167,7 @@ void usb_gadget_udc_reset(struct usb_gadget
> *gadget,
>  EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
>  
>  /**
> - * usb_gadget_udc_start - tells usb device controller to start up
> + * usb_gadget_udc_start_locked - tells usb device controller to
> start up
>   * @udc: The UDC to be started
>   *
>   * This call is issued by the UDC Class driver when it's about
> @@ -1135,8 +1178,11 @@ EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
>   * necessary to have it powered on.
>   *
>   * Returns zero on success, else negative errno.
> + *
> + * Caller should acquire connect_lock before invoking this function.
>   */
> -static inline int usb_gadget_udc_start(struct usb_udc *udc)
> +static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
> +       __must_hold(&udc->connect_lock)
>  {
>         int ret;
>  
> @@ -1153,7 +1199,7 @@ static inline int usb_gadget_udc_start(struct
> usb_udc *udc)
>  }
>  
>  /**
> - * usb_gadget_udc_stop - tells usb device controller we don't need
> it anymore
> + * usb_gadget_udc_stop_locked - tells usb device controller we don't
> need it anymore
>   * @udc: The UDC to be stopped
>   *
>   * This call is issued by the UDC Class driver after calling
> @@ -1162,8 +1208,11 @@ static inline int usb_gadget_udc_start(struct
> usb_udc *udc)
>   * The details are implementation specific, but it can go as
>   * far as powering off UDC completely and disable its data
>   * line pullups.
> + *
> + * Caller should acquire connect lock before invoking this function.
>   */
> -static inline void usb_gadget_udc_stop(struct usb_udc *udc)
> +static inline void usb_gadget_udc_stop_locked(struct usb_udc *udc)
> +       __must_hold(&udc->connect_lock)
>  {
>         if (!udc->started) {
>                 dev_err(&udc->dev, "UDC had already stopped\n");
> @@ -1322,6 +1371,7 @@ int usb_add_gadget(struct usb_gadget *gadget)
>  
>         udc->gadget = gadget;
>         gadget->udc = udc;
> +       mutex_init(&udc->connect_lock);
>  
>         udc->started = false;
>  
> @@ -1523,11 +1573,15 @@ static int gadget_bind_driver(struct device
> *dev)
>         if (ret)
>                 goto err_bind;
>  
> -       ret = usb_gadget_udc_start(udc);
> -       if (ret)
> +       mutex_lock(&udc->connect_lock);
> +       ret = usb_gadget_udc_start_locked(udc);
> +       if (ret) {
> +               mutex_unlock(&udc->connect_lock);
>                 goto err_start;
> +       }
>         usb_gadget_enable_async_callbacks(udc);
> -       usb_udc_connect_control(udc);
> +       usb_udc_connect_control_locked(udc);
> +       mutex_unlock(&udc->connect_lock);
>  
>         kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>         return 0;
> @@ -1558,12 +1612,14 @@ static void gadget_unbind_driver(struct
> device *dev)
>  
>         kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>  
> -       usb_gadget_disconnect(gadget);
> +       mutex_lock(&udc->connect_lock);
> +       usb_gadget_disconnect_locked(gadget);
>         usb_gadget_disable_async_callbacks(udc);
>         if (gadget->irq)
>                 synchronize_irq(gadget->irq);
>         udc->driver->unbind(gadget);
> -       usb_gadget_udc_stop(udc);
> +       usb_gadget_udc_stop_locked(udc);
> +       mutex_unlock(&udc->connect_lock);
>  
>         mutex_lock(&udc_lock);
>         driver->is_bound = false;
> @@ -1649,11 +1705,15 @@ static ssize_t soft_connect_store(struct
> device *dev,
>         }
>  
>         if (sysfs_streq(buf, "connect")) {
> -               usb_gadget_udc_start(udc);
> -               usb_gadget_connect(udc->gadget);
> +               mutex_lock(&udc->connect_lock);
> +               usb_gadget_udc_start_locked(udc);
> +               usb_gadget_connect_locked(udc->gadget);
> +               mutex_unlock(&udc->connect_lock);
>         } else if (sysfs_streq(buf, "disconnect")) {
> -               usb_gadget_disconnect(udc->gadget);
> -               usb_gadget_udc_stop(udc);
> +               mutex_lock(&udc->connect_lock);
> +               usb_gadget_disconnect_locked(udc->gadget);
> +               usb_gadget_udc_stop_locked(udc);
> +               mutex_unlock(&udc->connect_lock);
>         } else {
>                 dev_err(dev, "unsupported command '%s'\n", buf);
>                 ret = -EINVAL;
> 
> base-commit: d629c0e221cd99198b843d8351a0a9bfec6c0423





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux