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_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]>
Acked-by: Shuah Khan <[email protected]>
---
I tested this patch by building only the usbip-host module and loading it with
the running Kernel. To build I had to revert 6309727ef271 ("kthread: add
kthread_stop_put"), which moves the kthread_stop_put() away from usbip.
$ uname -r
6.6.5-arch1-1
$ make -C "/lib/modules/$(uname -r)/build" M="$PWD/drivers/usb/usbip" usbip-host.ko
$ sudo rmmod usbip-host
$ sudo insmod drivers/usb/usbip/usbip-host.ko
The first test was with my laptop camera. I attached the device while using
usbmon to capture the packages.
sudo usbip bind -b 3-7
sudo usbip attach -r localhost -b 3-7
Without the patch you can see the duplicated requests while attaching:
$ tshark -i usbmon3 -Y "usb.device_address == $(cat /sys/bus/usb/devices/3-7/devnum) && usb.setup.bRequest in {9,11}"
Capturing on 'usbmon3'
77 3.903685 host → 3.11.0 USB 64 SET CONFIGURATION Request
79 3.904687 host → 3.11.0 USB 64 SET CONFIGURATION Request
113 4.068943 host → 3.11.0 USB 64 SET INTERFACE Request
115 4.069126 host → 3.11.0 USB 64 SET INTERFACE Request
4 packets captured
With the patch the duplicate requests are no longer sent:
$ tshark -i usbmon3 -Y "usb.device_address == $(cat /sys/bus/usb/devices/3-7/devnum) && usb.setup.bRequest in {9,11}"
Capturing on 'usbmon3'
79 4.368262 host → 3.12.0 USB 64 SET CONFIGURATION Request
113 4.529754 host → 3.12.0 USB 64 SET INTERFACE Request
2 packets captured
To double check, that I didn't break something, I opened the video stream of
the camera and it still works.
The second test was with the embedded development board, with which I
originally saw the problem (a board with the i.MX 6ULZ SoC). It's
connected to a host with Ubuntu 20.04. To test the patch there (and also
to use it later), I created a usbip-backports project[1], that can build
the USB/IP modules on older Kernels. Once the backported usbip-host
module was loaded, I could successfully flash the board over USB/IP.
[1]: https://github.com/holesch/usbip-backports
Changes in v4:
- fix compile error
Changes in v3:
- handle errors in tweak_* routines: send URB if tweaking fails
Changes in v2:
- explain change in commit message
drivers/usb/usbip/stub_rx.c | 73 +++++++++++++++++++++++--------------
1 file changed, 46 insertions(+), 27 deletions(-)
diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index fc01b31bbb87..c1522a916181 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,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 +603,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
On Sun, Dec 17, 2023 at 08:30:40PM +0100, Simon Holesch wrote:
> 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.
Reproduced the behavior and this patch fixed the bahavior
>
> 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]>
> Acked-by: Shuah Khan <[email protected]>
> ---
> /*
> @@ -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,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]);
One question though, if there are mutiple urbs and one of them is
SET CONFIGURATION, then all of them would not be submitted,
as is_tweaked is a *global* flag instead of a per-urb flag.
Now it is assumed that when the urb is SET CONFIGURATION then
num_urbs is 1. I assume it just happens to be the case and I do
not know if it holds for all scenario.
>
> masking_bogus_flags(priv->urbs[i]);
> }
> @@ -594,22 +603,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
>
On Sun May 5, 2024 at 5:31 PM CEST, Hongren Zheng wrote:
> On Sun, Dec 17, 2023 at 08:30:40PM +0100, Simon Holesch wrote:
> > 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.
>
> Reproduced the behavior and this patch fixed the bahavior
Thank you for testing.
> > @@ -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,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]);
>
> One question though, if there are mutiple urbs and one of them is
> SET CONFIGURATION, then all of them would not be submitted,
> as is_tweaked is a *global* flag instead of a per-urb flag.
>
> Now it is assumed that when the urb is SET CONFIGURATION then
> num_urbs is 1. I assume it just happens to be the case and I do
> not know if it holds for all scenario.
To be honest, I didn't fully understand the num_urbs > 1 case. I assumed
this is for drivers not supporting SG and a long URB is just broken up
into multiple ones.
On Sun, May 05, 2024 at 07:54:36PM +0200, Simon Holesch wrote:
> On Sun May 5, 2024 at 5:31 PM CEST, Hongren Zheng wrote:
> > On Sun, Dec 17, 2023 at 08:30:40PM +0100, Simon Holesch wrote:
> > > 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.
> >
> > Reproduced the behavior and this patch fixed the bahavior
>
> Thank you for testing.
>
> > > @@ -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,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]);
> >
> > One question though, if there are mutiple urbs and one of them is
> > SET CONFIGURATION, then all of them would not be submitted,
> > as is_tweaked is a *global* flag instead of a per-urb flag.
> >
> > Now it is assumed that when the urb is SET CONFIGURATION then
> > num_urbs is 1. I assume it just happens to be the case and I do
> > not know if it holds for all scenario.
>
> To be honest, I didn't fully understand the num_urbs > 1 case. I assumed
> this is for drivers not supporting SG and a long URB is just broken up
> into multiple ones.
I misunderstood that code path. It is indeed multiple URBs for
one PDU when the host controller for the physical device does not support SG.
And one global is_tweaked flag is enough. I think adding a comment here
would make it easier to understand.
Reviewed-By: Hongren Zheng <[email protected]>
Tested-By: Hongren Zheng <[email protected]>