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@xxxxxxxxxxxxxxxx> ---
> >
> > 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@xxxxxxx>
>
> 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@xxxxxxxxxxxxxxxx>
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
