From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Date: Tue, 16 May 2017 12:47:32 +0300 From: Sakari Ailus To: Laurent Pinchart Subject: Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error Message-ID: <20170516094732.GM3227@valkosipuli.retiisi.org.uk> References: <1486136933-20328-1-git-send-email-sricharan@codeaurora.org> <1804176.uxVOFWagsb@avalon> <9952978e7ac69193daf67ccb77eebe29@codeaurora.org> <1924197.MWBQ7kvoOo@avalon> MIME-Version: 1.0 In-Reply-To: <1924197.MWBQ7kvoOo@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: okaya@codeaurora.org, Lorenzo Pieralisi , linux-arm-msm@vger.kernel.org, Joerg Roedel , Magnus Damm , Robin Murphy , Will Deacon , Linux-Renesas , ACPI Devel Maling List , iommu@lists.linux-foundation.org, Geert Uytterhoeven , Hanjun Guo , linux-pci , Bjorn Helgaas , tn@semihalf.com, sricharan@codeaurora.org, linux-arm-msm-owner@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Marek Szyprowski Content-Type: text/plain; charset="us-ascii" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+bjorn=helgaas.com@lists.infradead.org List-ID: Hi Laurent, On Tue, May 16, 2017 at 10:17:08AM +0300, Laurent Pinchart wrote: > Hi Sricharan, > > On Tuesday 16 May 2017 07:53:57 sricharan@codeaurora.org wrote: > > On 2017-05-16 03:04, Laurent Pinchart wrote: > > > On Monday 15 May 2017 23:37:16 Laurent Pinchart wrote: > > >> On Wednesday 03 May 2017 15:54:59 Sricharan R wrote: > > >>> On 5/3/2017 3:24 PM, Robin Murphy wrote: > > >>>> On 02/05/17 19:35, Geert Uytterhoeven wrote: > > >>>>> On Fri, Feb 3, 2017 at 4:48 PM, Sricharan R wrote: > > >>>>>> From: Laurent Pinchart > > >>>>>> > > >>>>>> 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. > > >>>>>> > > >>>>>> Tested-by: Marek Szyprowski > > >>>>>> Signed-off-by: Laurent Pichart > > >>>>>> > > >>>>>> Signed-off-by: Sricharan R > > >>>>> > > >>>>> This patch broke Renesas R-Car Gen3 platforms in renesas-drivers. > > >>>>> As the IOMMU nodes in DT are not yet enabled, all devices having > > >>>>> iommus properties in DT now fail to probe. > > >>>> > > >>>> How exactly do they fail to probe? Per d7b0558230e4, if there are no > > >>>> ops registered then they should merely defer until we reach the point > > >>>> of giving up and ignoring the IOMMU. Is it just that you have no other > > >>>> late-probing drivers or post-init module loads to kick the deferred > > >>>> queue after that point? I did try to find a way to explicitly kick it > > >>>> from a suitably late initcall, but there didn't seem to be any obvious > > >>>> public interface - anyone have any suggestions? > > >>>> > > >>>> I think that's more of a general problem with the probe deferral > > >>>> mechanism itself (I've seen the same thing happen with some of the > > >>>> CoreSight stuff on Juno due to the number of inter-component > > >>>> dependencies) rather than any specific fault of this series. > > >>> > > >>> I was thinking of an additional check like below to avoid the > > >>> situation ? > > >>> > > >>> From 499b6e662f60f23740b8880882b0a16f16434501 Mon Sep 17 00:00:00 2001 > > >>> From: Sricharan R > > >>> Date: Wed, 3 May 2017 13:16:59 +0530 > > >>> Subject: [PATCH] iommu: of: Fix check for returning EPROBE_DEFER > > >>> > > >>> While returning EPROBE_DEFER for iommu masters > > >>> take in to account of iommu nodes that could be > > >>> marked in DT as 'status=disabled', in which case > > >>> simply return NULL and let the master's probe > > >>> continue rather than deferring. > > >>> > > >>> Signed-off-by: Sricharan R > > >>> --- > > >>> > > >>> drivers/iommu/of_iommu.c | 1 + > > >>> 1 file changed, 1 insertion(+) > > >>> > > >>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > > >>> index 9f44ee8..e6e9bec 100644 > > >>> --- a/drivers/iommu/of_iommu.c > > >>> +++ b/drivers/iommu/of_iommu.c > > >>> @@ -118,6 +118,7 @@ static bool of_iommu_driver_present(struct > > >>> device_node *np) > > >>> > > >>> ops = iommu_ops_from_fwnode(fwnode); > > >>> if ((ops && !ops->of_xlate) || > > >>> + !of_device_is_available(iommu_spec->np) || > > >>> (!ops && !of_iommu_driver_present(iommu_spec->np))) > > >>> return NULL; > > >> > > >> This looks good to me, but won't be enough. The ipmmu-vmsa driver in > > >> v4.12-rc1 doesn't call iommu_device_register() and thus won't be found > > >> by iommu_ops_from_fwnode(). Furthermore, it doesn't IOMMU_OF_DECLARE(), > > >> and thus will always be considered as absent. > > >> > > >> I agree that the ipmmu-vmsa driver needs to be fixed, but it would > > >> have been nice to check existing IOMMU drivers before merging this patch > > >> series... > > > > > > Please pardon the question, but has this patch series been tested on > > > ARM32 ? > > > > > > When the device is probed the arch_setup_dma_ops() function is called. > > > It sets the device's dma_ops and the mapping (in > > > __arm_iommu_attach_device()). If probe is deferred, > > > arch_teardown_dma_ops() is called which in turn calls > > > arch_teardown_dma_ops(). This removes the mapping but doesn't touch the > > > dma_ops. The next time the device is probed, arch_setup_dma_ops() bails > > > out immediately as the dma_ops are already set, leaving us with a device > > > bound to IOMMU operations but with no mapping. This oopses later as soon > > > as the kernel tries to map memory for the device through the IOMMU. > > > > Resetting the dma_ops for arm32 was added in this patch [1], which I > > missed to send in the original series, but now have added to Russell's patch > > tracking system. > > Thank you. I fear that won't be enough though. > > > [1] https://patchwork.kernel.org/patch/9434105/ > > Quoting the patch: > > > arch_teardown_dma_ops() being the inverse of arch_setup_dma_ops() > > ,dma_ops should be cleared in the teardown path. Otherwise > > this causes problem when the probe of device is retried after > > being deferred. The device's iommu structures are cleared > > after EPROBEDEFER error, but on the next try dma_ops will still > > be set to old value, which is not right. > > > > Signed-off-by: Sricharan R > > Reviewed-by: Robin Murphy > > --- > > arch/arm/mm/dma-mapping.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > > index ab4f745..a40f03e 100644 > > --- a/arch/arm/mm/dma-mapping.c > > +++ b/arch/arm/mm/dma-mapping.c > > @@ -2358,6 +2358,7 @@ static void arm_teardown_iommu_dma_ops(struct device > *dev) > > __arm_iommu_detach_device(dev); > > arm_iommu_release_mapping(mapping); > > + set_dma_ops(dev, NULL); > > } > > #else > > The subject mentions arch_teardown_dma_ops(), which I think is correct, but > the patch adds the set_dma_ops() call to arm_teardown_iommu_dma_ops(). > > However, the situation is perhaps more complex. Note the check at the > beginning of arch_setup_dma_ops(): > > /* > * Don't override the dma_ops if they have already been set. Ideally > * this should be the only location where dma_ops are set, remove this > * check when all other callers of set_dma_ops will have disappeared. > */ > if (dev->dma_ops) > return; > > If you set the dma_ops to NULL in arm_teardown_iommu_dma_ops() or > arch_teardown_dma_ops(), the next call to arch_setup_dma_ops() will override > them. To be safe you should only set them to NULL if they have been set by > arch_setup_dma_ops(). More than that, arch_teardown_dma_ops() should probably > not call arm_teardown_iommu_dma_ops() at all if the dma_ops were set by > arm_iommu_attach_device() and not arch_teardown_dma_ops(). One option would be > to add a field to struct dev_archdata to store that information. To avoid > growing the structure, which is embedded in every struct device, you could > possibly turn the dma_coherent bool into a bitfield. > > @@ -19,7 +19,8 @@ struct dev_archdata { > #ifdef CONFIG_XEN > const struct dma_map_ops *dev_dma_ops; > #endif > - bool dma_coherent; > + bool dma_coherent:1; > + bool dma_ops_setup:1; > }; > > struct omap_device; > > I haven't checked, however, whether the dma_coherent field would need to be > accessed atomically, so this might be a bad idea. A bool bit field? :-) I think I'd just use bool for both. I wouldn't expect dma_coherent change once it has been set before device driver probe though. If you like a bit field, then I'd propose making it unsigned int. -- Regards, Sakari Ailus e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel