From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752209AbdKUM7l (ORCPT ); Tue, 21 Nov 2017 07:59:41 -0500 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:39920 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752021AbdKUM7k (ORCPT ); Tue, 21 Nov 2017 07:59:40 -0500 Subject: Re: [PATCH v2] iommu/ipmmu-vmsa: Do not replace bus IOMMU ops on driver init. To: Alex Williamson , Liviu Dudau Cc: Laurent Pinchart , Geert Uytterhoeven , Magnus Damm , LKML , IOMMU ML , Shawn Lin References: <20170918100444.21878-1-Liviu.Dudau@arm.com> <20170920141352.29377-1-Liviu.Dudau@arm.com> <938d213c-f207-218e-554f-08035eaa1e6d@arm.com> <20171120142514.GI5165@e110455-lin.cambridge.arm.com> <20171120150144.13390f70@t450s.home> From: Robin Murphy Message-ID: <4264fbdc-29dd-8d76-30cb-cc23c775c6f1@arm.com> Date: Tue, 21 Nov 2017 12:59:37 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171120150144.13390f70@t450s.home> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Alex, On 20/11/17 22:01, Alex Williamson wrote: > On Mon, 20 Nov 2017 14:25:14 +0000 > Liviu Dudau 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 >>>> Cc: Laurent Pinchart >>>> --- >>>> 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 "); >>>> >>> >> >