[PATCH] powerpc/npu-dma: Remove NPU DMA ops

[PATCH] powerpc/npu-dma: Remove NPU DMA ops

[Alistair Popple]

The NPU IOMMU is setup to mirror the parent PCIe device IOMMU
setup. Therefore it does not make sense to call dma operations such as
dma_map_page, etc. directly on these devices. The existing dma-ops
simply print a warning if they are ever called, however this is
unnecessary and the warnings are likely to go unnoticed.

It is instead simpler to remove these operations and let the generic
DMA code print warnings (eg. via a NULL pointer deref) in cases of
buggy drivers attempting dma operations on NVLink devices.

Signed-off-by: Alistair Popple <alistair@popple.id.au>
---
 arch/powerpc/platforms/powernv/npu-dma.c | 64 ++------------------------------
 1 file changed, 4 insertions(+), 60 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
index 6f60e0931922..75b935252981 100644
--- a/arch/powerpc/platforms/powernv/npu-dma.c
+++ b/arch/powerpc/platforms/powernv/npu-dma.c
@@ -102,63 +102,6 @@ struct pci_dev *pnv_pci_get_npu_dev(struct pci_dev *gpdev, int index)
 }
 EXPORT_SYMBOL(pnv_pci_get_npu_dev);

-#define NPU_DMA_OP_UNSUPPORTED()					\
-	dev_err_once(dev, "%s operation unsupported for NVLink devices\n", \
-		__func__)
-
-static void *dma_npu_alloc(struct device *dev, size_t size,
-			   dma_addr_t *dma_handle, gfp_t flag,
-			   unsigned long attrs)
-{
-	NPU_DMA_OP_UNSUPPORTED();
-	return NULL;
-}
-
-static void dma_npu_free(struct device *dev, size_t size,
-			 void *vaddr, dma_addr_t dma_handle,
-			 unsigned long attrs)
-{
-	NPU_DMA_OP_UNSUPPORTED();
-}
-
-static dma_addr_t dma_npu_map_page(struct device *dev, struct page *page,
-				   unsigned long offset, size_t size,
-				   enum dma_data_direction direction,
-				   unsigned long attrs)
-{
-	NPU_DMA_OP_UNSUPPORTED();
-	return 0;
-}
-
-static int dma_npu_map_sg(struct device *dev, struct scatterlist *sglist,
-			  int nelems, enum dma_data_direction direction,
-			  unsigned long attrs)
-{
-	NPU_DMA_OP_UNSUPPORTED();
-	return 0;
-}
-
-static int dma_npu_dma_supported(struct device *dev, u64 mask)
-{
-	NPU_DMA_OP_UNSUPPORTED();
-	return 0;
-}
-
-static u64 dma_npu_get_required_mask(struct device *dev)
-{
-	NPU_DMA_OP_UNSUPPORTED();
-	return 0;
-}
-
-static const struct dma_map_ops dma_npu_ops = {
-	.map_page		= dma_npu_map_page,
-	.map_sg			= dma_npu_map_sg,
-	.alloc			= dma_npu_alloc,
-	.free			= dma_npu_free,
-	.dma_supported		= dma_npu_dma_supported,
-	.get_required_mask	= dma_npu_get_required_mask,
-};
-
 /*
  * Returns the PE assoicated with the PCI device of the given
  * NPU. Returns the linked pci device if pci_dev != NULL.
@@ -270,10 +213,11 @@ static void pnv_npu_dma_set_32(struct pnv_ioda_pe *npe)
 	rc = pnv_npu_set_window(npe, 0, gpe->table_group.tables[0]);

 	/*
-	 * We don't initialise npu_pe->tce32_table as we always use
-	 * dma_npu_ops which are nops.
+	 * NVLink devices use the same TCE table configuration as
+	 * their parent device so drivers shouldn't be doing DMA
+	 * operations directly on these devices.
 	 */
-	set_dma_ops(&npe->pdev->dev, &dma_npu_ops);
+	set_dma_ops(&npe->pdev->dev, NULL);
 }

 /*
--
2.11.0

Re: [PATCH] powerpc/npu-dma: Remove NPU DMA ops

[Christoph Hellwig]

Please take my patch instead.  We have a kernel polcity to not keep
dead code around, and everyone including Linus and the attending IBMers
confirmed this.

On Tue, Oct 30, 2018 at 10:02:03PM +1100, Alistair Popple wrote:
> The NPU IOMMU is setup to mirror the parent PCIe device IOMMU
> setup. Therefore it does not make sense to call dma operations such as
> dma_map_page, etc. directly on these devices. The existing dma-ops
> simply print a warning if they are ever called, however this is
> unnecessary and the warnings are likely to go unnoticed.
> 
> It is instead simpler to remove these operations and let the generic
> DMA code print warnings (eg. via a NULL pointer deref) in cases of
> buggy drivers attempting dma operations on NVLink devices.
> 
> Signed-off-by: Alistair Popple <alistair@popple.id.au>
> ---
>  arch/powerpc/platforms/powernv/npu-dma.c | 64 ++------------------------------
>  1 file changed, 4 insertions(+), 60 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
> index 6f60e0931922..75b935252981 100644
> --- a/arch/powerpc/platforms/powernv/npu-dma.c
> +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> @@ -102,63 +102,6 @@ struct pci_dev *pnv_pci_get_npu_dev(struct pci_dev *gpdev, int index)
>  }
>  EXPORT_SYMBOL(pnv_pci_get_npu_dev);
> 
> -#define NPU_DMA_OP_UNSUPPORTED()					\
> -	dev_err_once(dev, "%s operation unsupported for NVLink devices\n", \
> -		__func__)
> -
> -static void *dma_npu_alloc(struct device *dev, size_t size,
> -			   dma_addr_t *dma_handle, gfp_t flag,
> -			   unsigned long attrs)
> -{
> -	NPU_DMA_OP_UNSUPPORTED();
> -	return NULL;
> -}
> -
> -static void dma_npu_free(struct device *dev, size_t size,
> -			 void *vaddr, dma_addr_t dma_handle,
> -			 unsigned long attrs)
> -{
> -	NPU_DMA_OP_UNSUPPORTED();
> -}
> -
> -static dma_addr_t dma_npu_map_page(struct device *dev, struct page *page,
> -				   unsigned long offset, size_t size,
> -				   enum dma_data_direction direction,
> -				   unsigned long attrs)
> -{
> -	NPU_DMA_OP_UNSUPPORTED();
> -	return 0;
> -}
> -
> -static int dma_npu_map_sg(struct device *dev, struct scatterlist *sglist,
> -			  int nelems, enum dma_data_direction direction,
> -			  unsigned long attrs)
> -{
> -	NPU_DMA_OP_UNSUPPORTED();
> -	return 0;
> -}
> -
> -static int dma_npu_dma_supported(struct device *dev, u64 mask)
> -{
> -	NPU_DMA_OP_UNSUPPORTED();
> -	return 0;
> -}
> -
> -static u64 dma_npu_get_required_mask(struct device *dev)
> -{
> -	NPU_DMA_OP_UNSUPPORTED();
> -	return 0;
> -}
> -
> -static const struct dma_map_ops dma_npu_ops = {
> -	.map_page		= dma_npu_map_page,
> -	.map_sg			= dma_npu_map_sg,
> -	.alloc			= dma_npu_alloc,
> -	.free			= dma_npu_free,
> -	.dma_supported		= dma_npu_dma_supported,
> -	.get_required_mask	= dma_npu_get_required_mask,
> -};
> -
>  /*
>   * Returns the PE assoicated with the PCI device of the given
>   * NPU. Returns the linked pci device if pci_dev != NULL.
> @@ -270,10 +213,11 @@ static void pnv_npu_dma_set_32(struct pnv_ioda_pe *npe)
>  	rc = pnv_npu_set_window(npe, 0, gpe->table_group.tables[0]);
> 
>  	/*
> -	 * We don't initialise npu_pe->tce32_table as we always use
> -	 * dma_npu_ops which are nops.
> +	 * NVLink devices use the same TCE table configuration as
> +	 * their parent device so drivers shouldn't be doing DMA
> +	 * operations directly on these devices.
>  	 */
> -	set_dma_ops(&npe->pdev->dev, &dma_npu_ops);
> +	set_dma_ops(&npe->pdev->dev, NULL);
>  }
> 
>  /*
> --
> 2.11.0
---end quoted text---

Re: [PATCH] powerpc/npu-dma: Remove NPU DMA ops

[Benjamin Herrenschmidt]

On Tue, 2018-10-30 at 13:58 +0100, Christoph Hellwig wrote:
> Please take my patch instead.  We have a kernel polcity to not keep
> dead code around, and everyone including Linus and the attending IBMers
> confirmed this.

Let's call a cat a cat ... ;-)

It's not *dead* code. It's code that is used by an out of tree driver
(which also happen not to be open source).

Just making things clear for everybody.

Cheers,
Ben.

> On Tue, Oct 30, 2018 at 10:02:03PM +1100, Alistair Popple wrote:
> > The NPU IOMMU is setup to mirror the parent PCIe device IOMMU
> > setup. Therefore it does not make sense to call dma operations such as
> > dma_map_page, etc. directly on these devices. The existing dma-ops
> > simply print a warning if they are ever called, however this is
> > unnecessary and the warnings are likely to go unnoticed.
> > 
> > It is instead simpler to remove these operations and let the generic
> > DMA code print warnings (eg. via a NULL pointer deref) in cases of
> > buggy drivers attempting dma operations on NVLink devices.
> > 
> > Signed-off-by: Alistair Popple <alistair@popple.id.au>
> > ---
> >  arch/powerpc/platforms/powernv/npu-dma.c | 64 ++------------------------------
> >  1 file changed, 4 insertions(+), 60 deletions(-)
> > 
> > diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
> > index 6f60e0931922..75b935252981 100644
> > --- a/arch/powerpc/platforms/powernv/npu-dma.c
> > +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> > @@ -102,63 +102,6 @@ struct pci_dev *pnv_pci_get_npu_dev(struct pci_dev *gpdev, int index)
> >  }
> >  EXPORT_SYMBOL(pnv_pci_get_npu_dev);
> > 
> > -#define NPU_DMA_OP_UNSUPPORTED()					\
> > -	dev_err_once(dev, "%s operation unsupported for NVLink devices\n", \
> > -		__func__)
> > -
> > -static void *dma_npu_alloc(struct device *dev, size_t size,
> > -			   dma_addr_t *dma_handle, gfp_t flag,
> > -			   unsigned long attrs)
> > -{
> > -	NPU_DMA_OP_UNSUPPORTED();
> > -	return NULL;
> > -}
> > -
> > -static void dma_npu_free(struct device *dev, size_t size,
> > -			 void *vaddr, dma_addr_t dma_handle,
> > -			 unsigned long attrs)
> > -{
> > -	NPU_DMA_OP_UNSUPPORTED();
> > -}
> > -
> > -static dma_addr_t dma_npu_map_page(struct device *dev, struct page *page,
> > -				   unsigned long offset, size_t size,
> > -				   enum dma_data_direction direction,
> > -				   unsigned long attrs)
> > -{
> > -	NPU_DMA_OP_UNSUPPORTED();
> > -	return 0;
> > -}
> > -
> > -static int dma_npu_map_sg(struct device *dev, struct scatterlist *sglist,
> > -			  int nelems, enum dma_data_direction direction,
> > -			  unsigned long attrs)
> > -{
> > -	NPU_DMA_OP_UNSUPPORTED();
> > -	return 0;
> > -}
> > -
> > -static int dma_npu_dma_supported(struct device *dev, u64 mask)
> > -{
> > -	NPU_DMA_OP_UNSUPPORTED();
> > -	return 0;
> > -}
> > -
> > -static u64 dma_npu_get_required_mask(struct device *dev)
> > -{
> > -	NPU_DMA_OP_UNSUPPORTED();
> > -	return 0;
> > -}
> > -
> > -static const struct dma_map_ops dma_npu_ops = {
> > -	.map_page		= dma_npu_map_page,
> > -	.map_sg			= dma_npu_map_sg,
> > -	.alloc			= dma_npu_alloc,
> > -	.free			= dma_npu_free,
> > -	.dma_supported		= dma_npu_dma_supported,
> > -	.get_required_mask	= dma_npu_get_required_mask,
> > -};
> > -
> >  /*
> >   * Returns the PE assoicated with the PCI device of the given
> >   * NPU. Returns the linked pci device if pci_dev != NULL.
> > @@ -270,10 +213,11 @@ static void pnv_npu_dma_set_32(struct pnv_ioda_pe *npe)
> >  	rc = pnv_npu_set_window(npe, 0, gpe->table_group.tables[0]);
> > 
> >  	/*
> > -	 * We don't initialise npu_pe->tce32_table as we always use
> > -	 * dma_npu_ops which are nops.
> > +	 * NVLink devices use the same TCE table configuration as
> > +	 * their parent device so drivers shouldn't be doing DMA
> > +	 * operations directly on these devices.
> >  	 */
> > -	set_dma_ops(&npe->pdev->dev, &dma_npu_ops);
> > +	set_dma_ops(&npe->pdev->dev, NULL);
> >  }
> > 
> >  /*
> > --
> > 2.11.0
> 
> ---end quoted text---

Re: [PATCH] powerpc/npu-dma: Remove NPU DMA ops

[Christoph Hellwig]

On Wed, Oct 31, 2018 at 12:09:29AM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2018-10-30 at 13:58 +0100, Christoph Hellwig wrote:
> > Please take my patch instead.  We have a kernel polcity to not keep
> > dead code around, and everyone including Linus and the attending IBMers
> > confirmed this.
> 
> Let's call a cat a cat ... ;-)
> 
> It's not *dead* code. It's code that is used by an out of tree driver
> (which also happen not to be open source).

Which clearly makes it a derived work of the kernel if it uses an
interface just create for it, and thus even more important to remove
it to not get anyone into legal trouble.

Re: [PATCH] powerpc/npu-dma: Remove NPU DMA ops

[Michael Ellerman]

Christoph Hellwig <hch@lst.de> writes:

> On Wed, Oct 31, 2018 at 12:09:29AM +1100, Benjamin Herrenschmidt wrote:
>> On Tue, 2018-10-30 at 13:58 +0100, Christoph Hellwig wrote:
>> > Please take my patch instead.  We have a kernel polcity to not keep
>> > dead code around, and everyone including Linus and the attending IBMers
>> > confirmed this.
>> 
>> Let's call a cat a cat ... ;-)
>> 
>> It's not *dead* code. It's code that is used by an out of tree driver
>> (which also happen not to be open source).

(Actually that part of the driver is MIT licensed, but yes other parts
 are not open source.)

> Which clearly makes it a derived work of the kernel if it uses an
> interface just create for it, and thus even more important to remove
> it to not get anyone into legal trouble.

We don't want driver code calling into firmware, that's the job of
platform code, so we would need the same API (or something similar) for
a GPL driver.

cheers

Re: powerpc/npu-dma: Remove NPU DMA ops

[Michael Ellerman]

On Tue, 2018-10-30 at 11:02:03 UTC, Alistair Popple wrote:
> The NPU IOMMU is setup to mirror the parent PCIe device IOMMU
> setup. Therefore it does not make sense to call dma operations such as
> dma_map_page, etc. directly on these devices. The existing dma-ops
> simply print a warning if they are ever called, however this is
> unnecessary and the warnings are likely to go unnoticed.
> 
> It is instead simpler to remove these operations and let the generic
> DMA code print warnings (eg. via a NULL pointer deref) in cases of
> buggy drivers attempting dma operations on NVLink devices.
> 
> Signed-off-by: Alistair Popple <alistair@popple.id.au>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/3182215dd0b2120fb942ed88430cfb

cheers