* Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
2017-05-16 7:17 ` Laurent Pinchart
@ 2017-05-16 9:47 ` Sakari Ailus
2017-05-16 13:40 ` sricharan
2017-05-16 14:04 ` Robin Murphy
2 siblings, 0 replies; 26+ messages in thread
From: Sakari Ailus @ 2017-05-16 9:47 UTC (permalink / raw)
To: Laurent Pinchart
Cc: sricharan, Linux-Renesas, Lorenzo Pieralisi, Magnus Damm,
linux-arm-msm, Joerg Roedel, Will Deacon, okaya,
ACPI Devel Maling List, iommu, Geert Uytterhoeven, Hanjun Guo,
linux-pci, Bjorn Helgaas, tn, Robin Murphy, linux-arm-msm-owner,
linux-arm-kernel, Marek Szyprowski
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 <laurent.pinchart+renesas@ideasonboard.com>
> > >>>>>>
> > >>>>>> 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 <m.szyprowski@samsung.com>
> > >>>>>> Signed-off-by: Laurent Pichart
> > >>>>>> <laurent.pinchart+renesas@ideasonboard.com>
> > >>>>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> > >>>>>
> > >>>>> 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 <sricharan@codeaurora.org>
> > >>> 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 <sricharan@codeaurora.org>
> > >>> ---
> > >>>
> > >>> 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 <sricharan@codeaurora.org>
> > Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> > ---
> > 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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
2017-05-16 7:17 ` Laurent Pinchart
2017-05-16 9:47 ` Sakari Ailus
@ 2017-05-16 13:40 ` sricharan
2017-05-16 14:06 ` Laurent Pinchart
2017-05-16 14:04 ` Robin Murphy
2 siblings, 1 reply; 26+ messages in thread
From: sricharan @ 2017-05-16 13:40 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Linux-Renesas, Lorenzo Pieralisi, Magnus Damm, linux-arm-msm,
Joerg Roedel, Will Deacon, okaya, ACPI Devel Maling List, iommu,
Geert Uytterhoeven, Hanjun Guo, linux-pci, Bjorn Helgaas, tn,
Robin Murphy, linux-arm-msm-owner, linux-arm-kernel,
Marek Szyprowski
Hi Laurent,
On 2017-05-16 12:47, 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 <laurent.pinchart+renesas@ideasonboard.com>
>> >>>>>>
>> >>>>>> 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 <m.szyprowski@samsung.com>
>> >>>>>> Signed-off-by: Laurent Pichart
>> >>>>>> <laurent.pinchart+renesas@ideasonboard.com>
>> >>>>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
>> >>>>>
>> >>>>> 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 <sricharan@codeaurora.org>
>> >>> 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 <sricharan@codeaurora.org>
>> >>> ---
>> >>>
>> >>> 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 <sricharan@codeaurora.org>
>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>> 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.
>
> Last but not least, a fix must be merged in v4.12, and the sooner the
> better.
>
ho, yet another combination. This seems to be a problem with
exynos_iommu,
ipmmu-vmsa, mtk_iommu_v1 which calls the arm_iommu_attach_device with
its
own custom mapping. They are calling arm_iommu_attach_device from the
add_device callback and that is not always replayed when the reprobe
happens
and these archs are storing the old mapping data in private structures
which
might not be cleared in the teardown path. I will post the fix that you
have
suggested.
Regards,
Sricharan
>> > I might be missing something obvious, but I don't see how this can
>> > work.
>> >
>> >>>>> This can be fixed by either:
>> >>>>> - Disabling CONFIG_IPMMU_VMSA, or
>> >>>>> - Reverting commit 7b07cbefb68d486f (but keeping "int ret = 0;").
>> >>>>>
>> >>>>> Note that this was a bit hard to investigate, as R-Car Gen3 support
>> >>>>> wasn't upstreamed yet, so bisection pointed to a merge commit.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
2017-05-16 13:40 ` sricharan
@ 2017-05-16 14:06 ` Laurent Pinchart
0 siblings, 0 replies; 26+ messages in thread
From: Laurent Pinchart @ 2017-05-16 14:06 UTC (permalink / raw)
To: sricharan
Cc: Linux-Renesas, Lorenzo Pieralisi, Magnus Damm, linux-arm-msm,
Joerg Roedel, Will Deacon, okaya, ACPI Devel Maling List, iommu,
Geert Uytterhoeven, Hanjun Guo, linux-pci, Bjorn Helgaas, tn,
Robin Murphy, linux-arm-msm-owner, linux-arm-kernel,
Marek Szyprowski
Hi Sricharan,
On Tuesday 16 May 2017 19:10:03 sricharan@codeaurora.org wrote:
> On 2017-05-16 12:47, Laurent Pinchart wrote:
> > 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 <laurent.pinchart+renesas@ideasonboard.com>
> >>>>>>>>
> >>>>>>>> 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 <m.szyprowski@samsung.com>
> >>>>>>>> Signed-off-by: Laurent Pichart
> >>>>>>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>>>>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> >>>>>>>
> >>>>>>> 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 <sricharan@codeaurora.org>
> >>>>> 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 <sricharan@codeaurora.org>
> >>>>> ---
> >>>>>
> >>>>> 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 <sricharan@codeaurora.org>
> >> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> >> ---
> >>
> >> 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.
> >
> > Last but not least, a fix must be merged in v4.12, and the sooner the
> > better.
>
> ho, yet another combination. This seems to be a problem with exynos_iommu,
> ipmmu-vmsa, mtk_iommu_v1 which calls the arm_iommu_attach_device with its
> own custom mapping. They are calling arm_iommu_attach_device from the
> add_device callback and that is not always replayed when the reprobe happens
> and these archs are storing the old mapping data in private structures which
> might not be cleared in the teardown path.
Yes, I know, it's messy :-/ There's a handful of non-IOMMU drivers calling
arm_iommu_attach_device() directly too. All these should be fixed, but in the
meantime, let's try not to break them.
> I will post the fix that you have suggested.
Thank you. You might want to use an unsigned int bitfield instead of a bool
bitfield as Sakari suggested. It would be nice to check the code setting the
dma_coherent field to make sure there will be no race with code setting the
new dma_ops_setup field (which might not be the best name, feel free to rename
it).
I have successfully test the patch, let me know if there's anything else I can
do to help.
> >>> I might be missing something obvious, but I don't see how this can
> >>> work.
> >>>
> >>>>>>> This can be fixed by either:
> >>>>>>> - Disabling CONFIG_IPMMU_VMSA, or
> >>>>>>> - Reverting commit 7b07cbefb68d486f (but keeping "int ret = 0;").
> >>>>>>>
> >>>>>>> Note that this was a bit hard to investigate, as R-Car Gen3 support
> >>>>>>> wasn't upstreamed yet, so bisection pointed to a merge commit.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
2017-05-16 7:17 ` Laurent Pinchart
2017-05-16 9:47 ` Sakari Ailus
2017-05-16 13:40 ` sricharan
@ 2017-05-16 14:04 ` Robin Murphy
2017-05-16 14:10 ` Laurent Pinchart
2 siblings, 1 reply; 26+ messages in thread
From: Robin Murphy @ 2017-05-16 14:04 UTC (permalink / raw)
To: Laurent Pinchart, sricharan
Cc: Geert Uytterhoeven, Will Deacon, Joerg Roedel, Lorenzo Pieralisi,
iommu, linux-arm-kernel, linux-arm-msm, Marek Szyprowski,
Bjorn Helgaas, linux-pci, ACPI Devel Maling List, tn, Hanjun Guo,
okaya, Magnus Damm, Linux-Renesas, linux-arm-msm-owner
Hi Laurent,
On 16/05/17 08:17, 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 <laurent.pinchart+renesas@ideasonboard.com>
>>>>>>>>
>>>>>>>> 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 <m.szyprowski@samsung.com>
>>>>>>>> Signed-off-by: Laurent Pichart
>>>>>>>> <laurent.pinchart+renesas@ideasonboard.com>
>>>>>>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
>>>>>>>
>>>>>>> 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 <sricharan@codeaurora.org>
>>>>> 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 <sricharan@codeaurora.org>
>>>>> ---
>>>>>
>>>>> 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 <sricharan@codeaurora.org>
>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>> 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().
Under what circumstances is that an issue? We'll only be tearing down
the DMA ops when unbinding the driver, and I think it would be erroneous
to expect the device to retain much state after that. Everything else
would be set up from scratch again if it get reprobed later, so why not
the DMA ops?
Robin.
> 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.
>
> Last but not least, a fix must be merged in v4.12, and the sooner the better.
>
>>> I might be missing something obvious, but I don't see how this can
>>> work.
>>>
>>>>>>> This can be fixed by either:
>>>>>>> - Disabling CONFIG_IPMMU_VMSA, or
>>>>>>> - Reverting commit 7b07cbefb68d486f (but keeping "int ret = 0;").
>>>>>>>
>>>>>>> Note that this was a bit hard to investigate, as R-Car Gen3 support
>>>>>>> wasn't upstreamed yet, so bisection pointed to a merge commit.
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
2017-05-16 14:04 ` Robin Murphy
@ 2017-05-16 14:10 ` Laurent Pinchart
2017-05-16 14:29 ` sricharan
2017-05-16 14:52 ` Robin Murphy
0 siblings, 2 replies; 26+ messages in thread
From: Laurent Pinchart @ 2017-05-16 14:10 UTC (permalink / raw)
To: Robin Murphy
Cc: sricharan, Geert Uytterhoeven, Will Deacon, Joerg Roedel,
Lorenzo Pieralisi, iommu, linux-arm-kernel, linux-arm-msm,
Marek Szyprowski, Bjorn Helgaas, linux-pci,
ACPI Devel Maling List, tn, Hanjun Guo, okaya, Magnus Damm,
Linux-Renesas, linux-arm-msm-owner
Hi Robin,
On Tuesday 16 May 2017 15:04:55 Robin Murphy wrote:
> On 16/05/17 08:17, Laurent Pinchart wrote:
> > On Tuesday 16 May 2017 07:53:57 sricharan@codeaurora.org wrote:
[snip]
> >> 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 <sricharan@codeaurora.org>
> >> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> >> ---
> >>
> >> 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().
>
> Under what circumstances is that an issue? We'll only be tearing down
> the DMA ops when unbinding the driver,
Or when deferring probe.
> and I think it would be erroneous to expect the device to retain much state
> after that. Everything else would be set up from scratch again if it get
> reprobed later, so why not the DMA ops?
Because the DMA ops might have been set elsewhere than arch_setup_dma_ops().
If you look at the patch that added the above warning, its commit message
states
commit 26b37b946a5c2658dbc37dd5d6df40aaa9685d70 (iommu-joerg/arm/core)
Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Date: Fri May 15 02:00:02 2015 +0300
arm: dma-mapping: Don't override dma_ops in arch_setup_dma_ops()
The arch_setup_dma_ops() function is in charge of setting dma_ops with a
call to set_dma_ops(). set_dma_ops() is also called from
- highbank and mvebu bus notifiers
- dmabounce (to be replaced with swiotlb)
- arm_iommu_attach_device
(arm_iommu_attach_device is itself called from IOMMU and bus master
device drivers)
To allow the arch_setup_dma_ops() call to be moved from device add time
to device probe time we must ensure that dma_ops already setup by any of
the above callers will not be overriden.
Aftering replacing dmabounce with swiotlb, converting IOMMU drivers to
of_xlate and taking care of highbank and mvebu, the workaround should be
removed.
I'm concerned about potentially breaking these if we unconditionally remove
the DMA ops and mapping.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
2017-05-16 14:10 ` Laurent Pinchart
@ 2017-05-16 14:29 ` sricharan
2017-05-16 14:46 ` Laurent Pinchart
2017-05-16 14:52 ` Robin Murphy
1 sibling, 1 reply; 26+ messages in thread
From: sricharan @ 2017-05-16 14:29 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Robin Murphy, Linux-Renesas, Lorenzo Pieralisi, Magnus Damm,
linux-arm-msm, Joerg Roedel, Will Deacon, okaya,
ACPI Devel Maling List, iommu, Geert Uytterhoeven, Hanjun Guo,
linux-pci, Bjorn Helgaas, tn, linux-arm-msm-owner,
linux-arm-kernel, Marek Szyprowski
Hi,
On 2017-05-16 19:40, Laurent Pinchart wrote:
> Hi Robin,
>
> On Tuesday 16 May 2017 15:04:55 Robin Murphy wrote:
>> On 16/05/17 08:17, Laurent Pinchart wrote:
>> > On Tuesday 16 May 2017 07:53:57 sricharan@codeaurora.org wrote:
>
> [snip]
>
>> >> 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 <sricharan@codeaurora.org>
>> >> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>> >> ---
>> >>
>> >> 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().
>>
>> Under what circumstances is that an issue? We'll only be tearing down
>> the DMA ops when unbinding the driver,
>
> Or when deferring probe.
>
>> and I think it would be erroneous to expect the device to retain much
>> state
>> after that. Everything else would be set up from scratch again if it
>> get
>> reprobed later, so why not the DMA ops?
>
> Because the DMA ops might have been set elsewhere than
> arch_setup_dma_ops().
> If you look at the patch that added the above warning, its commit
> message
> states
>
> commit 26b37b946a5c2658dbc37dd5d6df40aaa9685d70 (iommu-joerg/arm/core)
> Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Date: Fri May 15 02:00:02 2015 +0300
>
> arm: dma-mapping: Don't override dma_ops in arch_setup_dma_ops()
>
> The arch_setup_dma_ops() function is in charge of setting dma_ops
> with a
> call to set_dma_ops(). set_dma_ops() is also called from
>
> - highbank and mvebu bus notifiers
> - dmabounce (to be replaced with swiotlb)
> - arm_iommu_attach_device
>
> (arm_iommu_attach_device is itself called from IOMMU and bus master
> device drivers)
>
> To allow the arch_setup_dma_ops() call to be moved from device add
> time
> to device probe time we must ensure that dma_ops already setup by
> any of
> the above callers will not be overriden.
>
> Aftering replacing dmabounce with swiotlb, converting IOMMU drivers
> to
> of_xlate and taking care of highbank and mvebu, the workaround
> should be
> removed.
>
> I'm concerned about potentially breaking these if we unconditionally
> remove
> the DMA ops and mapping.
arch_teardown_dma_ops does nothing if there is
no mapping (not behind iommu), dma_ops without iommu is ok.
But when the arm_iommu_create_mapping/arm_iommu_attach_device
was called previously in the iommu driver, after we teardown,
that path in the iommu driver which called those functions is not
replayed.
Regards,
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
2017-05-16 14:29 ` sricharan
@ 2017-05-16 14:46 ` Laurent Pinchart
0 siblings, 0 replies; 26+ messages in thread
From: Laurent Pinchart @ 2017-05-16 14:46 UTC (permalink / raw)
To: sricharan
Cc: Robin Murphy, Linux-Renesas, Lorenzo Pieralisi, Magnus Damm,
linux-arm-msm, Joerg Roedel, Will Deacon, okaya,
ACPI Devel Maling List, iommu, Geert Uytterhoeven, Hanjun Guo,
linux-pci, Bjorn Helgaas, tn, linux-arm-msm-owner,
linux-arm-kernel, Marek Szyprowski
Hi Sricharan,
On Tuesday 16 May 2017 19:59:01 sricharan@codeaurora.org wrote:
> On 2017-05-16 19:40, Laurent Pinchart wrote:
> > On Tuesday 16 May 2017 15:04:55 Robin Murphy wrote:
> >> On 16/05/17 08:17, Laurent Pinchart wrote:
> >> > On Tuesday 16 May 2017 07:53:57 sricharan@codeaurora.org wrote:
> > [snip]
> >
> >>>> 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 <sricharan@codeaurora.org>
> >>>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> >>>> ---
> >>>>
> >>>> 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().
> >>
> >> Under what circumstances is that an issue? We'll only be tearing down
> >> the DMA ops when unbinding the driver,
> >
> > Or when deferring probe.
> >
> >> and I think it would be erroneous to expect the device to retain much
> >> state after that. Everything else would be set up from scratch again if
> >> it get reprobed later, so why not the DMA ops?
> >
> > Because the DMA ops might have been set elsewhere than
> > arch_setup_dma_ops(). If you look at the patch that added the above
> > warning, its commit message states
> >
> > commit 26b37b946a5c2658dbc37dd5d6df40aaa9685d70 (iommu-joerg/arm/core)
> > Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Date: Fri May 15 02:00:02 2015 +0300
> >
> > arm: dma-mapping: Don't override dma_ops in arch_setup_dma_ops()
> >
> > The arch_setup_dma_ops() function is in charge of setting dma_ops
> > with a call to set_dma_ops(). set_dma_ops() is also called from
> >
> > - highbank and mvebu bus notifiers
> > - dmabounce (to be replaced with swiotlb)
> > - arm_iommu_attach_device
> >
> > (arm_iommu_attach_device is itself called from IOMMU and bus master
> > device drivers)
> >
> > To allow the arch_setup_dma_ops() call to be moved from device add
> > time to device probe time we must ensure that dma_ops already setup by
> > any of the above callers will not be overriden.
> >
> > Aftering replacing dmabounce with swiotlb, converting IOMMU drivers
> > to of_xlate and taking care of highbank and mvebu, the workaround
> > should be removed.
> >
> > I'm concerned about potentially breaking these if we unconditionally
> > remove the DMA ops and mapping.
>
> arch_teardown_dma_ops does nothing if there is no mapping (not behind
> iommu), dma_ops without iommu is ok. But when the
> arm_iommu_create_mapping/arm_iommu_attach_device was called previously in
> the iommu driver, after we teardown, that path in the iommu driver which
> called those functions is not replayed.
I've had a look at the code in more details, and I'm not sure how we've
reached the current situation (as I haven't followed the multiple versions of
this patch series due to lack of time), but it's a very big mess.
arch_setup_dma_ops() is currently not the only way to create a mapping and
attach it to a device. Not only that, but with the current ARM32 dma-mapping
implementation, it can't be.
arch_setup_dma_ops() will create a separate mapping for every device. That's
certainly fine when every device has its own IOMMU instance (or at least its
own TLB in a shared IOMMU), but that's far from being true in all cases.
Renesas R-Car Gen2 hardware, for instance, share IOMMUs and TLBs between
multiple devices. That's why the ipmmu-vmsa driver creates the mapping
manually with arm_iommu_create_mapping() and attaches it to the device with
arm_iommu_attach_device(). We need to keep supporting this mechanism until the
ARM32 dma-mapping API is brought in line with the ARM64 implementation that
lets the IOMMU core manage the IOVA address space.
We're unfortunately far from that, so, for the time being, we need to keep
supporting arm_iommu_attach_device(). How that can work with deferred probing,
I will let you figure it out, but we need to at least fix the breakage for
v4.12.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
2017-05-16 14:10 ` Laurent Pinchart
2017-05-16 14:29 ` sricharan
@ 2017-05-16 14:52 ` Robin Murphy
2017-05-16 15:14 ` [PATCH] ARM: dma-mapping: Don't tear third-party mappings Laurent Pinchart
1 sibling, 1 reply; 26+ messages in thread
From: Robin Murphy @ 2017-05-16 14:52 UTC (permalink / raw)
To: Laurent Pinchart
Cc: sricharan, Geert Uytterhoeven, Will Deacon, Joerg Roedel,
Lorenzo Pieralisi, iommu, linux-arm-kernel, linux-arm-msm,
Marek Szyprowski, Bjorn Helgaas, linux-pci,
ACPI Devel Maling List, tn, Hanjun Guo, okaya, Magnus Damm,
Linux-Renesas, linux-arm-msm-owner
On 16/05/17 15:10, Laurent Pinchart wrote:
> Hi Robin,
>
> On Tuesday 16 May 2017 15:04:55 Robin Murphy wrote:
>> On 16/05/17 08:17, Laurent Pinchart wrote:
>>> On Tuesday 16 May 2017 07:53:57 sricharan@codeaurora.org wrote:
>
> [snip]
>
>>>> 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 <sricharan@codeaurora.org>
>>>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>>>> ---
>>>>
>>>> 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().
>>
>> Under what circumstances is that an issue? We'll only be tearing down
>> the DMA ops when unbinding the driver,
>
> Or when deferring probe.
>
>> and I think it would be erroneous to expect the device to retain much state
>> after that. Everything else would be set up from scratch again if it get
>> reprobed later, so why not the DMA ops?
>
> Because the DMA ops might have been set elsewhere than arch_setup_dma_ops().
> If you look at the patch that added the above warning, its commit message
> states
>
> commit 26b37b946a5c2658dbc37dd5d6df40aaa9685d70 (iommu-joerg/arm/core)
> Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Date: Fri May 15 02:00:02 2015 +0300
>
> arm: dma-mapping: Don't override dma_ops in arch_setup_dma_ops()
>
> The arch_setup_dma_ops() function is in charge of setting dma_ops with a
> call to set_dma_ops(). set_dma_ops() is also called from
>
> - highbank and mvebu bus notifiers
> - dmabounce (to be replaced with swiotlb)
> - arm_iommu_attach_device
>
> (arm_iommu_attach_device is itself called from IOMMU and bus master
> device drivers)
>
> To allow the arch_setup_dma_ops() call to be moved from device add time
> to device probe time we must ensure that dma_ops already setup by any of
> the above callers will not be overriden.
>
> Aftering replacing dmabounce with swiotlb, converting IOMMU drivers to
> of_xlate and taking care of highbank and mvebu, the workaround should be
> removed.
>
> I'm concerned about potentially breaking these if we unconditionally remove
> the DMA ops and mapping.
Ah, sorry, I see now - it was taking a long time to page the 32-bit code
back in, and I'd forgotten the specifics of the mess of competing
"default domain" notions. Indeed, it's not the device's driver expecting
any state to be preserved as I got stuck on, it's the IOMMU driver,
which does "know better" to an extent, expecting its changes to the
struct device to stick for the lifetime of that structure.
I agree there shouldn't be a disparity - arch_setup_dma_ops() only does
things given certain circumstances, so arch_teardown_dma_ops() should
only undo them under the same. With probe-deferral in place I'll be
reviving my work to convert this path over to IOMMU API default domains,
which will make some of these issues go away again, but in the meantime
I also agree that the most expedient fix is indeed to add a flag to say
whether the dma ops were automatically set or not (this is implicitly
true on arm64, which was partly what was tripping me up).
Robin.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] ARM: dma-mapping: Don't tear third-party mappings
2017-05-16 14:52 ` Robin Murphy
@ 2017-05-16 15:14 ` Laurent Pinchart
2017-05-16 15:47 ` Robin Murphy
0 siblings, 1 reply; 26+ messages in thread
From: Laurent Pinchart @ 2017-05-16 15:14 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Sricharan R, Robin Murphy, Joerg Roedel, Geert Uytterhoeven,
Will Deacon, iommu, linux-renesas-soc
arch_setup_dma_ops() is used in device probe code paths to create an
IOMMU mapping and attach it to the device. The function assumes that the
device is attached to a device-specific IOMMU instance (or at least a
device-specific TLB in a shared IOMMU instance) and thus creates a
separate mapping for every device.
On several systems (Renesas R-Car Gen2 being one of them), that
assumption is not true, and IOMMU mappings must be shared between
multiple devices. In those cases the IOMMU driver knows better than the
generic ARM dma-mapping layer and attaches mapping to devices manually
with arm_iommu_attach_device(), which sets the DMA ops for the device.
The arch_setup_dma_ops() function takes this into account and bails out
immediately if the device already has DMA ops assigned. However, the
corresponding arch_teardown_dma_ops() function, called from driver
unbind code paths (including probe deferral), will tear the mapping down
regardless of who created it. When the device is reprobed
arch_setup_dma_ops() will be called again but won't perform any
operation as the DMA ops will still be set.
We need to reset the DMA ops in arch_teardown_dma_ops() to fix this.
However, we can't do so unconditionally, as then a new mapping would be
created by arch_setup_dma_ops() when the device is reprobed, regardless
of whether the device needs to share a mapping or not. We must thus keep
track of whether arch_setup_dma_ops() created the mapping, and only in
that case tear it down in arch_teardown_dma_ops().
Keep track of that information in the dev_archdata structure. As the
structure is embedded in all instances of struct device let's not grow
it, but turn the existing dma_coherent bool field into a bitfield that
can be used for other purposes.
Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with deferred probing or error")
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
arch/arm/include/asm/device.h | 3 ++-
arch/arm/mm/dma-mapping.c | 5 +++++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
index 36ec9c8f6e16..3234fe9bba6e 100644
--- a/arch/arm/include/asm/device.h
+++ b/arch/arm/include/asm/device.h
@@ -19,7 +19,8 @@ struct dev_archdata {
#ifdef CONFIG_XEN
const struct dma_map_ops *dev_dma_ops;
#endif
- bool dma_coherent;
+ unsigned int dma_coherent:1;
+ unsigned int dma_ops_setup:1;
};
struct omap_device;
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index c742dfd2967b..e0272f9140e2 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2430,9 +2430,14 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
dev->dma_ops = xen_dma_ops;
}
#endif
+ dev->archdata.dma_ops_setup = true;
}
void arch_teardown_dma_ops(struct device *dev)
{
+ if (!dev->archdata.dma_ops_setup)
+ return;
+
arm_teardown_iommu_dma_ops(dev);
+ set_dma_ops(dev, NULL);
}
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH] ARM: dma-mapping: Don't tear third-party mappings
2017-05-16 15:14 ` [PATCH] ARM: dma-mapping: Don't tear third-party mappings Laurent Pinchart
@ 2017-05-16 15:47 ` Robin Murphy
2017-05-16 16:44 ` Laurent Pinchart
0 siblings, 1 reply; 26+ messages in thread
From: Robin Murphy @ 2017-05-16 15:47 UTC (permalink / raw)
To: Laurent Pinchart, linux-arm-kernel
Cc: Sricharan R, Joerg Roedel, Geert Uytterhoeven, Will Deacon, iommu,
linux-renesas-soc
On 16/05/17 16:14, Laurent Pinchart wrote:
> arch_setup_dma_ops() is used in device probe code paths to create an
> IOMMU mapping and attach it to the device. The function assumes that the
> device is attached to a device-specific IOMMU instance (or at least a
> device-specific TLB in a shared IOMMU instance) and thus creates a
> separate mapping for every device.
>
> On several systems (Renesas R-Car Gen2 being one of them), that
> assumption is not true, and IOMMU mappings must be shared between
> multiple devices. In those cases the IOMMU driver knows better than the
> generic ARM dma-mapping layer and attaches mapping to devices manually
> with arm_iommu_attach_device(), which sets the DMA ops for the device.
>
> The arch_setup_dma_ops() function takes this into account and bails out
> immediately if the device already has DMA ops assigned. However, the
> corresponding arch_teardown_dma_ops() function, called from driver
> unbind code paths (including probe deferral), will tear the mapping down
> regardless of who created it. When the device is reprobed
> arch_setup_dma_ops() will be called again but won't perform any
> operation as the DMA ops will still be set.
>
> We need to reset the DMA ops in arch_teardown_dma_ops() to fix this.
> However, we can't do so unconditionally, as then a new mapping would be
> created by arch_setup_dma_ops() when the device is reprobed, regardless
> of whether the device needs to share a mapping or not. We must thus keep
> track of whether arch_setup_dma_ops() created the mapping, and only in
> that case tear it down in arch_teardown_dma_ops().
>
> Keep track of that information in the dev_archdata structure. As the
> structure is embedded in all instances of struct device let's not grow
> it, but turn the existing dma_coherent bool field into a bitfield that
> can be used for other purposes.
>
> Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with deferred probing or error")
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> arch/arm/include/asm/device.h | 3 ++-
> arch/arm/mm/dma-mapping.c | 5 +++++
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
> index 36ec9c8f6e16..3234fe9bba6e 100644
> --- a/arch/arm/include/asm/device.h
> +++ b/arch/arm/include/asm/device.h
> @@ -19,7 +19,8 @@ struct dev_archdata {
> #ifdef CONFIG_XEN
> const struct dma_map_ops *dev_dma_ops;
> #endif
> - bool dma_coherent;
> + unsigned int dma_coherent:1;
This should only ever be accessed by the Xen DMA code via the
is_device_dma_coherent() helper, so I can't see the change of storage
type causing any problems.
> + unsigned int dma_ops_setup:1;
> };
>
> struct omap_device;
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index c742dfd2967b..e0272f9140e2 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -2430,9 +2430,14 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> dev->dma_ops = xen_dma_ops;
> }
> #endif
> + dev->archdata.dma_ops_setup = true;
> }
>
> void arch_teardown_dma_ops(struct device *dev)
> {
> + if (!dev->archdata.dma_ops_setup)
> + return;
> +
> arm_teardown_iommu_dma_ops(dev);
> + set_dma_ops(dev, NULL);
Should we clear dma_ops_setup here for symmetry? I guess in practice
it's down to the IOMMU driver so will never change after the first
probe, but it still feels like a bit of a nagging loose end.
With that (or firm reassurance that it's OK not to),
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
Apologies for being too arm64-focused in the earlier reviews and
overlooking this. Should the patch supersede 8674/1 currently in
Russell's incoming box?
Robin.
> }
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] ARM: dma-mapping: Don't tear third-party mappings
2017-05-16 15:47 ` Robin Murphy
@ 2017-05-16 16:44 ` Laurent Pinchart
2017-05-17 5:15 ` Sricharan R
0 siblings, 1 reply; 26+ messages in thread
From: Laurent Pinchart @ 2017-05-16 16:44 UTC (permalink / raw)
To: Robin Murphy
Cc: Laurent Pinchart, linux-arm-kernel, Sricharan R, Joerg Roedel,
Geert Uytterhoeven, Will Deacon, iommu, linux-renesas-soc
Hi Robin,
On Tuesday 16 May 2017 16:47:36 Robin Murphy wrote:
> On 16/05/17 16:14, Laurent Pinchart wrote:
> > arch_setup_dma_ops() is used in device probe code paths to create an
> > IOMMU mapping and attach it to the device. The function assumes that the
> > device is attached to a device-specific IOMMU instance (or at least a
> > device-specific TLB in a shared IOMMU instance) and thus creates a
> > separate mapping for every device.
> >
> > On several systems (Renesas R-Car Gen2 being one of them), that
> > assumption is not true, and IOMMU mappings must be shared between
> > multiple devices. In those cases the IOMMU driver knows better than the
> > generic ARM dma-mapping layer and attaches mapping to devices manually
> > with arm_iommu_attach_device(), which sets the DMA ops for the device.
> >
> > The arch_setup_dma_ops() function takes this into account and bails out
> > immediately if the device already has DMA ops assigned. However, the
> > corresponding arch_teardown_dma_ops() function, called from driver
> > unbind code paths (including probe deferral), will tear the mapping down
> > regardless of who created it. When the device is reprobed
> > arch_setup_dma_ops() will be called again but won't perform any
> > operation as the DMA ops will still be set.
> >
> > We need to reset the DMA ops in arch_teardown_dma_ops() to fix this.
> > However, we can't do so unconditionally, as then a new mapping would be
> > created by arch_setup_dma_ops() when the device is reprobed, regardless
> > of whether the device needs to share a mapping or not. We must thus keep
> > track of whether arch_setup_dma_ops() created the mapping, and only in
> > that case tear it down in arch_teardown_dma_ops().
> >
> > Keep track of that information in the dev_archdata structure. As the
> > structure is embedded in all instances of struct device let's not grow
> > it, but turn the existing dma_coherent bool field into a bitfield that
> > can be used for other purposes.
> >
> > Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with deferred
> > probing or error") Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com> ---
> >
> > arch/arm/include/asm/device.h | 3 ++-
> > arch/arm/mm/dma-mapping.c | 5 +++++
> > 2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
> > index 36ec9c8f6e16..3234fe9bba6e 100644
> > --- a/arch/arm/include/asm/device.h
> > +++ b/arch/arm/include/asm/device.h
> > @@ -19,7 +19,8 @@ struct dev_archdata {
> > #ifdef CONFIG_XEN
> > const struct dma_map_ops *dev_dma_ops;
> > #endif
> > - bool dma_coherent;
> > + unsigned int dma_coherent:1;
>
> This should only ever be accessed by the Xen DMA code via the
> is_device_dma_coherent() helper, so I can't see the change of storage
> type causing any problems.
Thank you for double-checking. I agree with your analysis.
> > + unsigned int dma_ops_setup:1;
> > };
> >
> > struct omap_device;
> > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> > index c742dfd2967b..e0272f9140e2 100644
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -2430,9 +2430,14 @@ void arch_setup_dma_ops(struct device *dev, u64
> > dma_base, u64 size,
> > dev->dma_ops = xen_dma_ops;
> > }
> > #endif
> > + dev->archdata.dma_ops_setup = true;
> > }
> >
> > void arch_teardown_dma_ops(struct device *dev)
> > {
> > + if (!dev->archdata.dma_ops_setup)
> > + return;
> > +
> > arm_teardown_iommu_dma_ops(dev);
> > + set_dma_ops(dev, NULL);
>
> Should we clear dma_ops_setup here for symmetry? I guess in practice
> it's down to the IOMMU driver so will never change after the first
> probe, but it still feels like a bit of a nagging loose end.
To make a difference, we would need an IOMMU driver that creates a mapping
after a first round of arch_setup_dma_ops() / arch_teardown_dma_ops() calls,
follow by a second round. I don't think this could happen, but if it did, I
believe we'd be screwed already, as there would be a time were an incorrect
mapping (created by arch_setup_dma_ops() while the IOMMU driver needs to take
care of mapping creation) exists.
> With that (or firm reassurance that it's OK not to),
>
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>
> Apologies for being too arm64-focused in the earlier reviews and
> overlooking this. Should the patch supersede 8674/1 currently in
> Russell's incoming box?
Yes I think it should. Could you please take care of that ?
You can also add my
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
as I've tested that this paptch restores proper IOMMU operation on the Renesas
R-Car H2 Lager board. I believe the problem related to Sricharan's patch
reported by Geert still affects us and needs to be addressed separately.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] ARM: dma-mapping: Don't tear third-party mappings
2017-05-16 16:44 ` Laurent Pinchart
@ 2017-05-17 5:15 ` Sricharan R
2017-05-17 11:36 ` sricharan
0 siblings, 1 reply; 26+ messages in thread
From: Sricharan R @ 2017-05-17 5:15 UTC (permalink / raw)
To: Laurent Pinchart, Robin Murphy
Cc: Laurent Pinchart, linux-arm-kernel, Joerg Roedel,
Geert Uytterhoeven, Will Deacon, iommu, linux-renesas-soc
Hi Laurent/Robin,
On 5/16/2017 10:14 PM, Laurent Pinchart wrote:
> Hi Robin,
>
> On Tuesday 16 May 2017 16:47:36 Robin Murphy wrote:
>> On 16/05/17 16:14, Laurent Pinchart wrote:
>>> arch_setup_dma_ops() is used in device probe code paths to create an
>>> IOMMU mapping and attach it to the device. The function assumes that the
>>> device is attached to a device-specific IOMMU instance (or at least a
>>> device-specific TLB in a shared IOMMU instance) and thus creates a
>>> separate mapping for every device.
>>>
>>> On several systems (Renesas R-Car Gen2 being one of them), that
>>> assumption is not true, and IOMMU mappings must be shared between
>>> multiple devices. In those cases the IOMMU driver knows better than the
>>> generic ARM dma-mapping layer and attaches mapping to devices manually
>>> with arm_iommu_attach_device(), which sets the DMA ops for the device.
>>>
>>> The arch_setup_dma_ops() function takes this into account and bails out
>>> immediately if the device already has DMA ops assigned. However, the
>>> corresponding arch_teardown_dma_ops() function, called from driver
>>> unbind code paths (including probe deferral), will tear the mapping down
>>> regardless of who created it. When the device is reprobed
>>> arch_setup_dma_ops() will be called again but won't perform any
>>> operation as the DMA ops will still be set.
>>>
>>> We need to reset the DMA ops in arch_teardown_dma_ops() to fix this.
>>> However, we can't do so unconditionally, as then a new mapping would be
>>> created by arch_setup_dma_ops() when the device is reprobed, regardless
>>> of whether the device needs to share a mapping or not. We must thus keep
>>> track of whether arch_setup_dma_ops() created the mapping, and only in
>>> that case tear it down in arch_teardown_dma_ops().
>>>
>>> Keep track of that information in the dev_archdata structure. As the
>>> structure is embedded in all instances of struct device let's not grow
>>> it, but turn the existing dma_coherent bool field into a bitfield that
>>> can be used for other purposes.
>>>
>>> Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with deferred
>>> probing or error") Signed-off-by: Laurent Pinchart
>>> <laurent.pinchart+renesas@ideasonboard.com> ---
>>>
>>> arch/arm/include/asm/device.h | 3 ++-
>>> arch/arm/mm/dma-mapping.c | 5 +++++
>>> 2 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
>>> index 36ec9c8f6e16..3234fe9bba6e 100644
>>> --- a/arch/arm/include/asm/device.h
>>> +++ b/arch/arm/include/asm/device.h
>>> @@ -19,7 +19,8 @@ struct dev_archdata {
>>> #ifdef CONFIG_XEN
>>> const struct dma_map_ops *dev_dma_ops;
>>> #endif
>>> - bool dma_coherent;
>>> + unsigned int dma_coherent:1;
>>
>> This should only ever be accessed by the Xen DMA code via the
>> is_device_dma_coherent() helper, so I can't see the change of storage
>> type causing any problems.
>
> Thank you for double-checking. I agree with your analysis.
>
>>> + unsigned int dma_ops_setup:1;
>>> };
>>>
>>> struct omap_device;
>>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>>> index c742dfd2967b..e0272f9140e2 100644
>>> --- a/arch/arm/mm/dma-mapping.c
>>> +++ b/arch/arm/mm/dma-mapping.c
>>> @@ -2430,9 +2430,14 @@ void arch_setup_dma_ops(struct device *dev, u64
>>> dma_base, u64 size,
>>> dev->dma_ops = xen_dma_ops;
>>> }
>>> #endif
>>> + dev->archdata.dma_ops_setup = true;
>>> }
>>>
>>> void arch_teardown_dma_ops(struct device *dev)
>>> {
>>> + if (!dev->archdata.dma_ops_setup)
>>> + return;
>>> +
>>> arm_teardown_iommu_dma_ops(dev);
>>> + set_dma_ops(dev, NULL);
>>
>> Should we clear dma_ops_setup here for symmetry? I guess in practice
>> it's down to the IOMMU driver so will never change after the first
>> probe, but it still feels like a bit of a nagging loose end.
>
> To make a difference, we would need an IOMMU driver that creates a mapping
> after a first round of arch_setup_dma_ops() / arch_teardown_dma_ops() calls,
> follow by a second round. I don't think this could happen, but if it did, I
> believe we'd be screwed already, as there would be a time were an incorrect
> mapping (created by arch_setup_dma_ops() while the IOMMU driver needs to take
> care of mapping creation) exists.
>
Feels correct not to reset this, the iommu drivers in question, seems to
creating mapping/attaching in add_device path (which gets called before the
clients gets probed) and when the iommu client gets deferred/reprobed that
does not happen again even after the first round.
>> With that (or firm reassurance that it's OK not to),
>>
>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>>
>> Apologies for being too arm64-focused in the earlier reviews and
>> overlooking this. Should the patch supersede 8674/1 currently in
>> Russell's incoming box?
>
> Yes I think it should. Could you please take care of that ?
>
> You can also add my
> was
> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> as I've tested that this paptch restores proper IOMMU operation on the Renesas
> R-Car H2 Lager board. I believe the problem related to Sricharan's patch
> reported by Geert still affects us and needs to be addressed separately.
Thanks for the above, i had the same thing to be posted, was just testing it once.
There are three patches [1][2], already posted and third one for the issue that Geert
pointed i did below (Geert had a patch little differently to ignore -ENODEV).
I had this question previously for not propagating errors apart from EPROBE_DEFER,
did not have an issue reported at that time. Anyways if the below is ok, i will
just send the 3 patches in one set for easy picking up ?
[1] https://lkml.org/lkml/2017/5/16/25
[2] The above one that you have.
[3] The below one, if its fine ?
>From 4b379d4b852c41d7b5904c9a9e53deda94039f0a Mon Sep 17 00:00:00 2001
From: Sricharan R <sricharan@codeaurora.org>
Date: Wed, 3 May 2017 14:54:11 +0530
Subject: [PATCH] of: iommu: Ignore all errors except EPROBE_DEFER
While deferring the probe of iommu masters,
xlate and add_device callback can passback error values
like -ENODEV, which means iommu cannot be connected
with that master for real reasons. So rather than
killing the master's probe for such errors, just
ignore the errors and let the master work without
an iommu.
Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
drivers/iommu/of_iommu.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e6e9bec..750ab07 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -237,6 +237,10 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
ops = ERR_PTR(err);
}
+ /* Ignore all other errors apart from EPROBE_DEFER */
+ if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER))
+ ops = NULL;
+
return ops;
}
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Regards,
Sricharan
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH] ARM: dma-mapping: Don't tear third-party mappings
2017-05-17 5:15 ` Sricharan R
@ 2017-05-17 11:36 ` sricharan
0 siblings, 0 replies; 26+ messages in thread
From: sricharan @ 2017-05-17 11:36 UTC (permalink / raw)
To: Laurent Pinchart, Robin Murphy
Cc: Laurent Pinchart, linux-arm-kernel, Joerg Roedel,
Geert Uytterhoeven, Will Deacon, iommu, linux-renesas-soc
On 2017-05-17 10:45, Sricharan R wrote:
> Hi Laurent/Robin,
>
> On 5/16/2017 10:14 PM, Laurent Pinchart wrote:
>> Hi Robin,
>>
>> On Tuesday 16 May 2017 16:47:36 Robin Murphy wrote:
>>> On 16/05/17 16:14, Laurent Pinchart wrote:
>>>> arch_setup_dma_ops() is used in device probe code paths to create an
>>>> IOMMU mapping and attach it to the device. The function assumes that
>>>> the
>>>> device is attached to a device-specific IOMMU instance (or at least
>>>> a
>>>> device-specific TLB in a shared IOMMU instance) and thus creates a
>>>> separate mapping for every device.
>>>>
>>>> On several systems (Renesas R-Car Gen2 being one of them), that
>>>> assumption is not true, and IOMMU mappings must be shared between
>>>> multiple devices. In those cases the IOMMU driver knows better than
>>>> the
>>>> generic ARM dma-mapping layer and attaches mapping to devices
>>>> manually
>>>> with arm_iommu_attach_device(), which sets the DMA ops for the
>>>> device.
>>>>
>>>> The arch_setup_dma_ops() function takes this into account and bails
>>>> out
>>>> immediately if the device already has DMA ops assigned. However, the
>>>> corresponding arch_teardown_dma_ops() function, called from driver
>>>> unbind code paths (including probe deferral), will tear the mapping
>>>> down
>>>> regardless of who created it. When the device is reprobed
>>>> arch_setup_dma_ops() will be called again but won't perform any
>>>> operation as the DMA ops will still be set.
>>>>
>>>> We need to reset the DMA ops in arch_teardown_dma_ops() to fix this.
>>>> However, we can't do so unconditionally, as then a new mapping would
>>>> be
>>>> created by arch_setup_dma_ops() when the device is reprobed,
>>>> regardless
>>>> of whether the device needs to share a mapping or not. We must thus
>>>> keep
>>>> track of whether arch_setup_dma_ops() created the mapping, and only
>>>> in
>>>> that case tear it down in arch_teardown_dma_ops().
>>>>
>>>> Keep track of that information in the dev_archdata structure. As the
>>>> structure is embedded in all instances of struct device let's not
>>>> grow
>>>> it, but turn the existing dma_coherent bool field into a bitfield
>>>> that
>>>> can be used for other purposes.
>>>>
>>>> Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with
>>>> deferred
>>>> probing or error") Signed-off-by: Laurent Pinchart
>>>> <laurent.pinchart+renesas@ideasonboard.com> ---
>>>>
>>>> arch/arm/include/asm/device.h | 3 ++-
>>>> arch/arm/mm/dma-mapping.c | 5 +++++
>>>> 2 files changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/include/asm/device.h
>>>> b/arch/arm/include/asm/device.h
>>>> index 36ec9c8f6e16..3234fe9bba6e 100644
>>>> --- a/arch/arm/include/asm/device.h
>>>> +++ b/arch/arm/include/asm/device.h
>>>> @@ -19,7 +19,8 @@ struct dev_archdata {
>>>> #ifdef CONFIG_XEN
>>>> const struct dma_map_ops *dev_dma_ops;
>>>> #endif
>>>> - bool dma_coherent;
>>>> + unsigned int dma_coherent:1;
>>>
>>> This should only ever be accessed by the Xen DMA code via the
>>> is_device_dma_coherent() helper, so I can't see the change of storage
>>> type causing any problems.
>>
>> Thank you for double-checking. I agree with your analysis.
>>
>>>> + unsigned int dma_ops_setup:1;
>>>> };
>>>>
>>>> struct omap_device;
>>>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>>>> index c742dfd2967b..e0272f9140e2 100644
>>>> --- a/arch/arm/mm/dma-mapping.c
>>>> +++ b/arch/arm/mm/dma-mapping.c
>>>> @@ -2430,9 +2430,14 @@ void arch_setup_dma_ops(struct device *dev,
>>>> u64
>>>> dma_base, u64 size,
>>>> dev->dma_ops = xen_dma_ops;
>>>> }
>>>> #endif
>>>> + dev->archdata.dma_ops_setup = true;
>>>> }
>>>>
>>>> void arch_teardown_dma_ops(struct device *dev)
>>>> {
>>>> + if (!dev->archdata.dma_ops_setup)
>>>> + return;
>>>> +
>>>> arm_teardown_iommu_dma_ops(dev);
>>>> + set_dma_ops(dev, NULL);
>>>
>>> Should we clear dma_ops_setup here for symmetry? I guess in practice
>>> it's down to the IOMMU driver so will never change after the first
>>> probe, but it still feels like a bit of a nagging loose end.
>>
>> To make a difference, we would need an IOMMU driver that creates a
>> mapping
>> after a first round of arch_setup_dma_ops() / arch_teardown_dma_ops()
>> calls,
>> follow by a second round. I don't think this could happen, but if it
>> did, I
>> believe we'd be screwed already, as there would be a time were an
>> incorrect
>> mapping (created by arch_setup_dma_ops() while the IOMMU driver needs
>> to take
>> care of mapping creation) exists.
>>
>
> Feels correct not to reset this, the iommu drivers in question, seems
> to
> creating mapping/attaching in add_device path (which gets called before
> the
> clients gets probed) and when the iommu client gets deferred/reprobed
> that
> does not happen again even after the first round.
Please ignore the above comment. I said that because I was doing the
dma_ops_setup in arm_iommu_attach_device. I posted the three fixes now
[1].
Accidentally removed you from CC, sorry for that.
Applied those patches on top of 8674/1 that Robin mentioned
below. So removed setting set_dma_ops(dev, NULL) from your patch.
Also please note that, I changed the Fixes: commit msg in your patch to
("of/acpi: Configure dma operations at probe time for platform/amba/pci
bus devices")
because that was one which started to invoke the teardown on the driver
release path.
[1] https://lkml.org/lkml/2017/5/17/344
Regards,
Sricharan
>
>>> With that (or firm reassurance that it's OK not to),
>>>
>>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>>>
>>> Apologies for being too arm64-focused in the earlier reviews and
>>> overlooking this. Should the patch supersede 8674/1 currently in
>>> Russell's incoming box?
>>
>> Yes I think it should. Could you please take care of that ?
>>
>> You can also add my
>> was
>> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> as I've tested that this paptch restores proper IOMMU operation on the
>> Renesas
>> R-Car H2 Lager board. I believe the problem related to Sricharan's
>> patch
>> reported by Geert still affects us and needs to be addressed
>> separately.
>
> Thanks for the above, i had the same thing to be posted, was just
> testing it once.
> There are three patches [1][2], already posted and third one for the
> issue that Geert
> pointed i did below (Geert had a patch little differently to ignore
> -ENODEV).
> I had this question previously for not propagating errors apart from
> EPROBE_DEFER,
> did not have an issue reported at that time. Anyways if the below is
> ok, i will
> just send the 3 patches in one set for easy picking up ?
>
> [1] https://lkml.org/lkml/2017/5/16/25
> [2] The above one that you have.
> [3] The below one, if its fine ?
>
> From 4b379d4b852c41d7b5904c9a9e53deda94039f0a Mon Sep 17 00:00:00 2001
> From: Sricharan R <sricharan@codeaurora.org>
> Date: Wed, 3 May 2017 14:54:11 +0530
> Subject: [PATCH] of: iommu: Ignore all errors except EPROBE_DEFER
>
> While deferring the probe of iommu masters,
> xlate and add_device callback can passback error values
> like -ENODEV, which means iommu cannot be connected
> with that master for real reasons. So rather than
> killing the master's probe for such errors, just
> ignore the errors and let the master work without
> an iommu.
>
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> ---
> drivers/iommu/of_iommu.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index e6e9bec..750ab07 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -237,6 +237,10 @@ const struct iommu_ops *of_iommu_configure(struct
> device *dev,
> ops = ERR_PTR(err);
> }
>
> + /* Ignore all other errors apart from EPROBE_DEFER */
> + if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER))
> + ops = NULL;
> +
> return ops;
> }
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member of Code Aurora Forum, hosted by The Linux Foundation
>
>
>
>>
^ permalink raw reply [flat|nested] 26+ messages in thread