From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="Un7nKMLr" Received: from mail-lf1-x132.google.com (mail-lf1-x132.google.com [IPv6:2a00:1450:4864:20::132]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 834DD1B2; Fri, 1 Dec 2023 03:22:50 -0800 (PST) Received: by mail-lf1-x132.google.com with SMTP id 2adb3069b0e04-50bc8b7d8ffso2926492e87.0; Fri, 01 Dec 2023 03:22:50 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1701429769; x=1702034569; darn=vger.kernel.org; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=tDYTbMEHTA+PA+CkrLL7nufZCEtkNLGajLlCGSCmAZE=; b=Un7nKMLrcWy2uoCJqpOaLDss3Gj3abCXvwCyxffhHx9GDYO+j9ePTjXbu4pFkaQORM LXi9HyxhRm4+q7VGqWDRZMSsX/2ze/oGgf/5Gv86fIGfMfFsIOnU1tyjJ4EycWdVfQlj wgMgnbRZPvj8bw2eb3ozTjNiWYmkIc6wYKNCOP2BO7i3vuG6PVlDucY566r03ZqOPGbO HvZ302HZMOoBJZlFrSjiHgqfCOCoPoUZ6CCRRCqfu8bn2ZtK93uTqip3jgKTRQWrllQG xHXkJPe9/FSvNWv++e035ZDuwREYrTXdY+jnhaD1lKNsz0JhDQoCmTsu2swKPo7O1tqH dxyw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701429769; x=1702034569; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=tDYTbMEHTA+PA+CkrLL7nufZCEtkNLGajLlCGSCmAZE=; b=wdDnDy22GLa5poh3lkIOQ2VemzBMP2cj6bQ0YdYDhhiPJP9Oba4osf8ju8Pw+rKXfl jMBpNTehQ4UISS6/V9NvZ4BJaGbjC+US1GkrZ3avyPFlT+niZGEW4tkd3IxguIJNJbf7 oBodZA92Md2Zu07e7BgNEPzTjEOOZt4qrxKIiaKBhPkrlIQwPVRwDo9c22YEexmbiY/N 2NJKaKpylbeznPrRCq/F2NsDYbKR4gKnAhXnnBBYBarRd1HhyfXShCRILlHB9Yi6oZ6R /1GFNdcm25Rw4ITM9mNn6tmCqwRyTQhxJOducshou6hDUKrea4LO2oVxvhYU3S042kDB SLLQ== X-Gm-Message-State: AOJu0Yw3al5atToZz6/xlhI23G9V436hkux2E0ZVUV4UjvBa2sszEq7t TdZNAV5Lo4HS3oRVLAmbQec= X-Google-Smtp-Source: AGHT+IF/kNTDoQQTbuA8uVZADKdRmi7YjxqAP2WFPIMlihKsslpNF/yUJ1acTPp4hs3YLYpCcnqKnw== X-Received: by 2002:a05:6512:488:b0:50b:d764:2916 with SMTP id v8-20020a056512048800b0050bd7642916mr403101lfq.174.1701429768197; Fri, 01 Dec 2023 03:22:48 -0800 (PST) Received: from orome.fritz.box (p200300e41f0fa600f22f74fffe1f3a53.dip0.t-ipconnect.de. [2003:e4:1f0f:a600:f22f:74ff:fe1f:3a53]) by smtp.gmail.com with ESMTPSA id r3-20020aa7d143000000b0053de19620b9sm1523779edo.2.2023.12.01.03.22.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 01 Dec 2023 03:22:47 -0800 (PST) Date: Fri, 1 Dec 2023 12:22:45 +0100 From: Thierry Reding To: Jason Gunthorpe Cc: David Airlie , Alyssa Rosenzweig , Albert Ou , asahi@lists.linux.dev, Catalin Marinas , Danilo Krummrich , Daniel Vetter , Dexuan Cui , devicetree@vger.kernel.org, dmaengine@vger.kernel.org, dri-devel@lists.freedesktop.org, David Woodhouse , Frank Rowand , Hanjun Guo , Haiyang Zhang , iommu@lists.linux.dev, Jon Hunter , Joerg Roedel , Karol Herbst , Krzysztof Kozlowski , "K. Y. Srinivasan" , Laxman Dewangan , Len Brown , linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-hyperv@vger.kernel.org, linux-mips@vger.kernel.org, linux-riscv@lists.infradead.org, linux-snps-arc@lists.infradead.org, linux-tegra@vger.kernel.org, Russell King , Lorenzo Pieralisi , Lyude Paul , Marek Szyprowski , nouveau@lists.freedesktop.org, Palmer Dabbelt , Paul Walmsley , "Rafael J. Wysocki" , Rob Herring , Robin Murphy , Sudeep Holla , Suravee Suthikulpanit , Sven Peter , Thomas Bogendoerfer , Vineet Gupta , Vinod Koul , Wei Liu , Will Deacon , Lu Baolu , Christoph Hellwig , Jerry Snitselaar , Hector Martin , Moritz Fischer , patches@lists.linux.dev, "Rafael J. Wysocki" , Rob Herring Subject: Re: [PATCH 08/10] iommu/tegra: Use tegra_dev_iommu_get_stream_id() in the remaining places Message-ID: References: <0-v1-720585788a7d+811b-iommu_fwspec_p1_jgg@nvidia.com> <8-v1-720585788a7d+811b-iommu_fwspec_p1_jgg@nvidia.com> <20231129192603.GA1387263@nvidia.com> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="TpTVMO/RkXn9/fAN" Content-Disposition: inline In-Reply-To: <20231129192603.GA1387263@nvidia.com> User-Agent: Mutt/2.2.12 (2023-09-09) --TpTVMO/RkXn9/fAN Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Nov 29, 2023 at 03:26:03PM -0400, Jason Gunthorpe wrote: > On Wed, Nov 29, 2023 at 05:23:13PM +0100, Thierry Reding wrote: > > > diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/t= egra186.c > > > index 533f85a4b2bdb7..3e4fbe94dd666e 100644 > > > --- a/drivers/memory/tegra/tegra186.c > > > +++ b/drivers/memory/tegra/tegra186.c > > > @@ -111,21 +111,21 @@ static void tegra186_mc_client_sid_override(str= uct tegra_mc *mc, > > > static int tegra186_mc_probe_device(struct tegra_mc *mc, struct devi= ce *dev) > > > { > > > #if IS_ENABLED(CONFIG_IOMMU_API) > > > - struct iommu_fwspec *fwspec =3D dev_iommu_fwspec_get(dev); > > > struct of_phandle_args args; > > > unsigned int i, index =3D 0; > > > + u32 sid; > > > =20 > > > + WARN_ON(!tegra_dev_iommu_get_stream_id(dev, &sid)); > >=20 > > I know the code previously didn't check for any errors, but we may want > > to do so now. If tegra_dev_iommu_get_stream_id() ever fails we may end > > up writing some undefined value into the override register. >=20 > My assumption was it never fails otherwise this probably already > doesn't work? I guess the point I was trying to make is that previously we would not have written anything to the stream ID register and so ignoring the error here might end up writing to a register that previously we would not have written to. Looking at the current code more closely I see now that the reason why we wouldn't have written to the register is because we would've crashed before. So I think this okay. >=20 > > I'm also unsure if WARN_ON() is appropriate here. I vaguely recall that > > ->probe_device() was called for all devices on the bus and not all of > > them may have been associated with the IOMMU. Not all of them may in > > fact access memory in the first place. >=20 > So you are thinkin that of_parse_phandle_with_args() is a NOP > sometimes so it will tolerate the failure? >=20 > Seems like the best thing to do is just continue to ignore it then? Yeah, exactly. It would've just skipped over everything, basically. > > Perhaps I'm misremembering and the IOMMU core now takes care of only > > calling this when fwspec is indeed valid? >=20 > Can't advise, I have no idea what tegra_mc_ops is for :) In a nutshell, it's a hook that allows us to configure the memory controller when a device is attached to the IOMMU. The memory controller contains a set of registers that specify which memory client uses which stream ID by default. For some devices this can be overridden (which is where tegra_dev_iommu_get_stream_id() comes into play in those drivers) and for other devices we can't override, which is when the memory controller defaults come into play. Anyway, I took a closer look at this and ran some tests. Turns out that tegra186_mc_probe_device() really only gets called for devices that have their fwspec properly initialized anyway, so I don't think there's anything special we need to do here. Strictly from a static analysis point of view I suppose we could now have a situation that sid is uninitialized when the call to tegra_dev_iommu_get_stream_id() fails and so using it in the loop is not correct, theoretically, but I think that's just not a case that we'll ever hit in practice. So either way is fine with me. I have a slight preference for just returning 0 in case tegra_dev_iommu_get_stream_id() fails, because it's simple to do and avoids any of these (theoretical) ambiguities. So whichever way you decide: Reviewed-by: Thierry Reding --TpTVMO/RkXn9/fAN Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmVpwgIACgkQ3SOs138+ s6HXLg/9HWFRkWBCsjCmwhdNLy/49eLCajrcOU069eBFBS0YM+rhIa4d/XwXv96C WEC1AzUuDGAVKvfWbkjzCOtBKr+o0psunt8sHYRX+PtyekOF270j3ud+7Ny/6dQk Ca9GevvG0yyYfEcSiowRZXKSzrhj4OSDS83QJbmBiyw3+VyA25+zO/C+Lzib1mG9 2Kn+od5hqOQFqylwazJAZy358DzVSmyF6iSR0kbmS5mvNrtWS/dT4Zeh2raYJgRn MF+f0u+1M8i7Iv65/I5sG83I086p9ictlV1qkMGHY01q7uCAo5p2EM6KlP8Qv/N1 W+DZQpciP9+ENmkcYjEiNEnNw4efMQThCtXbB4VYxb8Jo/To8eV2/sOGUoLdhwWV H8tPPCBfzYtSAsSNXpYK8gWQXCfaHjuO3SFe0itosbSHYw4x+SoECXTVi8L6GBwV jDEYuyrcakUhR+vsxuOXlP4TzcIiNoCf1lO8LnfWVjoHcA/1dG1uA6Bup6CtNe+A lS3xMmMXDVdZS49hw5EvUTd7Liu+si9RzLkmv4IGWBXlL01VTFYRygKZ1lZLKgPP lZWUFLZPunTK7Mlew7PLW7GJg1MzsEkM3htQmVfWzPL7D95RKk3ufiOimGdzknro ZaxbslDQOTUAoqgnKVnmzMJbfR+HTasb+X8EuoCb4G9dHDEPq1g= =hKzE -----END PGP SIGNATURE----- --TpTVMO/RkXn9/fAN--