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

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

[Ming Lei]

This patch sets pci device's dma ops as coherent if the root
controller is dma-coherent.

This patch fixes the bug in below link:

	https://bugs.launchpad.net/bugs/1386490

Cc: Catalin Marinas <Catalin.Marinas@arm.com>
Cc: Jon Masters <jcm@redhat.com>
Cc: Dann Frazier <dann.frazier@canonical.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 arch/arm64/kernel/pci.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index ce5836c..4b6444a 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -16,6 +16,7 @@
 #include <linux/mm.h>
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
+#include <linux/of_address.h>
 #include <linux/slab.h>
 
 #include <asm/pci-bridge.h>
@@ -37,6 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 	return res->start;
 }
 
+/* Inherit root controller's dma coherent ops */
+static void pci_dma_config(struct pci_dev *dev)
+{
+	struct pci_bus *bus = dev->bus;
+	struct device *host;
+
+	while (!pci_is_root_bus(bus)) {
+		bus = bus->parent;
+	}
+
+	host = bus->dev.parent->parent;
+	if (of_dma_is_coherent(host->of_node))
+		set_arch_dma_coherent_ops(&dev->dev);
+}
+
 /*
  * Try to assign the IRQ number from DT when adding a new device
  */
@@ -44,6 +60,7 @@ int pcibios_add_device(struct pci_dev *dev)
 {
 	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
 
+	pci_dma_config(dev);
 	return 0;
 }
 
-- 
1.7.9.5

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

[Jon Masters]

Ming,

Thanks for reviving this patch. We carry a similar one internally for Storm currently to handle the Mellanox part that you are trying to use with Trusty as well. There's also an ACPI variant based upon the _CCA provided for the root complex but the eventual upstream solution needs to be a bit more nuanced on AArch64 since PCI devices can imply properties of coherency in e.g. snoopable capability. So while this is an ok default/fallback/initial patch, PCI on AArch64 will need a bit more than this. We're looking at this at the moment (in addition to having the ACPI specification clarified to indicate that there is no safe default assumption, requiring that DMA masters always indicate their coherency or otherwise via a _CCA whether true or false).

Jon.

-- 
Computer Architect | Sent from my 64-bit #ARM Powered Tablet

> On Nov 27, 2014, at 12:41 AM, Ming Lei <ming.lei@canonical.com> wrote:
> 
> This patch sets pci device's dma ops as coherent if the root
> controller is dma-coherent.
> 
> This patch fixes the bug in below link:
> 
>    https://bugs.launchpad.net/bugs/1386490
> 
> Cc: Catalin Marinas <Catalin.Marinas@arm.com>
> Cc: Jon Masters <jcm@redhat.com>
> Cc: Dann Frazier <dann.frazier@canonical.com>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
> arch/arm64/kernel/pci.c |   17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index ce5836c..4b6444a 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -16,6 +16,7 @@
> #include <linux/mm.h>
> #include <linux/of_pci.h>
> #include <linux/of_platform.h>
> +#include <linux/of_address.h>
> #include <linux/slab.h>
> 
> #include <asm/pci-bridge.h>
> @@ -37,6 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>    return res->start;
> }
> 
> +/* Inherit root controller's dma coherent ops */
> +static void pci_dma_config(struct pci_dev *dev)
> +{
> +    struct pci_bus *bus = dev->bus;
> +    struct device *host;
> +
> +    while (!pci_is_root_bus(bus)) {
> +        bus = bus->parent;
> +    }
> +
> +    host = bus->dev.parent->parent;
> +    if (of_dma_is_coherent(host->of_node))
> +        set_arch_dma_coherent_ops(&dev->dev);
> +}
> +
> /*
>  * Try to assign the IRQ number from DT when adding a new device
>  */
> @@ -44,6 +60,7 @@ int pcibios_add_device(struct pci_dev *dev)
> {
>    dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> 
> +    pci_dma_config(dev);
>    return 0;
> }
> 
> -- 
> 1.7.9.5
> 

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

[Arnd Bergmann]

On Thursday 27 November 2014 02:39:59 Jon Masters wrote:
> We're looking at this at the moment (in addition to having the ACPI specification
> clarified to indicate that there is no safe default assumption, requiring that
> DMA masters always indicate their coherency or otherwise via a _CCA whether true
> or false). 

I think for arm64, this should be easy: since ACPI is only for servers, we can
rely on the fact that the devices have cache-coherent DMA all the time. Let's
not over-complicate the ACPI case with all the special hacks we need for embedded.

	Arnd

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

[Catalin Marinas]

On Thu, Nov 27, 2014 at 09:05:57AM +0000, Arnd Bergmann wrote:
> On Thursday 27 November 2014 02:39:59 Jon Masters wrote:
> > We're looking at this at the moment (in addition to having the ACPI specification
> > clarified to indicate that there is no safe default assumption, requiring that
> > DMA masters always indicate their coherency or otherwise via a _CCA whether true
> > or false). 
> 
> I think for arm64, this should be easy: since ACPI is only for servers, we can
> rely on the fact that the devices have cache-coherent DMA all the time. Let's
> not over-complicate the ACPI case with all the special hacks we need for embedded.

As usual, we need hw vendors to say what they expect/build here.

I think in the past (non-ARM) the assumption was that the DMA is
coherent on ACPI-capable systems unless otherwise stated but I've seen
discussions (as Jon mentioned above) that ACPI should always indicate
the coherency on ARM without any assumption.

Either way, I don't think it's a problem for the kernel. We just need to
change the default DMA ops to coherent when booting with ACPI (using
non-coherent ops for a coherent device is not safe as the CPU can
corrupt cache lines written by the device).

-- 
Catalin

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

[Arnd Bergmann]

On Thursday 27 November 2014 11:36:36 Catalin Marinas wrote:
> On Thu, Nov 27, 2014 at 09:05:57AM +0000, Arnd Bergmann wrote:
> > On Thursday 27 November 2014 02:39:59 Jon Masters wrote:
> > > We're looking at this at the moment (in addition to having the ACPI specification
> > > clarified to indicate that there is no safe default assumption, requiring that
> > > DMA masters always indicate their coherency or otherwise via a _CCA whether true
> > > or false). 
> > 
> > I think for arm64, this should be easy: since ACPI is only for servers, we can
> > rely on the fact that the devices have cache-coherent DMA all the time. Let's
> > not over-complicate the ACPI case with all the special hacks we need for embedded.
> 
> As usual, we need hw vendors to say what they expect/build here.
> 
> I think in the past (non-ARM) the assumption was that the DMA is
> coherent on ACPI-capable systems unless otherwise stated but I've seen
> discussions (as Jon mentioned above) that ACPI should always indicate
> the coherency on ARM without any assumption.

Yes, that is fine, but I think Linux should just BUG_ON() any device
on ACPI that doesn't have this flag set. You definitely want to have
the flags work both ways for Windows Phone, but we don't need to support
that hardware with Linux.

The SBSA is quite specific about requiring coherent DMA to work,
so this is a safe assumption to make. We can support all other systems
with DT without problems.

> Either way, I don't think it's a problem for the kernel. We just need to
> change the default DMA ops to coherent when booting with ACPI (using
> non-coherent ops for a coherent device is not safe as the CPU can
> corrupt cache lines written by the device).

We should probably default to the coherent IOMMU dma ops with ACPI
eventually, I'd assume that these machines also come with an IOMMU,
though if we ever want to support virtual machines with ACPI as well,
that probably wants the linear operations, and until the SMMU support
for PCI is done, we have to start out using the linear operations
as well.

	Arnd

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

[Arnd Bergmann]

On Thursday 27 November 2014 13:41:31 Ming Lei wrote:

> @@ -37,6 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  	return res->start;
>  }
>  
> +/* Inherit root controller's dma coherent ops */
> +static void pci_dma_config(struct pci_dev *dev)
> +{
> +	struct pci_bus *bus = dev->bus;
> +	struct device *host;
> +
> +	while (!pci_is_root_bus(bus)) {
> +		bus = bus->parent;
> +	}
> +
> +	host = bus->dev.parent->parent;
> +	if (of_dma_is_coherent(host->of_node))
> +		set_arch_dma_coherent_ops(&dev->dev);
> +}
> +

I think we need something more generic than this: This is not architecture
specific at all, and we have to deal with IOMMU, swiotlb, dma offset and
dma mask as well, coherency is definitely not the only issue.

We have the of_dma_configure() function that does some of this for platform
devices and that have to extend to make it work with IOMMUs, and the
common pci_device_add() function does some other subset in architecture
independent code for PCI, so I think that's where it should go.
We will need an architecture-specific helper to set the dma_map_ops
pointer, dma_parms, dma offset, and the iommu, but that is not PCI
specific, it just gets called from both the platform device and pci
device code.

Regarding the pcibios_add_device() function, I'd rather see the existing
code moved into the PCI core and the function removed, since it also
is not architecture specific. We should be able to delete the entire
arm64/pci.c file eventually.

	Arnd

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

[Ming Lei]

On Thu, Nov 27, 2014 at 5:03 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> Regarding the pcibios_add_device() function, I'd rather see the existing
> code moved into the PCI core and the function removed, since it also
> is not architecture specific. We should be able to delete the entire
> arm64/pci.c file eventually.

I totally agree.

Looks pcibios_*() are misused by ARCHs, which should have been
designed for addressing ARCH pcibios things, now these functions
are just a hook for ARCH code.

Specifically pci host controller driver can't be arch independent
at all because of ARCH's pcibios_*() implementation and
the specific data type defined by ARCH code.

Thanks,
Ming Lei

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

[Ming Lei]

On Thu, Nov 27, 2014 at 5:03 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> Regarding the pcibios_add_device() function, I'd rather see the existing
> code moved into the PCI core and the function removed, since it also
> is not architecture specific. We should be able to delete the entire
> arm64/pci.c file eventually.

I totally agree.

Looks pcibios_*() are misused by ARCHs, which should have been
designed for addressing ARCH pcibios things, now these functions
are just a hook for ARCH code.

Specifically pci host controller driver can't be arch independent
at all because of ARCH's pcibios_*() implementation and
the specific data type defined by ARCH code.

Thanks,
Ming Lei

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

[Will Deacon]

On Thu, Nov 27, 2014 at 09:03:38AM +0000, Arnd Bergmann wrote:
> On Thursday 27 November 2014 13:41:31 Ming Lei wrote:
> 
> > @@ -37,6 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
> >  	return res->start;
> >  }
> >  
> > +/* Inherit root controller's dma coherent ops */
> > +static void pci_dma_config(struct pci_dev *dev)
> > +{
> > +	struct pci_bus *bus = dev->bus;
> > +	struct device *host;
> > +
> > +	while (!pci_is_root_bus(bus)) {
> > +		bus = bus->parent;
> > +	}
> > +
> > +	host = bus->dev.parent->parent;
> > +	if (of_dma_is_coherent(host->of_node))
> > +		set_arch_dma_coherent_ops(&dev->dev);
> > +}
> > +
> 
> I think we need something more generic than this: This is not architecture
> specific at all, and we have to deal with IOMMU, swiotlb, dma offset and
> dma mask as well, coherency is definitely not the only issue.

Yeah, and we need to extend the IOMMU binding to express RequesterID ->
StreamID (master ID) mappings at the host controller, too.

Will

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

[Ming Lei]

On Thu, Nov 27, 2014 at 5:03 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 27 November 2014 13:41:31 Ming Lei wrote:
>
>> @@ -37,6 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>>       return res->start;
>>  }
>>
>> +/* Inherit root controller's dma coherent ops */
>> +static void pci_dma_config(struct pci_dev *dev)
>> +{
>> +     struct pci_bus *bus = dev->bus;
>> +     struct device *host;
>> +
>> +     while (!pci_is_root_bus(bus)) {
>> +             bus = bus->parent;
>> +     }
>> +
>> +     host = bus->dev.parent->parent;
>> +     if (of_dma_is_coherent(host->of_node))
>> +             set_arch_dma_coherent_ops(&dev->dev);
>> +}
>> +
>
> I think we need something more generic than this: This is not architecture
> specific at all, and we have to deal with IOMMU, swiotlb, dma offset and
> dma mask as well, coherency is definitely not the only issue.

That may take a bit long since ARCHs, dma, and pci subsystem are
involved about the change.

Given ARM64 PCI and related host controller driver are merged
already, could this patch be applied to fix current problem first?


Thanks,
Ming Lei

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

[Grygorii Strashko]

Hi Ming Lei,

On 12/04/2014 05:40 AM, Ming Lei wrote:
> On Thu, Nov 27, 2014 at 5:03 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thursday 27 November 2014 13:41:31 Ming Lei wrote:
>>
>>> @@ -37,6 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>>>        return res->start;
>>>   }
>>>
>>> +/* Inherit root controller's dma coherent ops */
>>> +static void pci_dma_config(struct pci_dev *dev)
>>> +{
>>> +     struct pci_bus *bus = dev->bus;
>>> +     struct device *host;
>>> +
>>> +     while (!pci_is_root_bus(bus)) {
>>> +             bus = bus->parent;
>>> +     }
>>> +
>>> +     host = bus->dev.parent->parent;
>>> +     if (of_dma_is_coherent(host->of_node))
>>> +             set_arch_dma_coherent_ops(&dev->dev);
>>> +}
>>> +
>>
>> I think we need something more generic than this: This is not architecture
>> specific at all, and we have to deal with IOMMU, swiotlb, dma offset and
>> dma mask as well, coherency is definitely not the only issue.
> 
> That may take a bit long since ARCHs, dma, and pci subsystem are
> involved about the change.
> 
> Given ARM64 PCI and related host controller driver are merged
> already, could this patch be applied to fix current problem first?

May be, the attached patch would be helpful for solving such sort of issues
(found it in my Warehouse 13:)

regards,
-grygorii

---
>From 5e078b1ba148aa280181c4f695e567875a0f4ae9 Mon Sep 17 00:00:00 2001
From: Grygorii Strashko <grygorii.strashko@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.

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@ti.com>
Signed-off-by: Murali Karicheri <m-karicheri2@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)
+{
+	struct device *parent_dev = parent;
+
+	/* 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;
+		dev->coherent_dma_mask = parent_dev->coherent_dma_mask;
+		dev->dma_parms	= parent_dev->dma_parms;
+		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);
+
 #ifdef CONFIG_MMU
 /*
  * remaps an array of PAGE_SIZE pages into another vm_area
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index d5d3881..7fbed05 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -127,6 +127,9 @@ static inline int dma_coerce_mask_and_coherent(struct device *dev, u64 mask)
 	return dma_set_mask_and_coherent(dev, mask);
 }
 
+extern void __weak dma_init_dev_from_parent(struct device *dev,
+	struct device *parent);
+
 extern u64 dma_get_required_mask(struct device *dev);
 
 #ifndef set_arch_dma_coherent_ops
-- 
1.9.1

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

[Arnd Bergmann]

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@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.

> 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@ti.com>
> Signed-off-by: Murali Karicheri <m-karicheri2@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

> +	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?

> +		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

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

[Ming Lei]

On Sat, Dec 6, 2014 at 12:05 AM, Arnd Bergmann <arnd@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@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@ti.com>
>> Signed-off-by: Murali Karicheri <m-karicheri2@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,

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

[Arnd Bergmann]

On Tuesday 09 December 2014 10:53:26 Ming Lei wrote:
> On Sat, Dec 6, 2014 at 12:05 AM, Arnd Bergmann <arnd@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@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()> 

struct pci_dev actually contains a place to store both dma_mask and
dma_parms, so no need for that.

> >> +     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.

PCI devices should generally start out with a 32-bit mask, which can
be modified by the driver as needed. The only case you need to be
worried about is when the PCI host controller has a mask that is
smaller than 32-bit (e.g. on some shmobile rcar implementations),
and in that case you need to set the same mask there. We also need the
same change for platform devices, which start out with a 32-bit mask
at the moment, even when the parent bus has a smaller address width.

> >
> >> +             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?

We have just merged a new arch_setup_dma_ops callback based on patches
from Will Deacon, and this is required to get iommu drivers to work
properly. The callback is used correctly by of_dma_configure, and we
just need to hook it up on arm64 (I think Robin already has a patch,
if Will doesn't) and call it from the PCI core. There really isn't
any magic involved here.

	Arnd

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

[Robin Murphy]

Hi Arnd,

On 10/12/14 16:06, Arnd Bergmann wrote:
[...]
>> 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?
>
> We have just merged a new arch_setup_dma_ops callback based on patches
> from Will Deacon, and this is required to get iommu drivers to work
> properly. The callback is used correctly by of_dma_configure, and we
> just need to hook it up on arm64 (I think Robin already has a patch,
> if Will doesn't) and call it from the PCI core. There really isn't
> any magic involved here.
>

Indeed, I have that as part of my arm64 IOMMU stuff, albeit with an 
additional dependency on the new Xen is_device_dma_coherent. Once the 
dust has settled on -rc1 I'll see what's in good enough shape to send out.

Robin.