Re: [PATCH 06/10] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Sricharan R <sricharan@xxxxxxxxxxxxxx>
>> [rm: massive PCI hacks]
>> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
>> ---
>>   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

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux