Re: [PATCH 2/4] iommu/omap: Play nice in multi-platform builds

[Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>]

Hi Thierry,

On 02/04/2015 08:31 AM, Laurent Pinchart wrote:
> Hi Thierry,
> 
> Thank you for the patch.
> 
> On Wednesday 04 February 2015 08:58:08 Thierry Reding wrote:
>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> The OMAP IOMMU driver unconditionally executes code and registers a
>> struct iommu_ops with the platform bus irrespective of whether it runs
>> on an OMAP SoC or not. This causes problems in multi-platform kernels
>> where drivers for other SoCs will no longer be able to register their
>> own struct iommu_ops or even try to use a struct iommu_ops for an IOMMU
>> that obviously isn't there.
>>
>> The smallest fix I could think of is to check for the existence of any
>> OMAP IOMMU devices in the device tree and skip initialization otherwise.
>>
>> This fixes a problem on Tegra20 where the DRM driver will try to use the
>> obviously non-existent OMAP IOMMU.
>>
>> Reported-by: Nicolas Chauvet <kwizart-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
>> Cc: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
>> Cc: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/iommu/omap-iommu.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
>> index bbb7dcef02d3..e4d4f133a3b3 100644
>> --- a/drivers/iommu/omap-iommu.c
>> +++ b/drivers/iommu/omap-iommu.c
>> @@ -1377,6 +1377,9 @@ static int __init omap_iommu_init(void)
>>  	const unsigned long flags = SLAB_HWCACHE_ALIGN;
>>  	size_t align = 1 << 10; /* L2 pagetable alignement */
>>
>> +	if (!of_find_matching_node(NULL, omap_iommu_of_match))
>> +		return 0;
>> +

Need to call the of_node_put on the cases it succeeds right, you should
change this to the semantics you used in the arm-smmu driver.

> 
> We should convert the omap-iommu driver to proper DT instantiation, but this 
> should do for now.

Agreed, and I think this check is better than the iommu_present check
used in some of the other IOMMU drivers, as that would work for a SoC
family only if that is the first one getting registered.

regards
Suman

> 
>>  	p = kmem_cache_create("iopte_cache", IOPTE_TABLE_SIZE, align, flags,
>>  			      iopte_cachep_ctor);
>>  	if (!p)
>> @@ -1394,6 +1397,9 @@ subsys_initcall(omap_iommu_init);
>>
>>  static void __exit omap_iommu_exit(void)
>>  {
>> +	if (!of_find_matching_node(NULL, omap_iommu_of_match))
>> +		return;
>> +
>>  	kmem_cache_destroy(iopte_cachep);
>>
>>  	platform_driver_unregister(&omap_iommu_driver);
> 
> The exit function will never be called as the omap-iommu driver is always 
> built-in. You could just remove the exit function.
>