LinuxLists.cc - [PATCH v2 1/2] usb: gadget: udc: core: Invoke usb_gadget

2023-04-06 06:29:49

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

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: [email protected]
Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler")
Signed-off-by: Badhri Jagan Sridharan <[email protected]>
---
* 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. */
+int usb_gadget_connect_locked(struct usb_gadget *gadget)
{
int ret = 0;

@@ -706,10 +702,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 +722,31 @@ 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 udc->connect_lock held. */
+int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
{
int ret = 0;

@@ -751,10 +758,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 +783,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 +827,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 +841,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 +866,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 +874,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 +1116,13 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state);

/* ------------------------------------------------------------------------- */

-static void usb_udc_connect_control(struct usb_udc *udc)
+/* Acquire udc_connect_control_lock before calling this function. */
+static void usb_udc_connect_control_locked(struct usb_udc *udc)
{
- 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 +1138,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 +1165,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
@@ -1136,7 +1177,7 @@ EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
*
* Returns zero on success, else negative errno.
*/
-static inline int usb_gadget_udc_start(struct usb_udc *udc)
+static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
{
int ret;

@@ -1153,7 +1194,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
@@ -1163,7 +1204,7 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc)
* far as powering off UDC completely and disable its data
* line pullups.
*/
-static inline void usb_gadget_udc_stop(struct usb_udc *udc)
+static inline void usb_gadget_udc_stop_locked(struct usb_udc *udc)
{
if (!udc->started) {
dev_err(&udc->dev, "UDC had already stopped\n");
@@ -1322,6 +1363,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 +1565,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 +1604,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 +1697,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
--
2.40.0.348.gf938b09366-goog

2023-04-06 06:56:37

by Greg Kroah-Hartman

[permalink] [raw]

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

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: [email protected]
> Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler")
> Signed-off-by: Badhri Jagan Sridharan <[email protected]>
> ---
> * 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

2023-04-06 09:33:23

by kernel test robot

[permalink] [raw]

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

2023-04-06 10:27:22

by kernel test robot

[permalink] [raw]

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

Hi Badhri,

kernel test robot noticed the following build warnings:

[auto build test WARNING on d629c0e221cd99198b843d8351a0a9bfec6c0423]

url: https://github.com/intel-lab-lkp/linux/commits/Badhri-Jagan-Sridharan/usb-gadget-udc-core-Prevent-redundant-calls-to-pullup/20230406-142708
base: d629c0e221cd99198b843d8351a0a9bfec6c0423
patch link: https://lore.kernel.org/r/20230406062549.2461917-1-badhri%40google.com
patch subject: [PATCH v2 1/2] usb: gadget: udc: core: Invoke usb_gadget_connect only when started
config: hexagon-randconfig-r041-20230403 (https://download.01.org/0day-ci/archive/20230406/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/2f12e8b0c9bf3d25df88c73b614c3e8d84bd7338
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Badhri-Jagan-Sridharan/usb-gadget-udc-core-Prevent-redundant-calls-to-pullup/20230406-142708
git checkout 2f12e8b0c9bf3d25df88c73b614c3e8d84bd7338
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/usb/gadget/udc/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

In file included from drivers/usb/gadget/udc/core.c:17:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/usb/gadget/udc/core.c:17:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/usb/gadget/udc/core.c:17:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> drivers/usb/gadget/udc/core.c:696:5: warning: no previous prototype for function 'usb_gadget_connect_locked' [-Wmissing-prototypes]
int usb_gadget_connect_locked(struct usb_gadget *gadget)
^
drivers/usb/gadget/udc/core.c:696:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int usb_gadget_connect_locked(struct usb_gadget *gadget)
^
static
>> drivers/usb/gadget/udc/core.c:749:5: warning: no previous prototype for function 'usb_gadget_disconnect_locked' [-Wmissing-prototypes]
int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
^
drivers/usb/gadget/udc/core.c:749:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
^
static
8 warnings generated.

vim +/usb_gadget_connect_locked +696 drivers/usb/gadget/udc/core.c

694
695 /* Internal version of usb_gadget_connect needs to be called with udc_connect_control_lock held. */
> 696 int usb_gadget_connect_locked(struct usb_gadget *gadget)
697 {
698 int ret = 0;
699
700 if (!gadget->ops->pullup) {
701 ret = -EOPNOTSUPP;
702 goto out;
703 }
704
705 if (gadget->deactivated || !gadget->udc->started) {
706 /*
707 * If gadget is deactivated we only save new state.
708 * Gadget will be connected automatically after activation.
709 *
710 * udc first needs to be started before gadget can be pulled up.
711 */
712 gadget->connected = true;
713 goto out;
714 }
715
716 ret = gadget->ops->pullup(gadget, 1);
717 if (!ret)
718 gadget->connected = 1;
719
720 out:
721 trace_usb_gadget_connect(gadget, ret);
722
723 return ret;
724 }
725
726 /**
727 * usb_gadget_connect - software-controlled connect to USB host
728 * @gadget:the peripheral being connected
729 *
730 * Enables the D+ (or potentially D-) pullup. The host will start
731 * enumerating this gadget when the pullup is active and a VBUS session
732 * is active (the link is powered).
733 *
734 * Returns zero on success, else negative errno.
735 */
736 int usb_gadget_connect(struct usb_gadget *gadget)
737 {
738 int ret;
739
740 mutex_lock(&gadget->udc->connect_lock);
741 ret = usb_gadget_connect_locked(gadget);
742 mutex_unlock(&gadget->udc->connect_lock);
743
744 return ret;
745 }
746 EXPORT_SYMBOL_GPL(usb_gadget_connect);
747
748 /* Internal version of usb_gadget_disconnect needs to be called with udc->connect_lock held. */
> 749 int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
750 {
751 int ret = 0;
752
753 if (!gadget->ops->pullup) {
754 ret = -EOPNOTSUPP;
755 goto out;
756 }
757
758 if (!gadget->connected)
759 goto out;
760
761 if (gadget->deactivated || !gadget->udc->started) {
762 /*
763 * If gadget is deactivated we only save new state.
764 * Gadget will stay disconnected after activation.
765 *
766 * udc should have been started before gadget being pulled down.
767 */
768 gadget->connected = false;
769 goto out;
770 }
771
772 ret = gadget->ops->pullup(gadget, 0);
773 if (!ret)
774 gadget->connected = 0;
775
776 mutex_lock(&udc_lock);
777 if (gadget->udc->driver)
778 gadget->udc->driver->disconnect(gadget);
779 mutex_unlock(&udc_lock);
780
781 out:
782 trace_usb_gadget_disconnect(gadget, ret);
783
784 return ret;
785 }
786

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

2023-04-07 03:44:35

by Badhri Jagan Sridharan

[permalink] [raw]

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

On Wed, Apr 5, 2023 at 11:37 PM Greg KH <[email protected]> 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: [email protected]
> > Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler")
> > Signed-off-by: Badhri Jagan Sridharan <[email protected]>
> > ---
> > * 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