2023-05-31 04:05:34

by Badhri Jagan Sridharan

[permalink] [raw]
Subject: [PATCH v5 2/3] 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.

This commit was reverted due to the crash reported in
https://lore.kernel.org/all/[email protected]/.
commit 16737e78d190 ("usb: gadget: udc: core: Offload usb_udc_vbus_handler processing")
addresses the crash reported.

Cc: [email protected]
Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler")
Signed-off-by: Badhri Jagan Sridharan <[email protected]>
---
v5 is the first version in this series.
---
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 44a9f32679b5..6ffe5fda8bb7 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -37,6 +37,10 @@ static const 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.
@@ -49,6 +53,7 @@ struct usb_udc {
bool vbus;
bool started;
struct work_struct vbus_work;
+ struct mutex connect_lock;
};

static struct class *udc_class;
@@ -688,17 +693,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;

@@ -707,10 +704,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;
@@ -725,22 +724,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;

@@ -752,10 +761,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;
@@ -775,6 +786,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);

/**
@@ -795,10 +830,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
@@ -808,6 +844,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);

@@ -831,6 +869,7 @@ int usb_gadget_activate(struct usb_gadget *gadget)
if (!gadget->deactivated)
goto out;

+ mutex_lock(&gadget->udc->connect_lock);
gadget->deactivated = false;

/*
@@ -838,7 +877,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);
@@ -1079,19 +1119,22 @@ 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);
}

static void vbus_event_work(struct work_struct *work)
{
struct usb_udc *udc = container_of(work, struct usb_udc, vbus_work);

- usb_udc_connect_control(udc);
+ mutex_lock(&udc->connect_lock);
+ usb_udc_connect_control_locked(udc);
+ mutex_unlock(&udc->connect_lock);
}

/**
@@ -1139,7 +1182,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
@@ -1150,8 +1193,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;

@@ -1168,7 +1214,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
@@ -1177,8 +1223,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");
@@ -1337,6 +1386,7 @@ int usb_add_gadget(struct usb_gadget *gadget)

udc->gadget = gadget;
gadget->udc = udc;
+ mutex_init(&udc->connect_lock);

udc->started = false;

@@ -1539,11 +1589,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;
@@ -1575,12 +1629,14 @@ static void gadget_unbind_driver(struct device *dev)
kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);

cancel_work_sync(&udc->vbus_work);
- 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;
@@ -1666,11 +1722,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;
--
2.41.0.rc0.172.g3f132b7071-goog



2023-05-31 18:41:24

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH v5 2/3] usb: gadget: udc: core: Invoke usb_gadget_connect only when started

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.

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

2023-05-31 23:14:49

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH v5 2/3] usb: gadget: udc: core: Invoke usb_gadget_connect only when started

On Wed, May 31, 2023 at 03:40:26PM -0700, Badhri Jagan Sridharan wrote:
> On Wed, May 31, 2023 at 10:55 AM Alan Stern <[email protected]> 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() ?

Okay, yes, that's a good point. It needs to be mentioned in the patch
description so that people will understand it is the real reason for
this change.

Alan Stern

2023-05-31 23:23:26

by Badhri Jagan Sridharan

[permalink] [raw]
Subject: Re: [PATCH v5 2/3] usb: gadget: udc: core: Invoke usb_gadget_connect only when started

On Wed, May 31, 2023 at 10:55 AM Alan Stern <[email protected]> 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

2023-06-02 17:41:18

by Badhri Jagan Sridharan

[permalink] [raw]
Subject: Re: [PATCH v5 2/3] usb: gadget: udc: core: Invoke usb_gadget_connect only when started

On Wed, May 31, 2023 at 3:55 PM Alan Stern <[email protected]> wrote:
>
> On Wed, May 31, 2023 at 03:40:26PM -0700, Badhri Jagan Sridharan wrote:
> > On Wed, May 31, 2023 at 10:55 AM Alan Stern <[email protected]> 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() ?
>
> Okay, yes, that's a good point. It needs to be mentioned in the patch
> description so that people will understand it is the real reason for
> this change.

Thanks Alan !
I had posted the v6 version of the series with 1/3 and 3/3 of v5
squashed together. I have made changes to address your concerns in v5.
Instead of adding a new lock, I used the connect_lock in this patch to
protect the allow_connect flag.
Eager to know your thoughts !

Regards,
Badhri

>
> Alan Stern