4.19-stable review patch. If anyone has any objections, please let me know.
------------------
From: Simon Holesch simon@holesch.de
[ Upstream commit 8b6b386f9aa936ed0c190446c71cf59d4a507690 ]
Skip submitting URBs, when identical requests were already sent in tweak_special_requests(). Instead call the completion handler directly to return the result of the URB.
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_CONFIGURATION 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 simon@holesch.de Acked-by: Shuah Khan skhan@linuxfoundation.org Reviewed-by: Hongren Zheng i@zenithal.me Tested-by: Hongren Zheng i@zenithal.me Link: https://lore.kernel.org/r/20240519141922.171460-1-simon@holesch.de Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Sasha Levin sashal@kernel.org --- drivers/usb/usbip/stub_rx.c | 77 ++++++++++++++++++++++++------------- 1 file changed, 50 insertions(+), 27 deletions(-)
diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c index b88eeaee637a..72501198be45 100644 --- a/drivers/usb/usbip/stub_rx.c +++ b/drivers/usb/usbip/stub_rx.c @@ -144,53 +144,62 @@ static int tweak_set_configuration_cmd(struct urb *urb) if (err && err != -ENODEV) dev_err(&sdev->udev->dev, "can't set config #%d, error %d\n", config, err); - return 0; + return err; }
static int tweak_reset_device_cmd(struct urb *urb) { struct stub_priv *priv = (struct stub_priv *) urb->context; struct stub_device *sdev = priv->sdev; + int err;
dev_info(&urb->dev->dev, "usb_queue_reset_device\n");
- if (usb_lock_device_for_reset(sdev->udev, NULL) < 0) { + err = usb_lock_device_for_reset(sdev->udev, NULL); + if (err < 0) { dev_err(&urb->dev->dev, "could not obtain lock to reset device\n"); - return 0; + return err; } - usb_reset_device(sdev->udev); + err = usb_reset_device(sdev->udev); usb_unlock_device(sdev->udev);
- return 0; + return err; }
/* * clear_halt, set_interface, and set_configuration require special tricks. + * Returns 1 if request was tweaked, 0 otherwise. */ -static void tweak_special_requests(struct urb *urb) +static int tweak_special_requests(struct urb *urb) { + int err; + 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 */ - tweak_clear_halt_cmd(urb); + err = tweak_clear_halt_cmd(urb);
else if (is_set_interface_cmd(urb)) /* tweak set_interface */ - tweak_set_interface_cmd(urb); + err = tweak_set_interface_cmd(urb);
else if (is_set_configuration_cmd(urb)) /* tweak set_configuration */ - tweak_set_configuration_cmd(urb); + err = tweak_set_configuration_cmd(urb);
else if (is_reset_device_cmd(urb)) - tweak_reset_device_cmd(urb); - else + err = tweak_reset_device_cmd(urb); + else { usbip_dbg_stub_rx("no need to tweak\n"); + return 0; + } + + return !err; }
/* @@ -468,6 +477,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 +590,11 @@ 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]); + /* + * all URBs belong to a single PDU, so a global is_tweaked flag is + * enough + */ + is_tweaked = tweak_special_requests(priv->urbs[i]);
masking_bogus_flags(priv->urbs[i]); } @@ -594,22 +607,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]); } }