Hi,
This series is spun out and expanded from my work to add P2PDMA support
to DMA map operations[1].
The P2PDMA work requires distinguishing different error conditions in
a map_sg operation. dma_map_sgtable() already allows for returning an
error code (where as dma_map_sg() is only allowed to return zero)
however, it currently only returns -EINVAL when a .map_sg() call returns
zero.
This series cleans up all .map_sg() implementations to return appropriate
error codes. After the cleanup, dma_map_sg() will still return zero,
however dma_map_sgtable() will pass the error code from the .map_sg()
call. Thanks go to Martn Oliveira for doing a lot of the cleanup of the
obscure implementations.
The patch set is based off of v5.14-rc1 and a git repo can be found
here:
https://github.com/sbates130272/linux-p2pmem map_sg_err_cleanup_v1
Thanks,
Logan
[1] https://lore.kernel.org/linux-block/20210513223203.5542-1-logang@deltatee.com/
--
Logan Gunthorpe (5):
dma-mapping: Allow map_sg() ops to return negative error codes
dma-direct: Return appropriate error code from dma_direct_map_sg()
iommu: Return full error code from iommu_map_sg[_atomic]()
dma-iommu: Return error code from iommu_dma_map_sg()
dma-mapping: Disallow .map_sg operations from returning zero on error
Martin Oliveira (11):
alpha: return error code from alpha_pci_map_sg()
ARM/dma-mapping: return error code from .map_sg() ops
ia64/sba_iommu: return error code from sba_map_sg_attrs()
MIPS/jazzdma: return error code from jazz_dma_map_sg()
powerpc/iommu: return error code from .map_sg() ops
s390/pci: return error code from s390_dma_map_sg()
sparc/iommu: return error codes from .map_sg() ops
parisc: return error code from .map_sg() ops
xen: swiotlb: return error code from xen_swiotlb_map_sg()
x86/amd_gart: return error code from gart_map_sg()
dma-mapping: return error code from dma_dummy_map_sg()
arch/alpha/kernel/pci_iommu.c | 10 +++-
arch/arm/mm/dma-mapping.c | 22 +++++---
arch/ia64/hp/common/sba_iommu.c | 9 +--
arch/mips/jazz/jazzdma.c | 2 +-
arch/powerpc/kernel/iommu.c | 4 +-
arch/powerpc/platforms/ps3/system-bus.c | 2 +-
arch/powerpc/platforms/pseries/vio.c | 5 +-
arch/s390/pci/pci_dma.c | 12 ++--
arch/sparc/kernel/iommu.c | 4 +-
arch/sparc/kernel/pci_sun4v.c | 4 +-
arch/sparc/mm/iommu.c | 2 +-
arch/x86/kernel/amd_gart_64.c | 16 +++---
drivers/iommu/dma-iommu.c | 20 ++++---
drivers/iommu/iommu.c | 15 +++--
drivers/parisc/ccio-dma.c | 2 +-
drivers/parisc/sba_iommu.c | 2 +-
drivers/xen/swiotlb-xen.c | 2 +-
include/linux/dma-map-ops.h | 6 +-
include/linux/dma-mapping.h | 35 +++---------
include/linux/iommu.h | 22 ++++----
kernel/dma/direct.c | 2 +-
kernel/dma/dummy.c | 2 +-
kernel/dma/mapping.c | 73 ++++++++++++++++++++++---
23 files changed, 165 insertions(+), 108 deletions(-)
base-commit: e73f0f0ee7541171d89f2e2491130c7771ba58d3
--
2.20.1
Now that the map_sg() op expects error codes instead of return zero on
error, convert dma_direct_map_sg() to return an error code. The
only error to return presently is EINVAL if a page could not
be mapped.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
kernel/dma/direct.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Convert to ssize_t return code so the return code from __iommu_map()
can be returned all the way down through dma_iommu_map_sg().
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
---
drivers/iommu/iommu.c | 15 +++++++--------
include/linux/iommu.h | 22 +++++++++++-----------
2 files changed, 18 insertions(+), 19 deletions(-)
Now that all the .map_sg operations have been converted to returning
proper error codes, drop the code to handle a zero return value,
add a warning if a zero is returned and update the comment for the
map_sg operation.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
include/linux/dma-map-ops.h | 8 +++-----
kernel/dma/mapping.c | 6 +++---
2 files changed, 6 insertions(+), 8 deletions(-)
Turns this into a negative error code while we're at it, just to keep
the callers sane?
Right, by this point returning the 0 would pass through
dma_map_sg_attrs() OK, but AFAICS dma_map_sgtable() would now get
confused and return success but with sgt->nents = 0. Coercing it to an
error code (which dma_map_sg_attrs() would then just change right back)
seems sensible for the sake of easy robustness.
Robin.
Allow dma_map_sgtable() to pass errors from the map_sg() ops. This
will be required for returning appropriate error codes when mapping
P2PDMA memory.
Introduce __dma_map_sg_attrs() which will return the raw error code
from the map_sg operation (whether it be negative or zero). Then add a
dma_map_sg_attrs() wrapper to convert any negative errors to zero to
satisfy the existing calling convention.
dma_map_sgtable() will convert a zero error return for old map_sg() ops
into a -EINVAL return and return any negative errors as reported.
This allows map_sg implementations to start returning multiple
negative error codes. Legacy map_sg implementations can continue
to return zero until they are all converted.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
include/linux/dma-map-ops.h | 8 +++-
include/linux/dma-mapping.h | 35 ++++--------------
kernel/dma/mapping.c | 73 +++++++++++++++++++++++++++++++++----
3 files changed, 78 insertions(+), 38 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-07-16 06:29:34
On Thu, Jul 15, 2021 at 10:45:29AM -0600, Logan Gunthorpe wrote:
+ * dma_map_sgtable() will return the error code returned and convert
+ * a zero return (for legacy implementations) into -EINVAL.
+ *
+ * dma_map_sg() will always return zero on any negative or zero
+ * return to satisfy its own calling convention.
*/
From: Martin Oliveira <redacted>
The .map_sg() op now expects an error code instead of zero on failure.
xen_swiotlb_map_sg() may only fail if xen_swiotlb_map_page() fails, but
xen_swiotlb_map_page() only supports returning errors as
DMA_MAPPING_ERROR. So coalesce all errors into EINVAL.
Signed-off-by: Martin Oliveira <redacted>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Konrad Rzeszutek Wilk <redacted>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
---
drivers/xen/swiotlb-xen.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Boris Ostrovsky <boris.ostrovsky@oracle.com> Date: 2021-07-19 20:47:20
On 7/15/21 12:45 PM, Logan Gunthorpe wrote:
From: Martin Oliveira <redacted>
The .map_sg() op now expects an error code instead of zero on failure.
xen_swiotlb_map_sg() may only fail if xen_swiotlb_map_page() fails, but
xen_swiotlb_map_page() only supports returning errors as
DMA_MAPPING_ERROR. So coalesce all errors into EINVAL.
From: Martin Oliveira <redacted>
The .map_sg() op now expects an error code instead of zero on failure.
So propagate the error from __s390_dma_map_sg() up.
Signed-off-by: Martin Oliveira <redacted>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Niklas Schnelle <schnelle@linux.ibm.com>
Cc: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Christian Borntraeger <redacted>
---
arch/s390/pci/pci_dma.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
On Thu, 2021-07-15 at 10:45 -0600, Logan Gunthorpe wrote:
quoted hunk
From: Martin Oliveira <redacted>
The .map_sg() op now expects an error code instead of zero on failure.
So propagate the error from __s390_dma_map_sg() up.
Signed-off-by: Martin Oliveira <redacted>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Niklas Schnelle <schnelle@linux.ibm.com>
Cc: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Christian Borntraeger <redacted>
---
arch/s390/pci/pci_dma.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
So the error codes we return are -ENOMEM if allocating a DMA
translation entry fails and -EINVAL if the DMA translation table hasn't
been initialized or the caller tries to map 0 sized memory. Are these
error codes that you would expect? If yes then this change looks good
to me.
Acked-by: Niklas Schnelle <schnelle@linux.ibm.com>
From: Martin Oliveira <redacted>
The .map_sg() op now expects an error code instead of zero on failure.
So make __dma_map_cont() return a valid errno (which is then propagated
to gart_map_sg() via dma_map_cont()) and return it in case of failure.
Also, return -EINVAL in case of invalid nents.
Signed-off-by: Martin Oliveira <redacted>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Niklas Schnelle <schnelle@linux.ibm.com>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Michael Ellerman <mpe@ellerman.id.au>
---
arch/x86/kernel/amd_gart_64.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
While we're at it - setting the ->dma_address to DMA_MAPPING_ERROR is
not part of the ->map_sg calling convention. Might be worth to fix
up while we're at it.
While we're at it - setting the ->dma_address to DMA_MAPPING_ERROR is
not part of the ->map_sg calling convention. Might be worth to fix
up while we're at it.
Especially since it's not even zeroing dma_length, which at least is a
documented indicator of validity (even if it's not strictly defined for
failure cases either).
Robin.
From: Martin Oliveira <redacted>
The .map_sg() op now expects an error code instead of zero on failure.
vdma_alloc() may fail for different reasons, but since it only supports
indicating an error via a return of DMA_MAPPING_ERROR, we coalesce the
different reasons into -EINVAL.
Signed-off-by: Martin Oliveira <redacted>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---
arch/mips/jazz/jazzdma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Martin Oliveira <redacted>
The .map_sg() op now expects an error code instead of zero on failure.
Propagate the error up if vio_dma_iommu_map_sg() fails.
ppc_iommu_map_sg() may fail either because of iommu_range_alloc() or
because of tbl->it_ops->set(). The former only supports returning an
error with DMA_MAPPING_ERROR and an examination of the latter indicates
that it may return arch-specific errors (for example,
tce_buildmulti_pSeriesLP()). Hence, coalesce all of those errors into
-EINVAL;
Signed-off-by: Martin Oliveira <redacted>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: Geoff Levand <geoff@infradead.org>
---
arch/powerpc/kernel/iommu.c | 4 ++--
arch/powerpc/platforms/ps3/system-bus.c | 2 +-
arch/powerpc/platforms/pseries/vio.c | 5 +++--
3 files changed, 6 insertions(+), 5 deletions(-)
From: Martin Oliveira <redacted>
The .map_sg() op now expects an error code instead of zero on failure.
Propagate the return of dma_mapping_error() up, if it is an errno.
sba_coalesce_chunks() may only presently fail if sba_alloc_range()
fails, which in turn only fails if the iommu is out of mapping
resources, hence a -ENOMEM is used in that case.
Signed-off-by: Martin Oliveira <redacted>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Niklas Schnelle <schnelle@linux.ibm.com>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---
arch/ia64/hp/common/sba_iommu.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
From: Martin Oliveira <redacted>
The .map_sg() op now expects an error code instead of zero on failure,
so propagate any errors that may happen all the way up.
Signed-off-by: Martin Oliveira <redacted>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---
arch/arm/mm/dma-mapping.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
From: Martin Oliveira <redacted>
The .map_sg() op now expects an error code instead of zero on failure.
pci_map_single_1() can fail for different reasons, but since the only
supported type of error return is DMA_MAPPING_ERROR, we coalesce those
errors into EINVAL.
ENOMEM is returned when no page tables can be allocated.
Signed-off-by: Martin Oliveira <redacted>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Richard Henderson <redacted>
Cc: Ivan Kokshaysky <redacted>
Cc: Matt Turner <mattst88@gmail.com>
---
arch/alpha/kernel/pci_iommu.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
@@ -685,8 +687,10 @@ static int alpha_pci_map_sg(struct device *dev, struct scatterlist *sg,if(out<end)out->dma_length=0;-if(out-start==0)+if(out-start==0){printk(KERN_WARNING"pci_map_sg failed: no entries?\n");+return-ENOMEM;+}DBGA("pci_map_sg: %ld entries\n",out-start);returnout-start;
@@ -699,7 +703,7 @@ static int alpha_pci_map_sg(struct device *dev, struct scatterlist *sg,entries.Unmapthemnow.*/if(out>start)pci_unmap_sg(pdev,start,out-start,dir);-return0;+return-ENOMEM;}/* Unmap a set of streaming mode DMA translations. Again, cpu read
Pass through appropriate error codes from iommu_dma_map_sg() now that
the error code will be passed through dma_map_sgtable().
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
---
drivers/iommu/dma-iommu.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-07-16 06:31:35
Careful here. What do all these errors from the low-level code mean
here? I think we need to clearly standardize on what we actually
return from ->map_sg and possibly document what the callers expect and
can do, and enforce that only those error are reported.
From: Martin Oliveira <redacted>
The .map_sg() op now expects an error code instead of zero on failure.
Returning an errno from __sbus_iommu_map_sg() results in
sbus_iommu_map_sg_gflush() and sbus_iommu_map_sg_pflush() returning an
errno, as those functions are wrappers around __sbus_iommu_map_sg().
Signed-off-by: Martin Oliveira <redacted>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Niklas Schnelle <schnelle@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
---
arch/sparc/kernel/iommu.c | 4 ++--
arch/sparc/kernel/pci_sun4v.c | 4 ++--
arch/sparc/mm/iommu.c | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
@@ -580,7 +580,7 @@ static int dma_4u_map_sg(struct device *dev, struct scatterlist *sglist,}spin_unlock_irqrestore(&iommu->lock,flags);-return0;+return-EINVAL;}/* If contexts are being used, they are the same in all of the mappings
From: Martin Oliveira <redacted>
The .map_sg() op now expects an error code instead of zero on failure.
The only errno to return is -ENODEV in the case when DMA is not
supported.
Signed-off-by: Martin Oliveira <redacted>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
kernel/dma/dummy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-07-15 16:53:23
On Thu, Jul 15, 2021 at 10:45:28AM -0600, Logan Gunthorpe wrote:
Hi,
This series is spun out and expanded from my work to add P2PDMA support
to DMA map operations[1].
The P2PDMA work requires distinguishing different error conditions in
a map_sg operation. dma_map_sgtable() already allows for returning an
error code (where as dma_map_sg() is only allowed to return zero)
however, it currently only returns -EINVAL when a .map_sg() call returns
zero.
This series cleans up all .map_sg() implementations to return appropriate
error codes. After the cleanup, dma_map_sg() will still return zero,
however dma_map_sgtable() will pass the error code from the .map_sg()
call. Thanks go to Martn Oliveira for doing a lot of the cleanup of the
obscure implementations.
The patch set is based off of v5.14-rc1 and a git repo can be found
here:
Have all the callers for dma_map_sg() been updated to check for error
codes? If not, isn't that a pre-requisit to this patch set?
From what I see in Linus' current tree, we still have cases today
where the return value of dma_map_sg() is compared with zero to
detect failure, so I think that needs fixing before we start changing
the dma_map_sg() implementation to return negative numbers.
I also notice that there are various places that don't check the
return value - and returning a negative number instead of zero may
well cause random other bits to be set in fields.
So, I think there's a fair amount of work to do in all the drivers
before this change can be considered.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 2021-07-15 10:53 a.m., Russell King (Oracle) wrote:
On Thu, Jul 15, 2021 at 10:45:28AM -0600, Logan Gunthorpe wrote:
quoted
Hi,
This series is spun out and expanded from my work to add P2PDMA support
to DMA map operations[1].
The P2PDMA work requires distinguishing different error conditions in
a map_sg operation. dma_map_sgtable() already allows for returning an
error code (where as dma_map_sg() is only allowed to return zero)
however, it currently only returns -EINVAL when a .map_sg() call returns
zero.
This series cleans up all .map_sg() implementations to return appropriate
error codes. After the cleanup, dma_map_sg() will still return zero,
however dma_map_sgtable() will pass the error code from the .map_sg()
call. Thanks go to Martn Oliveira for doing a lot of the cleanup of the
obscure implementations.
The patch set is based off of v5.14-rc1 and a git repo can be found
here:
Have all the callers for dma_map_sg() been updated to check for error
codes? If not, isn't that a pre-requisit to this patch set?
No. Perhaps I wasn't clear enough: This series is changing only
impelemntations of .map_sg(). It does *not* change the return code of
dma_map_sg(). dma_map_sg() will continue to return zero on error for the
foreseeable future. The dma_map_sgtable() call already allows returning
error codes and it will pass the new error code through. This is what
will be used in the P2PDMA work.
Logan