2023-11-26 23:54:29

by Simon Holesch

Subject: [PATCH] usbip: Don't submit special requests twice

Even though submitting those requests twice should be harmless, there
are USB devices that react poorly to some duplicated requests.

One example is the ChipIdea controller implementation in U-Boot: The
second SET_CONFIURATION request makes U-Boot disable and re-enable all
endpoints. Re-enabling an endpoint in the ChipIdea controller, however,
was broken until U-Boot commit b272c8792502 ("usb: ci: Fix gadget
reinit").

Signed-off-by: Simon Holesch <[email protected]>
---
drivers/usb/usbip/stub_rx.c | 50 ++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index fc01b31bbb87..f3fcc290ce5b 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -167,13 +167,13 @@ static int tweak_reset_device_cmd(struct urb *urb)
/*
* clear_halt, set_interface, and set_configuration require special tricks.
*/
-static void tweak_special_requests(struct urb *urb)
+static int tweak_special_requests(struct urb *urb)
{
if (!urb || !urb->setup_packet)
- return;
+ return 0;

if (usb_pipetype(urb->pipe) != PIPE_CONTROL)
- return;
+ return 0;

if (is_clear_halt_cmd(urb))
/* tweak clear_halt */
@@ -189,8 +189,12 @@ static void tweak_special_requests(struct urb *urb)

else if (is_reset_device_cmd(urb))
tweak_reset_device_cmd(urb);
- else
+ else {
usbip_dbg_stub_rx("no need to tweak\n");
+ return 0;
+ }
+
+ return 1;
}

/*
@@ -468,6 +472,7 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
int support_sg = 1;
int np = 0;
int ret, i;
+ int is_tweaked;

if (pipe == -1)
return;
@@ -580,8 +585,7 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
priv->urbs[i]->pipe = pipe;
priv->urbs[i]->complete = stub_complete;

- /* no need to submit an intercepted request, but harmless? */
- tweak_special_requests(priv->urbs[i]);
+ is_tweaked = tweak_special_requests(priv->urbs[i]);

masking_bogus_flags(priv->urbs[i]);
}
@@ -594,22 +598,32 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,

/* urb is now ready to submit */
for (i = 0; i < priv->num_urbs; i++) {
- ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL);
+ if (!is_tweaked) {
+ ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL);

- if (ret == 0)
- usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
- pdu->base.seqnum);
- else {
- dev_err(&udev->dev, "submit_urb error, %d\n", ret);
- usbip_dump_header(pdu);
- usbip_dump_urb(priv->urbs[i]);
+ if (ret == 0)
+ usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
+ pdu->base.seqnum);
+ else {
+ dev_err(&udev->dev, "submit_urb error, %d\n", ret);
+ usbip_dump_header(pdu);
+ usbip_dump_urb(priv->urbs[i]);

+ /*
+ * Pessimistic.
+ * This connection will be discarded.
+ */
+ usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
+ break;
+ }
+ } else {
/*
- * Pessimistic.
- * This connection will be discarded.
+ * An identical URB was already submitted in
+ * tweak_special_requests(). Skip submitting this URB to not
+ * duplicate the request.
*/
- usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
- break;
+ priv->urbs[i]->status = 0;
+ stub_complete(priv->urbs[i]);
}
}

--
2.43.0

2023-11-27 22:34:17

by Shuah Khan

Subject: Re: [PATCH] usbip: Don't submit special requests twice

On 11/26/23 16:48, Simon Holesch wrote:
> Even though submitting those requests twice should be harmless, there
> are USB devices that react poorly to some duplicated requests.
>

The change looks good to me. I want to see the change explained in
the commit log. I see the information on why the change is needed.

Please add a bit more information on the change.

> One example is the ChipIdea controller implementation in U-Boot: The
> second SET_CONFIURATION request makes U-Boot disable and re-enable all
> endpoints. Re-enabling an endpoint in the ChipIdea controller, however,
> was broken until U-Boot commit b272c8792502 ("usb: ci: Fix gadget
> reinit").
>
> Signed-off-by: Simon Holesch <[email protected]>

thanks,
-- Shuah