[1/1] pci: pass along the return value of dma_memory_rw
diff mbox series

Message ID 20191011070141.188713-2-its@irrelevant.dk
State New
Headers show
Series
  • pci: pass along the return value of dma_memory_rw
Related show

Commit Message

Klaus Birkelund Oct. 11, 2019, 7:01 a.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé Oct. 23, 2019, 11:13 p.m. UTC | #1
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>
Klaus Birkelund Nov. 11, 2019, 9:30 a.m. UTC | #2
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
Michael S. Tsirkin Nov. 11, 2019, 10:16 a.m. UTC | #3
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?
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?
Klaus Birkelund Nov. 11, 2019, 10:33 a.m. UTC | #4
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
Michael S. Tsirkin Nov. 11, 2019, 11 a.m. UTC | #5
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.

Patch
diff mbox series

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,