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 <simon@xxxxxxxxxx>
> Acked-by: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
> ---
> /*
> @@ -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
>
