| Message ID | 1430838729-21572-3-git-send-email-Suravee.Suthikulpanit@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 05 May 2015 10:12:06 Suravee Suthikulpanit wrote: > +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_for_cpu, > + .sync_single_for_device = __dummy_sync_single_for_device, > + .sync_sg_for_cpu = __dummy_sync_sg_for_cpu, > + .sync_sg_for_device = __dummy_sync_sg_for_device, > + .mapping_error = __dummy_mapping_error, > + .dma_supported = __dummy_dma_supported, > +}; > +EXPORT_SYMBOL(dummy_dma_ops); > + > This will clearly work, but I think it's easier to just leave the dma_mask pointer as NULL when creating the platform device, which should let the normal dma ops fail all the callbacks. Arnd
On 5/5/2015 10:44 AM, Arnd Bergmann wrote: > On Tuesday 05 May 2015 10:12:06 Suravee Suthikulpanit wrote: >> +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_for_cpu, >> + .sync_single_for_device = __dummy_sync_single_for_device, >> + .sync_sg_for_cpu = __dummy_sync_sg_for_cpu, >> + .sync_sg_for_device = __dummy_sync_sg_for_device, >> + .mapping_error = __dummy_mapping_error, >> + .dma_supported = __dummy_dma_supported, >> +}; >> +EXPORT_SYMBOL(dummy_dma_ops); >> + >> > > This will clearly work, but I think it's easier to just leave > the dma_mask pointer as NULL when creating the platform device, > which should let the normal dma ops fail all the callbacks. > > Arnd > However, codes in several places are making use of dma_map_ops without checking if the ops are NULL (i.e. include/asm-generic/dma-mapping-common.h and in arch-specific implementation). If setting it to NULL is what we are planning to support, we would need to scrub the current code to put NULL check. Also, would you consider if that is safe to do going forward? Thanks, Suravee
On Tuesday 05 May 2015 11:09:38 Suravee Suthikulanit wrote: > > However, codes in several places are making use of dma_map_ops without > checking if the ops are NULL (i.e. > include/asm-generic/dma-mapping-common.h and in arch-specific > implementation). If setting it to NULL is what we are planning to > support, we would need to scrub the current code to put NULL check. > Also, would you consider if that is safe to do going forward? > > I mean the dma_mask pointer, not dma_map_ops. Arnd
On 5/5/2015 11:12 AM, Arnd Bergmann wrote: > On Tuesday 05 May 2015 11:09:38 Suravee Suthikulanit wrote: >> >> However, codes in several places are making use of dma_map_ops without >> checking if the ops are NULL (i.e. >> include/asm-generic/dma-mapping-common.h and in arch-specific >> implementation). If setting it to NULL is what we are planning to >> support, we would need to scrub the current code to put NULL check. >> Also, would you consider if that is safe to do going forward? >> >> > > I mean the dma_mask pointer, not dma_map_ops. > > Arnd > Ah, got it. Sorry for confusion. Suravee
On 05/05/2015 11:13 AM, Suravee Suthikulanit wrote: > On 5/5/2015 11:12 AM, Arnd Bergmann wrote: >> On Tuesday 05 May 2015 11:09:38 Suravee Suthikulanit wrote: >>> >>> However, codes in several places are making use of dma_map_ops without >>> checking if the ops are NULL (i.e. >>> include/asm-generic/dma-mapping-common.h and in arch-specific >>> implementation). If setting it to NULL is what we are planning to >>> support, we would need to scrub the current code to put NULL check. >>> Also, would you consider if that is safe to do going forward? >>> >>> >> >> I mean the dma_mask pointer, not dma_map_ops. Except a lot of drivers will actually set the dma_mask pointer during probe (usually by setting dev->dma_mask = &dev->coherent_dma_mask or by calling dma_coerce_mask_and_coherent). So I think the dummy_dma_ops might be the safest way to go. Thanks, Tom >> >> Arnd >> > > Ah, got it. Sorry for confusion. > > Suravee > > _______________________________________________ > Linaro-acpi mailing list > Linaro-acpi@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/linaro-acpi
On Tuesday 05 May 2015 11:24:03 Tom Lendacky wrote: > On 05/05/2015 11:13 AM, Suravee Suthikulanit wrote: > > On 5/5/2015 11:12 AM, Arnd Bergmann wrote: > >> On Tuesday 05 May 2015 11:09:38 Suravee Suthikulanit wrote: > >>> > >>> However, codes in several places are making use of dma_map_ops without > >>> checking if the ops are NULL (i.e. > >>> include/asm-generic/dma-mapping-common.h and in arch-specific > >>> implementation). If setting it to NULL is what we are planning to > >>> support, we would need to scrub the current code to put NULL check. > >>> Also, would you consider if that is safe to do going forward? > >>> > >>> > >> > >> I mean the dma_mask pointer, not dma_map_ops. > > Except a lot of drivers will actually set the dma_mask pointer during > probe (usually by setting dev->dma_mask = &dev->coherent_dma_mask or by > calling dma_coerce_mask_and_coherent). So I think the dummy_dma_ops > might be the safest way to go. Those drivers are broken already, I don't think we should worry about them. Let's just fix them properly when we run into problems with them. Basically any use of dma_coerce_mask_and_coherent() is an annotation for a bug where a driver used to set dev->dma_mask = &dev->coherent_dma_mask manually. Arnd
Hi Suravee, On 05/05/15 16:12, Suravee Suthikulpanit wrote: > From http://www.uefi.org/sites/default/files/resources/ACPI_6.0.pdf, > section 6.2.17 _CCA states that ARM platforms require ACPI _CCA > object to be specified for DMA-cabpable devices. This patch introduces > ACPI_MUST_HAVE_CCA in arm64 Kconfig to specify such requirement. > > In this case of missing _CCA, arm64 would assign dummy_dma_ops > to disable DMA capability of the device. > > Signed-off-by: Mark Salter <msalter@redhat.com> > Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > --- [...] > +static void __dummy_sync_single_for_cpu(struct device *dev, > + dma_addr_t dev_addr, size_t size, > + enum dma_data_direction dir) > +{ > +} > + > +static void __dummy_sync_single_for_device(struct device *dev, > + dma_addr_t dev_addr, size_t size, > + enum dma_data_direction dir) > +{ > +} Minor point, but I don't see the need to have multiple dummy functions with identical signatures - just have a generic dummy_sync_single and assign it to both ops. > +static void __dummy_sync_sg_for_cpu(struct device *dev, > + struct scatterlist *sgl, int nelems, > + enum dma_data_direction dir) > +{ > +} > + > +static void __dummy_sync_sg_for_device(struct device *dev, > + struct scatterlist *sgl, int nelems, > + enum dma_data_direction dir) > +{ > +} Ditto here with dummy_sync_sg. I wonder if there's any argument for putting the dummy DMA ops somewhere common, like drivers/base/dma-mapping.c? Robin.
On 5/6/2015 5:08 AM, Robin Murphy wrote: > [...] >> +static void __dummy_sync_single_for_cpu(struct device *dev, >> + dma_addr_t dev_addr, size_t size, >> + enum dma_data_direction dir) >> +{ >> +} >> + >> +static void __dummy_sync_single_for_device(struct device *dev, >> + dma_addr_t dev_addr, size_t size, >> + enum dma_data_direction dir) >> +{ >> +} > > Minor point, but I don't see the need to have multiple dummy functions > with identical signatures - just have a generic dummy_sync_single and > assign it to both ops. > >> +static void __dummy_sync_sg_for_cpu(struct device *dev, >> + struct scatterlist *sgl, int nelems, >> + enum dma_data_direction dir) >> +{ >> +} >> + >> +static void __dummy_sync_sg_for_device(struct device *dev, >> + struct scatterlist *sgl, int nelems, >> + enum dma_data_direction dir) >> +{ >> +} > > Ditto here with dummy_sync_sg. Hi Robin, Good point. I'll take care of that in V3. > > I wonder if there's any argument for putting the dummy DMA ops somewhere > common, like drivers/base/dma-mapping.c? > > Robin. Hm.. If this approach will be adopted by other architectures, then it would make sense. Currently, this is only used by arm64. So, I think it is okay to leave this here for now. Thanks, Suravee
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 4269dba..4f4bbaaf 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -1,7 +1,9 @@ config ARM64 def_bool y select ACPI_GENERIC_GSI if ACPI + select ACPI_MUST_HAVE_CCA if ACPI select ACPI_REDUCED_HARDWARE_ONLY if ACPI + select ACPI_SUPPORT_CCA_ZERO if ACPI select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE select ARCH_HAS_ELF_RANDOMIZE select ARCH_HAS_GCOV_PROFILE_ALL diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h index 9437e3d..f0d6d0b 100644 --- a/arch/arm64/include/asm/dma-mapping.h +++ b/arch/arm64/include/asm/dma-mapping.h @@ -18,6 +18,7 @@ #ifdef __KERNEL__ +#include <linux/acpi.h> #include <linux/types.h> #include <linux/vmalloc.h> @@ -28,13 +29,23 @@ #define DMA_ERROR_CODE (~(dma_addr_t)0) extern struct dma_map_ops *dma_ops; +extern struct dma_map_ops dummy_dma_ops; static inline struct dma_map_ops *__generic_dma_ops(struct device *dev) { - if (unlikely(!dev) || !dev->archdata.dma_ops) + if (unlikely(!dev)) return dma_ops; - else + else if (dev->archdata.dma_ops) return dev->archdata.dma_ops; + else if (acpi_disabled) + return dma_ops; + + /* + * When ACPI is enabled, if arch_set_dma_ops is not called, + * we will disable device DMA capability by setting it + * to dummy_dma_ops. + */ + return &dummy_dma_ops; } static inline struct dma_map_ops *get_dma_ops(struct device *dev) @@ -48,6 +59,9 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev) static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, struct iommu_ops *iommu, bool coherent) { + if (!acpi_disabled && !dev->archdata.dma_ops) + dev->archdata.dma_ops = dma_ops; + dev->archdata.dma_coherent = coherent; } #define arch_setup_dma_ops arch_setup_dma_ops diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c index ef7d112..31d2b52 100644 --- a/arch/arm64/mm/dma-mapping.c +++ b/arch/arm64/mm/dma-mapping.c @@ -415,6 +415,110 @@ out: 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, + struct dma_attrs *attrs) +{ + return NULL; +} + +static void __dummy_free(struct device *dev, size_t size, + void *vaddr, dma_addr_t dma_handle, + struct dma_attrs *attrs) +{ +} + +static int __dummy_mmap(struct device *dev, + struct vm_area_struct *vma, + void *cpu_addr, dma_addr_t dma_addr, size_t size, + struct dma_attrs *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, + struct dma_attrs *attrs) +{ + return DMA_ERROR_CODE; +} + +static void __dummy_unmap_page(struct device *dev, dma_addr_t dev_addr, + size_t size, enum dma_data_direction dir, + struct dma_attrs *attrs) +{ +} + +static int __dummy_map_sg(struct device *dev, struct scatterlist *sgl, + int nelems, enum dma_data_direction dir, + struct dma_attrs *attrs) +{ + return 0; +} + +static void __dummy_unmap_sg(struct device *dev, + struct scatterlist *sgl, int nelems, + enum dma_data_direction dir, + struct dma_attrs *attrs) +{ +} + +static void __dummy_sync_single_for_cpu(struct device *dev, + dma_addr_t dev_addr, size_t size, + enum dma_data_direction dir) +{ +} + +static void __dummy_sync_single_for_device(struct device *dev, + dma_addr_t dev_addr, size_t size, + enum dma_data_direction dir) +{ +} + +static void __dummy_sync_sg_for_cpu(struct device *dev, + struct scatterlist *sgl, int nelems, + enum dma_data_direction dir) +{ +} + +static void __dummy_sync_sg_for_device(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; +} + +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_for_cpu, + .sync_single_for_device = __dummy_sync_single_for_device, + .sync_sg_for_cpu = __dummy_sync_sg_for_cpu, + .sync_sg_for_device = __dummy_sync_sg_for_device, + .mapping_error = __dummy_mapping_error, + .dma_supported = __dummy_dma_supported, +}; +EXPORT_SYMBOL(dummy_dma_ops); + static int __init arm64_dma_init(void) { int ret;