| Message ID | 20171011080041.12918-1-kishon@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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
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 { >
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
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.
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.
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
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 {
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(-)