Re: [PATCH v2] iommu/ipmmu-vmsa: Do not replace bus IOMMU ops on driver init.

[Robin Murphy <robin.murphy@arm.com>]

Hi Alex,

On 20/11/17 22:01, Alex Williamson wrote:
> On Mon, 20 Nov 2017 14:25:14 +0000
> Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> 
>> On Fri, Oct 13, 2017 at 04:48:45PM +0100, Robin Murphy wrote:
>>> Hi Joerg,
>>
>> Hi,
>>
>>>
>>> On 20/09/17 15:13, Liviu Dudau wrote:
>>>> If the IPMMU driver is compiled in the kernel it will replace the
>>>> platform bus IOMMU ops on running the ipmmu_init() function, regardless
>>>> if there is any IPMMU hardware present or not. This screws up systems
>>>> that just want to build a generic kernel that runs on multiple platforms
>>>> and use a different IOMMU implementation.
>>>>
>>>> Move the bus_set_iommu() call at the end of the ipmmu_probe() function
>>>> when we know that hardware is present. With current IOMMU framework it
>>>> should be safe (at least for OF case).
>>>>
>>>> Now that the ipmmu_init() and ipmmu_exit() functions are simple calls to
>>>> platform_driver_register() and platform_driver_unregister(), replace
>>>> them with the module_platform_driver() macro call.
>>>
>>> Are you OK with taking this patch as a fix for 4.14, or would you rather
>>> have something that can safely backport past 4.12 without implicit
>>> dependencies? This is a config/link-order dependent thing that's been
>>> lurking since the beginning, but only coming to light now that other
>>> drivers are changing their behaviour, so I don't think there's really a
>>> single Fixes: commit that can be singled out.
>>
>> Can someone update me on the fate of this patch? Can someone queue it
>> for the next release?
> 
> Sorry, this is another patch that wasn't on my radar while Joerg is out
> on paternity leave.  I didn't follow the replies to Laurent's question
> about ordering and perhaps this plays in to Robin asking about fixes
> for specific kernel versions.  It seems there are some changes
> elsewhere that somehow defer the ordering problem or don't matter on an
> Arm Juno board (whatever that is).  Can someone explain?

To clarify, the ipmmu-vmsa driver is not enabled in the arm64 defconfig, 
but turning it on causes crashes on non-Renesas platforms with different 
IOMMUs (e.g. the Juno dev board[1] which has ARM SMMUs), because it 
still relies on initcalls to initialise the IOMMU before its masters, 
whereas other drivers like arm-smmu have now transitioned to using the 
probe-deferral mechanism. Back when everything ran off initcalls, 
arm-smmu would get there first (thanks to link order) and we never saw a 
problem, but since the ipmmu-vmsa initcall doesn't check whether any 
IPMMU device is actually present in the system before grabbing the bus 
ops, it now breaks other drivers' probe-time setup.

>  If there's a
> desire for a stable tag for this, it seems like we need to know
> explicitly the range where it's safe to apply.  Also, the patch needs
> to be updated and re-evaluated in the presence of:
> 
> cda52fcd999f iommu/ipmmu-vmsa: Make use of IOMMU_OF_DECLARE()

Actually, I overlooked it the first time, but Liviu's patch did in fact 
need an IOMMU_OF_DECLARE() in order to correctly trigger probe-deferral 
and avoid Laurent's concerns. However, cda52fcd999f does now mostly 
supersede it (I've checked that the arm64 case is OK; 32-bit 
multiplatform kernels might possibly still be broken, but how much 
anyone's relying on IOMMU support in those I don't know).

Robin.

[1]:https://developer.arm.com/products/system-design/development-boards/juno-development-board

> 
> Thanks,
> Alex
> 
>>>> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
>>>> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>>> ---
>>>>   drivers/iommu/ipmmu-vmsa.c | 29 +++++------------------------
>>>>   1 file changed, 5 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
>>>> index 195d6e93ac718..31912997bffdf 100644
>>>> --- a/drivers/iommu/ipmmu-vmsa.c
>>>> +++ b/drivers/iommu/ipmmu-vmsa.c
>>>> @@ -966,10 +966,11 @@ static int ipmmu_probe(struct platform_device *pdev)
>>>>   		return ret;
>>>>   
>>>>   	/*
>>>> -	 * We can't create the ARM mapping here as it requires the bus to have
>>>> -	 * an IOMMU, which only happens when bus_set_iommu() is called in
>>>> -	 * ipmmu_init() after the probe function returns.
>>>> +	 * Now that we have validated the presence of the hardware, set
>>>> +	 * the bus IOMMU ops to enable future domain and device setup.
>>>>   	 */
>>>> +	if (!iommu_present(&platform_bus_type))
>>>> +		bus_set_iommu(&platform_bus_type, &ipmmu_ops);
>>>>   
>>>>   	platform_set_drvdata(pdev, mmu);
>>>>   
>>>> @@ -1006,27 +1007,7 @@ static struct platform_driver ipmmu_driver = {
>>>>   	.remove	= ipmmu_remove,
>>>>   };
>>>>   
>>>> -static int __init ipmmu_init(void)
>>>> -{
>>>> -	int ret;
>>>> -
>>>> -	ret = platform_driver_register(&ipmmu_driver);
>>>> -	if (ret < 0)
>>>> -		return ret;
>>>> -
>>>> -	if (!iommu_present(&platform_bus_type))
>>>> -		bus_set_iommu(&platform_bus_type, &ipmmu_ops);
>>>> -
>>>> -	return 0;
>>>> -}
>>>> -
>>>> -static void __exit ipmmu_exit(void)
>>>> -{
>>>> -	return platform_driver_unregister(&ipmmu_driver);
>>>> -}
>>>> -
>>>> -subsys_initcall(ipmmu_init);
>>>> -module_exit(ipmmu_exit);
>>>> +module_platform_driver(ipmmu_driver);
>>>>   
>>>>   MODULE_DESCRIPTION("IOMMU API for Renesas VMSA-compatible IPMMU");
>>>>   MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
>>>>    
>>>    
>>
>