From: Russell King - ARM Linux <linux@armlinux.org.uk>
To: Robin Murphy <robin.murphy@arm.com>
Cc: Sricharan <sricharan@codeaurora.org>,
will.deacon@arm.com, joro@8bytes.org,
iommu@lists.linux-foundation.org,
linux-arm-kernel@lists.infradead.org,
linux-arm-msm@vger.kernel.org, laurent.pinchart@ideasonboard.com,
m.szyprowski@samsung.com, tfiga@chromium.org,
srinivas.kandagatla@linaro.org
Subject: Re: [PATCH V3 6/8] arm: dma-mapping: Reset the device's dma_ops
Date: Tue, 23 May 2017 18:53:19 +0100 [thread overview]
Message-ID: <20170523175319.GA22219@n2100.armlinux.org.uk> (raw)
In-Reply-To: <a9debbce-17dc-aeaa-59bd-668aa15b97fe@arm.com>
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.
next prev parent reply other threads:[~2017-05-23 17:53 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20161004170414eucas1p141bebe16e1bf241862833e7ad0270c72@eucas1p1.samsung.com>
2016-10-04 17:03 ` [PATCH V3 0/8] IOMMU probe deferral support Sricharan R
[not found] ` <1475600632-21289-1-git-send-email-sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2016-10-04 17:03 ` [PATCH V3 1/8] arm: dma-mapping: Don't override dma_ops in arch_setup_dma_ops() Sricharan R
2016-10-04 17:03 ` [PATCH V3 4/8] drivers: platform: Configure dma operations at probe time Sricharan R
2016-10-26 14:07 ` Robin Murphy
2016-10-26 15:04 ` Sricharan
2016-10-27 10:49 ` Lorenzo Pieralisi
2016-11-02 7:05 ` Sricharan
2016-10-04 17:03 ` [PATCH V3 6/8] arm: dma-mapping: Reset the device's dma_ops Sricharan R
2016-10-26 15:07 ` Robin Murphy
[not found] ` <a3d4533f-165d-f444-7681-141479617a18-5wv7dgnIgG8@public.gmane.org>
2016-10-27 3:37 ` Sricharan
2017-05-23 16:25 ` Russell King - ARM Linux
[not found] ` <20170523162507.GA1729-l+eeeJia6m9URfEZ8mYm6t73F7V6hmMc@public.gmane.org>
2017-05-23 16:55 ` Robin Murphy
2017-05-23 17:53 ` Russell King - ARM Linux [this message]
[not found] ` <20170523175319.GA22219-l+eeeJia6m9URfEZ8mYm6t73F7V6hmMc@public.gmane.org>
2017-05-23 21:46 ` Laurent Pinchart
2017-05-23 22:42 ` Russell King - ARM Linux
[not found] ` <20170523224216.GI22219-l+eeeJia6m9URfEZ8mYm6t73F7V6hmMc@public.gmane.org>
2017-05-24 10:31 ` Sricharan R
[not found] ` <c4ad7341-fa9f-81b7-a41c-417144c4f842-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2017-05-24 11:26 ` Laurent Pinchart
2017-05-24 11:38 ` Sricharan R
2017-05-25 15:05 ` Russell King - ARM Linux
[not found] ` <20170525150540.GJ22219-l+eeeJia6m9URfEZ8mYm6t73F7V6hmMc@public.gmane.org>
2017-05-26 5:18 ` Sricharan R
2017-05-26 14:04 ` Laurent Pinchart
2016-10-10 12:36 ` [PATCH V3 0/8] IOMMU probe deferral support Marek Szyprowski
2016-10-17 6:58 ` Sricharan
[not found] ` <12cfb59f-f7ca-d4df-eb7f-42348e357979-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2016-10-12 6:24 ` Sricharan
2016-10-24 6:34 ` Marek Szyprowski
[not found] ` <b9e4e81f-3b3e-951f-df62-d640275aae71-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2016-10-24 12:30 ` Sricharan
2016-10-17 7:02 ` Sricharan
2016-10-25 6:25 ` Archit Taneja
2016-10-04 17:03 ` [PATCH V3 2/8] of: dma: Move range size workaround to of_dma_get_range() Sricharan R
2016-10-04 17:03 ` [PATCH V3 3/8] of: dma: Make of_dma_deconfigure() public Sricharan R
2016-10-04 17:03 ` [PATCH V3 5/8] iommu: of: Handle IOMMU lookup failure with deferred probing or error Sricharan R
[not found] ` <1475600632-21289-6-git-send-email-sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2016-10-26 14:52 ` Robin Murphy
[not found] ` <f08e65b4-f755-897c-f776-40f0d6788251-5wv7dgnIgG8@public.gmane.org>
2016-10-27 2:55 ` Sricharan
2016-10-04 17:03 ` [PATCH V3 7/8] arm/arm64: dma-mapping: Call iommu's remove_device callback during device detach Sricharan R
[not found] ` <1475600632-21289-8-git-send-email-sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2016-10-26 15:16 ` Robin Murphy
2016-10-27 5:16 ` Sricharan
2016-10-04 17:03 ` [PATCH V3 8/8] arm64: dma-mapping: Remove the notifier trick to handle early setting of dma_ops Sricharan R
[not found] ` <1475600632-21289-9-git-send-email-sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2016-10-07 15:40 ` Sricharan
2016-10-26 15:34 ` Robin Murphy
2016-10-27 5:19 ` Sricharan
2016-10-25 14:35 ` [PATCH V3 0/8] IOMMU probe deferral support Robin Murphy
[not found] ` <60ee8066-f167-e9df-ae3e-4138f1133bad-5wv7dgnIgG8@public.gmane.org>
2016-10-26 14:44 ` Sricharan
2016-10-26 17:14 ` Robin Murphy
[not found] ` <421e2b14-0231-d376-02a0-097423120b3d-5wv7dgnIgG8@public.gmane.org>
2016-10-27 8:37 ` Sricharan
2016-11-03 22:25 ` Sricharan
2016-11-04 15:16 ` Sricharan
2016-11-07 19:13 ` Will Deacon
2016-11-07 19:22 ` Robin Murphy
2016-11-09 6:24 ` Sricharan
2016-11-09 16:59 ` Will Deacon
2016-11-14 3:41 ` Sricharan
2016-11-20 15:11 ` Sricharan
2016-11-23 19:54 ` Robin Murphy
[not found] ` <918128b9-cdb0-1454-000a-146cee7a05ea-5wv7dgnIgG8@public.gmane.org>
2016-11-24 16:10 ` Sricharan
2016-11-24 19:11 ` Robin Murphy
2016-11-28 17:42 ` Sricharan
2016-11-28 18:13 ` Lorenzo Pieralisi
2016-11-30 0:34 ` Sricharan
2016-11-30 12:07 ` Lorenzo Pieralisi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170523175319.GA22219@n2100.armlinux.org.uk \
--to=linux@armlinux.org.uk \
--cc=iommu@lists.linux-foundation.org \
--cc=joro@8bytes.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=robin.murphy@arm.com \
--cc=sricharan@codeaurora.org \
--cc=srinivas.kandagatla@linaro.org \
--cc=tfiga@chromium.org \
--cc=will.deacon@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).