lkml.org 
[lkml]   [2017]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] iommu/ipmmu-vmsa: Do not replace bus IOMMU ops on driver init.
From
Date
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>");
>>>>
>>>
>>
>

\
 
 \ /
  Last update: 2017-11-21 14:00    [W:0.115 / U:2.252 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site