[RFC PATCH] ARM64: PCI: inherit root controller's dma-coherent

[Ming Lei]

On Sat, Dec 6, 2014 at 12:05 AM, Arnd Bergmann <arnd at arndb.de> wrote:
> On Friday 05 December 2014 15:26:27 Grygorii Strashko wrote:
>> ---
>> From 5e078b1ba148aa280181c4f695e567875a0f4ae9 Mon Sep 17 00:00:00 2001
>> From: Grygorii Strashko <grygorii.strashko at ti.com>
>> Date: Fri, 28 Feb 2014 19:11:39 +0200
>> Subject: [RFC PATCH] common: dma-mapping: introduce dma_init_dev_from_parent() helper
>>
>> Now, in Kernel, Parent device's DMA configuration has to by applied
>> to the child as is, to enable DMA support for this device. Usually, this
>> is happened in places where child device is manually instantiated by
>> Parent device device using Platform APIs
>> (see drivers/usb/dwc3/host.c or drivers/pci/probe.c:pci_device_add()
>>  for example).
>>
>> The DMA configuration is represented in Device data structure was
>> extended recently (dma_pfn_offset was added) and going to extended
>> in future to support IOMMU. Therefore, the code used by drivers to
>> copy DMA configuration from parent to child device isn't working
>> properly, for example:
>> (drivers/usb/dwc3/host.c)
>>       dma_set_coherent_mask(&xhci->dev, dwc->dev->coherent_dma_mask);
>>
>>       xhci->dev.parent        = dwc->dev;
>>       xhci->dev.dma_mask      = dwc->dev->dma_mask;
>>       xhci->dev.dma_parms     = dwc->dev->dma_parms;
>> above code will miss to copy dma_pfn_offset.
>
>
> I'm not too thrilled about this implementation either. It immediately
> breaks as soon as we have IOMMU support, as the IOMMU configuration
> is by definition device dependent, and you have introduced a few bugs
> by copying the pointers to dma_mask and dma_parms.

Using devm_kmalloc() for allocating dma_mask might be helpful for
the issue, but still need to support the default setting to be overrided
by IOMMU code, which is related with the timing of calling
dma_init_dev_from_parent().

>
>> Hence, intoroduce common dma_init_dev_from_parent() helper to
>> initialize device's DMA parameters using from parent device's configuration,
>> use __weak to allow arches to overwrite it if needed.
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko at ti.com>
>> Signed-off-by: Murali Karicheri <m-karicheri2 at ti.com>
>> ---
>>  drivers/base/dma-mapping.c  | 25 +++++++++++++++++++++++++
>>  include/linux/dma-mapping.h |  3 +++
>>  2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
>> index 9e8bbdd..4556698 100644
>> --- a/drivers/base/dma-mapping.c
>> +++ b/drivers/base/dma-mapping.c
>> @@ -270,6 +270,31 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
>>  }
>>  EXPORT_SYMBOL(dma_common_mmap);
>>
>> +/*
>> + * Initialize device's DMA parameters using parent device
>> + * @dev: Device to be configured
>> + * @parent: Pointer on parent device to get DMA configuration from
>> + *
>> + * parent can be NULL - then DMA configuration will be teken from dev->parent.
>> + */
>> +void __weak dma_init_dev_from_parent(struct device *dev, struct device *parent)
>> +{
>
> Please let's try to avoid weak functions

Yes, __weak isn't friendly at all.

>
>> +     struct device *parent_dev = parent;
>
> One of these two is not needed.
>
>> +     /* if parent is NULL assume, dev->parent is the one to use */
>> +     if (!parent_dev)
>> +             parent_dev = dev->parent;
>> +
>> +     if (is_device_dma_capable(parent_dev)) {
>> +             dev->dma_mask   = parent_dev->dma_mask;
>
> This is always wrong: dev->dma_mask is a pointer!
>
>> +             dev->coherent_dma_mask = parent_dev->coherent_dma_mask;
>
> What if someone has already set a mask that is greater than 32-bit?

Yes, that is a bit complicated because there may be default
dma_mask which was setup before creating the device.

>
>> +             dev->dma_parms  = parent_dev->dma_parms;
>
> same  problem here.
>
>> +             dev->dma_pfn_offset = parent_dev->dma_pfn_offset;
>> +             set_dma_ops(dev, get_dma_ops(parent_dev));
>> +     }
>> +}
>> +EXPORT_SYMBOL(dma_init_dev_from_parent);

Arnd, given it isn't easy to figure out one good solution for moving
dma configuration into generic code(driver core, pci, .) now, could
the issue be fixed first?

Thanks,