Re: [PATCH 1/1] pci: pass along the return value of dma_memory_rw

[Klaus Birkelund]

On Mon, Nov 11, 2019 at 05:16:41AM -0500, Michael S. Tsirkin wrote:
> On Mon, Nov 11, 2019 at 10:30:07AM +0100, Klaus Birkelund wrote:
> > On Thu, Oct 24, 2019 at 01:13:36AM +0200, Philippe Mathieu-Daudé wrote:
> > > On 10/11/19 9:01 AM, Klaus Jensen wrote:
> > > > Some might actually care about the return value of dma_memory_rw. So
> > > > let us pass it along instead of ignoring it.
> > > > 
> > > > Signed-off-by: Klaus Jensen <address@hidden>
> > > > ---
> > > >   include/hw/pci/pci.h | 3 +--
> > > >   1 file changed, 1 insertion(+), 2 deletions(-)
> > > > 
> > > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > > index f3f0ffd5fb78..4e95bb847857 100644
> > > > --- a/include/hw/pci/pci.h
> > > > +++ b/include/hw/pci/pci.h
> > > > @@ -779,8 +779,7 @@ static inline AddressSpace 
> > > > *pci_get_address_space(PCIDevice *dev)
> > > >   static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
> > > >                                void *buf, dma_addr_t len, DMADirection 
> > > > dir)
> > > >   {
> > > > -    dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
> > > > -    return 0;
> > > > +    return dma_memory_rw(pci_get_address_space(dev), addr, buf, len, 
> > > > dir);
> > > >   }
> > > >   static inline int pci_dma_read(PCIDevice *dev, dma_addr_t addr,
> > > > 
> > > 
> > > Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> > 
> > Gentle ping on this.
> > 
> > This fix is required for the nvme device to start passing some of the
> > nasty tests from blktests that flips bus mastering while doing I/O.
> > 
> > 
> > Cheers,
> > Klaus
> 
> So I looked and it does not seem like anyone at all checks the
> return value. While this makes the patch safe, how come it
> helps anyone at all?

I have a series[1] under review for the nvme device (I should have
mentioned that). There is a certain test (block/011) from blktests[2],
that disables the PCI device while doing I/O by flipping the bus master
register. QEMU correctly understands this which causes the dma_memory_rw
calls to fail while the device is not a bus master. For the NVMe device
to pass that test, it needs to know that it could not do the DMA,
otherwise it will just think that a completion queue entry was
successfully posted or data was correctly read.

> Maybe this is just infrastructure to allow checks in the
> future, in this case do we need this for the freeze?
> Or can it wait until after the release?
> 

The series I'm mentioning won't be going into the release, so yeah - it
can surely wait. I was just pinging to make sure someone would pick it
up at some point :)
 
    [1]: https://patchwork.kernel.org/cover/11190045/
    [2]: https://github.com/osandov/blktests