When a usb device is disconnected the transfer service functions bails
before appropraite transfer error flags are set.
This patch sets the appropriate condition code OHCI_CC_DEVICENOTRESPONDING
when a device is disconnected and consequently has no response on the USB bus.

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2081

Signed-off-by: Ryan Wendland <wendland@live.com.au>
---
 hw/usb/hcd-ohci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index acd6016980..8cd25d74af 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -980,7 +980,8 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
         dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
         if (dev == NULL) {
             trace_usb_ohci_td_dev_error();
-            return 1;
+            OHCI_SET_BM(td.flags, TD_CC, OHCI_CC_DEVICENOTRESPONDING);
+            goto exit_and_retire;
         }
         ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
         if (ohci->async_td) {
@@ -1087,6 +1088,7 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
         ed->head |= OHCI_ED_H;
     }
 
+exit_and_retire:
     /* Retire this TD */
     ed->head &= ~OHCI_DPTR_MASK;
     ed->head |= td.next & OHCI_DPTR_MASK;
-- 
2.34.1

On Sat, 22 Jun 2024 at 13:57, Ryan Wendland <wendland@live.com.au> wrote:
>
> When a usb device is disconnected the transfer service functions bails
> before appropraite transfer error flags are set.

(typo: "appropriate")

> This patch sets the appropriate condition code OHCI_CC_DEVICENOTRESPONDING
> when a device is disconnected and consequently has no response on the USB bus.
>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2081
>
> Signed-off-by: Ryan Wendland <wendland@live.com.au>
> ---
>  hw/usb/hcd-ohci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index acd6016980..8cd25d74af 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -980,7 +980,8 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
>          dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
>          if (dev == NULL) {
>              trace_usb_ohci_td_dev_error();
> -            return 1;
> +            OHCI_SET_BM(td.flags, TD_CC, OHCI_CC_DEVICENOTRESPONDING);
> +            goto exit_and_retire;
>          }
>          ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
>          if (ohci->async_td) {
> @@ -1087,6 +1088,7 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
>          ed->head |= OHCI_ED_H;
>      }
>
> +exit_and_retire:
>      /* Retire this TD */
>      ed->head &= ~OHCI_DPTR_MASK;
>      ed->head |= td.next & OHCI_DPTR_MASK;

Thanks for this patch; I have a couple of questions:

(1) Do we also need to do something similar for the call in
ohci_service_iso_td() ?

(2) The error handling path for the other way we can
set the DEVICENOTRESPONDING flag also does:
 * set done_count to 0
 * OR in OCHI_ED_H into ed->head

Do we need to do those things here ? (My guess is "yes".)

thanks
-- PMM