[PATCH] ARM: dma-mapping: Don't tear third-party mappings
sricharan at codeaurora.org
sricharan at codeaurora.org
Wed May 17 04:36:54 PDT 2017
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 at 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 at 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 at 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 at 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 at 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
>
>
>
>>
More information about the linux-arm-kernel
mailing list