Re: [PATCH v1] hw/usb/hcd-ehci: Handle get_dwords() failures in async writeback

On Thu, 13 Aug 2026 at 08:24, Jamin Lin <[email protected]> wrote:
>
> Coverity reports that ehci_writeback_async_complete_packet() ignores
> the return value of get_dwords() when reading the QH and qTD.
>
> Handle read failures in the same way as QH and qTD verification
> failures by freeing the packet and returning early.
>
> Signed-off-by: Jamin Lin <[email protected]>
> ---
>  hw/usb/hcd-ehci.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 451a918e9f..d43951975a 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -533,11 +533,11 @@ static void ehci_writeback_async_complete_packet(EHCIPacket *p)
>      /* Verify the qh + qtd, like we do when going through fetchqh & fetchqtd */
>      memset(&qh, 0, sizeof(qh));
>      memset(&qtd, 0, sizeof(qtd));
> -    get_dwords(q->ehci, NLPTR_GET(q->qhaddr),
> -               (uint32_t *) &qh, ehci_qh_dwords(q->ehci));
> -    get_dwords(q->ehci, NLPTR_GET(q->qtdaddr),
> -               (uint32_t *) &qtd, ehci_qtd_dwords(q->ehci));
> -    if (!ehci_verify_qh(q, &qh) || !ehci_verify_qtd(p, &qtd)) {
> +    if (get_dwords(q->ehci, NLPTR_GET(q->qhaddr),
> +                   (uint32_t *) &qh, ehci_qh_dwords(q->ehci)) < 0 ||
> +        get_dwords(q->ehci, NLPTR_GET(q->qtdaddr),
> +                   (uint32_t *) &qtd, ehci_qtd_dwords(q->ehci)) < 0 ||
> +        !ehci_verify_qh(q, &qh) || !ehci_verify_qtd(p, &qtd)) {
>          p->async = EHCI_ASYNC_INITIALIZED;
>          ehci_free_packet(p);
>          return;

Looking at the coverity report, my question is whether echi->as
can ever actually be NULL. This is the address space we use to
do DMA, so it feels like every EHCI device must set that up
somehow. ehci_sysbus_init() does. So does usb_ehci_pci_realize().
usb_ehci_pci_write_config() can change it, but never to NULL.

If echi->as is always non-NULL then we could change get_dwords()
and put_dwords() to return "void".

Alternatively, maybe get_dwords() and put_dwords() should be
checking the return value from dma_memory_write() and
dma_memory_read() so that they fail if the DMA fails...

-- PMM