linux-kernel - Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 30 Sep 2020 19:48:50 -0700
From:   Nicolin Chen <nicoleotsuka@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Thierry Reding <thierry.reding@...il.com>, joro@...tes.org,
        krzk@...nel.org, vdumpa@...dia.com, jonathanh@...dia.com,
        linux-tegra@...r.kernel.org, iommu@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and
 .attach_dev

On Thu, Oct 01, 2020 at 05:06:19AM +0300, Dmitry Osipenko wrote:
> 01.10.2020 04:26, Nicolin Chen пишет:
> > On Thu, Oct 01, 2020 at 12:56:46AM +0300, Dmitry Osipenko wrote:
> >> 01.10.2020 00:32, Nicolin Chen пишет:
> >>> On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote:
> >>>> ...
> >>>>>> It looks to me like the only reason why you need this new global API is
> >>>>>> because PCI devices may not have a device tree node with a phandle to
> >>>>>> the IOMMU. However, SMMU support for PCI will only be enabled if the
> >>>>>> root complex has an iommus property, right? In that case, can't we
> >>>>>> simply do something like this:
> >>>>>>
> >>>>>> 	if (dev_is_pci(dev))
> >>>>>> 		np = find_host_bridge(dev)->of_node;
> >>>>>> 	else
> >>>>>> 		np = dev->of_node;
> >>>>>>
> >>>>>> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
> >>>>>> sure that exists.
> >>>>>>
> >>>>>> Once we have that we can still iterate over the iommus property and do
> >>>>>> not need to rely on this global variable.
> >>>>>
> >>>>> I agree that it'd work. But I was hoping to simplify the code
> >>>>> here if it's possible. Looks like we have an argument on this
> >>>>> so I will choose to go with your suggestion above for now.
> >>>>
> >>>> This patch removed more lines than were added. If this will be opposite
> >>>> for the Thierry's suggestion, then it's probably not a great suggestion.
> >>>
> >>> Sorry, I don't quite understand this comments. Would you please
> >>> elaborate what's this "it" being "not a great suggestion"?
> >>>
> >>
> >> I meant that you should try to implement Thierry's solution, but if the
> >> end result will be worse than the current patch, then you shouldn't make
> >> a v4, but get back to this discussion in order to choose the best option
> >> and make everyone agree on it.
> > 
> > I see. Thanks for the reply. And here is a sample implementation:
> 
> That's what I supposed to happen :) The new variant adds code and
> complexity, while old did the opposite. Hence the old variant is clearly
> more attractive, IMO.

I personally am not a fan of adding a path for PCI device either,
since PCI/IOMMU cores could have taken care of it while the same
path can't be used for other buses.

If we can't come to an agreement on globalizing mc pointer, would
it be possible to pass tegra_mc_driver through tegra_smmu_probe()
so we can continue to use driver_find_device_by_fwnode() as v1?

v1: https://lkml.org/lkml/2020/9/26/68