ARM64: PCI: inherit root controller's dma-coherent

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 <[email protected]>

Cc Jon Masters <[email protected]>

Cc Dann Frazier <[email protected]>

Signed-off-by Ming Lei <[email protected]>

---
arch/arm64/kernel/pci.c | 17 +++++++++++++++++

1 file changed, 17 insertions(+)

arch/arm64/kernel/pci.c +17 -0

index ce5836c..4b6444a 100644
@@ -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;
 }
 

Signature · 1 line

1.7.9.5

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

Quoted 4 lines · 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.

Quoted 9 lines · Arnd Bergmann

Quoted 4 lines · 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).

arch/arm64/kernel/pci.c +5 -0

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

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.

Quoted 15 lines · On Thursday 27 November 2014 13:41:31 Ming Lei wrote:

arch/arm64/kernel/pci.c +11 -0

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

arch/arm64/kernel/pci.c resource_size_t pcibios_align_resource(void *data, const struct resource *res, · line 55

Quoted 4 lines

arch/arm64/kernel/pci.c +4 -0

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

Quoted 15 lines · On Thursday 27 November 2014 13:41:31 Ming Lei wrote:

arch/arm64/kernel/pci.c +11 -0

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

arch/arm64/kernel/pci.c resource_size_t pcibios_align_resource(void *data, const struct resource *res, · line 55

Quoted 4 lines

arch/arm64/kernel/pci.c +4 -0

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

Quoted 17 lines · Arnd Bergmann

Quoted 15 lines · On Thursday 27 November 2014 13:41:31 Ming Lei wrote:

(quoted diff) +11 -0

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

Quoted 8 lines

Review resource_size_t pcibios_align_resource(void *data, const struct resource *res, · line 55

Quoted 4 lines

(quoted diff) +4 -0

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

From 5e078b1ba148aa280181c4f695e567875a0f4ae9 Mon Sep 17 00:00:00 2001

---
From 5e078b1ba148aa280181c4f695e567875a0f4ae9 Mon Sep 17 00:00:00 2001
From: Grygorii Strashko <[email protected]>
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 <[email protected]>

Signed-off-by Murali Karicheri <[email protected]>

---
 drivers/base/dma-mapping.c  | 25 +++++++++++++++++++++++++
 include/linux/dma-mapping.h |  3 +++
 2 files changed, 28 insertions(+)

drivers/base/dma-mapping.c +0 -0

index 9e8bbdd..4556698 100644
@@ -270,6 +270,31 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
 }
 EXPORT_SYMBOL(dma_common_mmap);

Quoted 19 lines · On Friday 05 December 2014 15:26:27 Grygorii Strashko wrote:

---
From 5e078b1ba148aa280181c4f695e567875a0f4ae9 Mon Sep 17 00:00:00 2001
From: Grygorii Strashko <[email protected]>
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)

Quoted 6 lines

      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.

Quoted 20 lines

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 <[email protected]>

Signed-off-by Murali Karicheri <[email protected]>

---
 drivers/base/dma-mapping.c  | 25 +++++++++++++++++++++++++
 include/linux/dma-mapping.h |  3 +++
 2 files changed, 28 insertions(+)

drivers/base/dma-mapping.c +0 -0

index 9e8bbdd..4556698 100644
@@ -270,6 +270,31 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
 }
 EXPORT_SYMBOL(dma_common_mmap);
 

drivers/base/dma-mapping.c int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, · line 281

Quoted 9 lines

drivers/base/dma-mapping.c +9 -0

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

drivers/base/dma-mapping.c int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, · line 282

Quoted 2 lines

drivers/base/dma-mapping.c +1 -0

+     struct device *parent_dev = parent;

One of these two is not needed.

drivers/base/dma-mapping.c int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, · line 289

Quoted 6 lines

drivers/base/dma-mapping.c +6 -0

+     /* 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!

drivers/base/dma-mapping.c int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, · line 290

Quoted 1 line

drivers/base/dma-mapping.c +1 -0

+             dev->coherent_dma_mask = parent_dev->coherent_dma_mask;

What if someone has already set a mask that is greater than 32-bit?

drivers/base/dma-mapping.c int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, · line 291

Quoted 2 lines

drivers/base/dma-mapping.c +1 -0

+             dev->dma_parms  = parent_dev->dma_parms;

same problem here.

drivers/base/dma-mapping.c int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, · line 296

Quoted 5 lines

drivers/base/dma-mapping.c +5 -0

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

Quoted 28 lines · Arnd Bergmann

Quoted 19 lines · On Friday 05 December 2014 15:26:27 Grygorii Strashko wrote:

---
From 5e078b1ba148aa280181c4f695e567875a0f4ae9 Mon Sep 17 00:00:00 2001
From: Grygorii Strashko <[email protected]>
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)

Quoted 6 lines

      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.

Quoted 4 lines

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()>

Quoted 11 lines

Quoted 1 line

+ struct device *parent_dev = parent;

One of these two is not needed.

Quoted 6 lines

drivers/base/dma-mapping.c +6 -0

+     /* 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;

Quoted 5 lines

This is always wrong: dev->dma_mask is a pointer!

Quoted 1 line

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

Quoted 5 lines

Quoted 2 lines

+ dev->dma_parms = parent_dev->dma_parms;

same problem here.

Quoted 5 lines

drivers/base/dma-mapping.c int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, · line 296

Quoted 5 lines

drivers/base/dma-mapping.c +5 -0

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

Quoted 3 lines

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.