cleanup ->dma_configure calling conventions

cleanup ->dma_configure calling conventions

[Christoph Hellwig]

Hi all,

this series cleans up a bit how we call ->dma_configure and how
we clean up after tearing down a device that the method did set
up.

[PATCH 1/4] arm-nommu: don't define arch_teardown_dma_ops

[Christoph Hellwig]

We can just use the default implementation.

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

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index 8436f6ade57d..965b7c846ecb 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -100,8 +100,10 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 extern void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 			       const struct iommu_ops *iommu, bool coherent);
 
+#ifdef CONFIG_MMU
 #define arch_teardown_dma_ops arch_teardown_dma_ops
 extern void arch_teardown_dma_ops(struct device *dev);
+#endif
 
 /* do not use this function in a driver */
 static inline bool is_device_dma_coherent(struct device *dev)
diff --git a/arch/arm/mm/dma-mapping-nommu.c b/arch/arm/mm/dma-mapping-nommu.c
index f448a0663b10..aa7aba302e76 100644
--- a/arch/arm/mm/dma-mapping-nommu.c
+++ b/arch/arm/mm/dma-mapping-nommu.c
@@ -237,7 +237,3 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 
 	set_dma_ops(dev, dma_ops);
 }
-
-void arch_teardown_dma_ops(struct device *dev)
-{
-}
-- 
2.18.0

Re: [PATCH 1/4] arm-nommu: don't define arch_teardown_dma_ops

[Robin Murphy]

On 27/08/18 09:47, Christoph Hellwig wrote:
> We can just use the default implementation.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   arch/arm/include/asm/dma-mapping.h | 2 ++
>   arch/arm/mm/dma-mapping-nommu.c    | 4 ----
>   2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
> index 8436f6ade57d..965b7c846ecb 100644
> --- a/arch/arm/include/asm/dma-mapping.h
> +++ b/arch/arm/include/asm/dma-mapping.h
> @@ -100,8 +100,10 @@ static inline unsigned long dma_max_pfn(struct device *dev)
>   extern void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>   			       const struct iommu_ops *iommu, bool coherent);
>   
> +#ifdef CONFIG_MMU
>   #define arch_teardown_dma_ops arch_teardown_dma_ops
>   extern void arch_teardown_dma_ops(struct device *dev);
> +#endif
>   
>   /* do not use this function in a driver */
>   static inline bool is_device_dma_coherent(struct device *dev)
> diff --git a/arch/arm/mm/dma-mapping-nommu.c b/arch/arm/mm/dma-mapping-nommu.c
> index f448a0663b10..aa7aba302e76 100644
> --- a/arch/arm/mm/dma-mapping-nommu.c
> +++ b/arch/arm/mm/dma-mapping-nommu.c
> @@ -237,7 +237,3 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>   
>   	set_dma_ops(dev, dma_ops);
>   }
> -
> -void arch_teardown_dma_ops(struct device *dev)
> -{
> -}
> 

Re: [PATCH 1/4] arm-nommu: don't define arch_teardown_dma_ops

[Vladimir Murzin]

On 27/08/18 09:47, Christoph Hellwig wrote:
> We can just use the default implementation.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/arm/include/asm/dma-mapping.h | 2 ++
>  arch/arm/mm/dma-mapping-nommu.c    | 4 ----
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
> index 8436f6ade57d..965b7c846ecb 100644
> --- a/arch/arm/include/asm/dma-mapping.h
> +++ b/arch/arm/include/asm/dma-mapping.h
> @@ -100,8 +100,10 @@ static inline unsigned long dma_max_pfn(struct device *dev)
>  extern void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  			       const struct iommu_ops *iommu, bool coherent);
>  
> +#ifdef CONFIG_MMU
>  #define arch_teardown_dma_ops arch_teardown_dma_ops
>  extern void arch_teardown_dma_ops(struct device *dev);
> +#endif
>  
>  /* do not use this function in a driver */
>  static inline bool is_device_dma_coherent(struct device *dev)
> diff --git a/arch/arm/mm/dma-mapping-nommu.c b/arch/arm/mm/dma-mapping-nommu.c
> index f448a0663b10..aa7aba302e76 100644
> --- a/arch/arm/mm/dma-mapping-nommu.c
> +++ b/arch/arm/mm/dma-mapping-nommu.c
> @@ -237,7 +237,3 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  
>  	set_dma_ops(dev, dma_ops);
>  }
> -
> -void arch_teardown_dma_ops(struct device *dev)
> -{
> -}
> 

FWIW:

Reviewed-by: Vladimir Murzin <vladimir.murzin@arm.com>

Thanks
Vladimir

[PATCH 2/4] dma-mapping: remove dma_configure

[Christoph Hellwig]

There is no good reason for this indirection given that the method
always exists.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/base/dd.c           |  8 +++++---
 include/linux/dma-mapping.h |  6 ------
 kernel/dma/mapping.c        | 10 ----------
 3 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index edfc9f0b1180..65128cf8427c 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -480,9 +480,11 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	if (ret)
 		goto pinctrl_bind_failed;
 
-	ret = dma_configure(dev);
-	if (ret)
-		goto dma_failed;
+	if (dev->bus->dma_configure) {
+		ret = dev->bus->dma_configure(dev);
+		if (ret)
+			goto dma_failed;
+	}
 
 	if (driver_sysfs_add(dev)) {
 		printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n",
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 1db6a6b46d0d..1c6c7c09bcf2 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -754,14 +754,8 @@ dma_mark_declared_memory_occupied(struct device *dev,
 #endif /* CONFIG_HAVE_GENERIC_DMA_COHERENT */
 
 #ifdef CONFIG_HAS_DMA
-int dma_configure(struct device *dev);
 void dma_deconfigure(struct device *dev);
 #else
-static inline int dma_configure(struct device *dev)
-{
-	return 0;
-}
-
 static inline void dma_deconfigure(struct device *dev) {}
 #endif
 
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index d2a92ddaac4d..25607ceb4a50 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -328,16 +328,6 @@ void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags)
 }
 #endif
 
-/*
- * enables DMA API use for a device
- */
-int dma_configure(struct device *dev)
-{
-	if (dev->bus->dma_configure)
-		return dev->bus->dma_configure(dev);
-	return 0;
-}
-
 void dma_deconfigure(struct device *dev)
 {
 	of_dma_deconfigure(dev);
-- 
2.18.0

Re: [PATCH 2/4] dma-mapping: remove dma_configure

[Robin Murphy]

On 27/08/18 09:47, Christoph Hellwig wrote:
> There is no good reason for this indirection given that the method
> always exists.

And indeed with the way it works these days we don't really want 
dma_configure() to look like something which can be called from anywhere.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/base/dd.c           |  8 +++++---
>   include/linux/dma-mapping.h |  6 ------
>   kernel/dma/mapping.c        | 10 ----------
>   3 files changed, 5 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index edfc9f0b1180..65128cf8427c 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -480,9 +480,11 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>   	if (ret)
>   		goto pinctrl_bind_failed;
>   
> -	ret = dma_configure(dev);
> -	if (ret)
> -		goto dma_failed;
> +	if (dev->bus->dma_configure) {
> +		ret = dev->bus->dma_configure(dev);
> +		if (ret)
> +			goto dma_failed;
> +	}
>   
>   	if (driver_sysfs_add(dev)) {
>   		printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n",
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 1db6a6b46d0d..1c6c7c09bcf2 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -754,14 +754,8 @@ dma_mark_declared_memory_occupied(struct device *dev,
>   #endif /* CONFIG_HAVE_GENERIC_DMA_COHERENT */
>   
>   #ifdef CONFIG_HAS_DMA
> -int dma_configure(struct device *dev);
>   void dma_deconfigure(struct device *dev);
>   #else
> -static inline int dma_configure(struct device *dev)
> -{
> -	return 0;
> -}
> -
>   static inline void dma_deconfigure(struct device *dev) {}
>   #endif
>   
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index d2a92ddaac4d..25607ceb4a50 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -328,16 +328,6 @@ void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags)
>   }
>   #endif
>   
> -/*
> - * enables DMA API use for a device
> - */
> -int dma_configure(struct device *dev)
> -{
> -	if (dev->bus->dma_configure)
> -		return dev->bus->dma_configure(dev);
> -	return 0;
> -}
> -
>   void dma_deconfigure(struct device *dev)
>   {
>   	of_dma_deconfigure(dev);
> 

[PATCH 3/4] dma-mapping: remove dma_deconfigure

[Christoph Hellwig]

This goes through a lot of hooks just to call arch_teardown_dma_ops.
Replace it with a direct call instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/acpi/arm64/iort.c   |  2 +-
 drivers/acpi/scan.c         | 10 ----------
 drivers/base/dd.c           |  4 ++--
 drivers/of/device.c         | 12 ------------
 include/acpi/acpi_bus.h     |  1 -
 include/linux/acpi.h        |  2 --
 include/linux/dma-mapping.h |  6 ------
 include/linux/of_device.h   |  3 ---
 kernel/dma/mapping.c        |  6 ------
 9 files changed, 3 insertions(+), 43 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 08f26db2da7e..2a361e22d38d 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -1428,7 +1428,7 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,
 	return 0;
 
 dma_deconfigure:
-	acpi_dma_deconfigure(&pdev->dev);
+	arch_teardown_dma_ops(&pdev->dev);
 dev_put:
 	platform_device_put(pdev);
 
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index e1b6231cfa1c..56676a56b3e3 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1469,16 +1469,6 @@ int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
 }
 EXPORT_SYMBOL_GPL(acpi_dma_configure);
 
-/**
- * acpi_dma_deconfigure - Tear-down DMA configuration for the device.
- * @dev: The pointer to the device
- */
-void acpi_dma_deconfigure(struct device *dev)
-{
-	arch_teardown_dma_ops(dev);
-}
-EXPORT_SYMBOL_GPL(acpi_dma_deconfigure);
-
 static void acpi_init_coherency(struct acpi_device *adev)
 {
 	unsigned long long cca = 0;
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 65128cf8427c..169412ee4ae8 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -539,7 +539,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	goto done;
 
 probe_failed:
-	dma_deconfigure(dev);
+	arch_teardown_dma_ops(dev);
 dma_failed:
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
@@ -968,7 +968,7 @@ static void __device_release_driver(struct device *dev, struct device *parent)
 			drv->remove(dev);
 
 		device_links_driver_cleanup(dev);
-		dma_deconfigure(dev);
+		arch_teardown_dma_ops(dev);
 
 		devres_release_all(dev);
 		dev->driver = NULL;
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 5957cd4fa262..c7fa5a9697c9 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -170,18 +170,6 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
 }
 EXPORT_SYMBOL_GPL(of_dma_configure);
 
-/**
- * of_dma_deconfigure - Clean up DMA configuration
- * @dev:	Device for which to clean up DMA configuration
- *
- * Clean up all configuration performed by of_dma_configure_ops() and free all
- * resources that have been allocated.
- */
-void of_dma_deconfigure(struct device *dev)
-{
-	arch_teardown_dma_ops(dev);
-}
-
 int of_device_register(struct platform_device *pdev)
 {
 	device_initialize(&pdev->dev);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index ba4dd54f2c82..53600f527a70 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -595,7 +595,6 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev);
 int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
 		       u64 *size);
 int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr);
-void acpi_dma_deconfigure(struct device *dev);
 
 struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
 					   u64 address, bool check_children);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index de8d3d3fa651..af4628979d13 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -831,8 +831,6 @@ static inline int acpi_dma_configure(struct device *dev,
 	return 0;
 }
 
-static inline void acpi_dma_deconfigure(struct device *dev) { }
-
 #define ACPI_PTR(_ptr)	(NULL)
 
 static inline void acpi_device_set_enumerated(struct acpi_device *adev)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 1c6c7c09bcf2..1423b69f3cc9 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -753,12 +753,6 @@ dma_mark_declared_memory_occupied(struct device *dev,
 }
 #endif /* CONFIG_HAVE_GENERIC_DMA_COHERENT */
 
-#ifdef CONFIG_HAS_DMA
-void dma_deconfigure(struct device *dev);
-#else
-static inline void dma_deconfigure(struct device *dev) {}
-#endif
-
 /*
  * Managed DMA API
  */
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index 165fd302b442..8d31e39dd564 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -58,7 +58,6 @@ static inline struct device_node *of_cpu_device_node_get(int cpu)
 int of_dma_configure(struct device *dev,
 		     struct device_node *np,
 		     bool force_dma);
-void of_dma_deconfigure(struct device *dev);
 #else /* CONFIG_OF */
 
 static inline int of_driver_match_device(struct device *dev,
@@ -113,8 +112,6 @@ static inline int of_dma_configure(struct device *dev,
 {
 	return 0;
 }
-static inline void of_dma_deconfigure(struct device *dev)
-{}
 #endif /* CONFIG_OF */
 
 #endif /* _LINUX_OF_DEVICE_H */
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index 25607ceb4a50..3540cb399bd2 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -327,9 +327,3 @@ void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags)
 	vunmap(cpu_addr);
 }
 #endif
-
-void dma_deconfigure(struct device *dev)
-{
-	of_dma_deconfigure(dev);
-	acpi_dma_deconfigure(dev);
-}
-- 
2.18.0

Re: [PATCH 3/4] dma-mapping: remove dma_deconfigure

[Robin Murphy]

On 27/08/18 09:47, Christoph Hellwig wrote:
> This goes through a lot of hooks just to call arch_teardown_dma_ops.
> Replace it with a direct call instead.

Agreed. We originally had the deconfigure() hooks for symmetry in case 
there might need to be some firmware-specific state to tear down, but 
with the benefit of a couple of years' hindsight now it really doesn't 
look like that's ever likely to be necessary.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/acpi/arm64/iort.c   |  2 +-
>   drivers/acpi/scan.c         | 10 ----------
>   drivers/base/dd.c           |  4 ++--
>   drivers/of/device.c         | 12 ------------
>   include/acpi/acpi_bus.h     |  1 -
>   include/linux/acpi.h        |  2 --
>   include/linux/dma-mapping.h |  6 ------
>   include/linux/of_device.h   |  3 ---
>   kernel/dma/mapping.c        |  6 ------
>   9 files changed, 3 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 08f26db2da7e..2a361e22d38d 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -1428,7 +1428,7 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,
>   	return 0;
>   
>   dma_deconfigure:
> -	acpi_dma_deconfigure(&pdev->dev);
> +	arch_teardown_dma_ops(&pdev->dev);
>   dev_put:
>   	platform_device_put(pdev);
>   
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index e1b6231cfa1c..56676a56b3e3 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1469,16 +1469,6 @@ int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
>   }
>   EXPORT_SYMBOL_GPL(acpi_dma_configure);
>   
> -/**
> - * acpi_dma_deconfigure - Tear-down DMA configuration for the device.
> - * @dev: The pointer to the device
> - */
> -void acpi_dma_deconfigure(struct device *dev)
> -{
> -	arch_teardown_dma_ops(dev);
> -}
> -EXPORT_SYMBOL_GPL(acpi_dma_deconfigure);
> -
>   static void acpi_init_coherency(struct acpi_device *adev)
>   {
>   	unsigned long long cca = 0;
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 65128cf8427c..169412ee4ae8 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -539,7 +539,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>   	goto done;
>   
>   probe_failed:
> -	dma_deconfigure(dev);
> +	arch_teardown_dma_ops(dev);
>   dma_failed:
>   	if (dev->bus)
>   		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> @@ -968,7 +968,7 @@ static void __device_release_driver(struct device *dev, struct device *parent)
>   			drv->remove(dev);
>   
>   		device_links_driver_cleanup(dev);
> -		dma_deconfigure(dev);
> +		arch_teardown_dma_ops(dev);
>   
>   		devres_release_all(dev);
>   		dev->driver = NULL;
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 5957cd4fa262..c7fa5a9697c9 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -170,18 +170,6 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
>   }
>   EXPORT_SYMBOL_GPL(of_dma_configure);
>   
> -/**
> - * of_dma_deconfigure - Clean up DMA configuration
> - * @dev:	Device for which to clean up DMA configuration
> - *
> - * Clean up all configuration performed by of_dma_configure_ops() and free all
> - * resources that have been allocated.
> - */
> -void of_dma_deconfigure(struct device *dev)
> -{
> -	arch_teardown_dma_ops(dev);
> -}
> -
>   int of_device_register(struct platform_device *pdev)
>   {
>   	device_initialize(&pdev->dev);
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index ba4dd54f2c82..53600f527a70 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -595,7 +595,6 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev);
>   int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
>   		       u64 *size);
>   int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr);
> -void acpi_dma_deconfigure(struct device *dev);
>   
>   struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
>   					   u64 address, bool check_children);
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index de8d3d3fa651..af4628979d13 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -831,8 +831,6 @@ static inline int acpi_dma_configure(struct device *dev,
>   	return 0;
>   }
>   
> -static inline void acpi_dma_deconfigure(struct device *dev) { }
> -
>   #define ACPI_PTR(_ptr)	(NULL)
>   
>   static inline void acpi_device_set_enumerated(struct acpi_device *adev)
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 1c6c7c09bcf2..1423b69f3cc9 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -753,12 +753,6 @@ dma_mark_declared_memory_occupied(struct device *dev,
>   }
>   #endif /* CONFIG_HAVE_GENERIC_DMA_COHERENT */
>   
> -#ifdef CONFIG_HAS_DMA
> -void dma_deconfigure(struct device *dev);
> -#else
> -static inline void dma_deconfigure(struct device *dev) {}
> -#endif
> -
>   /*
>    * Managed DMA API
>    */
> diff --git a/include/linux/of_device.h b/include/linux/of_device.h
> index 165fd302b442..8d31e39dd564 100644
> --- a/include/linux/of_device.h
> +++ b/include/linux/of_device.h
> @@ -58,7 +58,6 @@ static inline struct device_node *of_cpu_device_node_get(int cpu)
>   int of_dma_configure(struct device *dev,
>   		     struct device_node *np,
>   		     bool force_dma);
> -void of_dma_deconfigure(struct device *dev);
>   #else /* CONFIG_OF */
>   
>   static inline int of_driver_match_device(struct device *dev,
> @@ -113,8 +112,6 @@ static inline int of_dma_configure(struct device *dev,
>   {
>   	return 0;
>   }
> -static inline void of_dma_deconfigure(struct device *dev)
> -{}
>   #endif /* CONFIG_OF */
>   
>   #endif /* _LINUX_OF_DEVICE_H */
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index 25607ceb4a50..3540cb399bd2 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -327,9 +327,3 @@ void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags)
>   	vunmap(cpu_addr);
>   }
>   #endif
> -
> -void dma_deconfigure(struct device *dev)
> -{
> -	of_dma_deconfigure(dev);
> -	acpi_dma_deconfigure(dev);
> -}
> 

[PATCH 4/4] dma-mapping: clear dev->dma_ops in arch_teardown_dma_ops

[Christoph Hellwig]

There is no reason to leave the per-device dma_ops around when
deconfiguring a device, so move this code from arm64 into the
common code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/arm64/include/asm/dma-mapping.h | 5 -----
 arch/arm64/mm/dma-mapping.c          | 5 -----
 include/linux/dma-mapping.h          | 5 ++++-
 3 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index b7847eb8a7bb..0a2d13332545 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -39,11 +39,6 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 			const struct iommu_ops *iommu, bool coherent);
 #define arch_setup_dma_ops	arch_setup_dma_ops
 
-#ifdef CONFIG_IOMMU_DMA
-void arch_teardown_dma_ops(struct device *dev);
-#define arch_teardown_dma_ops	arch_teardown_dma_ops
-#endif
-
 /* do not use this function in a driver */
 static inline bool is_device_dma_coherent(struct device *dev)
 {
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 072c51fb07d7..cdcb73db9ea2 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -862,11 +862,6 @@ static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 		 dev_name(dev));
 }
 
-void arch_teardown_dma_ops(struct device *dev)
-{
-	dev->dma_ops = NULL;
-}
-
 #else
 
 static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 1423b69f3cc9..eafd6f318e78 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -664,7 +664,10 @@ static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
 #endif
 
 #ifndef arch_teardown_dma_ops
-static inline void arch_teardown_dma_ops(struct device *dev) { }
+static inline void arch_teardown_dma_ops(struct device *dev)
+{
+	dev->dma_ops = NULL;
+}
 #endif
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
-- 
2.18.0

Re: [PATCH 4/4] dma-mapping: clear dev->dma_ops in arch_teardown_dma_ops

[Robin Murphy]

On 27/08/18 09:47, Christoph Hellwig wrote:
> There is no reason to leave the per-device dma_ops around when
> deconfiguring a device, so move this code from arm64 into the
> common code.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   arch/arm64/include/asm/dma-mapping.h | 5 -----
>   arch/arm64/mm/dma-mapping.c          | 5 -----
>   include/linux/dma-mapping.h          | 5 ++++-
>   3 files changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
> index b7847eb8a7bb..0a2d13332545 100644
> --- a/arch/arm64/include/asm/dma-mapping.h
> +++ b/arch/arm64/include/asm/dma-mapping.h
> @@ -39,11 +39,6 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>   			const struct iommu_ops *iommu, bool coherent);
>   #define arch_setup_dma_ops	arch_setup_dma_ops
>   
> -#ifdef CONFIG_IOMMU_DMA
> -void arch_teardown_dma_ops(struct device *dev);
> -#define arch_teardown_dma_ops	arch_teardown_dma_ops
> -#endif
> -
>   /* do not use this function in a driver */
>   static inline bool is_device_dma_coherent(struct device *dev)
>   {
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 072c51fb07d7..cdcb73db9ea2 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -862,11 +862,6 @@ static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>   		 dev_name(dev));
>   }
>   
> -void arch_teardown_dma_ops(struct device *dev)
> -{
> -	dev->dma_ops = NULL;
> -}
> -
>   #else
>   
>   static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 1423b69f3cc9..eafd6f318e78 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -664,7 +664,10 @@ static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
>   #endif
>   
>   #ifndef arch_teardown_dma_ops
> -static inline void arch_teardown_dma_ops(struct device *dev) { }
> +static inline void arch_teardown_dma_ops(struct device *dev)
> +{
> +	dev->dma_ops = NULL;
> +}
>   #endif
>   
>   static inline unsigned int dma_get_max_seg_size(struct device *dev)
> 

Re: [PATCH 4/4] dma-mapping: clear dev->dma_ops in arch_teardown_dma_ops

[Guenter Roeck]

Hi,

On Mon, Aug 27, 2018 at 10:47:11AM +0200, Christoph Hellwig wrote:
> There is no reason to leave the per-device dma_ops around when
> deconfiguring a device, so move this code from arm64 into the
> common code.
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>

This patch causes various ppc images to crash in -next due to NULL
DMA ops in dma_alloc_attrs().

Looking into the code, the macio driver tries to instantiate on
the 0000:f0:0d.0 PCI address (the driver maps to all Apple PCI IDs)
and fails. This results in a call to arch_teardown_dma_ops(). Later,
the same device pointer is used to instantiate ohci-pci, which
crashes in probe because the dma_ops pointer has been cleared.

I don't claim to fully understand the code, but to me it looks like
the pci device is allocated and waiting for a driver to attach to.
See arch/powerpc/kernel/pci_of_scan.c:of_create_pci_dev().
If attaching a driver (here: macio) fails, the same device pointer
is then reused for the next matching driver until a match is found
and the device is successfully instantiated. Of course this fails
quite badly if the device pointer has been scrubbed after the first
failure.

I don't know if this is generic PCI behavior or ppc specific.
I am copying pci and ppc maintainers for additional input.

Either case, reverting the patch fixes the problem.

Guenter

---
ohci-pci 0000:f0:0d.0: OHCI PCI host controller
ohci-pci 0000:f0:0d.0: new USB bus registered, assigned bus number 1
------------[ cut here ]------------
kernel BUG at ./include/linux/dma-mapping.h:516!
Oops: Exception in kernel mode, sig: 5 [#1]
BE PREEMPT SMP NR_CPUS=4 NUMA PowerMac
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W
4.19.0-rc4-next-20180921-dirty #1
NIP:  c00000000086b824 LR: c00000000086b5dc CTR: c00000000086dd70
REGS: c00000003d30f0b0 TRAP: 0700   Tainted: G        W
(4.19.0-rc4-next-20180921-dirty)
MSR:  800000000002b032 <SF,EE,FP,ME,IR,DR,RI>  CR: 28008842  XER: 00000000
IRQMASK: 0 
GPR00: c00000000086b5dc c00000003d30f330 c000000001199900 c00000003d3ce898 
GPR04: c00000000115b798 c00000003d8c3a48 0000000000000000 0000000000000000 
GPR08: 0000000000000000 ffffffffffffff00 0000000000000000 0000000000000020 
GPR12: 0000000024008442 c000000001299000 c00000000000f720 0000000000000000 
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 
GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 
GPR24: 0000000000000000 c0000000010452e8 c000000001045420 0000000000000080 
GPR28: 000000000000001c c000000001045408 c00000003d3ce898 c00000003d8c3a30 
NIP [c00000000086b824] .ohci_init+0x564/0x570
LR [c00000000086b5dc] .ohci_init+0x31c/0x570
Call Trace:
[c00000003d30f330] [c00000000086b5dc] .ohci_init+0x31c/0x570 (unreliable)
[c00000003d30f3c0] [c00000000086de58] .ohci_pci_reset+0xa8/0xb0
[c00000003d30f440] [c0000000008335ec] .usb_add_hcd+0x35c/0x9b0
[c00000003d30f510] [c00000000084ea90] .usb_hcd_pci_probe+0x320/0x510
[c00000003d30f5c0] [c0000000006c7df8] .local_pci_probe+0x68/0x140
[c00000003d30f660] [c0000000006c92a4] .pci_device_probe+0x144/0x210
[c00000003d30f710] [c00000000074cd48] .really_probe+0x2a8/0x3c0
[c00000003d30f7b0] [c00000000074d100] .driver_probe_device+0x80/0x170
[c00000003d30f840] [c00000000074d33c] .__driver_attach+0x14c/0x150
[c00000003d30f8d0] [c000000000749c6c] .bus_for_each_dev+0xac/0x100
[c00000003d30f970] [c00000000074c334] .driver_attach+0x34/0x50
[c00000003d30f9f0] [c00000000074b9b8] .bus_add_driver+0x178/0x2f0
[c00000003d30fa90] [c00000000074e560] .driver_register+0x90/0x1a0
[c00000003d30fb10] [c0000000006c707c] .__pci_register_driver+0x6c/0x90
[c00000003d30fba0] [c000000000f39f14] .ohci_pci_init+0x90/0xac
[c00000003d30fc10] [c00000000000f380] .do_one_initcall+0x70/0x2d0
[c00000003d30fce0] [c000000000edfca4] .kernel_init_freeable+0x3b8/0x4b0
[c00000003d30fdb0] [c00000000000f744] .kernel_init+0x24/0x160
[c00000003d30fe30] [c00000000000b7a4] .ret_from_kernel_thread+0x58/0x74

---
# bad: [46c163a036b41a29b0ec3c475bf97515d755ff41] Add linux-next specific files for 20180921
# good: [7876320f88802b22d4e2daf7eb027dd14175a0f8] Linux 4.19-rc4
git bisect start 'HEAD' 'v4.19-rc4'
# bad: [03b5533c4d89cc558063a98fa4201a5d2b4eb1f7] Merge remote-tracking branch 'crypto/master'
git bisect bad 03b5533c4d89cc558063a98fa4201a5d2b4eb1f7
# bad: [62c54071a46255d59e26e95528b80bf432796cb4] Merge remote-tracking branch 'v9fs/9p-next'
git bisect bad 62c54071a46255d59e26e95528b80bf432796cb4
# bad: [1eee72bfcf0977daca74e9f902956adbb4f38847] Merge remote-tracking branch 'realtek/for-next'
git bisect bad 1eee72bfcf0977daca74e9f902956adbb4f38847
# good: [30d3220045f49c707bbeec1d35423bd60488c433] Merge remote-tracking branch 'scsi-fixes/fixes'
git bisect good 30d3220045f49c707bbeec1d35423bd60488c433
# bad: [a31a9772a2aa569dc279468da4be555b737e51f8] Merge remote-tracking branch 'at91/at91-next'
git bisect bad a31a9772a2aa569dc279468da4be555b737e51f8
# bad: [3f52a0ad91857e78a5feed28327eafa11c44412c] Merge remote-tracking branch 'arm-soc/for-next'
git bisect bad 3f52a0ad91857e78a5feed28327eafa11c44412c
# good: [92c76bf9685778b2b7e5d6b4c93d74d9ef5d54a7] Merge remote-tracking branch 'leaks/leaks-next'
git bisect good 92c76bf9685778b2b7e5d6b4c93d74d9ef5d54a7
# good: [4e7afff85160ffaa236785591126cf52e11f077c] Merge branch 'fixes' into for-next
git bisect good 4e7afff85160ffaa236785591126cf52e11f077c
# bad: [5748e1b35ba28368515d850e8087929a3a65e055] MIPS: don't select DMA_MAYBE_COHERENT from DMA_PERDEV_COHERENT
git bisect bad 5748e1b35ba28368515d850e8087929a3a65e055
# good: [ccf640f4c9988653ef884672381b03b9be247bec] dma-mapping: remove dma_configure
git bisect good ccf640f4c9988653ef884672381b03b9be247bec
# bad: [46053c73685411915d3de50c5a0045beef32806b] dma-mapping: clear dev->dma_ops in arch_teardown_dma_ops
git bisect bad 46053c73685411915d3de50c5a0045beef32806b
# good: [dc3c05504d38849f77149cb962caeaedd1efa127] dma-mapping: remove dma_deconfigure
git bisect good dc3c05504d38849f77149cb962caeaedd1efa127
# first bad commit: [46053c73685411915d3de50c5a0045beef32806b] dma-mapping: clear dev->dma_ops in arch_teardown_dma_ops

Re: [PATCH 4/4] dma-mapping: clear dev->dma_ops in arch_teardown_dma_ops

[Michael Ellerman]

Guenter Roeck <linux@roeck-us.net> writes:
> Hi,
>
> On Mon, Aug 27, 2018 at 10:47:11AM +0200, Christoph Hellwig wrote:
>> There is no reason to leave the per-device dma_ops around when
>> deconfiguring a device, so move this code from arm64 into the
>> common code.
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>
> This patch causes various ppc images to crash in -next due to NULL
> DMA ops in dma_alloc_attrs().

I finally remembered where you autobuilder is :)

https://kerneltests.org/builders/qemu-ppc-next/builds/970/steps/qemubuildcommand_1/logs/stdio

Looks like mac99 at least is failing.

Just reproduced it on my setup.

> Looking into the code, the macio driver tries to instantiate on
> the 0000:f0:0d.0 PCI address (the driver maps to all Apple PCI IDs)
> and fails. This results in a call to arch_teardown_dma_ops(). Later,
> the same device pointer is used to instantiate ohci-pci, which
> crashes in probe because the dma_ops pointer has been cleared.
>
> I don't claim to fully understand the code, but to me it looks like
> the pci device is allocated and waiting for a driver to attach to.
> See arch/powerpc/kernel/pci_of_scan.c:of_create_pci_dev().
> If attaching a driver (here: macio) fails, the same device pointer
> is then reused for the next matching driver until a match is found
> and the device is successfully instantiated. Of course this fails
> quite badly if the device pointer has been scrubbed after the first
> failure.
>
> I don't know if this is generic PCI behavior or ppc specific.
> I am copying pci and ppc maintainers for additional input.

I would guess this is some PPC special sauce  O_o

Will have a look.

cheers

Re: [PATCH 4/4] dma-mapping: clear dev->dma_ops in arch_teardown_dma_ops

[Christoph Hellwig]

Looking at the code I think this commit is simply broken for
architectures not using arch_setup_dma_ops, but instead setting up
the dma ops through arch specific magic.

I'll revert the patch.

Re: [PATCH 4/4] dma-mapping: clear dev->dma_ops in arch_teardown_dma_ops

[Michael Ellerman]

Christoph Hellwig <hch@lst.de> writes:

> Looking at the code I think this commit is simply broken for
> architectures not using arch_setup_dma_ops, but instead setting up
> the dma ops through arch specific magic.

I arch_setup_dma_ops() called from of_dma_configure(), but
pci_dma_configure() doesn't call that for this device:

static int pci_dma_configure(struct device *dev)
{
	...
	if (IS_ENABLED(CONFIG_OF) && bridge->parent &&
	    bridge->parent->of_node) {
		ret = of_dma_configure(dev, bridge->parent->of_node, true);

bridge->parent is NULL.

So I don't think arch_setup_dma_ops() would help here?

> I'll revert the patch.

Thanks.

cheers

Re: [PATCH 4/4] dma-mapping: clear dev->dma_ops in arch_teardown_dma_ops

[Robin Murphy]

On 25/09/18 21:16, Christoph Hellwig wrote:
> Looking at the code I think this commit is simply broken for
> architectures not using arch_setup_dma_ops, but instead setting up
> the dma ops through arch specific magic.
> 
> I'll revert the patch.

Ugh, sorry about missing that too. Ack to a revert - thinking about 
those PPC symptoms, it might actually be that other architectures could 
also get into the same pickle just by unbinding and rebinding a driver 
(e.g. switching to VFIO then back again).

Robin.