From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.9 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 81A48C433E0 for ; Tue, 19 Jan 2021 09:05:59 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 292B623100 for ; Tue, 19 Jan 2021 09:05:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 292B623100 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=bootlin.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type:Cc: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Tmm3DffCAvWV18QaR6waNhZO0lW/gFaOr0iL3tPjtd8=; b=ku1uxGW8pePoRpF9OXCfBuDUV w+9ezye3SlJQ67hoOrBV2cdYlmmN7dhjnOFSzEXzGRdc2X2VOQiAOuFFxSJfshpY7/U9BKptHjedx gIdRt38CNz7mDC31GG0+aNbrxHbFjqVhRp+hmVwatHFjtY/bm1wu2kLXyEKerjjUgkfB2TgaZn/QU K88u3GL2tucVTySSI/6WI0DKMMRWhix8jGjUR91t6rSoCuJPl8S6/JWkNIFxPXG43OCt6NWAfixsx aaBRYabyv6xQCrJ3LVMPnla9XVk1T4HZ8PVX5XuXXTGS/eiBZOBm4I2TnhNtUWiFfqOEkPHOXuB/v OB7mJmTBA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l1mwI-0005pe-Gf; Tue, 19 Jan 2021 09:04:38 +0000 Received: from relay6-d.mail.gandi.net ([217.70.183.198]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1l1mwE-0005oK-E6 for linux-arm-kernel@lists.infradead.org; Tue, 19 Jan 2021 09:04:37 +0000 X-Originating-IP: 93.29.109.196 Received: from aptenodytes (196.109.29.93.rev.sfr.net [93.29.109.196]) (Authenticated sender: paul.kocialkowski@bootlin.com) by relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 4AD73C002A; Tue, 19 Jan 2021 09:04:28 +0000 (UTC) Date: Tue, 19 Jan 2021 10:04:27 +0100 From: Paul Kocialkowski To: Robin Murphy Subject: Re: [PATCH 1/2] of: device: Allow DMA range map to be set before of_dma_configure_id Message-ID: References: <20210115175831.1184260-1-paul.kocialkowski@bootlin.com> <5c7946f3-b56e-da00-a750-be097c7ceb32@arm.com> MIME-Version: 1.0 In-Reply-To: <5c7946f3-b56e-da00-a750-be097c7ceb32@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210119_040434_675895_FD2184B7 X-CRM114-Status: GOOD ( 44.95 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, Jernej Skrabec , Daniel Vetter , linux-kernel@vger.kernel.org, Maxime Ripard , Chen-Yu Tsai , Rob Herring , Thomas Petazzoni , Frank Rowand , linux-arm-kernel@lists.infradead.org Content-Type: multipart/mixed; boundary="===============0892073243925330831==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============0892073243925330831== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="VWPV4+qJKHV+hHEv" Content-Disposition: inline --VWPV4+qJKHV+hHEv Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Robin, On Mon 18 Jan 21, 13:27, Robin Murphy wrote: > On 2021-01-16 17:07, Paul Kocialkowski wrote: > > Hi Robin, > >=20 > > Le Sat 16 Jan 21, 14:57, Robin Murphy a =C3=A9crit : > > > On 2021-01-15 17:58, Paul Kocialkowski wrote: > > > > A mechanism was recently introduced for the sunxi architecture where > > > > the DMA offset for specific devices (under the MBUS) is set by a co= mmon > > > > driver (sunxi_mbus). This driver calls dma_direct_set_offset to set > > > > the device's dma_range_map manually. > > > >=20 > > > > However this information was overwritten by of_dma_configure_id, wh= ich > > > > obtains the map from of_dma_get_range (or keeps it NULL when it fai= ls > > > > and the force_dma argument is true, which is the case for platform > > > > devices). > > > >=20 > > > > As a result, the dma_range_map was always overwritten and the mecha= nism > > > > could not correctly take effect. > > > >=20 > > > > This adds a check to ensure that no previous DMA range map is > > > > overwritten and prints a warning when the map was already set while > > > > also being available from dt. In this case, the map that was already > > > > set is kept. > > >=20 > > > Hang on, the hard-coded offset is only intended to be installed when = there > > > *isn't* anything described in DT, in which case of_dma_get_range() sh= ould > > > always bail out early without touching it anyway. This sounds like > > > something's not quite right in the MBUS driver, so I don't think work= ing > > > around it in core code is really the right thing to do. > >=20 > > That's right, there is no practical case where the two are in conflict. > > The problem that I'm solving here is that dev->dma_range_map is *always* > > assigned, even when of_dma_get_range bailed and map still is NULL. > >=20 > > This has the effect of always overwriting what the MBUS driver does > > (and leaking its memory too). >=20 > Oh, I should have looked closer at of_dma_configure_id() itself. I was > assuming that b4bdc4fbf8d0 had been tested and at least *could* work, but > apparently not :/ >=20 > Indeed it seems there was a fundamental oversight in e0d072782c73 > ("dma-mapping: introduce DMA range map, supplanting dma_pfn_offset"), > equivalent to the same one I previously made myself with bus_dma_mask and > fixed in 6778be4e5209 ("of/device: Really only set bus DMA mask when > appropriate"). I think same simpler fix is the right one for this case to= o, > i.e. just move the assignment to dev->dma_range_map up under the "if (!re= t)" > condition. Assigning it this late - after the IOMMU and arch setup - looks > wrong anyway, even if it isn't currently an issue in practice (in princip= le > an IOMMU driver *could* start trying to figure out reserved regions around > DMA windows for a device as early as its .of_xlate callback, even though > that's not the intent of the API design). Okay sounds good, I'll resend a patch with the assignment under if (!ret)! > Luckily dma_range_map hasn't been hooked up in the equivalent ACPI path y= et, > so you're off the hook for fixing that too :) Good for me :) Cheers, Paul > > > Do you have a case where one of the relevant devices inherits a "dma-= ranges" > > > via the regular hierarchy without indirecting via an "interconnects" > > > reference? Currently you're only checking for the latter, so that wou= ld be > > > one way things could go awry (although to be a problem, said "dma-ran= ges" > > > would also have to encode something *other* than the appropriate MBUS > > > offset, which implies an incorrect or at least inaccurately-structure= d DT as > > > well). > >=20 > > No, I think things are good in that regard. No messed up dt or anything= like > > that :) > >=20 > > Cheers, > >=20 > > Paul > >=20 > > > Robin. > > >=20 > > > > Fixes: b4bdc4fbf8d0 ("soc: sunxi: Deal with the MBUS DMA offsets in= a central place") > > > > Signed-off-by: Paul Kocialkowski > > > > --- > > > > drivers/of/device.c | 9 ++++++++- > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > >=20 > > > > diff --git a/drivers/of/device.c b/drivers/of/device.c > > > > index aedfaaafd3e7..db1b8634c2c7 100644 > > > > --- a/drivers/of/device.c > > > > +++ b/drivers/of/device.c > > > > @@ -181,7 +181,14 @@ int of_dma_configure_id(struct device *dev, st= ruct device_node *np, > > > > arch_setup_dma_ops(dev, dma_start, size, iommu, coherent); > > > > - dev->dma_range_map =3D map; > > > > + if (!dev->dma_range_map) { > > > > + dev->dma_range_map =3D map; > > > > + } else if (map) { > > > > + dev_warn(dev, > > > > + "DMA range map was already set, ignoring range map from dt\n"); > > > > + kfree(map); > > > > + } > > > > + > > > > return 0; > > > > } > > > > EXPORT_SYMBOL_GPL(of_dma_configure_id); > > > >=20 > >=20 --=20 Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com --VWPV4+qJKHV+hHEv Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEEJZpWjZeIetVBefti3cLmz3+fv9EFAmAGoJsACgkQ3cLmz3+f v9EHhwf/elOyvLNl99Y6DkOE0jrgwROy6MwAy6sEWzlI8Ks3G9JqNi7yLpQ3A1Ak 0TvY9aChoHCSaoKMC/GoyxdbZrP662sELa4833LTLXNoLC7SZLMsqGeYDVriCA7p VBJC4kJNyHXknWLDF4NMEe0m7UApXI7bmckz+eImWYiQQJvw8JHKTgnzTt8NDfHw E5auZMZvtj5JJI2u9NriJUZeFxnwo4a9izHHTNsWWkWXXLZAieG3PCI+rdrOObmG 9W2e1FdctysCIFwzRRFTn2t9u1j9+epnFTNw+C1BueuUKCM/Q6ZGHbOeEveakbas 58uZgm9ppLgEZdCgFjduv7F1hjajvA== =j/jd -----END PGP SIGNATURE----- --VWPV4+qJKHV+hHEv-- --===============0892073243925330831== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============0892073243925330831==--