Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

From: Dmitry Osipenko <digetx@gmail.com>
To: Nicolin Chen <nicoleotsuka@gmail.com>
Cc: thierry.reding@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()
Date: Mon, 5 Oct 2020 11:41:08 +0300	[thread overview]
Message-ID: <689c3d90-e05c-d36a-bf37-0bec100040f5@gmail.com> (raw)
In-Reply-To: <20201004215731.GA21420@Asurada-Nvidia>

05.10.2020 00:57, Nicolin Chen пишет:
> On Sat, Oct 03, 2020 at 05:06:42PM +0300, Dmitry Osipenko wrote:
>> 03.10.2020 09:59, Nicolin Chen пишет:
>>>  static int tegra_smmu_of_xlate(struct device *dev,
>>>  			       struct of_phandle_args *args)
>>>  {
>>> +	struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
>>> +	struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
>>>  	u32 id = args->args[0];
>>>  
>>> +	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.
> 
> 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.

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.

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.

The MC is created early and at that time everything is sequential, so
it's indeed should be safe to remove the check.

>> I'm asking this question second time now, please don't ignore review
>> comments next time.
> 
> I think I missed your reply or misunderstood it.
> 

Okay!