[PATCH] arm64: remove dummy_dma_ops

[PATCH] arm64: remove dummy_dma_ops

[Christoph Hellwig]

Returning NULL from get_arch_dma_ops makes all DMA mapping routines
retourn sensible errors, so remove the dummy ops.

Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 arch/arm64/include/asm/dma-mapping.h |  4 +-
 arch/arm64/mm/dma-mapping.c          | 92 ----------------------------
 2 files changed, 1 insertion(+), 95 deletions(-)

diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index b7847eb8a7bb7..3dd7c10efc745 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -24,15 +24,13 @@
 #include <xen/xen.h>
 #include <asm/xen/hypervisor.h>
 
-extern const struct dma_map_ops dummy_dma_ops;
-
 static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus)
 {
 	/*
 	 * We expect no ISA devices, and all other DMA masters are expected to
 	 * have someone call arch_setup_dma_ops at device creation time.
 	 */
-	return &dummy_dma_ops;
+	return NULL;
 }
 
 void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 072c51fb07d73..4fdce2db6f12d 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -406,98 +406,6 @@ static int __init atomic_pool_init(void)
 	return -ENOMEM;
 }
 
-/********************************************
- * The following APIs are for dummy DMA ops *
- ********************************************/
-
-static void *__dummy_alloc(struct device *dev, size_t size,
-			   dma_addr_t *dma_handle, gfp_t flags,
-			   unsigned long attrs)
-{
-	return NULL;
-}
-
-static void __dummy_free(struct device *dev, size_t size,
-			 void *vaddr, dma_addr_t dma_handle,
-			 unsigned long attrs)
-{
-}
-
-static int __dummy_mmap(struct device *dev,
-			struct vm_area_struct *vma,
-			void *cpu_addr, dma_addr_t dma_addr, size_t size,
-			unsigned long attrs)
-{
-	return -ENXIO;
-}
-
-static dma_addr_t __dummy_map_page(struct device *dev, struct page *page,
-				   unsigned long offset, size_t size,
-				   enum dma_data_direction dir,
-				   unsigned long attrs)
-{
-	return 0;
-}
-
-static void __dummy_unmap_page(struct device *dev, dma_addr_t dev_addr,
-			       size_t size, enum dma_data_direction dir,
-			       unsigned long attrs)
-{
-}
-
-static int __dummy_map_sg(struct device *dev, struct scatterlist *sgl,
-			  int nelems, enum dma_data_direction dir,
-			  unsigned long attrs)
-{
-	return 0;
-}
-
-static void __dummy_unmap_sg(struct device *dev,
-			     struct scatterlist *sgl, int nelems,
-			     enum dma_data_direction dir,
-			     unsigned long attrs)
-{
-}
-
-static void __dummy_sync_single(struct device *dev,
-				dma_addr_t dev_addr, size_t size,
-				enum dma_data_direction dir)
-{
-}
-
-static void __dummy_sync_sg(struct device *dev,
-			    struct scatterlist *sgl, int nelems,
-			    enum dma_data_direction dir)
-{
-}
-
-static int __dummy_mapping_error(struct device *hwdev, dma_addr_t dma_addr)
-{
-	return 1;
-}
-
-static int __dummy_dma_supported(struct device *hwdev, u64 mask)
-{
-	return 0;
-}
-
-const struct dma_map_ops dummy_dma_ops = {
-	.alloc                  = __dummy_alloc,
-	.free                   = __dummy_free,
-	.mmap                   = __dummy_mmap,
-	.map_page               = __dummy_map_page,
-	.unmap_page             = __dummy_unmap_page,
-	.map_sg                 = __dummy_map_sg,
-	.unmap_sg               = __dummy_unmap_sg,
-	.sync_single_for_cpu    = __dummy_sync_single,
-	.sync_single_for_device = __dummy_sync_single,
-	.sync_sg_for_cpu        = __dummy_sync_sg,
-	.sync_sg_for_device     = __dummy_sync_sg,
-	.mapping_error          = __dummy_mapping_error,
-	.dma_supported          = __dummy_dma_supported,
-};
-EXPORT_SYMBOL(dummy_dma_ops);
-
 static int __init arm64_dma_init(void)
 {
 	if (swiotlb_force == SWIOTLB_FORCE ||
-- 
2.18.0

Re: [PATCH] arm64: remove dummy_dma_ops

[Robin Murphy]

On 02/08/18 13:13, Christoph Hellwig wrote:
> Returning NULL from get_arch_dma_ops makes all DMA mapping routines
> retourn sensible errors, so remove the dummy ops.

Does it? AFAICS all of the non-optional callbacks will still either 
BUG_ON(!ops) or blindly dereference the null pointer. Have I lost track 
of another cleanup patch somewhere?

FWIW the point of dummy_dma_ops is to fail safe when the ACPI tables 
fail to specify _CCA for a device - of course that *should* be caught 
during firmware development, but if something does slip through because 
the firmware developer used an older kernel (or never tested device x at 
all), then the difference to the end user between "device x can't do 
DMA" and "new kernel blows up" is pretty significant.

Robin.

> Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> ---
>   arch/arm64/include/asm/dma-mapping.h |  4 +-
>   arch/arm64/mm/dma-mapping.c          | 92 ----------------------------
>   2 files changed, 1 insertion(+), 95 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
> index b7847eb8a7bb7..3dd7c10efc745 100644
> --- a/arch/arm64/include/asm/dma-mapping.h
> +++ b/arch/arm64/include/asm/dma-mapping.h
> @@ -24,15 +24,13 @@
>   #include <xen/xen.h>
>   #include <asm/xen/hypervisor.h>
>   
> -extern const struct dma_map_ops dummy_dma_ops;
> -
>   static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus)
>   {
>   	/*
>   	 * We expect no ISA devices, and all other DMA masters are expected to
>   	 * have someone call arch_setup_dma_ops at device creation time.
>   	 */
> -	return &dummy_dma_ops;
> +	return NULL;
>   }
>   
>   void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 072c51fb07d73..4fdce2db6f12d 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -406,98 +406,6 @@ static int __init atomic_pool_init(void)
>   	return -ENOMEM;
>   }
>   
> -/********************************************
> - * The following APIs are for dummy DMA ops *
> - ********************************************/
> -
> -static void *__dummy_alloc(struct device *dev, size_t size,
> -			   dma_addr_t *dma_handle, gfp_t flags,
> -			   unsigned long attrs)
> -{
> -	return NULL;
> -}
> -
> -static void __dummy_free(struct device *dev, size_t size,
> -			 void *vaddr, dma_addr_t dma_handle,
> -			 unsigned long attrs)
> -{
> -}
> -
> -static int __dummy_mmap(struct device *dev,
> -			struct vm_area_struct *vma,
> -			void *cpu_addr, dma_addr_t dma_addr, size_t size,
> -			unsigned long attrs)
> -{
> -	return -ENXIO;
> -}
> -
> -static dma_addr_t __dummy_map_page(struct device *dev, struct page *page,
> -				   unsigned long offset, size_t size,
> -				   enum dma_data_direction dir,
> -				   unsigned long attrs)
> -{
> -	return 0;
> -}
> -
> -static void __dummy_unmap_page(struct device *dev, dma_addr_t dev_addr,
> -			       size_t size, enum dma_data_direction dir,
> -			       unsigned long attrs)
> -{
> -}
> -
> -static int __dummy_map_sg(struct device *dev, struct scatterlist *sgl,
> -			  int nelems, enum dma_data_direction dir,
> -			  unsigned long attrs)
> -{
> -	return 0;
> -}
> -
> -static void __dummy_unmap_sg(struct device *dev,
> -			     struct scatterlist *sgl, int nelems,
> -			     enum dma_data_direction dir,
> -			     unsigned long attrs)
> -{
> -}
> -
> -static void __dummy_sync_single(struct device *dev,
> -				dma_addr_t dev_addr, size_t size,
> -				enum dma_data_direction dir)
> -{
> -}
> -
> -static void __dummy_sync_sg(struct device *dev,
> -			    struct scatterlist *sgl, int nelems,
> -			    enum dma_data_direction dir)
> -{
> -}
> -
> -static int __dummy_mapping_error(struct device *hwdev, dma_addr_t dma_addr)
> -{
> -	return 1;
> -}
> -
> -static int __dummy_dma_supported(struct device *hwdev, u64 mask)
> -{
> -	return 0;
> -}
> -
> -const struct dma_map_ops dummy_dma_ops = {
> -	.alloc                  = __dummy_alloc,
> -	.free                   = __dummy_free,
> -	.mmap                   = __dummy_mmap,
> -	.map_page               = __dummy_map_page,
> -	.unmap_page             = __dummy_unmap_page,
> -	.map_sg                 = __dummy_map_sg,
> -	.unmap_sg               = __dummy_unmap_sg,
> -	.sync_single_for_cpu    = __dummy_sync_single,
> -	.sync_single_for_device = __dummy_sync_single,
> -	.sync_sg_for_cpu        = __dummy_sync_sg,
> -	.sync_sg_for_device     = __dummy_sync_sg,
> -	.mapping_error          = __dummy_mapping_error,
> -	.dma_supported          = __dummy_dma_supported,
> -};
> -EXPORT_SYMBOL(dummy_dma_ops);
> -
>   static int __init arm64_dma_init(void)
>   {
>   	if (swiotlb_force == SWIOTLB_FORCE ||
> 

Re: [PATCH] arm64: remove dummy_dma_ops

[Christoph Hellwig]

On Thu, Aug 02, 2018 at 01:32:17PM +0100, Robin Murphy wrote:
> On 02/08/18 13:13, Christoph Hellwig wrote:
>> Returning NULL from get_arch_dma_ops makes all DMA mapping routines
>> retourn sensible errors, so remove the dummy ops.
>
> Does it? AFAICS all of the non-optional callbacks will still either 
> BUG_ON(!ops) or blindly dereference the null pointer. Have I lost track of 
> another cleanup patch somewhere?

First thing any driver needs to do is dma_set_mask, which first calls
dma_supported:

static inline int dma_supported(struct device *dev, u64 mask)
{
        const struct dma_map_ops *ops = get_dma_ops(dev);

	if (!ops)
		return 0;
	...
}

Re: [PATCH] arm64: remove dummy_dma_ops

[Robin Murphy]

On 02/08/18 13:43, Christoph Hellwig wrote:
> On Thu, Aug 02, 2018 at 01:32:17PM +0100, Robin Murphy wrote:
>> On 02/08/18 13:13, Christoph Hellwig wrote:
>>> Returning NULL from get_arch_dma_ops makes all DMA mapping routines
>>> retourn sensible errors, so remove the dummy ops.
>>
>> Does it? AFAICS all of the non-optional callbacks will still either
>> BUG_ON(!ops) or blindly dereference the null pointer. Have I lost track of
>> another cleanup patch somewhere?
> 
> First thing any driver needs to do is dma_set_mask, which first calls
> dma_supported:
> 
> static inline int dma_supported(struct device *dev, u64 mask)
> {
>          const struct dma_map_ops *ops = get_dma_ops(dev);
> 
> 	if (!ops)
> 		return 0;
> 	...
> }
> 

Yeah, apart from all the drivers that don't. After last week's fun I'm 
not putting any reliance in that assumption ;)

Of course, the combination of a cheeky legacy driver assuming 32-bit DMA 
*and* busted firmware failing to describe that device properly should be 
rare enough that this change is almost certainly a non-issue in 
practice, but I'm still never going to say I love it.

Robin.