Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

[Dmitry Osipenko]

03.10.2020 02:53, Nicolin Chen пишет:
> On Fri, Oct 02, 2020 at 11:12:18PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 22:45, Nicolin Chen пишет:
>>> On Fri, Oct 02, 2020 at 05:41:50PM +0300, Dmitry Osipenko wrote:
>>>> 02.10.2020 09:08, Nicolin Chen пишет:
>>>>>  static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>>>>>  				 struct device *dev)
>>>>>  {
>>>>> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>>>>>  	struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>>>>>  	struct tegra_smmu_as *as = to_smmu_as(domain);
>>>>> -	struct device_node *np = dev->of_node;
>>>>> -	struct of_phandle_args args;
>>>>>  	unsigned int index = 0;
>>>>>  	int err = 0;
>>>>>  
>>>>> -	while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
>>>>> -					   &args)) {
>>>>> -		unsigned int swgroup = args.args[0];
>>>>> -
>>>>> -		if (args.np != smmu->dev->of_node) {
>>>>> -			of_node_put(args.np);
>>>>> -			continue;
>>>>> -		}
>>>>> -
>>>>> -		of_node_put(args.np);
>>>>> +	if (!fwspec)
>>>>> +		return -ENOENT;
>>>>
>>>> Could the !fwspec ever be true here as well?
>>>
>>> There are multiple callers of this function. It's really not that
>>> straightforward to track every one of them. So I'd rather have it
>>> here as other iommu drivers do. We are human beings, so we could
>>> have missed something somewhere, especially callers are not from
>>> tegra-* drivers.
>>>
>>
>> I'm looking at the IOMMU core and it requires device to be in IOMMU
>> group before attach_dev() could be called.
>>
>> The group can't be assigned to device without the fwspec, see
>> tegra_smmu_device_group().
>>
>> Seems majority of IOMMU drivers are checking dev_iommu_priv_get() for
>> NULL in attach_dev(), some not checking anything, some check both and
>> only arm-smmu checks the fwspec.
> 
> As I said a couple of days ago, I don't like to assume that the
> callers won't change. And this time, it's from open code. So I
> don't want to assume that there won't be a change.
> 
> If you are confident that there is no need to add such a check,
> please send patches to remove those checks in those drivers to
> see if others would agree. I would be willing to remove it after
> that. Otherwise, I'd like to keep this.
> 
> Thanks for the review.
> 

I haven't tried to check every code path very thoroughly, expecting you
to do it since you're making this patch. Maybe there is a real reason
why majority of drivers do the checks and it would be good to know why.
Although, it's not critical in this particular case and indeed the
checks could be improved later on.

It looks to me that at least will be a bit better/cleaner to check the
dev_iommu_priv_get() for NULL instead of fwspec because the private
variable depends on the fwspec presence and there is a similar check in
probe_device, hence checks will be more consistent.