* [PATCH] arm64: remove dummy_dma_ops
@ 2018-08-02 12:13 Christoph Hellwig
2018-08-02 12:32 ` Robin Murphy
0 siblings, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2018-08-02 12:13 UTC (permalink / raw)
To: linux-arm-kernel
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@lst.de>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH] arm64: remove dummy_dma_ops
2018-08-02 12:13 [PATCH] arm64: remove dummy_dma_ops Christoph Hellwig
@ 2018-08-02 12:32 ` Robin Murphy
2018-08-02 12:43 ` Christoph Hellwig
0 siblings, 1 reply; 4+ messages in thread
From: Robin Murphy @ 2018-08-02 12:32 UTC (permalink / raw)
To: linux-arm-kernel
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@lst.de>
> ---
> 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 ||
>
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH] arm64: remove dummy_dma_ops
2018-08-02 12:32 ` Robin Murphy
@ 2018-08-02 12:43 ` Christoph Hellwig
2018-08-03 15:05 ` Robin Murphy
0 siblings, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2018-08-02 12:43 UTC (permalink / raw)
To: linux-arm-kernel
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;
...
}
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH] arm64: remove dummy_dma_ops
2018-08-02 12:43 ` Christoph Hellwig
@ 2018-08-03 15:05 ` Robin Murphy
0 siblings, 0 replies; 4+ messages in thread
From: Robin Murphy @ 2018-08-03 15:05 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-08-03 15:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-08-02 12:13 [PATCH] arm64: remove dummy_dma_ops Christoph Hellwig
2018-08-02 12:32 ` Robin Murphy
2018-08-02 12:43 ` Christoph Hellwig
2018-08-03 15:05 ` Robin Murphy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).