[PATCH V3 6/8] arm: dma-mapping: Reset the device's dma_ops

[Sricharan R]

Hi Laurent,

On 5/24/2017 4:56 PM, Laurent Pinchart wrote:
> Hello,
> 
> On Wednesday 24 May 2017 16:01:45 Sricharan R wrote:
>> On 5/24/2017 4:12 AM, Russell King - ARM Linux wrote:
>>> On Wed, May 24, 2017 at 12:46:51AM +0300, Laurent Pinchart wrote:
>>>> On Tuesday 23 May 2017 18:53:19 Russell King - ARM Linux wrote:
>>>>> On Tue, May 23, 2017 at 05:55:57PM +0100, Robin Murphy wrote:
>>>>>> On 23/05/17 17:25, Russell King - ARM Linux wrote:
>>>>>>> So, I've come to apply this patch (since it's finally landed in the
>>>>>>> patch system), and I'm not convinced that the commit message is really
>>>>>>> up to scratch.
>>>>>>>
>>>>>>> The current commit message looks like this:
>>>>>>>
>>>>>>> "   ARM: 8674/1: dma-mapping: Reset the device's dma_ops
>>>>>>>
>>>>>>>     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."
>>>>>>>
>>>>>>> It is obviously a fix, but a fix for which patch?  Looking at the
>>>>>>> history, we have "arm: dma-mapping: Don't override dma_ops in
>>>>>>> arch_setup_dma_ops()" which I would have guessed is the appropriate
>>>>>>> one, but this post-dates your patch (it's very recent, v4.12-rc
>>>>>>> recent.)
>>>>>>>
>>>>>>> So, I need more description about the problem you were seeing when
>>>>>>> you first proposed this patch.
>>>>>>>
>>>>>>> How does leaving the dma_ops in place prior to "arm: dma-mapping:
>>>>>>> Don't override dma_ops in arch_setup_dma_ops()" cause problems for
>>>>>>> deferred probing?
>>>>>>>
>>>>>>> What patch is your change trying to fix?  In other words, how far
>>>>>>> back does this patch need to be backported?
>>>>>>
>>>>>> In effect, it's fixing a latent inconsistency that's been present since
>>>>>> its introduction in 4bb25789ed28. However, that has clearly not proven
>>>>>> to be an issue in practice since then. With 09515ef5ddad we start
>>>>>> actually calling arch_teardown_dma_ops() in a manner that might leave
>>>>>> things partially initialised if anyone starts removing and reprobing
>>>>>> drivers, but so far that's still from code inspection[1] rather than
>>>>>> anyone hitting it.
>>>>>>
>>>>>> Given that the changes which tickle it are fresh in -rc1 I'd say
>>>>>> there's no need to backport this, but at the same time it shouldn't do
>>>>>> any damage if you really want to.
>>>>>
>>>>> Well, looking at this, I'm not convinced that much of it is correct.
>>>>>
>>>>> 1) set_dma_ops() is in arch_setup_dma_ops() but this patch adds
>>>>>    the unsetting of the DMA ops inside arm_teardown_iommu_dma_ops()
>>>>>    rather than arch_teardown_dma_ops().
>>>>>    
>>>>>    This doesn't strike me as being particularly symmetric.
>>>>>    arm_teardown_iommu_dma_ops() is arm_setup_iommu_dma_ops()'s
>>>>>    counterpart.
>>>>>
>>>>> 2) arch_setup_dma_ops(), the recent patch to add the existing dma_ops
>>>>>    check, and Xen - Xen wants to override the DMA ops if in the
>>>>>    initial domain.  It's not clear (at least to me) whether the recent
>>>>>    patch adding the dma_ops check took account of this or not.
>>>>>
>>>>> 3) random places seem to fiddle with the dma_ops - notice that
>>>>>    arm_iommu_detach_device() sets the dma_ops to NULL.
>>>>>    
>>>>>    In fact, I think moving __arm_iommu_detach_device() into
>>>>>    arm_iommu_detach_device(), calling arm_iommu_detach_device(),
>>>>>    and getting rid of the explicit set_dma_ops(, NULL) in this
>>>>>    path would be a good first step.
>>>>>
>>>>> 4) I think arch_setup_dma_ops() is over-complex.
>>>>>
>>>>> So, in summary, this code is a mess today, and that means it's not
>>>>> obviously correct - which is bad.  This needs sorting.
>>>>
>>>> We've reached the same conclusion independently, but I'll refrain from
>>>> commenting on whether that's a good or bad thing :-)
>>>>
>>>> I don't think this patch should be applied, as it could break Xen (and
>>>> other platforms/drivers that set the DMA operations manually) by
>>>> resetting DMA operations at device remove() time even if they have been
>>>> set independently of arch_setup_dma_ops().
>>>
>>> That will only occur if the dma ops have been overriden once the DMA
>>> operations have been setup via arch_setup_dma_ops. What saves it from
>>> wholesale NULLing of the DMA operations is the check for a valid
>>> dma_iommu_mapping structure in arm_teardown_iommu_dma_ops().  This only
>>> exists when arm_setup_iommu_dma_ops() has attached a mapping to the
>>> device.
> 
> Unfortunately I don't think that's always the case. The dma_iommu_mapping is 
> also set by direct callers of arm_iommu_attach_device(), namely
> 
> - the Renesas R-Car IOMMU driver (ipmmu-vmsa)
> - the Mediatek IOMMU driver (mtk-iommu-v1)
> - the Exynos DRM driver
> - the OMAP3 ISP driver
> 
> All these need to be fixed, but that's not v4.12-rc material. At least in the 
> ipmmu-vmsa case, which is the one I noticed the problem with, 
> arm_iommu_attach_device() is called before arch_setup_dma_ops(). 
> arch_setup_dma_ops() then exits immediately when called due to the
> 
>         if (dev->dma_ops)
>                 return;
> 
> check at the beginning of the function. We must ensure that in that case 
> arch_teardown_dma_ops() will not remove the mapping or set the DMA ops to 
> NULL, and testing to_dma_iommu_mapping(dev) won't help.
> 

Right, agree. Same issue highlighted in #1, #2 below and the fixes posted [1]
for 4.12-rc.

>> Right, only if the dma ops are set and no dma_iommu_mapping is created for
>> the device, then arch_teardown_iommu_dma_ops does nothing.
>>
>> Firstly, this patch for resetting the dma_ops in teardown was required
>> only when arch_setup_dma_ops has created both the mapping and dma_ops
>> for the device. Because mappings that were created in arch_setup_dma_ops
>> are cleared in teardown, so dma ops should also be reset. But this can be
>> done by calling arm_iommu_detach_device() from arm_teardown_iommu_dma_ops
>> to avoid explicitly calling set_dma_ops again, probably this what was
>> suggested in #3 above ?
>>
>> Really sorry for the mess, but below cleanups looks required otherwise,
>>
>> 1) set_dma_ops is called for mach-highbank, mach-mevbu during the
>>    BUS_NOTIFY_ADD_DEVICE. That should be removed and made to come from
>>    DT path and arch_setup_dma_ops. dmabounce.c also does set_dma_ops,
>>    not very sure how to handle that (swiotlb ?), are call
>>    dmabounce_register_dev during the device's probe instead to have the
>>    dma_set_ops overriding later in probe ?
> 
> All this needs to be addressed, but it's definitely not v4.12-rc material.
> 
>> 2) arm_iommu_attach_device is called from ipmmu-vmsa.c, mtk_iommu_v1.c,
>>    iommu drivers, from the iommu add_device callback, called
>>    from BUS_NOTIFY_ADD_DEVICE notifier. This is a problem because,
>>    with probe-deferral, this can be overridden in arch_setup_dma_ops
>>    during device probe and cleared in teardown path. But the add_device
>>    callback notifier is not called again when the device gets reprobed
>>    again.
>>
>>    With probe deferral, add_device callback also gets called from
>>    of_iommu_configure during device probe, so the above drivers should
>>    be adapted to properly register the iommu_ops to have its add_device
>>    called from of_iommu_configure path. mtk_iommu_v1.c seems to be fine,
>>    but ipmmu-vmsa.c should be adapted. otherwise if the devices attached
>>    to those iommus call arm_iommu_attach_device from its probe path to
>>    override the default ops set in arch_setup_dma_ops, then all is fine.
>>    This seems to be the case with exynos_drm_iommu.c, omap3isp/isp.c.
> 
> Same here, this needs to be addressed, but not in v4.12-rc. We need a simpler 
> fix for v4.12-rc.
> 

Right, not for 4.12-rc, but as a better cleanup beyond, so
wanted to discuss the right sequence further. The fixes posted are the ones
for 4.12-rc.

>>    If the above two are done, the overridding of the default dma_ops and
>>    mapping should happen after arch_setup_dma_ops is called and also
>>    overridden every time the device gets reprobed. That should help to get
>>    rid of couple of fixes that has been added.
>>
>> 3) As Laurent already pointed out earlier, return error codes from some of
>>    the IOMMU apis needs to standardized.
>>
>>    Please let me know if its right way of doing it ?
> 
> Again, the patch I propose is the simplest v4.12-rc fix I can think of, short 
> of reverting your complete IOMMU probe deferral patch series. Let's focus on 
> the v4.12-rc fix, and then discuss how to move forward in v4.13 and beyond.
> 

Agree.

[1] https://lkml.org/lkml/2017/5/23/411


Regards,
 Sricharan
-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation