[PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
Previously the driver relies on bus_set_iommu() in .probe() to call
in .probe_device() function so each client can poll iommus property
in DTB to configure fwspec via tegra_smmu_configure(). According to
the comments in .probe(), this is a bit of a hack. And this doesn't
work for a client that doesn't exist in DTB, PCI device for example.
Actually when a device/client gets probed, the of_iommu_configure()
will call in .probe_device() function again, with a prepared fwspec
from of_iommu_configure() that reads the SWGROUP id in DTB as we do
in tegra-smmu driver.
Additionally, as new helper function tegra_get_memory_controller()
is introduced, there's no need to poll the iommus property in order
to get mc->smmu pointers or SWGROUP id.
This patch reworks .probe_device() and .attach_dev() by doing:
1) Using fwspec to get swgroup id in .attach_dev/.dettach_dev()
2) Removing DT polling code, tegra_smmu_find/tegra_smmu_configure()
3) Calling tegra_get_memory_controller() in .probe_device() instead
4) Also dropping the hack in .probe() that's no longer needed.
Signed-off-by: Nicolin Chen
---
Changelog
v1->v2
* Replaced in .probe_device() tegra_smmu_find/tegra_smmu_configure()
with tegra_get_memory_controller call.
* Dropped the hack in tegra_smmu_probe().
drivers/iommu/tegra-smmu.c | 144 ++---
1 file changed, 36 insertions(+), 108 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..cf4981369828 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -61,6 +61,8 @@ struct tegra_smmu_as {
u32 attr;
};
+static const struct iommu_ops tegra_smmu_ops;
+
static struct tegra_smmu_as *to_smmu_as(struct iommu_domain *dom)
{
return container_of(dom, struct tegra_smmu_as, domain);
@@ -484,60 +486,50 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu
*smmu,
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 || fwspec->ops != &tegra_smmu_ops)
+ return -ENOENT;
+ for (index = 0; index < fwspec->num_ids; index++) {
err = tegra_smmu_as_prepare(smmu, as);
- if (err < 0)
- return err;
+ if (err)
+ goto disable;
- tegra_smmu_enable(smmu, swgroup, as->id);
- index++;
+ tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
}
if (index == 0)
return -ENODEV;
return 0;
+
+disable:
+ while (index--) {
+ tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
+ tegra_smmu_as_unprepare(smmu, as);
+ }
+
+ return err;
}
static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device
*dev)
{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct tegra_smmu_as *as = to_smmu_as(domain);
- struct device_node *np = dev->of_node;
struct tegra_smmu *smmu = as->smmu;
- struct of_phandle_args args;
unsigned int index = 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 || fwspec->ops != &tegra_smmu_ops)
+ return;
- tegra_smmu_disable(smmu, swgroup, as->id);
+ for (index = 0; index < fwspec->num_ids; index++) {
+ tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
tegra_smmu_as_unprepare(smmu, as);
- index++;
}
}
@@ -807,80 +799,26 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct
iommu_domain *domain,
return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
}
-static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
-{
- struct platform_device *pdev;
- struct tegra_mc *mc;
-
- pdev = of_find_device_by_node(np);
- if (!pdev)
- return NULL;
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 03:30, Nicolin Chen пишет: > static void tegra_smmu_release_device(struct device *dev) The tegra_get_memory_controller() uses of_find_device_by_node(), hence tegra_smmu_release_device() should put_device(mc) in order to balance back the refcounting. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 03:30, Nicolin Chen пишет: > + /* An invalid mc pointer means mc and smmu drivers are not ready */ > + if (IS_ERR_OR_NULL(mc)) tegra_get_memory_controller() doesn't return NULL. > + return ERR_PTR(-EPROBE_DEFER); ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 08:10, Dmitry Osipenko пишет: > 30.09.2020 03:30, Nicolin Chen пишет: >> static void tegra_smmu_release_device(struct device *dev) > > The tegra_get_memory_controller() uses of_find_device_by_node(), hence > tegra_smmu_release_device() should put_device(mc) in order to balance > back the refcounting. > Actually, the put_device(mc) should be right after tegra_get_memory_controller() in tegra_smmu_probe_device() because SMMU is a part of MC, hence MC can't just go away. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 03:30, Nicolin Chen пишет: > + /* > + * IOMMU core allows -ENODEV return to carry on. So bypass any call > + * from bus_set_iommu() during tegra_smmu_probe(), as a device will > + * call in again via of_iommu_configure when fwspec is prepared. > + */ > + if (!mc->smmu || !fwspec || fwspec->ops != &tegra_smmu_ops) > return ERR_PTR(-ENODEV); The !mc->smmu can't be true. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 03:30, 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 || fwspec->ops != &tegra_smmu_ops)
> + return -ENOENT;
s/&tegra_smmu_ops/smmu->iommu.ops/
Secondly, is it even possible that fwspec could be NULL here or that
fwspec->ops != smmu->ops?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 08:24:02AM +0300, Dmitry Osipenko wrote: > 30.09.2020 03:30, Nicolin Chen пишет: > > + /* > > +* IOMMU core allows -ENODEV return to carry on. So bypass any call > > +* from bus_set_iommu() during tegra_smmu_probe(), as a device will > > +* call in again via of_iommu_configure when fwspec is prepared. > > +*/ > > + if (!mc->smmu || !fwspec || fwspec->ops != &tegra_smmu_ops) > > return ERR_PTR(-ENODEV); > > The !mc->smmu can't be true. Are you sure? I have removed the "mc->smmu = smmu" in probe() with this change. So the only time "mc->smmu == !NULL" is after probe() of SMMU driver is returned. As my comments says, tegra_smmu_probe() calls in this function via bus_set_iommu(), so mc->smmu can be NULL in this case. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 08:39:54AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 03:30, 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 || fwspec->ops != &tegra_smmu_ops)
> > + return -ENOENT;
>
> s/&tegra_smmu_ops/smmu->iommu.ops/
>
> Secondly, is it even possible that fwspec could be NULL here or that
> fwspec->ops != smmu->ops?
I am following what's in the arm-smmu driver, as I think it'd be
a common practice to do such a check in such a way.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 08:39, Nicolin Chen пишет: > On Wed, Sep 30, 2020 at 08:24:02AM +0300, Dmitry Osipenko wrote: >> 30.09.2020 03:30, Nicolin Chen пишет: >>> + /* >>> +* IOMMU core allows -ENODEV return to carry on. So bypass any call >>> +* from bus_set_iommu() during tegra_smmu_probe(), as a device will >>> +* call in again via of_iommu_configure when fwspec is prepared. >>> +*/ >>> + if (!mc->smmu || !fwspec || fwspec->ops != &tegra_smmu_ops) >>> return ERR_PTR(-ENODEV); >> >> The !mc->smmu can't be true. > > Are you sure? I have removed the "mc->smmu = smmu" in probe() with > this change. So the only time "mc->smmu == !NULL" is after probe() > of SMMU driver is returned. As my comments says, tegra_smmu_probe() > calls in this function via bus_set_iommu(), so mc->smmu can be NULL > in this case. > I missed that. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 08:11:52AM +0300, Dmitry Osipenko wrote: > 30.09.2020 03:30, Nicolin Chen пишет: > > + /* An invalid mc pointer means mc and smmu drivers are not ready */ > > + if (IS_ERR_OR_NULL(mc)) > > tegra_get_memory_controller() doesn't return NULL. Well, I don't want to assume that it'd do that forever, and the NULL check of IS_ERR_OR_NULL is marked "unlikely" so it doesn't hurt to have. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 08:20:50AM +0300, Dmitry Osipenko wrote: > 30.09.2020 08:10, Dmitry Osipenko пишет: > > 30.09.2020 03:30, Nicolin Chen пишет: > >> static void tegra_smmu_release_device(struct device *dev) > > > > The tegra_get_memory_controller() uses of_find_device_by_node(), hence > > tegra_smmu_release_device() should put_device(mc) in order to balance > > back the refcounting. > > > > Actually, the put_device(mc) should be right after > tegra_get_memory_controller() in tegra_smmu_probe_device() because SMMU > is a part of MC, hence MC can't just go away. Will add it. Thanks for pointing it out! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 08:41, Nicolin Chen пишет:
> On Wed, Sep 30, 2020 at 08:39:54AM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 03:30, 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 || fwspec->ops != &tegra_smmu_ops)
>>> + return -ENOENT;
>>
>> s/&tegra_smmu_ops/smmu->iommu.ops/
>>
>> Secondly, is it even possible that fwspec could be NULL here or that
>> fwspec->ops != smmu->ops?
>
> I am following what's in the arm-smmu driver, as I think it'd be
> a common practice to do such a check in such a way.
>
Please check whether it's really needed. It looks like it was needed
sometime ago, but that's not true anymore.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 08:49, Nicolin Chen пишет: > On Wed, Sep 30, 2020 at 08:11:52AM +0300, Dmitry Osipenko wrote: >> 30.09.2020 03:30, Nicolin Chen пишет: >>> + /* An invalid mc pointer means mc and smmu drivers are not ready */ >>> + if (IS_ERR_OR_NULL(mc)) >> >> tegra_get_memory_controller() doesn't return NULL. > > Well, I don't want to assume that it'd do that forever, and the > NULL check of IS_ERR_OR_NULL is marked "unlikely" so it doesn't > hurt to have. > I don't see any reasons why it won't do that forever. Secondly, public function can't be changed randomly without updating all the callers. Hence there is no need to handle cases that can't ever happen and it hurts readability of the code + original error code is missed. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 09:10:38AM +0300, Dmitry Osipenko wrote: > 30.09.2020 08:49, Nicolin Chen пишет: > > On Wed, Sep 30, 2020 at 08:11:52AM +0300, Dmitry Osipenko wrote: > >> 30.09.2020 03:30, Nicolin Chen пишет: > >>> + /* An invalid mc pointer means mc and smmu drivers are not ready */ > >>> + if (IS_ERR_OR_NULL(mc)) > >> > >> tegra_get_memory_controller() doesn't return NULL. > > > > Well, I don't want to assume that it'd do that forever, and the > > NULL check of IS_ERR_OR_NULL is marked "unlikely" so it doesn't > > hurt to have. > > > > I don't see any reasons why it won't do that forever. > > Secondly, public function can't be changed randomly without updating all > the callers. > > Hence there is no need to handle cases that can't ever happen and it > hurts readability of the code + original error code is missed. I don't quite understand why an extra "_OR_NULL" would hurt readabilitybut I'd take a step back and use IS_ERR(). ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 08:59:45AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 08:41, Nicolin Chen пишет:
> > On Wed, Sep 30, 2020 at 08:39:54AM +0300, Dmitry Osipenko wrote:
> >> 30.09.2020 03:30, 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 || fwspec->ops != &tegra_smmu_ops)
> >>> + return -ENOENT;
> >>
> >> s/&tegra_smmu_ops/smmu->iommu.ops/
> >>
> >> Secondly, is it even possible that fwspec could be NULL here or that
> >> fwspec->ops != smmu->ops?
> >
> > I am following what's in the arm-smmu driver, as I think it'd be
> > a common practice to do such a check in such a way.
> >
>
> Please check whether it's really needed. It looks like it was needed
> sometime ago, but that's not true anymore.
Given that most iommu drivers have ->ops check, I'd like to
have it also for safety. If someday that's not true anymore,
I'd expect someone to update all existing drivers.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 09:13, Nicolin Chen пишет: > On Wed, Sep 30, 2020 at 09:10:38AM +0300, Dmitry Osipenko wrote: >> 30.09.2020 08:49, Nicolin Chen пишет: >>> On Wed, Sep 30, 2020 at 08:11:52AM +0300, Dmitry Osipenko wrote: 30.09.2020 03:30, Nicolin Chen пишет: > + /* An invalid mc pointer means mc and smmu drivers are not ready */ > + if (IS_ERR_OR_NULL(mc)) tegra_get_memory_controller() doesn't return NULL. >>> >>> Well, I don't want to assume that it'd do that forever, and the >>> NULL check of IS_ERR_OR_NULL is marked "unlikely" so it doesn't >>> hurt to have. >>> >> >> I don't see any reasons why it won't do that forever. >> >> Secondly, public function can't be changed randomly without updating all >> the callers. >> >> Hence there is no need to handle cases that can't ever happen and it >> hurts readability of the code + original error code is missed. > > I don't quite understand why an extra "_OR_NULL" would hurt > readabilitybut I'd take a step back and use IS_ERR(). > The tegra_get_memory_controller() doesn't return NULL, hence the NULL-check is misleading. If I was reading that code for the first time and notice such a thing, then instantly I'd have a much lower credibility to the whole code. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 08:20:50AM +0300, Dmitry Osipenko wrote: > 30.09.2020 08:10, Dmitry Osipenko пишет: > > 30.09.2020 03:30, Nicolin Chen пишет: > >> static void tegra_smmu_release_device(struct device *dev) > > > > The tegra_get_memory_controller() uses of_find_device_by_node(), hence > > tegra_smmu_release_device() should put_device(mc) in order to balance > > back the refcounting. > > > > Actually, the put_device(mc) should be right after > tegra_get_memory_controller() in tegra_smmu_probe_device() because SMMU > is a part of MC, hence MC can't just go away. Hmm..I found that there is no put_device() call in tegra20-devfreq. Should we just put_device(pdev->dev) in tegra_get_memory_controller? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
