Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 5 Oct 2020 12:56:38 +0200
From:   Thierry Reding <thierry.reding@gmail.com>
To:     Dmitry Osipenko <digetx@gmail.com>
Cc:     Nicolin Chen <nicoleotsuka@gmail.com>, joro@8bytes.org,
        vdumpa@nvidia.com, jonathanh@nvidia.com,
        linux-tegra@vger.kernel.org, iommu@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
Message-ID: <20201005105638.GO425362@ulmo>
References: <20201003065947.18671-1-nicoleotsuka@gmail.com>
 <20201003065947.18671-3-nicoleotsuka@gmail.com>
 <4a5a5b1c-080a-327a-1e2f-dc087948e1a1@gmail.com>
 <20201004215731.GA21420@Asurada-Nvidia>
 <689c3d90-e05c-d36a-bf37-0bec100040f5@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="KDy5+/kaQUC2a3Gw"
Content-Disposition: inline
In-Reply-To: <689c3d90-e05c-d36a-bf37-0bec100040f5@gmail.com>
User-Agent: Mutt/1.14.7 (2020-08-29)
Precedence: bulk


--KDy5+/kaQUC2a3Gw
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Oct 05, 2020 at 11:41:08AM +0300, Dmitry Osipenko wrote:
> 05.10.2020 00:57, Nicolin Chen =D0=BF=D0=B8=D1=88=D0=B5=D1=82:
> > On Sat, Oct 03, 2020 at 05:06:42PM +0300, Dmitry Osipenko wrote:
> >> 03.10.2020 09:59, Nicolin Chen =D0=BF=D0=B8=D1=88=D0=B5=D1=82:
> >>>  static int tegra_smmu_of_xlate(struct device *dev,
> >>>  			       struct of_phandle_args *args)
> >>>  {
> >>> +	struct platform_device *iommu_pdev =3D of_find_device_by_node(args-=
>np);
> >>> +	struct tegra_mc *mc =3D platform_get_drvdata(iommu_pdev);
> >>>  	u32 id =3D args->args[0];
> >>> =20
> >>> +	put_device(&iommu_pdev->dev);
> >>> +
> >>> +	if (!mc || !mc->smmu)
> >>> +		return -EPROBE_DEFER;
> >>
> >> I'm not very excited by seeing code in the patches that can't be
> >> explained by the patch authors and will appreciate if you could provide
> >> a detailed explanation about why this NULL checking is needed because I
> >> think it is unneeded, especially given that other IOMMU drivers don't
> >> have such check.
> >=20
> > This function could be called from of_iommu_configure(), which is
> > a part of other driver's probe(). So I think it's safer to have a
> > check. Yet, given mc driver is added to the "arch_initcall" stage,
> > you are probably right that there's no really need at this moment
> > because all clients should be called after mc/smmu are inited. So
> > I'll resend a v6, if that makes you happy.
>=20
> I wanted to get the explanation :) I'm very curious why it's actually
> needed because I'm not 100% sure whether it's not needed at all.
>=20
> I'd assume that the only possible problem could be if some device is
> created in parallel with the MC probing and there is no locking that
> could prevent this in the drivers core. It's not apparent to me whether
> this situation could happen at all in practice.
>=20
> The MC is created early and at that time everything is sequential, so
> it's indeed should be safe to remove the check.

I think I now remember exactly why the "hack" in tegra_smmu_probe()
exists. The reason is that the MC driver does this:

	mc->smmu =3D tegra_smmu_probe(...);

That means that mc->smmu is going to be NULL until tegra_smmu_probe()
has finished. But tegra_smmu_probe() calls bus_set_iommu() and that in
turn calls ->probe_device(). So the purpose of the "hack" in the
tegra_smmu_probe() function was to make sure mc->smmu was available at
that point, because, well, it is already known, but we haven't gotten
around to storing it yet.

->of_xlate() can theoretically be called as early as right after
bus_set_iommu() via of_iommu_configure() if that is called in parallel
with tegra_smmu_probe(). I think that's very unlikely, but I'm not 100%
sure that it can't happen.

In any case, I do agree with Dmitry that we should have a comment here
explaining why this is necessary. Even if we're completely certain that
this is necessary, it's not obvious and therefore should get that
comment. And if we're not certain that it's necessary, it's probably
also good to mention that in the comment so that eventually it can be
determined or the check removed if it proves to be unnecessary.

Thierry

--KDy5+/kaQUC2a3Gw
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAl96++MACgkQ3SOs138+
s6GRgBAAqmH9PfL0fo3tHWKylZ/CG+Es9277l5AGEZHysCC9KkyssEr0Om2gAKZ0
kQKxk5vjbooZbfu+0PWRCJ00FXfXZ4TbwVuyv3xjltqfmqM1RsrMYq/RcmM8bTbh
j62j9Gi9milT8vw+7dNUF+3Ys5ZoK+xcD0ak8KjsvS9ey76vh5EZAH+LX5sdhzdP
CbvEK3MKAd5QAhfTuRpUjl+MtsVvUoqJklLHpO+jE5mSDyZT0HSCmwIO1UYNn58P
VwupVaxbz0aQdBLMCzkGXD7dzE/gsMoOZS3XiS+C4OPp538N60jIgnC6RY/CpITJ
bqrwfr6IUiw5JejUbIacQfqtwLsts8eM23dw8kCDhVl3kqtp+hh1tNvarG30SsO5
HtgYkPjPloGm9bLTQALE0zhacm9fqouGbC+vY6ylrJZ3YpHGFNUCtuw2nFGG9x7t
yhxEiiNu+0syuI3kQ1DN6gbeTdWPTCGoZEHygt92oL2CziPWmyCkwHlhuG88t08h
iotw38Jp0eoYAq7Q02NwUjtXMkRQrb7pMIg8L8SKZ7i2rmDet/vTk5fQ0T22ncsH
lm6mfrBCUvZTNmT2JcEaf8vgncTi+CGIUF+Ex79IOkkaq0DLe82G5CGMxzE+LF9L
oQ3SgSuVeeII+snXcopbqhevrLsTnECBrTOooIOiJiErxXDLFhk=
=AIlO
-----END PGP SIGNATURE-----

--KDy5+/kaQUC2a3Gw--