| Message ID | 1480465344-11862-7-git-send-email-sricharan@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sricharan and Robin, On 2016-11-30 01:22, Sricharan R wrote: > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > Failures to look up an IOMMU when parsing the DT iommus property need to > be handled separately from the .of_xlate() failures to support deferred > probing. > > The lack of a registered IOMMU can be caused by the lack of a driver for > the IOMMU, the IOMMU device probe not having been performed yet, having > been deferred, or having failed. > > The first case occurs when the device tree describes the bus master and > IOMMU topology correctly but no device driver exists for the IOMMU yet > or the device driver has not been compiled in. Return NULL, the caller > will configure the device without an IOMMU. > > The second and third cases are handled by deferring the probe of the bus > master device which will eventually get reprobed after the IOMMU. > > The last case is currently handled by deferring the probe of the bus > master device as well. A mechanism to either configure the bus master > device without an IOMMU or to fail the bus master device probe depending > on whether the IOMMU is optional or mandatory would be a good > enhancement. > > Signed-off-by: Laurent Pichart <laurent.pinchart+renesas@ideasonboard.com> > Signed-off-by: Sricharan R <sricharan@codeaurora.org> > [rm: massive PCI hacks] > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/base/dma-mapping.c | 4 ++-- > drivers/iommu/dma-iommu.c | 1 + > drivers/iommu/of_iommu.c | 5 +++-- > drivers/of/device.c | 9 +++++++-- > drivers/pci/probe.c | 6 ++++-- > include/linux/of_device.h | 9 ++++++--- > include/linux/pci.h | 4 ++-- > 7 files changed, 25 insertions(+), 13 deletions(-) > > diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c > index b2a5629..576fdfb 100644 > --- a/drivers/base/dma-mapping.c > +++ b/drivers/base/dma-mapping.c > @@ -351,9 +351,9 @@ void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags) > int dma_configure(struct device *dev) > { > if (dev_is_pci(dev)) > - pci_dma_configure(dev); > + return pci_dma_configure(dev); > else if (dev->of_node) > - of_dma_configure(dev, dev->of_node); > + return of_dma_configure(dev, dev->of_node); > return 0; > } > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > index c5ab866..d2a7a46 100644 > --- a/drivers/iommu/dma-iommu.c > +++ b/drivers/iommu/dma-iommu.c > @@ -148,6 +148,7 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base, > base_pfn = max_t(unsigned long, 1, base >> order); > end_pfn = (base + size - 1) >> order; > > + dev_info(dev, "0x%llx 0x%llx, 0x%llx 0x%llx, 0x%llx 0x%llx\n", base, size, domain->geometry.aperture_start, domain->geometry.aperture_end, This causes a NULL pointer dereference if caller passes NULL device pointer. There is such caller in drivers/gpu/drm/exynos/exynos_drm_iommu.h. Trivial to fix as it looks like a leftover from developement or debugging stage. > *dev->dma_mask, dev->coherent_dma_mask); > /* Check the domain allows at least some access to the device... */ > if (domain->geometry.force_aperture) { > if (base > domain->geometry.aperture_end || > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index 349bd1d..9529d6c 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -23,6 +23,7 @@ > #include <linux/of.h> > #include <linux/of_iommu.h> > #include <linux/of_pci.h> > +#include <linux/pci.h> > #include <linux/slab.h> > > static const struct of_device_id __iommu_of_table_sentinel > @@ -223,7 +224,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, > ops = ERR_PTR(err); > } > > - return IS_ERR(ops) ? NULL : ops; > + return ops; > } > > static int __init of_iommu_init(void) > @@ -234,7 +235,7 @@ static int __init of_iommu_init(void) > for_each_matching_node_and_match(np, matches, &match) { > const of_iommu_init_fn init_fn = match->data; > > - if (init_fn(np)) > + if (init_fn && init_fn(np)) > pr_err("Failed to initialise IOMMU %s\n", > of_node_full_name(np)); > } > diff --git a/drivers/of/device.c b/drivers/of/device.c > index 1c843e2..d58595c 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -82,7 +82,7 @@ int of_device_add(struct platform_device *ofdev) > * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events > * to fix up DMA configuration. > */ > -void of_dma_configure(struct device *dev, struct device_node *np) > +int of_dma_configure(struct device *dev, struct device_node *np) > { > u64 dma_addr, paddr, size; > int ret; > @@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct device_node *np) > ret = of_dma_get_range(np, &dma_addr, &paddr, &size); > if (ret < 0) { > dma_addr = offset = 0; > - size = dev->coherent_dma_mask + 1; > + size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1); > } else { > offset = PFN_DOWN(paddr - dma_addr); > dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); > @@ -129,10 +129,15 @@ void of_dma_configure(struct device *dev, struct device_node *np) > coherent ? " " : " not "); > > iommu = of_iommu_configure(dev, np); > + if (IS_ERR(iommu)) > + return PTR_ERR(iommu); > + > dev_dbg(dev, "device is%sbehind an iommu\n", > iommu ? " " : " not "); > > arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent); > + > + return 0; > } > EXPORT_SYMBOL_GPL(of_dma_configure); > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 04af770..6316cae 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1724,13 +1724,14 @@ static void pci_set_msi_domain(struct pci_dev *dev) > * Function to update PCI devices's DMA configuration using the same > * info from the OF node or ACPI node of host bridge's parent (if any). > */ > -void pci_dma_configure(struct device *dev) > +int pci_dma_configure(struct device *dev) > { > struct device *bridge = pci_get_host_bridge_device(to_pci_dev(dev)); > + int ret = 0; > > if (IS_ENABLED(CONFIG_OF) && > bridge->parent && bridge->parent->of_node) { > - of_dma_configure(dev, bridge->parent->of_node); > + ret = of_dma_configure(dev, bridge->parent->of_node); > } else if (has_acpi_companion(bridge)) { > struct acpi_device *adev = to_acpi_device_node(bridge->fwnode); > enum dev_dma_attr attr = acpi_get_dma_attr(adev); > @@ -1742,6 +1743,7 @@ void pci_dma_configure(struct device *dev) > } > > pci_put_host_bridge_device(bridge); > + return ret; > } > > void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) > diff --git a/include/linux/of_device.h b/include/linux/of_device.h > index d20a31a..6dca65c 100644 > --- a/include/linux/of_device.h > +++ b/include/linux/of_device.h > @@ -55,7 +55,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) > return of_node_get(cpu_dev->of_node); > } > > -void of_dma_configure(struct device *dev, struct device_node *np); > +int of_dma_configure(struct device *dev, struct device_node *np); > void of_dma_deconfigure(struct device *dev); > #else /* CONFIG_OF */ > > @@ -99,8 +99,11 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) > { > return NULL; > } > -static inline void of_dma_configure(struct device *dev, struct device_node *np) > -{} > + > +static inline int of_dma_configure(struct device *dev, struct device_node *np) > +{ > + return 0; > +} > static inline void of_dma_deconfigure(struct device *dev) > {} > #endif /* CONFIG_OF */ > diff --git a/include/linux/pci.h b/include/linux/pci.h > index d04f651..989ca44 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -870,7 +870,7 @@ struct resource *pci_find_parent_resource(const struct pci_dev *dev, > #define dev_is_pf(d) ((dev_is_pci(d) ? to_pci_dev(d)->is_physfn : false)) > #define dev_num_vf(d) ((dev_is_pci(d) ? pci_num_vf(to_pci_dev(d)) : 0)) > > -void pci_dma_configure(struct device *dev); > +int pci_dma_configure(struct device *dev); > > /* Generic PCI functions exported to card drivers */ > > @@ -1604,7 +1604,7 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus, > #define dev_is_pf(d) (false) > #define dev_num_vf(d) (0) > > -static inline void pci_dma_configure(struct device *dev) { } > +static inline int pci_dma_configure(struct device *dev) { return 0; } > > #endif /* CONFIG_PCI */ > Best regards
Hi, >Hi Sricharan and Robin, > > >On 2016-11-30 01:22, Sricharan R wrote: >> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >> >> Failures to look up an IOMMU when parsing the DT iommus property need to >> be handled separately from the .of_xlate() failures to support deferred >> probing. >> >> The lack of a registered IOMMU can be caused by the lack of a driver for >> the IOMMU, the IOMMU device probe not having been performed yet, having >> been deferred, or having failed. >> >> The first case occurs when the device tree describes the bus master and >> IOMMU topology correctly but no device driver exists for the IOMMU yet >> or the device driver has not been compiled in. Return NULL, the caller >> will configure the device without an IOMMU. >> >> The second and third cases are handled by deferring the probe of the bus >> master device which will eventually get reprobed after the IOMMU. >> >> The last case is currently handled by deferring the probe of the bus >> master device as well. A mechanism to either configure the bus master >> device without an IOMMU or to fail the bus master device probe depending >> on whether the IOMMU is optional or mandatory would be a good >> enhancement. >> >> Signed-off-by: Laurent Pichart <laurent.pinchart+renesas@ideasonboard.com> >> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >> [rm: massive PCI hacks] >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> --- >> drivers/base/dma-mapping.c | 4 ++-- >> drivers/iommu/dma-iommu.c | 1 + >> drivers/iommu/of_iommu.c | 5 +++-- >> drivers/of/device.c | 9 +++++++-- >> drivers/pci/probe.c | 6 ++++-- >> include/linux/of_device.h | 9 ++++++--- >> include/linux/pci.h | 4 ++-- >> 7 files changed, 25 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c >> index b2a5629..576fdfb 100644 >> --- a/drivers/base/dma-mapping.c >> +++ b/drivers/base/dma-mapping.c >> @@ -351,9 +351,9 @@ void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags) >> int dma_configure(struct device *dev) >> { >> if (dev_is_pci(dev)) >> - pci_dma_configure(dev); >> + return pci_dma_configure(dev); >> else if (dev->of_node) >> - of_dma_configure(dev, dev->of_node); >> + return of_dma_configure(dev, dev->of_node); >> return 0; >> } >> >> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c >> index c5ab866..d2a7a46 100644 >> --- a/drivers/iommu/dma-iommu.c >> +++ b/drivers/iommu/dma-iommu.c >> @@ -148,6 +148,7 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base, >> base_pfn = max_t(unsigned long, 1, base >> order); >> end_pfn = (base + size - 1) >> order; >> >> + dev_info(dev, "0x%llx 0x%llx, 0x%llx 0x%llx, 0x%llx 0x%llx\n", base, size, domain->geometry.aperture_start, domain- >>geometry.aperture_end, > >This causes a NULL pointer dereference if caller passes NULL device pointer. >There is such caller in drivers/gpu/drm/exynos/exynos_drm_iommu.h. >Trivial to fix as it looks like a leftover from developement or >debugging stage. > My bad, i should have removed it before posting. Noticed this, but missed to remove it. Will do so. Regards, Sricharan
On 30/11/16 07:54, Marek Szyprowski wrote: > Hi Sricharan and Robin, > > > On 2016-11-30 01:22, Sricharan R wrote: >> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >> >> Failures to look up an IOMMU when parsing the DT iommus property need to >> be handled separately from the .of_xlate() failures to support deferred >> probing. >> >> The lack of a registered IOMMU can be caused by the lack of a driver for >> the IOMMU, the IOMMU device probe not having been performed yet, having >> been deferred, or having failed. >> >> The first case occurs when the device tree describes the bus master and >> IOMMU topology correctly but no device driver exists for the IOMMU yet >> or the device driver has not been compiled in. Return NULL, the caller >> will configure the device without an IOMMU. >> >> The second and third cases are handled by deferring the probe of the bus >> master device which will eventually get reprobed after the IOMMU. >> >> The last case is currently handled by deferring the probe of the bus >> master device as well. A mechanism to either configure the bus master >> device without an IOMMU or to fail the bus master device probe depending >> on whether the IOMMU is optional or mandatory would be a good >> enhancement. >> >> Signed-off-by: Laurent Pichart >> <laurent.pinchart+renesas@ideasonboard.com> >> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >> [rm: massive PCI hacks] >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> --- >> drivers/base/dma-mapping.c | 4 ++-- >> drivers/iommu/dma-iommu.c | 1 + >> drivers/iommu/of_iommu.c | 5 +++-- >> drivers/of/device.c | 9 +++++++-- >> drivers/pci/probe.c | 6 ++++-- >> include/linux/of_device.h | 9 ++++++--- >> include/linux/pci.h | 4 ++-- >> 7 files changed, 25 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c >> index b2a5629..576fdfb 100644 >> --- a/drivers/base/dma-mapping.c >> +++ b/drivers/base/dma-mapping.c >> @@ -351,9 +351,9 @@ void dma_common_free_remap(void *cpu_addr, size_t >> size, unsigned long vm_flags) >> int dma_configure(struct device *dev) >> { >> if (dev_is_pci(dev)) >> - pci_dma_configure(dev); >> + return pci_dma_configure(dev); >> else if (dev->of_node) >> - of_dma_configure(dev, dev->of_node); >> + return of_dma_configure(dev, dev->of_node); >> return 0; >> } >> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c >> index c5ab866..d2a7a46 100644 >> --- a/drivers/iommu/dma-iommu.c >> +++ b/drivers/iommu/dma-iommu.c >> @@ -148,6 +148,7 @@ int iommu_dma_init_domain(struct iommu_domain >> *domain, dma_addr_t base, >> base_pfn = max_t(unsigned long, 1, base >> order); >> end_pfn = (base + size - 1) >> order; >> + dev_info(dev, "0x%llx 0x%llx, 0x%llx 0x%llx, 0x%llx 0x%llx\n", >> base, size, domain->geometry.aperture_start, >> domain->geometry.aperture_end, > > This causes a NULL pointer dereference if caller passes NULL device > pointer. > There is such caller in drivers/gpu/drm/exynos/exynos_drm_iommu.h. > Trivial to fix as it looks like a leftover from developement or > debugging stage. Yes, this is some development crap which was never intended to go upstream. Hence "massive PCI hacks" ;) Other than the first two patches, the rest of the stuff from me here was just an experiment which I'm not entirely convinced by the outcome of - I don't particularly like the resulting fragmentation of having pci_dma_configure() awkwardly floating around on its own in pci.c. Robin. >> *dev->dma_mask, dev->coherent_dma_mask); >> /* Check the domain allows at least some access to the device... */ >> if (domain->geometry.force_aperture) { >> if (base > domain->geometry.aperture_end || >> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c >> index 349bd1d..9529d6c 100644 >> --- a/drivers/iommu/of_iommu.c >> +++ b/drivers/iommu/of_iommu.c >> @@ -23,6 +23,7 @@ >> #include <linux/of.h> >> #include <linux/of_iommu.h> >> #include <linux/of_pci.h> >> +#include <linux/pci.h> >> #include <linux/slab.h> >> static const struct of_device_id __iommu_of_table_sentinel >> @@ -223,7 +224,7 @@ const struct iommu_ops *of_iommu_configure(struct >> device *dev, >> ops = ERR_PTR(err); >> } >> - return IS_ERR(ops) ? NULL : ops; >> + return ops; >> } >> static int __init of_iommu_init(void) >> @@ -234,7 +235,7 @@ static int __init of_iommu_init(void) >> for_each_matching_node_and_match(np, matches, &match) { >> const of_iommu_init_fn init_fn = match->data; >> - if (init_fn(np)) >> + if (init_fn && init_fn(np)) >> pr_err("Failed to initialise IOMMU %s\n", >> of_node_full_name(np)); >> } >> diff --git a/drivers/of/device.c b/drivers/of/device.c >> index 1c843e2..d58595c 100644 >> --- a/drivers/of/device.c >> +++ b/drivers/of/device.c >> @@ -82,7 +82,7 @@ int of_device_add(struct platform_device *ofdev) >> * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE >> events >> * to fix up DMA configuration. >> */ >> -void of_dma_configure(struct device *dev, struct device_node *np) >> +int of_dma_configure(struct device *dev, struct device_node *np) >> { >> u64 dma_addr, paddr, size; >> int ret; >> @@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct >> device_node *np) >> ret = of_dma_get_range(np, &dma_addr, &paddr, &size); >> if (ret < 0) { >> dma_addr = offset = 0; >> - size = dev->coherent_dma_mask + 1; >> + size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1); >> } else { >> offset = PFN_DOWN(paddr - dma_addr); >> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); >> @@ -129,10 +129,15 @@ void of_dma_configure(struct device *dev, struct >> device_node *np) >> coherent ? " " : " not "); >> iommu = of_iommu_configure(dev, np); >> + if (IS_ERR(iommu)) >> + return PTR_ERR(iommu); >> + >> dev_dbg(dev, "device is%sbehind an iommu\n", >> iommu ? " " : " not "); >> arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent); >> + >> + return 0; >> } >> EXPORT_SYMBOL_GPL(of_dma_configure); >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 04af770..6316cae 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -1724,13 +1724,14 @@ static void pci_set_msi_domain(struct pci_dev >> *dev) >> * Function to update PCI devices's DMA configuration using the same >> * info from the OF node or ACPI node of host bridge's parent (if any). >> */ >> -void pci_dma_configure(struct device *dev) >> +int pci_dma_configure(struct device *dev) >> { >> struct device *bridge = >> pci_get_host_bridge_device(to_pci_dev(dev)); >> + int ret = 0; >> if (IS_ENABLED(CONFIG_OF) && >> bridge->parent && bridge->parent->of_node) { >> - of_dma_configure(dev, bridge->parent->of_node); >> + ret = of_dma_configure(dev, bridge->parent->of_node); >> } else if (has_acpi_companion(bridge)) { >> struct acpi_device *adev = to_acpi_device_node(bridge->fwnode); >> enum dev_dma_attr attr = acpi_get_dma_attr(adev); >> @@ -1742,6 +1743,7 @@ void pci_dma_configure(struct device *dev) >> } >> pci_put_host_bridge_device(bridge); >> + return ret; >> } >> void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) >> diff --git a/include/linux/of_device.h b/include/linux/of_device.h >> index d20a31a..6dca65c 100644 >> --- a/include/linux/of_device.h >> +++ b/include/linux/of_device.h >> @@ -55,7 +55,7 @@ static inline struct device_node >> *of_cpu_device_node_get(int cpu) >> return of_node_get(cpu_dev->of_node); >> } >> -void of_dma_configure(struct device *dev, struct device_node *np); >> +int of_dma_configure(struct device *dev, struct device_node *np); >> void of_dma_deconfigure(struct device *dev); >> #else /* CONFIG_OF */ >> @@ -99,8 +99,11 @@ static inline struct device_node >> *of_cpu_device_node_get(int cpu) >> { >> return NULL; >> } >> -static inline void of_dma_configure(struct device *dev, struct >> device_node *np) >> -{} >> + >> +static inline int of_dma_configure(struct device *dev, struct >> device_node *np) >> +{ >> + return 0; >> +} >> static inline void of_dma_deconfigure(struct device *dev) >> {} >> #endif /* CONFIG_OF */ >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index d04f651..989ca44 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -870,7 +870,7 @@ struct resource *pci_find_parent_resource(const >> struct pci_dev *dev, >> #define dev_is_pf(d) ((dev_is_pci(d) ? to_pci_dev(d)->is_physfn : >> false)) >> #define dev_num_vf(d) ((dev_is_pci(d) ? pci_num_vf(to_pci_dev(d)) : 0)) >> -void pci_dma_configure(struct device *dev); >> +int pci_dma_configure(struct device *dev); >> /* Generic PCI functions exported to card drivers */ >> @@ -1604,7 +1604,7 @@ static inline struct pci_dev >> *pci_get_bus_and_slot(unsigned int bus, >> #define dev_is_pf(d) (false) >> #define dev_num_vf(d) (0) >> -static inline void pci_dma_configure(struct device *dev) { } >> +static inline int pci_dma_configure(struct device *dev) { return 0; } >> #endif /* CONFIG_PCI */ >> > > Best regards
Hi Robin, >On 30/11/16 07:54, Marek Szyprowski wrote: >> Hi Sricharan and Robin, >> >> >> On 2016-11-30 01:22, Sricharan R wrote: >>> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >>> >>> Failures to look up an IOMMU when parsing the DT iommus property need to >>> be handled separately from the .of_xlate() failures to support deferred >>> probing. >>> >>> The lack of a registered IOMMU can be caused by the lack of a driver for >>> the IOMMU, the IOMMU device probe not having been performed yet, having >>> been deferred, or having failed. >>> >>> The first case occurs when the device tree describes the bus master and >>> IOMMU topology correctly but no device driver exists for the IOMMU yet >>> or the device driver has not been compiled in. Return NULL, the caller >>> will configure the device without an IOMMU. >>> >>> The second and third cases are handled by deferring the probe of the bus >>> master device which will eventually get reprobed after the IOMMU. >>> >>> The last case is currently handled by deferring the probe of the bus >>> master device as well. A mechanism to either configure the bus master >>> device without an IOMMU or to fail the bus master device probe depending >>> on whether the IOMMU is optional or mandatory would be a good >>> enhancement. >>> >>> Signed-off-by: Laurent Pichart >>> <laurent.pinchart+renesas@ideasonboard.com> >>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>> [rm: massive PCI hacks] >>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >>> --- >>> drivers/base/dma-mapping.c | 4 ++-- >>> drivers/iommu/dma-iommu.c | 1 + >>> drivers/iommu/of_iommu.c | 5 +++-- >>> drivers/of/device.c | 9 +++++++-- >>> drivers/pci/probe.c | 6 ++++-- >>> include/linux/of_device.h | 9 ++++++--- >>> include/linux/pci.h | 4 ++-- >>> 7 files changed, 25 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c >>> index b2a5629..576fdfb 100644 >>> --- a/drivers/base/dma-mapping.c >>> +++ b/drivers/base/dma-mapping.c >>> @@ -351,9 +351,9 @@ void dma_common_free_remap(void *cpu_addr, size_t >>> size, unsigned long vm_flags) >>> int dma_configure(struct device *dev) >>> { >>> if (dev_is_pci(dev)) >>> - pci_dma_configure(dev); >>> + return pci_dma_configure(dev); >>> else if (dev->of_node) >>> - of_dma_configure(dev, dev->of_node); >>> + return of_dma_configure(dev, dev->of_node); >>> return 0; >>> } >>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c >>> index c5ab866..d2a7a46 100644 >>> --- a/drivers/iommu/dma-iommu.c >>> +++ b/drivers/iommu/dma-iommu.c >>> @@ -148,6 +148,7 @@ int iommu_dma_init_domain(struct iommu_domain >>> *domain, dma_addr_t base, >>> base_pfn = max_t(unsigned long, 1, base >> order); >>> end_pfn = (base + size - 1) >> order; >>> + dev_info(dev, "0x%llx 0x%llx, 0x%llx 0x%llx, 0x%llx 0x%llx\n", >>> base, size, domain->geometry.aperture_start, >>> domain->geometry.aperture_end, >> >> This causes a NULL pointer dereference if caller passes NULL device >> pointer. >> There is such caller in drivers/gpu/drm/exynos/exynos_drm_iommu.h. >> Trivial to fix as it looks like a leftover from developement or >> debugging stage. > >Yes, this is some development crap which was never intended to go >upstream. Hence "massive PCI hacks" ;) > >Other than the first two patches, the rest of the stuff from me here was >just an experiment which I'm not entirely convinced by the outcome of - >I don't particularly like the resulting fragmentation of having >pci_dma_configure() awkwardly floating around on its own in pci.c. Ha, sorry looks like that i have misunderstood that then. I thought that the V3 changes that i had for pci was hacky and the reworked patches in your latest branch were correct. So do you suggest that the pci_dma_configure gets removed and the pci related changes gets handled in of_iommu_configure /acpi_dma_configure itself ? Regards, Sricharan
diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c index b2a5629..576fdfb 100644 --- a/drivers/base/dma-mapping.c +++ b/drivers/base/dma-mapping.c @@ -351,9 +351,9 @@ void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags) int dma_configure(struct device *dev) { if (dev_is_pci(dev)) - pci_dma_configure(dev); + return pci_dma_configure(dev); else if (dev->of_node) - of_dma_configure(dev, dev->of_node); + return of_dma_configure(dev, dev->of_node); return 0; } diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index c5ab866..d2a7a46 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -148,6 +148,7 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base, base_pfn = max_t(unsigned long, 1, base >> order); end_pfn = (base + size - 1) >> order; + dev_info(dev, "0x%llx 0x%llx, 0x%llx 0x%llx, 0x%llx 0x%llx\n", base, size, domain->geometry.aperture_start, domain->geometry.aperture_end, *dev->dma_mask, dev->coherent_dma_mask); /* Check the domain allows at least some access to the device... */ if (domain->geometry.force_aperture) { if (base > domain->geometry.aperture_end || diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index 349bd1d..9529d6c 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -23,6 +23,7 @@ #include <linux/of.h> #include <linux/of_iommu.h> #include <linux/of_pci.h> +#include <linux/pci.h> #include <linux/slab.h> static const struct of_device_id __iommu_of_table_sentinel @@ -223,7 +224,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, ops = ERR_PTR(err); } - return IS_ERR(ops) ? NULL : ops; + return ops; } static int __init of_iommu_init(void) @@ -234,7 +235,7 @@ static int __init of_iommu_init(void) for_each_matching_node_and_match(np, matches, &match) { const of_iommu_init_fn init_fn = match->data; - if (init_fn(np)) + if (init_fn && init_fn(np)) pr_err("Failed to initialise IOMMU %s\n", of_node_full_name(np)); } diff --git a/drivers/of/device.c b/drivers/of/device.c index 1c843e2..d58595c 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -82,7 +82,7 @@ int of_device_add(struct platform_device *ofdev) * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events * to fix up DMA configuration. */ -void of_dma_configure(struct device *dev, struct device_node *np) +int of_dma_configure(struct device *dev, struct device_node *np) { u64 dma_addr, paddr, size; int ret; @@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct device_node *np) ret = of_dma_get_range(np, &dma_addr, &paddr, &size); if (ret < 0) { dma_addr = offset = 0; - size = dev->coherent_dma_mask + 1; + size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1); } else { offset = PFN_DOWN(paddr - dma_addr); dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); @@ -129,10 +129,15 @@ void of_dma_configure(struct device *dev, struct device_node *np) coherent ? " " : " not "); iommu = of_iommu_configure(dev, np); + if (IS_ERR(iommu)) + return PTR_ERR(iommu); + dev_dbg(dev, "device is%sbehind an iommu\n", iommu ? " " : " not "); arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent); + + return 0; } EXPORT_SYMBOL_GPL(of_dma_configure); diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 04af770..6316cae 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1724,13 +1724,14 @@ static void pci_set_msi_domain(struct pci_dev *dev) * Function to update PCI devices's DMA configuration using the same * info from the OF node or ACPI node of host bridge's parent (if any). */ -void pci_dma_configure(struct device *dev) +int pci_dma_configure(struct device *dev) { struct device *bridge = pci_get_host_bridge_device(to_pci_dev(dev)); + int ret = 0; if (IS_ENABLED(CONFIG_OF) && bridge->parent && bridge->parent->of_node) { - of_dma_configure(dev, bridge->parent->of_node); + ret = of_dma_configure(dev, bridge->parent->of_node); } else if (has_acpi_companion(bridge)) { struct acpi_device *adev = to_acpi_device_node(bridge->fwnode); enum dev_dma_attr attr = acpi_get_dma_attr(adev); @@ -1742,6 +1743,7 @@ void pci_dma_configure(struct device *dev) } pci_put_host_bridge_device(bridge); + return ret; } void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) diff --git a/include/linux/of_device.h b/include/linux/of_device.h index d20a31a..6dca65c 100644 --- a/include/linux/of_device.h +++ b/include/linux/of_device.h @@ -55,7 +55,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) return of_node_get(cpu_dev->of_node); } -void of_dma_configure(struct device *dev, struct device_node *np); +int of_dma_configure(struct device *dev, struct device_node *np); void of_dma_deconfigure(struct device *dev); #else /* CONFIG_OF */ @@ -99,8 +99,11 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) { return NULL; } -static inline void of_dma_configure(struct device *dev, struct device_node *np) -{} + +static inline int of_dma_configure(struct device *dev, struct device_node *np) +{ + return 0; +} static inline void of_dma_deconfigure(struct device *dev) {} #endif /* CONFIG_OF */ diff --git a/include/linux/pci.h b/include/linux/pci.h index d04f651..989ca44 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -870,7 +870,7 @@ struct resource *pci_find_parent_resource(const struct pci_dev *dev, #define dev_is_pf(d) ((dev_is_pci(d) ? to_pci_dev(d)->is_physfn : false)) #define dev_num_vf(d) ((dev_is_pci(d) ? pci_num_vf(to_pci_dev(d)) : 0)) -void pci_dma_configure(struct device *dev); +int pci_dma_configure(struct device *dev); /* Generic PCI functions exported to card drivers */ @@ -1604,7 +1604,7 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus, #define dev_is_pf(d) (false) #define dev_num_vf(d) (0) -static inline void pci_dma_configure(struct device *dev) { } +static inline int pci_dma_configure(struct device *dev) { return 0; } #endif /* CONFIG_PCI */