Hi Peter,
> > 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...
>
Thanks for the review and the suggestion.
I have sent a v2 that drops the dead NULL check, checks the MemTxResult
of dma_memory_read()/dma_memory_write() instead, and makes put_dwords()
return void since no caller can act on a failed writeback.
https://patchwork.kernel.org/project/qemu-devel/list/?series=1146899
Thanks,
Jamin
> -- PMM
