| Message ID | 20191011070141.188713-2-its@irrelevant.dk |
|---|---|
| State | New |
| Headers | show |
| Series | pci: pass along the return value of dma_memory_rw | expand |
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>
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
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?
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
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.
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,
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(-)