[PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

Hi Robin,

On Tuesday 16 May 2017 15:04:55 Robin Murphy wrote:

On 16/05/17 08:17, Laurent Pinchart wrote:

quoted

On Tuesday 16 May 2017 07:53:57 sricharan at codeaurora.org wrote:

[snip]

quoted

quoted

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 <redacted>
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)

quoted

  	__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 [off-list ref]
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