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

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Nov 11, 2019 at 11:33:17AM +0100, Klaus Birkelund wrote:
> 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 <k.jensen@samsung.com>
> > > > > ---
> > > > >   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é <philmd@redhat.com>
> > > 
> > > 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

It's probably best to just include it in the series.
When you do feel free to include

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

and mention that the return code has no existing
users so it's safe to change.