[PATCH 0/5] Misc OHCI clean ups

[PATCH 0/5] Misc OHCI clean ups

[BALATON Zoltan]

Hello,

I have these patches from last October when we've looked at what
causes problems with mac99 and USB. We've found the main problem is
likely not allowing pending packets per endpoint which we did not fix
but these patches came out of debugging that and trying to improve the
device model so eventually the real problem could be fixed more
easily. So these are just clean ups and fixing one potential issue
with isochronous transfers breaking pending async packet but it does
not solve all problems OHCI currently has. I'm sending it anyway as I
don't plan to work further on this so this series could be taken as is
for now.

Regards,

BALATON Zoltan (5):
  usb/ohci: Move trace point and log ep number to help debugging
  usb/ohci: Move cancelling async packet to ohci_stop_endpoints()
  usb/ohci: Move USBPortOps related functions together
  usb/ohci: Merge ohci_async_cancel_device() into ohci_child_detach()
  usb/ohci: Don't use packet from OHCIState for isochronous transfers

 hw/usb/hcd-ohci.c   | 295 +++++++++++++++++++++-----------------------
 hw/usb/trace-events |   2 +-
 2 files changed, 144 insertions(+), 153 deletions(-)

-- 
2.30.2

[PATCH 1/5] usb/ohci: Move trace point and log ep number to help debugging

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/usb/hcd-ohci.c   | 14 +++++++-------
 hw/usb/trace-events |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index a93d6b2e98..f915cc5473 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -1033,21 +1033,21 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
         ohci->async_td = 0;
         ohci->async_complete = false;
     } else {
+        dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
+        if (dev == NULL) {
+            trace_usb_ohci_td_dev_error();
+            return 1;
+        }
+        ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
         if (ohci->async_td) {
             /* ??? The hardware should allow one active packet per
                endpoint.  We only allow one active packet per controller.
                This should be sufficient as long as devices respond in a
                timely manner.
             */
-            trace_usb_ohci_td_too_many_pending();
+            trace_usb_ohci_td_too_many_pending(ep->nr);
             return 1;
         }
-        dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
-        if (dev == NULL) {
-            trace_usb_ohci_td_dev_error();
-            return 1;
-        }
-        ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
         usb_packet_setup(&ohci->usb_packet, pid, ep, 0, addr, !flag_r,
                          OHCI_BM(td.flags, TD_DI) == 0);
         usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, pktlen);
diff --git a/hw/usb/trace-events b/hw/usb/trace-events
index b8287b63f1..9773cb5330 100644
--- a/hw/usb/trace-events
+++ b/hw/usb/trace-events
@@ -51,7 +51,7 @@ usb_ohci_td_skip_async(void) ""
 usb_ohci_td_pkt_hdr(uint32_t addr, int64_t pktlen, int64_t len, const char *s, int flag_r, uint32_t cbp, uint32_t be) " TD @ 0x%.8x %" PRId64 " of %" PRId64 " bytes %s r=%d cbp=0x%.8x be=0x%.8x"
 usb_ohci_td_pkt_short(const char *dir, const char *buf) "%s data: %s"
 usb_ohci_td_pkt_full(const char *dir, const char *buf) "%s data: %s"
-usb_ohci_td_too_many_pending(void) ""
+usb_ohci_td_too_many_pending(int ep) "ep=%d"
 usb_ohci_td_packet_status(int status) "status=%d"
 usb_ohci_ed_read_error(uint32_t addr) "ED read error at 0x%x"
 usb_ohci_ed_pkt(uint32_t cur, int h, int c, uint32_t head, uint32_t tail, uint32_t next) "ED @ 0x%.8x h=%u c=%u\n  head=0x%.8x tailp=0x%.8x next=0x%.8x"
-- 
2.30.2

[PATCH 2/5] usb/ohci: Move cancelling async packet to ohci_stop_endpoints()

[BALATON Zoltan]

This is always done before calling this function so remove duplicated
code and do it within the function at one place.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/usb/hcd-ohci.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index f915cc5473..6d762973eb 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -369,6 +369,10 @@ void ohci_stop_endpoints(OHCIState *ohci)
     USBDevice *dev;
     int i, j;
 
+    if (ohci->async_td) {
+        usb_cancel_packet(&ohci->usb_packet);
+        ohci->async_td = 0;
+    }
     for (i = 0; i < ohci->num_ports; i++) {
         dev = ohci->rhport[i].port.dev;
         if (dev && dev->attached) {
@@ -398,10 +402,6 @@ static void ohci_roothub_reset(OHCIState *ohci)
             usb_port_reset(&port->port);
         }
     }
-    if (ohci->async_td) {
-        usb_cancel_packet(&ohci->usb_packet);
-        ohci->async_td = 0;
-    }
     ohci_stop_endpoints(ohci);
 }
 
@@ -1277,10 +1277,6 @@ static void ohci_frame_boundary(void *opaque)
 
     /* Cancel all pending packets if either of the lists has been disabled.  */
     if (ohci->old_ctl & (~ohci->ctl) & (OHCI_CTL_BLE | OHCI_CTL_CLE)) {
-        if (ohci->async_td) {
-            usb_cancel_packet(&ohci->usb_packet);
-            ohci->async_td = 0;
-        }
         ohci_stop_endpoints(ohci);
     }
     ohci->old_ctl = ohci->ctl;
-- 
2.30.2

[PATCH 3/5] usb/ohci: Move USBPortOps related functions together

[BALATON Zoltan]

This also allows removing two forward declarations

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/usb/hcd-ohci.c | 204 +++++++++++++++++++++++-----------------------
 1 file changed, 100 insertions(+), 104 deletions(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 6d762973eb..d1907afd3f 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -58,8 +58,6 @@ struct ohci_hcca {
 #define ED_WBACK_OFFSET offsetof(struct ohci_ed, head)
 #define ED_WBACK_SIZE   4
 
-static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev);
-
 /* Bitfields for the first word of an Endpoint Desciptor.  */
 #define OHCI_ED_FA_SHIFT  0
 #define OHCI_ED_FA_MASK   (0x7f<<OHCI_ED_FA_SHIFT)
@@ -261,92 +259,6 @@ static inline void ohci_set_interrupt(OHCIState *ohci, uint32_t intr)
     ohci_intr_update(ohci);
 }
 
-/* Attach or detach a device on a root hub port.  */
-static void ohci_attach(USBPort *port1)
-{
-    OHCIState *s = port1->opaque;
-    OHCIPort *port = &s->rhport[port1->index];
-    uint32_t old_state = port->ctrl;
-
-    /* set connect status */
-    port->ctrl |= OHCI_PORT_CCS | OHCI_PORT_CSC;
-
-    /* update speed */
-    if (port->port.dev->speed == USB_SPEED_LOW) {
-        port->ctrl |= OHCI_PORT_LSDA;
-    } else {
-        port->ctrl &= ~OHCI_PORT_LSDA;
-    }
-
-    /* notify of remote-wakeup */
-    if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) {
-        ohci_set_interrupt(s, OHCI_INTR_RD);
-    }
-
-    trace_usb_ohci_port_attach(port1->index);
-
-    if (old_state != port->ctrl) {
-        ohci_set_interrupt(s, OHCI_INTR_RHSC);
-    }
-}
-
-static void ohci_detach(USBPort *port1)
-{
-    OHCIState *s = port1->opaque;
-    OHCIPort *port = &s->rhport[port1->index];
-    uint32_t old_state = port->ctrl;
-
-    ohci_async_cancel_device(s, port1->dev);
-
-    /* set connect status */
-    if (port->ctrl & OHCI_PORT_CCS) {
-        port->ctrl &= ~OHCI_PORT_CCS;
-        port->ctrl |= OHCI_PORT_CSC;
-    }
-    /* disable port */
-    if (port->ctrl & OHCI_PORT_PES) {
-        port->ctrl &= ~OHCI_PORT_PES;
-        port->ctrl |= OHCI_PORT_PESC;
-    }
-    trace_usb_ohci_port_detach(port1->index);
-
-    if (old_state != port->ctrl) {
-        ohci_set_interrupt(s, OHCI_INTR_RHSC);
-    }
-}
-
-static void ohci_wakeup(USBPort *port1)
-{
-    OHCIState *s = port1->opaque;
-    OHCIPort *port = &s->rhport[port1->index];
-    uint32_t intr = 0;
-    if (port->ctrl & OHCI_PORT_PSS) {
-        trace_usb_ohci_port_wakeup(port1->index);
-        port->ctrl |= OHCI_PORT_PSSC;
-        port->ctrl &= ~OHCI_PORT_PSS;
-        intr = OHCI_INTR_RHSC;
-    }
-    /* Note that the controller can be suspended even if this port is not */
-    if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) {
-        trace_usb_ohci_remote_wakeup(s->name);
-        /* This is the one state transition the controller can do by itself */
-        s->ctl &= ~OHCI_CTL_HCFS;
-        s->ctl |= OHCI_USB_RESUME;
-        /* In suspend mode only ResumeDetected is possible, not RHSC:
-         * see the OHCI spec 5.1.2.3.
-         */
-        intr = OHCI_INTR_RD;
-    }
-    ohci_set_interrupt(s, intr);
-}
-
-static void ohci_child_detach(USBPort *port1, USBDevice *child)
-{
-    OHCIState *s = port1->opaque;
-
-    ohci_async_cancel_device(s, child);
-}
-
 static USBDevice *ohci_find_device(OHCIState *ohci, uint8_t addr)
 {
     USBDevice *dev;
@@ -634,17 +546,6 @@ static int ohci_copy_iso_td(OHCIState *ohci,
     return 0;
 }
 
-static void ohci_process_lists(OHCIState *ohci, int completion);
-
-static void ohci_async_complete_packet(USBPort *port, USBPacket *packet)
-{
-    OHCIState *ohci = container_of(packet, OHCIState, usb_packet);
-
-    trace_usb_ohci_async_complete();
-    ohci->async_complete = true;
-    ohci_process_lists(ohci, 1);
-}
-
 #define USUB(a, b) ((int16_t)((uint16_t)(a) - (uint16_t)(b)))
 
 static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
@@ -1789,6 +1690,41 @@ static void ohci_mem_write(void *opaque,
     }
 }
 
+static const MemoryRegionOps ohci_mem_ops = {
+    .read = ohci_mem_read,
+    .write = ohci_mem_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+/* USBPortOps */
+static void ohci_attach(USBPort *port1)
+{
+    OHCIState *s = port1->opaque;
+    OHCIPort *port = &s->rhport[port1->index];
+    uint32_t old_state = port->ctrl;
+
+    /* set connect status */
+    port->ctrl |= OHCI_PORT_CCS | OHCI_PORT_CSC;
+
+    /* update speed */
+    if (port->port.dev->speed == USB_SPEED_LOW) {
+        port->ctrl |= OHCI_PORT_LSDA;
+    } else {
+        port->ctrl &= ~OHCI_PORT_LSDA;
+    }
+
+    /* notify of remote-wakeup */
+    if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) {
+        ohci_set_interrupt(s, OHCI_INTR_RD);
+    }
+
+    trace_usb_ohci_port_attach(port1->index);
+
+    if (old_state != port->ctrl) {
+        ohci_set_interrupt(s, OHCI_INTR_RHSC);
+    }
+}
+
 static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev)
 {
     if (ohci->async_td &&
@@ -1799,11 +1735,71 @@ static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev)
     }
 }
 
-static const MemoryRegionOps ohci_mem_ops = {
-    .read = ohci_mem_read,
-    .write = ohci_mem_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
+static void ohci_child_detach(USBPort *port1, USBDevice *child)
+{
+    OHCIState *s = port1->opaque;
+
+    ohci_async_cancel_device(s, child);
+}
+
+static void ohci_detach(USBPort *port1)
+{
+    OHCIState *s = port1->opaque;
+    OHCIPort *port = &s->rhport[port1->index];
+    uint32_t old_state = port->ctrl;
+
+    ohci_async_cancel_device(s, port1->dev);
+
+    /* set connect status */
+    if (port->ctrl & OHCI_PORT_CCS) {
+        port->ctrl &= ~OHCI_PORT_CCS;
+        port->ctrl |= OHCI_PORT_CSC;
+    }
+    /* disable port */
+    if (port->ctrl & OHCI_PORT_PES) {
+        port->ctrl &= ~OHCI_PORT_PES;
+        port->ctrl |= OHCI_PORT_PESC;
+    }
+    trace_usb_ohci_port_detach(port1->index);
+
+    if (old_state != port->ctrl) {
+        ohci_set_interrupt(s, OHCI_INTR_RHSC);
+    }
+}
+
+static void ohci_wakeup(USBPort *port1)
+{
+    OHCIState *s = port1->opaque;
+    OHCIPort *port = &s->rhport[port1->index];
+    uint32_t intr = 0;
+    if (port->ctrl & OHCI_PORT_PSS) {
+        trace_usb_ohci_port_wakeup(port1->index);
+        port->ctrl |= OHCI_PORT_PSSC;
+        port->ctrl &= ~OHCI_PORT_PSS;
+        intr = OHCI_INTR_RHSC;
+    }
+    /* Note that the controller can be suspended even if this port is not */
+    if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) {
+        trace_usb_ohci_remote_wakeup(s->name);
+        /* This is the one state transition the controller can do by itself */
+        s->ctl &= ~OHCI_CTL_HCFS;
+        s->ctl |= OHCI_USB_RESUME;
+        /* In suspend mode only ResumeDetected is possible, not RHSC:
+         * see the OHCI spec 5.1.2.3.
+         */
+        intr = OHCI_INTR_RD;
+    }
+    ohci_set_interrupt(s, intr);
+}
+
+static void ohci_async_complete_packet(USBPort *port, USBPacket *packet)
+{
+    OHCIState *ohci = container_of(packet, OHCIState, usb_packet);
+
+    trace_usb_ohci_async_complete();
+    ohci->async_complete = true;
+    ohci_process_lists(ohci, 1);
+}
 
 static USBPortOps ohci_port_ops = {
     .attach = ohci_attach,
-- 
2.30.2

Re: [PATCH 3/5] usb/ohci: Move USBPortOps related functions together

[Philippe Mathieu-Daudé via]

On 16/1/22 14:20, BALATON Zoltan wrote:
> This also allows removing two forward declarations
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/usb/hcd-ohci.c | 204 +++++++++++++++++++++++-----------------------
>   1 file changed, 100 insertions(+), 104 deletions(-)

> +static const MemoryRegionOps ohci_mem_ops = {
> +    .read = ohci_mem_read,
> +    .write = ohci_mem_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +/* USBPortOps */
> +static void ohci_attach(USBPort *port1)
> +{
> +    OHCIState *s = port1->opaque;
> +    OHCIPort *port = &s->rhport[port1->index];
> +    uint32_t old_state = port->ctrl;
> +
> +    /* set connect status */
> +    port->ctrl |= OHCI_PORT_CCS | OHCI_PORT_CSC;
> +
> +    /* update speed */
> +    if (port->port.dev->speed == USB_SPEED_LOW) {
> +        port->ctrl |= OHCI_PORT_LSDA;
> +    } else {
> +        port->ctrl &= ~OHCI_PORT_LSDA;
> +    }
> +
> +    /* notify of remote-wakeup */
> +    if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) {
> +        ohci_set_interrupt(s, OHCI_INTR_RD);
> +    }
> +
> +    trace_usb_ohci_port_attach(port1->index);
> +
> +    if (old_state != port->ctrl) {
> +        ohci_set_interrupt(s, OHCI_INTR_RHSC);
> +    }
> +}
> +
>   static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev)
>   {
>       if (ohci->async_td &&
> @@ -1799,11 +1735,71 @@ static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev)
>       }
>   }

We could have ohci_attach() just before ohci*_detach(),
anyhow:

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> -static const MemoryRegionOps ohci_mem_ops = {
> -    .read = ohci_mem_read,
> -    .write = ohci_mem_write,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> -};
> +static void ohci_child_detach(USBPort *port1, USBDevice *child)
> +{
> +    OHCIState *s = port1->opaque;
> +
> +    ohci_async_cancel_device(s, child);
> +}
> +
> +static void ohci_detach(USBPort *port1)
> +{
> +    OHCIState *s = port1->opaque;
> +    OHCIPort *port = &s->rhport[port1->index];
> +    uint32_t old_state = port->ctrl;
> +
> +    ohci_async_cancel_device(s, port1->dev);
> +
> +    /* set connect status */
> +    if (port->ctrl & OHCI_PORT_CCS) {
> +        port->ctrl &= ~OHCI_PORT_CCS;
> +        port->ctrl |= OHCI_PORT_CSC;
> +    }
> +    /* disable port */
> +    if (port->ctrl & OHCI_PORT_PES) {
> +        port->ctrl &= ~OHCI_PORT_PES;
> +        port->ctrl |= OHCI_PORT_PESC;
> +    }
> +    trace_usb_ohci_port_detach(port1->index);
> +
> +    if (old_state != port->ctrl) {
> +        ohci_set_interrupt(s, OHCI_INTR_RHSC);
> +    }
> +}

Re: [PATCH 3/5] usb/ohci: Move USBPortOps related functions together

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2148 bytes --]

On Sun, 16 Jan 2022, Philippe Mathieu-Daudé wrote:
> On 16/1/22 14:20, BALATON Zoltan wrote:
>> This also allows removing two forward declarations
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/usb/hcd-ohci.c | 204 +++++++++++++++++++++++-----------------------
>>   1 file changed, 100 insertions(+), 104 deletions(-)
>
>> +static const MemoryRegionOps ohci_mem_ops = {
>> +    .read = ohci_mem_read,
>> +    .write = ohci_mem_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
>> +
>> +/* USBPortOps */
>> +static void ohci_attach(USBPort *port1)
>> +{
>> +    OHCIState *s = port1->opaque;
>> +    OHCIPort *port = &s->rhport[port1->index];
>> +    uint32_t old_state = port->ctrl;
>> +
>> +    /* set connect status */
>> +    port->ctrl |= OHCI_PORT_CCS | OHCI_PORT_CSC;
>> +
>> +    /* update speed */
>> +    if (port->port.dev->speed == USB_SPEED_LOW) {
>> +        port->ctrl |= OHCI_PORT_LSDA;
>> +    } else {
>> +        port->ctrl &= ~OHCI_PORT_LSDA;
>> +    }
>> +
>> +    /* notify of remote-wakeup */
>> +    if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) {
>> +        ohci_set_interrupt(s, OHCI_INTR_RD);
>> +    }
>> +
>> +    trace_usb_ohci_port_attach(port1->index);
>> +
>> +    if (old_state != port->ctrl) {
>> +        ohci_set_interrupt(s, OHCI_INTR_RHSC);
>> +    }
>> +}
>> +
>>   static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev)
>>   {
>>       if (ohci->async_td &&
>> @@ -1799,11 +1735,71 @@ static void ohci_async_cancel_device(OHCIState 
>> *ohci, USBDevice *dev)
>>       }
>>   }
>
> We could have ohci_attach() just before ohci*_detach(),

The ohci_child_detach() function (which the next patch merges with 
ohci_async_cancel_device) is used by ohci_detach() but not ohci_attach() 
that's why I've put it between attach and detach. Attach and detach are 
less related than ohci_child_detach and ohci_detach in my opinion. Attach 
and detach are still close enough after the next patch but without moving 
child_detach and detach apart.

> anyhow:
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks for the review.

Regards,
BALATON Zoltan

[PATCH 4/5] usb/ohci: Merge ohci_async_cancel_device() into ohci_child_detach()

[BALATON Zoltan]

These two do the same and only used once so no need to have two
functions, simplify by merging them.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/usb/hcd-ohci.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index d1907afd3f..e87f5bd813 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -1725,8 +1725,10 @@ static void ohci_attach(USBPort *port1)
     }
 }
 
-static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev)
+static void ohci_child_detach(USBPort *port1, USBDevice *dev)
 {
+    OHCIState *ohci = port1->opaque;
+
     if (ohci->async_td &&
         usb_packet_is_inflight(&ohci->usb_packet) &&
         ohci->usb_packet.ep->dev == dev) {
@@ -1735,20 +1737,13 @@ static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev)
     }
 }
 
-static void ohci_child_detach(USBPort *port1, USBDevice *child)
-{
-    OHCIState *s = port1->opaque;
-
-    ohci_async_cancel_device(s, child);
-}
-
 static void ohci_detach(USBPort *port1)
 {
     OHCIState *s = port1->opaque;
     OHCIPort *port = &s->rhport[port1->index];
     uint32_t old_state = port->ctrl;
 
-    ohci_async_cancel_device(s, port1->dev);
+    ohci_child_detach(port1, port1->dev);
 
     /* set connect status */
     if (port->ctrl & OHCI_PORT_CCS) {
-- 
2.30.2

Re: [PATCH 4/5] usb/ohci: Merge ohci_async_cancel_device() into ohci_child_detach()

[Philippe Mathieu-Daudé via]

On 16/1/22 14:20, BALATON Zoltan wrote:
> These two do the same and only used once so no need to have two
> functions, simplify by merging them.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/usb/hcd-ohci.c | 13 ++++---------
>   1 file changed, 4 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH 5/5] usb/ohci: Don't use packet from OHCIState for isochronous transfers

[BALATON Zoltan]

Since isochronous transfers cannot be handled async (the function
returns error in that case) we don't need to remember the packet.
Avoid using the usb_packet field in OHCIState (as that can be a
waiting async packet on another endpoint) and allocate and use a local
USBPacket for the iso transfer instead. After this we don't have to
care if we're called from a completion callback or not so we can drop
that parameter as well.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/usb/hcd-ohci.c | 70 +++++++++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 33 deletions(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index e87f5bd813..5e5a93e54f 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -548,8 +548,7 @@ static int ohci_copy_iso_td(OHCIState *ohci,
 
 #define USUB(a, b) ((int16_t)((uint16_t)(a) - (uint16_t)(b)))
 
-static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
-                               int completion)
+static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed)
 {
     int dir;
     size_t len = 0;
@@ -559,6 +558,9 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
     int i;
     USBDevice *dev;
     USBEndpoint *ep;
+    USBPacket *pkt;
+    uint8_t buf[8192];
+    bool int_req;
     struct ohci_iso_td iso_td;
     uint32_t addr;
     uint16_t starting_frame;
@@ -693,40 +695,42 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
     } else {
         len = end_addr - start_addr + 1;
     }
-    if (len > sizeof(ohci->usb_buf)) {
-        len = sizeof(ohci->usb_buf);
+    if (len > sizeof(buf)) {
+        len = sizeof(buf);
     }
 
     if (len && dir != OHCI_TD_DIR_IN) {
-        if (ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len,
+        if (ohci_copy_iso_td(ohci, start_addr, end_addr, buf, len,
                              DMA_DIRECTION_TO_DEVICE)) {
             ohci_die(ohci);
             return 1;
         }
     }
 
-    if (!completion) {
-        bool int_req = relative_frame_number == frame_count &&
-                       OHCI_BM(iso_td.flags, TD_DI) == 0;
-        dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
-        if (dev == NULL) {
-            trace_usb_ohci_td_dev_error();
-            return 1;
-        }
-        ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
-        usb_packet_setup(&ohci->usb_packet, pid, ep, 0, addr, false, int_req);
-        usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, len);
-        usb_handle_packet(dev, &ohci->usb_packet);
-        if (ohci->usb_packet.status == USB_RET_ASYNC) {
-            usb_device_flush_ep_queue(dev, ep);
-            return 1;
-        }
+    dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
+    if (dev == NULL) {
+        trace_usb_ohci_td_dev_error();
+        return 1;
     }
-    if (ohci->usb_packet.status == USB_RET_SUCCESS) {
-        ret = ohci->usb_packet.actual_length;
+    ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
+    pkt = g_new0(USBPacket, 1);
+    usb_packet_init(pkt);
+    int_req = relative_frame_number == frame_count &&
+              OHCI_BM(iso_td.flags, TD_DI) == 0;
+    usb_packet_setup(pkt, pid, ep, 0, addr, false, int_req);
+    usb_packet_addbuf(pkt, buf, len);
+    usb_handle_packet(dev, pkt);
+    if (pkt->status == USB_RET_ASYNC) {
+        usb_device_flush_ep_queue(dev, ep);
+        g_free(pkt);
+        return 1;
+    }
+    if (pkt->status == USB_RET_SUCCESS) {
+        ret = pkt->actual_length;
     } else {
-        ret = ohci->usb_packet.status;
+        ret = pkt->status;
     }
+    g_free(pkt);
 
     trace_usb_ohci_iso_td_so(start_offset, end_offset, start_addr, end_addr,
                              str, len, ret);
@@ -734,7 +738,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
     /* Writeback */
     if (dir == OHCI_TD_DIR_IN && ret >= 0 && ret <= len) {
         /* IN transfer succeeded */
-        if (ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret,
+        if (ohci_copy_iso_td(ohci, start_addr, end_addr, buf, ret,
                              DMA_DIRECTION_FROM_DEVICE)) {
             ohci_die(ohci);
             return 1;
@@ -1057,7 +1061,7 @@ exit_no_retire:
 }
 
 /* Service an endpoint list.  Returns nonzero if active TD were found.  */
-static int ohci_service_ed_list(OHCIState *ohci, uint32_t head, int completion)
+static int ohci_service_ed_list(OHCIState *ohci, uint32_t head)
 {
     struct ohci_ed ed;
     uint32_t next_ed;
@@ -1108,7 +1112,7 @@ static int ohci_service_ed_list(OHCIState *ohci, uint32_t head, int completion)
                     break;
             } else {
                 /* Handle isochronous endpoints */
-                if (ohci_service_iso_td(ohci, &ed, completion))
+                if (ohci_service_iso_td(ohci, &ed))
                     break;
             }
         }
@@ -1136,20 +1140,20 @@ static void ohci_sof(OHCIState *ohci)
 }
 
 /* Process Control and Bulk lists.  */
-static void ohci_process_lists(OHCIState *ohci, int completion)
+static void ohci_process_lists(OHCIState *ohci)
 {
     if ((ohci->ctl & OHCI_CTL_CLE) && (ohci->status & OHCI_STATUS_CLF)) {
         if (ohci->ctrl_cur && ohci->ctrl_cur != ohci->ctrl_head) {
             trace_usb_ohci_process_lists(ohci->ctrl_head, ohci->ctrl_cur);
         }
-        if (!ohci_service_ed_list(ohci, ohci->ctrl_head, completion)) {
+        if (!ohci_service_ed_list(ohci, ohci->ctrl_head)) {
             ohci->ctrl_cur = 0;
             ohci->status &= ~OHCI_STATUS_CLF;
         }
     }
 
     if ((ohci->ctl & OHCI_CTL_BLE) && (ohci->status & OHCI_STATUS_BLF)) {
-        if (!ohci_service_ed_list(ohci, ohci->bulk_head, completion)) {
+        if (!ohci_service_ed_list(ohci, ohci->bulk_head)) {
             ohci->bulk_cur = 0;
             ohci->status &= ~OHCI_STATUS_BLF;
         }
@@ -1173,7 +1177,7 @@ static void ohci_frame_boundary(void *opaque)
         int n;
 
         n = ohci->frame_number & 0x1f;
-        ohci_service_ed_list(ohci, le32_to_cpu(hcca.intr[n]), 0);
+        ohci_service_ed_list(ohci, le32_to_cpu(hcca.intr[n]));
     }
 
     /* Cancel all pending packets if either of the lists has been disabled.  */
@@ -1181,7 +1185,7 @@ static void ohci_frame_boundary(void *opaque)
         ohci_stop_endpoints(ohci);
     }
     ohci->old_ctl = ohci->ctl;
-    ohci_process_lists(ohci, 0);
+    ohci_process_lists(ohci);
 
     /* Stop if UnrecoverableError happened or ohci_sof will crash */
     if (ohci->intr_status & OHCI_INTR_UE) {
@@ -1793,7 +1797,7 @@ static void ohci_async_complete_packet(USBPort *port, USBPacket *packet)
 
     trace_usb_ohci_async_complete();
     ohci->async_complete = true;
-    ohci_process_lists(ohci, 1);
+    ohci_process_lists(ohci);
 }
 
 static USBPortOps ohci_port_ops = {
-- 
2.30.2

Re: [PATCH 0/5] Misc OHCI clean ups

[BALATON Zoltan]

On Sun, 16 Jan 2022, BALATON Zoltan wrote:
> Hello,
>
> I have these patches from last October when we've looked at what
> causes problems with mac99 and USB. We've found the main problem is
> likely not allowing pending packets per endpoint which we did not fix
> but these patches came out of debugging that and trying to improve the
> device model so eventually the real problem could be fixed more
> easily. So these are just clean ups and fixing one potential issue
> with isochronous transfers breaking pending async packet but it does
> not solve all problems OHCI currently has. I'm sending it anyway as I
> don't plan to work further on this so this series could be taken as is
> for now.

Ping?

> Regards,
>
> BALATON Zoltan (5):
>  usb/ohci: Move trace point and log ep number to help debugging
>  usb/ohci: Move cancelling async packet to ohci_stop_endpoints()
>  usb/ohci: Move USBPortOps related functions together
>  usb/ohci: Merge ohci_async_cancel_device() into ohci_child_detach()
>  usb/ohci: Don't use packet from OHCIState for isochronous transfers
>
> hw/usb/hcd-ohci.c   | 295 +++++++++++++++++++++-----------------------
> hw/usb/trace-events |   2 +-
> 2 files changed, 144 insertions(+), 153 deletions(-)
>
>

Re: [PATCH 0/5] Misc OHCI clean ups

[Gerd Hoffmann]

On Mon, Jan 24, 2022 at 06:09:12PM +0100, BALATON Zoltan wrote:
> On Sun, 16 Jan 2022, BALATON Zoltan wrote:
> > Hello,
> > 
> > I have these patches from last October when we've looked at what
> > causes problems with mac99 and USB. We've found the main problem is
> > likely not allowing pending packets per endpoint which we did not fix
> > but these patches came out of debugging that and trying to improve the
> > device model so eventually the real problem could be fixed more
> > easily. So these are just clean ups and fixing one potential issue
> > with isochronous transfers breaking pending async packet but it does
> > not solve all problems OHCI currently has. I'm sending it anyway as I
> > don't plan to work further on this so this series could be taken as is
> > for now.
> 
> Ping?

Queued now.

thanks,
  Gerd

Re: [PATCH 0/5] Misc OHCI clean ups

[Gerd Hoffmann]

On Tue, Jan 25, 2022 at 08:44:30AM +0100, Gerd Hoffmann wrote:
> On Mon, Jan 24, 2022 at 06:09:12PM +0100, BALATON Zoltan wrote:
> > On Sun, 16 Jan 2022, BALATON Zoltan wrote:
> > > Hello,
> > > 
> > > I have these patches from last October when we've looked at what
> > > causes problems with mac99 and USB. We've found the main problem is
> > > likely not allowing pending packets per endpoint which we did not fix
> > > but these patches came out of debugging that and trying to improve the
> > > device model so eventually the real problem could be fixed more
> > > easily. So these are just clean ups and fixing one potential issue
> > > with isochronous transfers breaking pending async packet but it does
> > > not solve all problems OHCI currently has. I'm sending it anyway as I
> > > don't plan to work further on this so this series could be taken as is
> > > for now.
> > 
> > Ping?
> 
> Queued now.

Oops, have to take that back, checkpatch throws errors:

HEAD is now at 00abd6f54a24 usb/ohci: Don't use packet from OHCIState for isochronous transfers

error: no note found for object 00abd6f54a246db5877ceb466755374b297d97cf.
ERROR: braces {} are necessary for all arms of this statement
#131: FILE: hw/usb/hcd-ohci.c:1115:
+                if (ohci_service_iso_td(ohci, &ed))
[...]

total: 1 errors, 0 warnings, 153 lines checked

Please check all patches & resend.

take care,
  Gerd

Re: [PATCH 0/5] Misc OHCI clean ups

[BALATON Zoltan]

On Tue, 25 Jan 2022, Gerd Hoffmann wrote:
> On Tue, Jan 25, 2022 at 08:44:30AM +0100, Gerd Hoffmann wrote:
>> On Mon, Jan 24, 2022 at 06:09:12PM +0100, BALATON Zoltan wrote:
>>> On Sun, 16 Jan 2022, BALATON Zoltan wrote:
>>>> Hello,
>>>>
>>>> I have these patches from last October when we've looked at what
>>>> causes problems with mac99 and USB. We've found the main problem is
>>>> likely not allowing pending packets per endpoint which we did not fix
>>>> but these patches came out of debugging that and trying to improve the
>>>> device model so eventually the real problem could be fixed more
>>>> easily. So these are just clean ups and fixing one potential issue
>>>> with isochronous transfers breaking pending async packet but it does
>>>> not solve all problems OHCI currently has. I'm sending it anyway as I
>>>> don't plan to work further on this so this series could be taken as is
>>>> for now.
>>>
>>> Ping?
>>
>> Queued now.
>
> Oops, have to take that back, checkpatch throws errors:
>
> HEAD is now at 00abd6f54a24 usb/ohci: Don't use packet from OHCIState for isochronous transfers
>
> error: no note found for object 00abd6f54a246db5877ceb466755374b297d97cf.
> ERROR: braces {} are necessary for all arms of this statement
> #131: FILE: hw/usb/hcd-ohci.c:1115:
> +                if (ohci_service_iso_td(ohci, &ed))
> [...]
>
> total: 1 errors, 0 warnings, 153 lines checked
>
> Please check all patches & resend.

Sorry, I'll resend. I usually run checkpatch before submitting and I think 
I've checked these when first sent them and haven't got the error but 
maybe this was a last minute change. Not sure how it was missed.

Regards,
BALATON Zoltan