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@xxxxxxxxxxxxxx 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@xxxxxxxxxxxxxx>
> >> Reviewed-by: Robin Murphy <robin.murphy@xxxxxxx>
> >> ---
> >>
> >> 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@xxxxxxxxxxxxxxxx>
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
