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

[sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org]

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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> >> Reviewed-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> >> ---
>> >>
>> >>   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-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> 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,