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

[Laurent Pinchart]

Hi Russell,

On Thursday 25 May 2017 16:05:41 Russell King - ARM Linux wrote:
> On Wed, May 24, 2017 at 02:26:13PM +0300, Laurent Pinchart wrote:
> > 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.
>
> Except, I don't think it fixes the problem that Sricharan is trying to
> fix, namely that of DMA ops that have been setup, torn down, and are
> trying to be re-setup again.

You're right, it's two separate problems, and we need to fix them both.

> The issue here is that results in a stale setup, because the dma_ops are
> left in-place from the first iommu setup, but the iommu mapping has been
> disposed of.
> 
> Your patch only avoids the problem of tearing down someone else's DMA
> ops.  We need a combination of both patches together.

Yes exactly. I should read e-mails to the end before starting typing the reply 
:-)

> What needs to happen for Sricharan's problem to be resolved is:
> 
> 1. move all of __arm_iommu_detach_device() into arm_iommu_detach_device().
> 2. replace the __arm_iommu_detach_device() call in
> arm_teardown_iommu_dma_ops() with arm_iommu_detach_device().
> 
> as I don't see any need to have a different order in
> arm_teardown_iommu_dma_ops().

-- 
Regards,

Laurent Pinchart

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Sricharan R]

Hi Russell,

On 5/25/2017 8:35 PM, Russell King - ARM Linux wrote:
> On Wed, May 24, 2017 at 02:26:13PM +0300, Laurent Pinchart wrote:
>> 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.
> 
> Except, I don't think it fixes the problem that Sricharan is trying to
> fix, namely that of DMA ops that have been setup, torn down, and are
> trying to be re-setup again.  The issue here is that results in a stale
> setup, because the dma_ops are left in-place from the first iommu setup,
> but the iommu mapping has been disposed of.
> 
> Your patch only avoids the problem of tearing down someone else's DMA
> ops.  We need a combination of both patches together.
> 
> What needs to happen for Sricharan's problem to be resolved is:
> 
> 1. move all of __arm_iommu_detach_device() into arm_iommu_detach_device().
> 2. replace the __arm_iommu_detach_device() call in 
> arm_teardown_iommu_dma_ops()
>with arm_iommu_detach_device().
> 
> as I don't see any need to have a different order in
> arm_teardown_iommu_dma_ops().
> 

Right, both patches are required and i was also thining the same thing about
using arm_iommu_detach_device from arm_teardown_iommu_dma_ops instead. Will
repost with this.

Regards,
 Sricharan

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Russell King - ARM Linux]

On Wed, May 24, 2017 at 02:26:13PM +0300, Laurent Pinchart wrote:
> 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.

Except, I don't think it fixes the problem that Sricharan is trying to
fix, namely that of DMA ops that have been setup, torn down, and are
trying to be re-setup again.  The issue here is that results in a stale
setup, because the dma_ops are left in-place from the first iommu setup,
but the iommu mapping has been disposed of.

Your patch only avoids the problem of tearing down someone else's DMA
ops.  We need a combination of both patches together.

What needs to happen for Sricharan's problem to be resolved is:

1. move all of __arm_iommu_detach_device() into arm_iommu_detach_device().
2. replace the __arm_iommu_detach_device() call in arm_teardown_iommu_dma_ops()
   with arm_iommu_detach_device().

as I don't see any need to have a different order in
arm_teardown_iommu_dma_ops().

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [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 

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

[Laurent Pinchart]

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

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

[Sricharan R]

Hi Russell/Laurent/Robin,

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:
>> Hi Russell,
>>
>> 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.
> 

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 

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

[Russell King - ARM Linux]

On Wed, May 24, 2017 at 12:46:51AM +0300, Laurent Pinchart wrote:
> Hi Russell,
> 
> 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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Laurent Pinchart]

Hi Russell,

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().

The IOMMU probe deferral support patch series was merged in v4.12-rc1 and 
breaks IOMMU operations on several platforms. We need a fix for v4.12-rc that 
should be as nonintrusive as possible, as a larger cleanup is likely not -rc 
material. Beside reverting the whole series, the simplest option I came up 
with was [1]. Note that this is not the only fix needed to fix v4.12-rc1 IOMMU 
breakage, there are four more patches in the series that Sricharan plans to 
get merged soon. I don't think there are dependencies between the other four 
drivers/ patches and the arch/arm/ patch, so the latter could be merged 
independently through your tree as soon as it's deemed ready.

[1] https://www.spinics.net/lists/arm-kernel/msg583019.html

-- 
Regards,

Laurent Pinchart

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Russell King - ARM Linux]

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Robin Murphy]

On 23/05/17 17:25, Russell King - ARM Linux wrote:
> On Thu, Oct 27, 2016 at 09:07:23AM +0530, Sricharan wrote:
>> Hi Robin,
>>
>>> -Original Message-
>>> From: Robin Murphy [mailto:robin.mur...@arm.com]
>>> Sent: Wednesday, October 26, 2016 8:37 PM
>>> To: Sricharan R <sricha...@codeaurora.org>; will.dea...@arm.com; 
>>> j...@8bytes.org; iommu@lists.linux-foundation.org; linux-arm-
>>> ker...@lists.infradead.org; linux-arm-...@vger.kernel.org; 
>>> laurent.pinch...@ideasonboard.com; m.szyprow...@samsung.com;
>>> tf...@chromium.org; srinivas.kandaga...@linaro.org
>>> Subject: Re: [PATCH V3 6/8] arm: dma-mapping: Reset the device's dma_ops
>>>
>>> On 04/10/16 18:03, Sricharan R wrote:
>>>> The dma_ops for the device is not getting set to NULL in
>>>> arch_tear_down_dma_ops and this causes an issue when the
>>>> device's probe gets deferred and retried. So reset the
>>>> dma_ops to NULL.
>>>
>>> Reviewed-by: Robin Murphy <robin.mur...@arm.com>
>>>
>>
>>  Thanks.
>>
>>> This seems like it could stand independently from the rest of the series
>>> - might be worth rewording the commit message to more general terms,
>>> i.e. arch_teardown_dma_ops() being the inverse of arch_setup_dma_ops()
>>> thus clearing the ops set by the latter, and sending it to Russell as a
>>> fix in its own right.
>>
>>   Ok, will reword the commit log and push this separately.
> 
> 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.

Robin.

[1]:https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg14301.html
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Russell King - ARM Linux]

On Thu, Oct 27, 2016 at 09:07:23AM +0530, Sricharan wrote:
> Hi Robin,
> 
> >-Original Message-
> >From: Robin Murphy [mailto:robin.mur...@arm.com]
> >Sent: Wednesday, October 26, 2016 8:37 PM
> >To: Sricharan R <sricha...@codeaurora.org>; will.dea...@arm.com; 
> >j...@8bytes.org; iommu@lists.linux-foundation.org; linux-arm-
> >ker...@lists.infradead.org; linux-arm-...@vger.kernel.org; 
> >laurent.pinch...@ideasonboard.com; m.szyprow...@samsung.com;
> >tf...@chromium.org; srinivas.kandaga...@linaro.org
> >Subject: Re: [PATCH V3 6/8] arm: dma-mapping: Reset the device's dma_ops
> >
> >On 04/10/16 18:03, Sricharan R wrote:
> >> The dma_ops for the device is not getting set to NULL in
> >> arch_tear_down_dma_ops and this causes an issue when the
> >> device's probe gets deferred and retried. So reset the
> >> dma_ops to NULL.
> >
> >Reviewed-by: Robin Murphy <robin.mur...@arm.com>
> >
> 
>  Thanks.
> 
> >This seems like it could stand independently from the rest of the series
> >- might be worth rewording the commit message to more general terms,
> >i.e. arch_teardown_dma_ops() being the inverse of arch_setup_dma_ops()
> >thus clearing the ops set by the latter, and sending it to Russell as a
> >fix in its own right.
> 
>   Ok, will reword the commit log and push this separately.

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?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Sricharan]

Hi Robin,

>-Original Message-
>From: Robin Murphy [mailto:robin.mur...@arm.com]
>Sent: Wednesday, October 26, 2016 8:37 PM
>To: Sricharan R <sricha...@codeaurora.org>; will.dea...@arm.com; 
>j...@8bytes.org; iommu@lists.linux-foundation.org; linux-arm-
>ker...@lists.infradead.org; linux-arm-...@vger.kernel.org; 
>laurent.pinch...@ideasonboard.com; m.szyprow...@samsung.com;
>tf...@chromium.org; srinivas.kandaga...@linaro.org
>Subject: Re: [PATCH V3 6/8] arm: dma-mapping: Reset the device's dma_ops
>
>On 04/10/16 18:03, Sricharan R wrote:
>> The dma_ops for the device is not getting set to NULL in
>> arch_tear_down_dma_ops and this causes an issue when the
>> device's probe gets deferred and retried. So reset the
>> dma_ops to NULL.
>
>Reviewed-by: Robin Murphy <robin.mur...@arm.com>
>

 Thanks.

>This seems like it could stand independently from the rest of the series
>- might be worth rewording the commit message to more general terms,
>i.e. arch_teardown_dma_ops() being the inverse of arch_setup_dma_ops()
>thus clearing the ops set by the latter, and sending it to Russell as a
>fix in its own right.

  Ok, will reword the commit log and push this separately.

Regards,
 Sricharan


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Robin Murphy]

On 04/10/16 18:03, Sricharan R wrote:
> The dma_ops for the device is not getting set to NULL in
> arch_tear_down_dma_ops and this causes an issue when the
> device's probe gets deferred and retried. So reset the
> dma_ops to NULL.

Reviewed-by: Robin Murphy 

This seems like it could stand independently from the rest of the series
- might be worth rewording the commit message to more general terms,
i.e. arch_teardown_dma_ops() being the inverse of arch_setup_dma_ops()
thus clearing the ops set by the latter, and sending it to Russell as a
fix in its own right.

Robin.

> Signed-off-by: Sricharan R 
> ---
>  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 dde6514..b9191f0 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -2295,6 +2295,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
> 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu