[PATCH] of: Devices with pci_epf_bus_type require DMA configuration

[PATCH] of: Devices with pci_epf_bus_type require DMA configuration

[Kishon Vijay Abraham I]

pci-epc-core.c invokes of_dma_configure in order to configure
the coherent_dma_mask/dma_mask of endpoint function device. This is
required for dma_alloc_coherent to succeed in pci function driver
(pci-epf-test.c). However after
commit 723288836628bc1c08 ("of: restrict DMA configuration"),
of_dma_configure doesn't configure the coherent_dma_mask/dma_mask
of endpoint function device (since it doesn't have dma-ranges
property), resulting in dma_alloc_coherent in pci endpoint function
driver to to fail. Fix it by making sure of_dma_configure configures
coherent_dma_mask/dma_mask irrespective of whether the node has
dma-ranges property or not.

Fixes: 723288836628bc1c08 ("of: restrict DMA configuration")
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/of/device.c                 | 4 +++-
 drivers/pci/endpoint/pci-epf-core.c | 3 +--
 include/linux/pci-epf.h             | 1 +
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 64b710265d39..881cf80a3d69 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -10,6 +10,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/slab.h>
 #include <linux/pci.h>
+#include <linux/pci-epf.h>
 #include <linux/platform_device.h>
 #include <linux/amba/bus.h>
 
@@ -105,7 +106,8 @@ int of_dma_configure(struct device *dev, struct device_node *np)
 #ifdef CONFIG_ARM_AMBA
 		    dev->bus != &amba_bustype &&
 #endif
-		    dev->bus != &platform_bus_type)
+		    dev->bus != &platform_bus_type &&
+		    dev->bus != &pci_epf_bus_type)
 			return ret == -ENODEV ? 0 : ret;
 
 		dma_addr = offset = 0;
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index ae1611a62808..6f354ec6be71 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -26,7 +26,6 @@
 #include <linux/pci-epf.h>
 #include <linux/pci-ep-cfs.h>
 
-static struct bus_type pci_epf_bus_type;
 static const struct device_type pci_epf_type;
 
 /**
@@ -344,7 +343,7 @@ static int pci_epf_device_remove(struct device *dev)
 	return ret;
 }
 
-static struct bus_type pci_epf_bus_type = {
+struct bus_type pci_epf_bus_type = {
 	.name		= "pci-epf",
 	.match		= pci_epf_device_match,
 	.probe		= pci_epf_device_probe,
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index 60d551a9a1ba..83a3fb7e6ac1 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -16,6 +16,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/pci.h>
 
+extern struct bus_type pci_epf_bus_type;
 struct pci_epf;
 
 enum pci_barno {
-- 
2.11.0

Re: [PATCH] of: Devices with pci_epf_bus_type require DMA configuration

[Rob Herring]

On Wed, Oct 11, 2017 at 3:00 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> pci-epc-core.c invokes of_dma_configure in order to configure
> the coherent_dma_mask/dma_mask of endpoint function device. This is
> required for dma_alloc_coherent to succeed in pci function driver
> (pci-epf-test.c). However after
> commit 723288836628bc1c08 ("of: restrict DMA configuration"),
> of_dma_configure doesn't configure the coherent_dma_mask/dma_mask
> of endpoint function device (since it doesn't have dma-ranges
> property), resulting in dma_alloc_coherent in pci endpoint function
> driver to to fail. Fix it by making sure of_dma_configure configures
> coherent_dma_mask/dma_mask irrespective of whether the node has
> dma-ranges property or not.
>
> Fixes: 723288836628bc1c08 ("of: restrict DMA configuration")
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/of/device.c                 | 4 +++-
>  drivers/pci/endpoint/pci-epf-core.c | 3 +--
>  include/linux/pci-epf.h             | 1 +
>  3 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 64b710265d39..881cf80a3d69 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -10,6 +10,7 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/slab.h>
>  #include <linux/pci.h>
> +#include <linux/pci-epf.h>
>  #include <linux/platform_device.h>
>  #include <linux/amba/bus.h>
>
> @@ -105,7 +106,8 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>  #ifdef CONFIG_ARM_AMBA
>                     dev->bus != &amba_bustype &&
>  #endif
> -                   dev->bus != &platform_bus_type)
> +                   dev->bus != &platform_bus_type &&
> +                   dev->bus != &pci_epf_bus_type)

This is going to fail when this PCI is disabled. There's some
restructuring of this code proposed that will help you that I copied
you on.

Rob

Re: [PATCH] of: Devices with pci_epf_bus_type require DMA configuration

[Robin Murphy]

On 11/10/17 09:00, Kishon Vijay Abraham I wrote:
> pci-epc-core.c invokes of_dma_configure in order to configure
> the coherent_dma_mask/dma_mask of endpoint function device. This is
> required for dma_alloc_coherent to succeed in pci function driver
> (pci-epf-test.c). However after
> commit 723288836628bc1c08 ("of: restrict DMA configuration"),
> of_dma_configure doesn't configure the coherent_dma_mask/dma_mask
> of endpoint function device (since it doesn't have dma-ranges
> property), resulting in dma_alloc_coherent in pci endpoint function
> driver to to fail. Fix it by making sure of_dma_configure configures
> coherent_dma_mask/dma_mask irrespective of whether the node has
> dma-ranges property or not.

Frankly, what the endpoint stuff is doing looks wrong anyway. As I
understand it, the endpoint functions aren't real devices, just a
partitioning of resources - the only piece of hardware actually doing
DMA is the EPC itself, which should already have been configured
appropriately as a platform device.

It seems to me that the EPF BAR allocations should just be using the EPC
device directly, rather than trying to pretend the EPFs are distinct DMA
masters.

Furthermore, now that I've looked:

> 	dma_addr_t phys_addr;

please no :(

(I can easily think of more than one system with an EP-capable DWC PCIe
block integrated behind an IOMMU)

Robin.

> 
> Fixes: 723288836628bc1c08 ("of: restrict DMA configuration")
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/of/device.c                 | 4 +++-
>  drivers/pci/endpoint/pci-epf-core.c | 3 +--
>  include/linux/pci-epf.h             | 1 +
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 64b710265d39..881cf80a3d69 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -10,6 +10,7 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/slab.h>
>  #include <linux/pci.h>
> +#include <linux/pci-epf.h>
>  #include <linux/platform_device.h>
>  #include <linux/amba/bus.h>
>  
> @@ -105,7 +106,8 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>  #ifdef CONFIG_ARM_AMBA
>  		    dev->bus != &amba_bustype &&
>  #endif
> -		    dev->bus != &platform_bus_type)
> +		    dev->bus != &platform_bus_type &&
> +		    dev->bus != &pci_epf_bus_type)
>  			return ret == -ENODEV ? 0 : ret;
>  
>  		dma_addr = offset = 0;
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index ae1611a62808..6f354ec6be71 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -26,7 +26,6 @@
>  #include <linux/pci-epf.h>
>  #include <linux/pci-ep-cfs.h>
>  
> -static struct bus_type pci_epf_bus_type;
>  static const struct device_type pci_epf_type;
>  
>  /**
> @@ -344,7 +343,7 @@ static int pci_epf_device_remove(struct device *dev)
>  	return ret;
>  }
>  
> -static struct bus_type pci_epf_bus_type = {
> +struct bus_type pci_epf_bus_type = {
>  	.name		= "pci-epf",
>  	.match		= pci_epf_device_match,
>  	.probe		= pci_epf_device_probe,
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index 60d551a9a1ba..83a3fb7e6ac1 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -16,6 +16,7 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/pci.h>
>  
> +extern struct bus_type pci_epf_bus_type;
>  struct pci_epf;
>  
>  enum pci_barno {
> 

Re: [PATCH] of: Devices with pci_epf_bus_type require DMA configuration

[Kishon Vijay Abraham I]

Hi,

On Wednesday 11 October 2017 10:15 PM, Robin Murphy wrote:
> On 11/10/17 09:00, Kishon Vijay Abraham I wrote:
>> pci-epc-core.c invokes of_dma_configure in order to configure
>> the coherent_dma_mask/dma_mask of endpoint function device. This is
>> required for dma_alloc_coherent to succeed in pci function driver
>> (pci-epf-test.c). However after
>> commit 723288836628bc1c08 ("of: restrict DMA configuration"),
>> of_dma_configure doesn't configure the coherent_dma_mask/dma_mask
>> of endpoint function device (since it doesn't have dma-ranges
>> property), resulting in dma_alloc_coherent in pci endpoint function
>> driver to to fail. Fix it by making sure of_dma_configure configures
>> coherent_dma_mask/dma_mask irrespective of whether the node has
>> dma-ranges property or not.
> 
> Frankly, what the endpoint stuff is doing looks wrong anyway. As I
> understand it, the endpoint functions aren't real devices, just a
> partitioning of resources - the only piece of hardware actually doing
> DMA is the EPC itself, which should already have been configured
> appropriately as a platform device.

EPF devices use EPC devices which in turn use the actual platform device for
configuring the hardware. IMO the devices in one layer shouldn't have to
explicitly use devices in another layer other than using clearly defined API's.
Here platform_device is the bottom later, above which is epc_device and on top
is epf_device.

The idea is just by doing the initial setup in the framework, the epf driver be
able to use APIs like dma_alloc_coherent using it's own *device* rather than
the EPC's "device".
> 
> It seems to me that the EPF BAR allocations should just be using the EPC
> device directly, rather than trying to pretend the EPFs are distinct DMA
> masters.
> 
> Furthermore, now that I've looked:
> 
>> 	dma_addr_t phys_addr;
> 
> please no :(
> 
> (I can easily think of more than one system with an EP-capable DWC PCIe
> block integrated behind an IOMMU)

hmm.. haven't used IOMMU but won't setting up parent-child relationship between
EPC and EPF help in the case of IOMMU?

Thanks
Kishon

Re: [PATCH] of: Devices with pci_epf_bus_type require DMA configuration

[Christoph Hellwig]

On Mon, Oct 23, 2017 at 11:13:23AM +0530, Kishon Vijay Abraham I wrote:
> EPF devices use EPC devices which in turn use the actual platform device for
> configuring the hardware. IMO the devices in one layer shouldn't have to
> explicitly use devices in another layer other than using clearly defined API's.
> Here platform_device is the bottom later, above which is epc_device and on top
> is epf_device.
> 
> The idea is just by doing the initial setup in the framework, the epf driver be
> able to use APIs like dma_alloc_coherent using it's own *device* rather than
> the EPC's "device".

That's a little strange - normally we only call dma_map* routines on
the actual physical bus devices.

Re: [PATCH] of: Devices with pci_epf_bus_type require DMA configuration

[Robin Murphy]

On 23/10/17 06:43, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Wednesday 11 October 2017 10:15 PM, Robin Murphy wrote:
>> On 11/10/17 09:00, Kishon Vijay Abraham I wrote:
>>> pci-epc-core.c invokes of_dma_configure in order to configure
>>> the coherent_dma_mask/dma_mask of endpoint function device. This is
>>> required for dma_alloc_coherent to succeed in pci function driver
>>> (pci-epf-test.c). However after
>>> commit 723288836628bc1c08 ("of: restrict DMA configuration"),
>>> of_dma_configure doesn't configure the coherent_dma_mask/dma_mask
>>> of endpoint function device (since it doesn't have dma-ranges
>>> property), resulting in dma_alloc_coherent in pci endpoint function
>>> driver to to fail. Fix it by making sure of_dma_configure configures
>>> coherent_dma_mask/dma_mask irrespective of whether the node has
>>> dma-ranges property or not.
>>
>> Frankly, what the endpoint stuff is doing looks wrong anyway. As I
>> understand it, the endpoint functions aren't real devices, just a
>> partitioning of resources - the only piece of hardware actually doing
>> DMA is the EPC itself, which should already have been configured
>> appropriately as a platform device.
> 
> EPF devices use EPC devices which in turn use the actual platform device for
> configuring the hardware. IMO the devices in one layer shouldn't have to
> explicitly use devices in another layer other than using clearly defined API's.
> Here platform_device is the bottom later, above which is epc_device and on top
> is epf_device.

Note that the "sysdev"-type model that I'm implying already has
precedent elsewhere, e.g. in the USB layer. Since you already have
things abstracted behind the pci_epf_{alloc,free}_space() API, this
seems like a simple change to make.

> The idea is just by doing the initial setup in the framework, the epf driver be
> able to use APIs like dma_alloc_coherent using it's own *device* rather than
> the EPC's "device".

OK, but when would that actually happen? Please correct me if I've got
it wrong, but as best I understand it, the hardware extent of the EPF is
some registers controlling the config space that the EPC exposes to the
other end of the PCI link - the only actual DMA happening will be in
response to PCI traffic in and out of the EPF's BARs, between the EPC
and the memory backing those BAR regions which is already controlled by
your API. Sure, the EPF *driver* is free to access whatever memory it
feels like, but as a software construct that doesn't constitute DMA.

To put it another way, if an endpoint driver *does* call
dma_alloc_coherent(epf_device, ...), what does it then do with the
resulting DMA address?

>> It seems to me that the EPF BAR allocations should just be using the EPC
>> device directly, rather than trying to pretend the EPFs are distinct DMA
>> masters.
>>
>> Furthermore, now that I've looked:
>>
>>> 	dma_addr_t phys_addr;
>>
>> please no :(
>>
>> (I can easily think of more than one system with an EP-capable DWC PCIe
>> block integrated behind an IOMMU)
> 
> hmm.. haven't used IOMMU but won't setting up parent-child relationship between
> EPC and EPF help in the case of IOMMU?

I was merely referring to the characterisation throughout this code that
a DMA address is a physical address; it isn't, for any number of reasons
(IOMMUs are just the most obvious). Fortunately it's only a cosmetic
naming problem, the actual usage looks OK.

Robin.

Re: [PATCH] of: Devices with pci_epf_bus_type require DMA configuration

[Kishon Vijay Abraham I]

Hi,

On Monday 23 October 2017 06:35 PM, Robin Murphy wrote:
> On 23/10/17 06:43, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Wednesday 11 October 2017 10:15 PM, Robin Murphy wrote:
>>> On 11/10/17 09:00, Kishon Vijay Abraham I wrote:
>>>> pci-epc-core.c invokes of_dma_configure in order to configure
>>>> the coherent_dma_mask/dma_mask of endpoint function device. This is
>>>> required for dma_alloc_coherent to succeed in pci function driver
>>>> (pci-epf-test.c). However after
>>>> commit 723288836628bc1c08 ("of: restrict DMA configuration"),
>>>> of_dma_configure doesn't configure the coherent_dma_mask/dma_mask
>>>> of endpoint function device (since it doesn't have dma-ranges
>>>> property), resulting in dma_alloc_coherent in pci endpoint function
>>>> driver to to fail. Fix it by making sure of_dma_configure configures
>>>> coherent_dma_mask/dma_mask irrespective of whether the node has
>>>> dma-ranges property or not.
>>>
>>> Frankly, what the endpoint stuff is doing looks wrong anyway. As I
>>> understand it, the endpoint functions aren't real devices, just a
>>> partitioning of resources - the only piece of hardware actually doing
>>> DMA is the EPC itself, which should already have been configured
>>> appropriately as a platform device.
>>
>> EPF devices use EPC devices which in turn use the actual platform device for
>> configuring the hardware. IMO the devices in one layer shouldn't have to
>> explicitly use devices in another layer other than using clearly defined API's.
>> Here platform_device is the bottom later, above which is epc_device and on top
>> is epf_device.
> 
> Note that the "sysdev"-type model that I'm implying already has
> precedent elsewhere, e.g. in the USB layer. Since you already have
> things abstracted behind the pci_epf_{alloc,free}_space() API, this
> seems like a simple change to make.

I got your point. The device in struct pci_epc is also a virtual device (it
doesn't have a driver associated with it). So we'll have to use something like
epf->epc->dev.parent to actually use the platform device.

It has a couple of indirections but it should be okay to change a couple of
API's I think. With that of_dma_configure can also be removed from
pci-epc-core.c. I'll send a patch fixing those.
> 
>> The idea is just by doing the initial setup in the framework, the epf driver be
>> able to use APIs like dma_alloc_coherent using it's own *device* rather than
>> the EPC's "device".
> 
> OK, but when would that actually happen? Please correct me if I've got
> it wrong, but as best I understand it, the hardware extent of the EPF is
> some registers controlling the config space that the EPC exposes to the
> other end of the PCI link - the only actual DMA happening will be in
> response to PCI traffic in and out of the EPF's BARs, between the EPC
> and the memory backing those BAR regions which is already controlled by
> your API. Sure, the EPF *driver* is free to access whatever memory it

That's correct. The allocated memory is used only using BARs.
> feels like, but as a software construct that doesn't constitute DMA.
> 
> To put it another way, if an endpoint driver *does* call
> dma_alloc_coherent(epf_device, ...), what does it then do with the
> resulting DMA address?
> 
>>> It seems to me that the EPF BAR allocations should just be using the EPC
>>> device directly, rather than trying to pretend the EPFs are distinct DMA
>>> masters.
>>>
>>> Furthermore, now that I've looked:
>>>
>>>> 	dma_addr_t phys_addr;
>>>
>>> please no :(
>>>
>>> (I can easily think of more than one system with an EP-capable DWC PCIe
>>> block integrated behind an IOMMU)
>>
>> hmm.. haven't used IOMMU but won't setting up parent-child relationship between
>> EPC and EPF help in the case of IOMMU?
> 
> I was merely referring to the characterisation throughout this code that
> a DMA address is a physical address; it isn't, for any number of reasons
> (IOMMUs are just the most obvious). Fortunately it's only a cosmetic
> naming problem, the actual usage looks OK.

right, that was named incorrectly.

Thanks
Kishon