usb: udc: indroduce more api for lower gadget driver

PATCH In review Linyu Yuan 2 participants 9 messages Jun 19, 2021 – Jun 21, 2021

Subsystems (2)

USB DesignWare USB3 DRD IP driver

lore

Share & tools

lore

Linyu Yuan

author

2021-06-19 15:44 UTC #0 lore raw

v3 cover usb: udc: indroduce more api for lower gadget driver

void usb_gadget_udc_disconnect(struct usb_gadget *); void usb_gadget_udc_suspend(struct usb_gadget *); void usb_gadget_udc_resume(struct usb_gadget *); int usb_gadget_udc_setup(struct usb_gadget *,

			const struct usb_ctrlrequest *);

dwc3 is first driver to use these apis.

v3: fix mail format
v3: fix mail format

Linyu Yuan (2):
usb: udc: core: hide struct usb_gadget_driver to gadget driver
usb: dwc3: fix race of usb_gadget_driver operation

 drivers/usb/dwc3/core.h       |  2 --
 drivers/usb/dwc3/ep0.c        |  6 +---
 drivers/usb/dwc3/gadget.c     | 53 +++++++++--------------------------
 drivers/usb/gadget/udc/core.c | 47 ++++++++++++++++++++++++++++++-
 include/linux/usb/gadget.h    |  6 ++++
 5 files changed, 66 insertions(+), 48 deletions(-)

Signature · 1 line

2.25.1

Linyu Yuan

author

2021-06-19 15:44 UTC #1 lore raw

PATCH v3 1/2 usb: udc: core: hide struct usb_gadget_driver to gadget driver

currently most gadget driver have a pointer to save struct usb_gadget_driver from upper layer, it allow upper layer set and unset of the pointer.

there is race that upper layer unset the pointer first, but gadget driver use the pointer later,
and it cause system crash due to NULL pointer access.

Signed-off-by Linyu Yuan <[email protected]>

---

v3: no change
v2: no change

 drivers/usb/gadget/udc/core.c | 47 ++++++++++++++++++++++++++++++++++-
 include/linux/usb/gadget.h    |  6 +++++
 2 files changed, 52 insertions(+), 1 deletion(-)

drivers/usb/gadget/udc/core.c +46 -1

index b7f0b1ebaaa8..44af6ff281d3 100644
@@ -1052,6 +1052,45 @@ void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
 }
 EXPORT_SYMBOL_GPL(usb_udc_vbus_handler);
 
+void usb_gadget_udc_disconnect(struct usb_gadget *gadget)
+{
+	struct usb_udc *udc = gadget->udc;
+
+	if (udc && udc->driver && udc->driver->disconnect)
+		udc->driver->disconnect(gadget);
+}
+EXPORT_SYMBOL_GPL(usb_gadget_udc_disconnect);
+
+void usb_gadget_udc_suspend(struct usb_gadget *gadget)
+{
+	struct usb_udc *udc = gadget->udc;
+
+	if (udc && udc->driver && udc->driver->suspend)
+		udc->driver->suspend(gadget);
+}
+EXPORT_SYMBOL_GPL(usb_gadget_udc_suspend);
+
+void usb_gadget_udc_resume(struct usb_gadget *gadget)
+{
+	struct usb_udc *udc = gadget->udc;
+
+	if (udc && udc->driver && udc->driver->resume)
+		udc->driver->resume(gadget);
+}
+EXPORT_SYMBOL_GPL(usb_gadget_udc_resume);
+
+int usb_gadget_udc_setup(struct usb_gadget *gadget,
+			const struct usb_ctrlrequest *ctrl)
+{
+	struct usb_udc *udc = gadget->udc;
+
+	if (udc && udc->driver)
+		return udc->driver->setup(gadget, ctrl);
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_udc_setup);
+
 /**
  * usb_gadget_udc_reset - notifies the udc core that bus reset occurs
  * @gadget: The gadget which bus reset occurs

@@ -1064,7 +1103,13 @@ EXPORT_SYMBOL_GPL(usb_udc_vbus_handler);
 void usb_gadget_udc_reset(struct usb_gadget *gadget,
 		struct usb_gadget_driver *driver)
 {
-	driver->reset(gadget);
+	struct usb_udc *udc = gadget->udc;
+
+	if (driver)
+		driver->reset(gadget);
+	else if (udc && udc->driver)
+		udc->driver->reset(gadget);
+
 	usb_gadget_set_state(gadget, USB_STATE_DEFAULT);
 }
 EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);

include/linux/usb/gadget.h +6 -0

index 75c7538e350a..85f4c87ba5b3 100644
@@ -881,6 +881,12 @@ extern void usb_gadget_set_state(struct usb_gadget *gadget,
 
 /*-------------------------------------------------------------------------*/
 
+extern void usb_gadget_udc_disconnect(struct usb_gadget *gadget);
+extern void usb_gadget_udc_suspend(struct usb_gadget *gadget);
+extern void usb_gadget_udc_resume(struct usb_gadget *gadget);
+extern int usb_gadget_udc_setup(struct usb_gadget *gadget,
+			const struct usb_ctrlrequest *ctrl);
+
 /* utility to tell udc core that the bus reset occurs */
 extern void usb_gadget_udc_reset(struct usb_gadget *gadget,
 		struct usb_gadget_driver *driver);

Signature · 1 line

2.25.1

Alan Stern

2021-06-20 02:13 UTC #2 lore raw

Re: [PATCH v3 1/2] usb: udc: core: hide struct usb_gadget_driver to gadget driver

Quoted 7 lines · Linyu Yuan

currently most gadget driver have a pointer to save struct usb_gadget_driver from upper layer, it allow upper layer set and unset of the pointer.
there is race that upper layer unset the pointer first, but gadget driver use the pointer later,
and it cause system crash due to NULL pointer access.

This race has already been fixed in Greg's usb-next branch. See commit 7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and following commits 04145a03db9d ("USB: UDC: Implement udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:
Implement udc_async_callbacks in net2280").

You just need to write a corresponding patch implementing the async_callbacks op for dwc3.

Alan Stern

[email protected]

author

2021-06-20 03:46 UTC #3 lore raw

Re: [PATCH v3 1/2] usb: udc: core: hide struct usb_gadget_driver to gadget driver

Quoted 9 lines · Alan Stern

Quoted 7 lines · Linyu Yuan
currently most gadget driver have a pointer to save struct usb_gadget_driver from upper layer, it allow upper layer set and unset of the pointer.
there is race that upper layer unset the pointer first, but gadget driver use the pointer later,
and it cause system crash due to NULL pointer access.

Quoted 6 lines

This race has already been fixed in Greg's usb-next branch. See commit 7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and following commits 04145a03db9d ("USB: UDC: Implement udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:
Implement udc_async_callbacks in net2280").

thanks, this is better, lower driver only need change several places.

Quoted 2 lines

You just need to write a corresponding patch implementing the async_callbacks op for dwc3.

yes, i will do.

Quoted 2 lines

Alan Stern

[email protected]

author

2021-06-20 03:53 UTC #4 lore raw

Re: [PATCH v3 1/2] usb: udc: core: hide struct usb_gadget_driver to gadget driver

Quoted 16 lines · On 2021-06-20 11:46, [email protected] wrote:

Quoted 9 lines · Alan Stern
Quoted 7 lines · Linyu Yuan
currently most gadget driver have a pointer to save struct usb_gadget_driver from upper layer, it allow upper layer set and unset of the pointer.
there is race that upper layer unset the pointer first, but gadget driver use the pointer later,
and it cause system crash due to NULL pointer access.
Quoted 6 lines
This race has already been fixed in Greg's usb-next branch. See commit 7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and following commits 04145a03db9d ("USB: UDC: Implement udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:
Implement udc_async_callbacks in net2280").

Quoted 5 lines

thanks, this is better, lower driver only need change several places.
Quoted 2 lines
You just need to write a corresponding patch implementing the async_callbacks op for dwc3.
yes, i will do.
Quoted 1 line

Alan, i want to discuss your suggestion again in b42e8090ba93 ("USB: UDC:
Implement udc_async_callbacks in net2280")

+ if (dev->async_callbacks) { ----> if CPU1 saw this is true
+ spin_unlock(&dev->lock); ---> CPU2 get lock after this unlock,
it will set async_callbacks to false, then follow call also crash, right ?
+ tmp = dev->driver->setup(&dev->gadget, &u.r);
+ spin_lock(&dev->lock);
+ }

Quoted 1 line

Quoted 1 line
Alan Stern

Alan Stern

2021-06-20 13:47 UTC #5 lore raw

Re: [PATCH v3 1/2] usb: udc: core: hide struct usb_gadget_driver to gadget driver

Quoted 33 lines · On Sun, Jun 20, 2021 at 11:53:18AM +0800, [email protected] wrote:

Quoted 17 lines · On 2021-06-20 11:46, [email protected] wrote:
Quoted 9 lines · Alan Stern
Quoted 7 lines · Linyu Yuan
currently most gadget driver have a pointer to save struct usb_gadget_driver from upper layer, it allow upper layer set and unset of the pointer.
there is race that upper layer unset the pointer first, but gadget driver use the pointer later,
and it cause system crash due to NULL pointer access.
Quoted 7 lines
This race has already been fixed in Greg's usb-next branch. See commit
7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and following commits 04145a03db9d ("USB: UDC: Implement udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:
Implement udc_async_callbacks in net2280").
Quoted 5 lines
thanks, this is better, lower driver only need change several places.
Quoted 2 lines
You just need to write a corresponding patch implementing the async_callbacks op for dwc3.
yes, i will do.
Quoted 1 line
Alan, i want to discuss your suggestion again in b42e8090ba93 ("USB: UDC:
Implement udc_async_callbacks in net2280")
+ if (dev->async_callbacks) { ----> if CPU1 saw this
is true
+ spin_unlock(&dev->lock); ---> CPU2 get lock
after this unlock,
it will set async_callbacks to false, then follow call also crash, right ?
+ tmp = dev->driver->setup(&dev->gadget,
&u.r);

Review

Quoted 2 lines

(quoted diff) +2 -0

+                               spin_lock(&dev->lock);
+                       }

No, this is okay. The reason is because usb_gadget_remove_driver (CPU2 in your example) does this:

        usb_gadget_disable_async_callbacks(udc);
        if (udc->gadget->irq)
                synchronize_irq(udc->gadget->irq);
        udc->driver->unbind(udc->gadget);
        usb_gadget_udc_stop(udc);

The synchronize_irq call will make CPU2 wait until CPU1 has finished handling the interrupt for the setup packet. The system won't crash, because dev->driver->setup will be called before unbind and udc_stop instead of after.

Alan Stern

[email protected]

author

2021-06-21 01:37 UTC #6 lore raw

Re: [PATCH v3 1/2] usb: udc: core: hide struct usb_gadget_driver to gadget driver

Quoted 48 lines · Alan Stern

Quoted 35 lines · On Sun, Jun 20, 2021 at 11:53:18AM +0800, [email protected] wrote:
Quoted 17 lines · On 2021-06-20 11:46, [email protected] wrote:
Quoted 9 lines · Alan Stern
Quoted 7 lines · Linyu Yuan
currently most gadget driver have a pointer to save struct usb_gadget_driver from upper layer, it allow upper layer set and unset of the pointer.
there is race that upper layer unset the pointer first, but gadget driver use the pointer later,
and it cause system crash due to NULL pointer access.
Quoted 7 lines
This race has already been fixed in Greg's usb-next branch. See commit
7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and following commits 04145a03db9d ("USB: UDC: Implement udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:
Implement udc_async_callbacks in net2280").
Quoted 5 lines
thanks, this is better, lower driver only need change several places.
Quoted 2 lines
You just need to write a corresponding patch implementing the async_callbacks op for dwc3.
yes, i will do.
Quoted 1 line
Alan, i want to discuss your suggestion again in b42e8090ba93 ("USB: UDC:
Implement udc_async_callbacks in net2280")
+ if (dev->async_callbacks) { ----> if CPU1 saw
this
is true
+ spin_unlock(&dev->lock); ---> CPU2 get
lock
after this unlock,
it will set async_callbacks to false, then follow call also crash, right ?
+ tmp = dev->driver->setup(&dev->gadget,
&u.r);
Review
Quoted 2 lines
(quoted diff) +2 -0
+                               spin_lock(&dev->lock);
+                       }
No, this is okay. The reason is because usb_gadget_remove_driver (CPU2 in your example) does this:
        usb_gadget_disable_async_callbacks(udc);
        if (udc->gadget->irq)
                synchronize_irq(udc->gadget->irq);
        udc->driver->unbind(udc->gadget);
        usb_gadget_udc_stop(udc);

Quoted 4 lines

The synchronize_irq call will make CPU2 wait until CPU1 has finished handling the interrupt for the setup packet. The system won't crash, because dev->driver->setup will be called before unbind and udc_stop instead of after.

still several question,
1. how about suspend calll dev->driver->suspend ?
2. will 04145a03db9d ("USB: UDC: Implement udc_async_callbacks in dummy-hcd") backport to LTS branch ?
3. how about coding style ? so following code
if (foo->gadget_driver && foo->gadget_driver->resume)
change to
if (foo->asnyc_callbacks && foo->gadget_driver->resume)

Quoted 2 lines

Alan Stern

Alan Stern

2021-06-21 13:53 UTC #7 lore raw

Re: [PATCH v3 1/2] usb: udc: core: hide struct usb_gadget_driver to gadget driver

Quoted 51 lines · On Mon, Jun 21, 2021 at 09:37:34AM +0800, [email protected] wrote:

Quoted 50 lines · Alan Stern
Quoted 37 lines · On Sun, Jun 20, 2021 at 11:53:18AM +0800, [email protected] wrote:
Quoted 17 lines · On 2021-06-20 11:46, [email protected] wrote:
Quoted 9 lines · Alan Stern
Quoted 7 lines · Linyu Yuan
currently most gadget driver have a pointer to save struct usb_gadget_driver from upper layer, it allow upper layer set and unset of the pointer.
there is race that upper layer unset the pointer first, but gadget driver use the pointer later,
and it cause system crash due to NULL pointer access.
Quoted 7 lines
This race has already been fixed in Greg's usb-next branch. See commit
7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and following commits 04145a03db9d ("USB: UDC: Implement udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:
Implement udc_async_callbacks in net2280").
Quoted 5 lines
thanks, this is better, lower driver only need change several places.
Quoted 2 lines
You just need to write a corresponding patch implementing the async_callbacks op for dwc3.
yes, i will do.
Quoted 1 line
Alan, i want to discuss your suggestion again in b42e8090ba93 ("USB:
UDC:
Implement udc_async_callbacks in net2280")
+ if (dev->async_callbacks) { ----> if CPU1
saw this
is true
+ spin_unlock(&dev->lock); ---> CPU2
get lock
after this unlock,
it will set async_callbacks to false, then follow call also crash, right ?
+ tmp = dev->driver->setup(&dev->gadget,
&u.r);
Review
Quoted 2 lines
(quoted diff) +2 -0
+                               spin_lock(&dev->lock);
+                       }
No, this is okay. The reason is because usb_gadget_remove_driver (CPU2 in your example) does this:
        usb_gadget_disable_async_callbacks(udc);
        if (udc->gadget->irq)
                synchronize_irq(udc->gadget->irq);
        udc->driver->unbind(udc->gadget);
        usb_gadget_udc_stop(udc);

Quoted 7 lines

Quoted 4 lines
The synchronize_irq call will make CPU2 wait until CPU1 has finished handling the interrupt for the setup packet. The system won't crash, because dev->driver->setup will be called before unbind and udc_stop instead of after.
still several question,
1. how about suspend calll dev->driver->suspend ?

The same reasoning applies. The synchronize_irq call will make CPU2 wait until CPU1 has finished handling the interrupt for the USB bus suspend. The system won't crash, because dev->driver->suspend will be called before unbind and udc_stop instead of after.

Quoted 2 lines

2. will 04145a03db9d ("USB: UDC: Implement udc_async_callbacks in dummy-hcd") backport to LTS branch ?

None of these commits are marked for back-porting to the -stable kernels. The race they fix does not occur often.

If you the commits to be applied to the LTS stable kernels, you can ask Greg KH to do it.

Quoted 4 lines

3. how about coding style ? so following code if (foo->gadget_driver && foo->gadget_driver->resume) change to
if (foo->asnyc_callbacks && foo->gadget_driver->resume)

I don't understand this question.

Alan Stern

Linyu Yuan

author

2021-06-19 15:44 UTC #8 lore raw

PATCH v3 2/2 usb: dwc3: fix race of usb_gadget_driver operation

In reply to Linyu Yuan

there is following race condition,

CPU1 CPU2
dwc3_runtime_suspend() dwc3_gadget_stop()
spin_lock(&dwc->lock)
dwc3_gadget_suspend()
dwc3_disconnect_gadget()
dwc->gadget_driver != NULL
spin_unlock(&dwc->lock)

                              spin_lock(&dwc->lock)
                              dwc->gadget_driver = NULL
                              spin_unlock(&dwc->lock)

dwc->gadget_driver->disconnect(dwc->gadget);

system will crash when access NULL pointer.

this change will remove pointer of upper layer
struct usb_gadget_driver,
use UDC safe api to access necessary function.

Signed-off-by Linyu Yuan <[email protected]>

---

v3: no change
v2: no change

 drivers/usb/dwc3/core.h   |  2 --
 drivers/usb/dwc3/ep0.c    |  6 +----
 drivers/usb/dwc3/gadget.c | 53 ++++++++++-----------------------------
 3 files changed, 14 insertions(+), 47 deletions(-)

drivers/usb/dwc3/core.h +0 -2

index dccdf13b5f9e..4db64d103446 100644
@@ -962,7 +962,6 @@ struct dwc3_scratchpad_array {
  * @ev_buf: struct dwc3_event_buffer pointer
  * @eps: endpoint array
  * @gadget: device side representation of the peripheral controller
- * @gadget_driver: pointer to the gadget driver
  * @clks: array of clocks
  * @num_clks: number of clocks
  * @reset: reset control

@@ -1108,7 +1107,6 @@ struct dwc3 {
 	struct dwc3_ep		*eps[DWC3_ENDPOINTS_NUM];
 
 	struct usb_gadget	*gadget;
-	struct usb_gadget_driver *gadget_driver;
 
 	struct clk_bulk_data	*clks;
 	int			num_clks;

drivers/usb/dwc3/ep0.c +1 -5

index 3cd294264372..89f63be00902 100644
@@ -600,7 +600,7 @@ static int dwc3_ep0_delegate_req(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
 	int ret;
 
 	spin_unlock(&dwc->lock);
-	ret = dwc->gadget_driver->setup(dwc->gadget, ctrl);
+	ret = usb_gadget_udc_setup(dwc->gadget, ctrl);
 	spin_lock(&dwc->lock);
 	return ret;
 }

@@ -795,9 +795,6 @@ static void dwc3_ep0_inspect_setup(struct dwc3 *dwc,
 	int ret = -EINVAL;
 	u32 len;
 
-	if (!dwc->gadget_driver)
-		goto out;
-
 	trace_dwc3_ctrl_req(ctrl);
 
 	len = le16_to_cpu(ctrl->wLength);

@@ -819,7 +816,6 @@ static void dwc3_ep0_inspect_setup(struct dwc3 *dwc,
 	if (ret == USB_GADGET_DELAYED_STATUS)
 		dwc->delayed_status = true;
 
-out:
 	if (ret < 0)
 		dwc3_ep0_stall_and_restart(dwc);
 }

drivers/usb/dwc3/gadget.c +13 ~~-40~~

109 lines not on this page · raw message

Signature · 1 line

2.25.1