Re: [PATCH v9 3/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

[Vivek Gautam]

Hi Robin,


On 3/14/2018 11:16 PM, Robin Murphy wrote:
> On 13/03/18 08:55, Vivek Gautam wrote:
> > From: Sricharan R <sricharan@xxxxxxxxxxxxxx>
> >
> > The smmu device probe/remove and add/remove master device callbacks
> > gets called when the smmu is not linked to its master, that is without
> > the context of the master device. So calling runtime apis in those 
> > places
> > separately.
> >
> > Signed-off-by: Sricharan R <sricharan@xxxxxxxxxxxxxx>
> > [vivek: Cleanup pm runtime calls]
> > Signed-off-by: Vivek Gautam <vivek.gautam@xxxxxxxxxxxxxx>
> > Reviewed-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
> > ---
> > Â drivers/iommu/arm-smmu.c | 95 
> > ++++++++++++++++++++++++++++++++++++++++++++----
> > Â 1 file changed, 87 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index d5873d545024..56a04ae80bf3 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -268,6 +268,20 @@ static struct arm_smmu_option_prop 
> > arm_smmu_options[] = {
> > ÂÂÂÂÂ { 0, NULL},
> > Â };
> > Â +static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
> > +{
> > +ÂÂÂ if (pm_runtime_enabled(smmu->dev))
> > +ÂÂÂÂÂÂÂ return pm_runtime_get_sync(smmu->dev);
> > +
> > +ÂÂÂ return 0;
> > +}
> > +
> > +static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
> > +{
> > +ÂÂÂ if (pm_runtime_enabled(smmu->dev))
> > +ÂÂÂÂÂÂÂ pm_runtime_put(smmu->dev);
> > +}
> > +
> > Â static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain 
> > *dom)
> > Â {
> > ÂÂÂÂÂ return container_of(dom, struct arm_smmu_domain, domain);
> > @@ -913,11 +927,15 @@ static void 
> > arm_smmu_destroy_domain_context(struct iommu_domain *domain)
> > ÂÂÂÂÂ struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > ÂÂÂÂÂ struct arm_smmu_device *smmu = smmu_domain->smmu;
> > ÂÂÂÂÂ struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> > -ÂÂÂ int irq;
> > +ÂÂÂ int ret, irq;
> > Â ÂÂÂÂÂ if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
> > ÂÂÂÂÂÂÂÂÂ return;
> > Â +ÂÂÂ ret = arm_smmu_rpm_get(smmu);
> > +ÂÂÂ if (ret < 0)
> > +ÂÂÂÂÂÂÂ return;
> > +
> > ÂÂÂÂÂ /*
> > ÂÂÂÂÂÂ * Disable the context bank and free the page tables before 
> > freeing
> > ÂÂÂÂÂÂ * it.
> > @@ -932,6 +950,8 @@ static void 
> > arm_smmu_destroy_domain_context(struct iommu_domain *domain)
> > Â ÂÂÂÂÂ free_io_pgtable_ops(smmu_domain->pgtbl_ops);
> > ÂÂÂÂÂ __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx);
> > +
> > +ÂÂÂ arm_smmu_rpm_put(smmu);
> > Â }
> > Â Â static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> > @@ -1213,10 +1233,15 @@ static int arm_smmu_attach_dev(struct 
> > iommu_domain *domain, struct device *dev)
> > ÂÂÂÂÂÂÂÂÂ return -ENODEV;
> > Â ÂÂÂÂÂ smmu = fwspec_smmu(fwspec);
> > +
> > +ÂÂÂ ret = arm_smmu_rpm_get(smmu);
> > +ÂÂÂ if (ret < 0)
> > +ÂÂÂÂÂÂÂ return ret;
> > +
> > ÂÂÂÂÂ /* Ensure that the domain is finalised */
> > ÂÂÂÂÂ ret = arm_smmu_init_domain_context(domain, smmu);
> > ÂÂÂÂÂ if (ret < 0)
> > -ÂÂÂÂÂÂÂ return ret;
> > +ÂÂÂÂÂÂÂ goto rpm_put;
> > Â ÂÂÂÂÂ /*
> > ÂÂÂÂÂÂ * Sanity check the domain. We don't support domains across
> > @@ -1230,29 +1255,47 @@ static int arm_smmu_attach_dev(struct 
> > iommu_domain *domain, struct device *dev)
> > ÂÂÂÂÂ }
> > Â ÂÂÂÂÂ /* Looks ok, so add the device to the domain */
> > -ÂÂÂ return arm_smmu_domain_add_master(smmu_domain, fwspec);
> > +ÂÂÂ ret = arm_smmu_domain_add_master(smmu_domain, fwspec);
> > +
> > +rpm_put:
> > +ÂÂÂ arm_smmu_rpm_put(smmu);
> > +ÂÂÂ return ret;
> > Â }
> > Â Â static int arm_smmu_map(struct iommu_domain *domain, unsigned 
> > long iova,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ phys_addr_t paddr, size_t size, int prot)
> > Â {
> > ÂÂÂÂÂ struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
> > +ÂÂÂ struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > +ÂÂÂ struct arm_smmu_device *smmu = smmu_domain->smmu;
>
> Nit: please use arm_smmu_domain for ops as well (as it was before 
> 523d7423e21b), or consistently elide it for smmu - the mixture of both 
> methods is just a horrible mess (here and in unmap).

Sure, will make it consistent for arm_smmu_device (in both places - 
map/unmap)

ÂÂÂÂ struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu;

> > +ÂÂÂ int ret;
> > Â ÂÂÂÂÂ if (!ops)
> > ÂÂÂÂÂÂÂÂÂ return -ENODEV;
> > Â -ÂÂÂ return ops->map(ops, iova, paddr, size, prot);
> > +ÂÂÂ arm_smmu_rpm_get(smmu);
> > +ÂÂÂ ret = ops->map(ops, iova, paddr, size, prot);
> > +ÂÂÂ arm_smmu_rpm_put(smmu);
> > +
> > +ÂÂÂ return ret;
> > Â }
> > Â Â static size_t arm_smmu_unmap(struct iommu_domain *domain, 
> > unsigned long iova,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ size_t size)
> > Â {
> > ÂÂÂÂÂ struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
> > +ÂÂÂ struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > +ÂÂÂ struct arm_smmu_device *smmu = smmu_domain->smmu;
> > +ÂÂÂ size_t ret;
> > Â ÂÂÂÂÂ if (!ops)
> > ÂÂÂÂÂÂÂÂÂ return 0;
> > Â -ÂÂÂ return ops->unmap(ops, iova, size);
> > +ÂÂÂ arm_smmu_rpm_get(smmu);
> > +ÂÂÂ ret = ops->unmap(ops, iova, size);
> > +ÂÂÂ arm_smmu_rpm_put(smmu);
> > +
> > +ÂÂÂ return ret;
> > Â }
> > Â Â static void arm_smmu_iotlb_sync(struct iommu_domain *domain)
> > @@ -1407,14 +1450,22 @@ static int arm_smmu_add_device(struct device 
> > *dev)
> > ÂÂÂÂÂ while (i--)
> > ÂÂÂÂÂÂÂÂÂ cfg->smendx[i] = INVALID_SMENDX;
> > Â +ÂÂÂ ret = arm_smmu_rpm_get(smmu);
> > +ÂÂÂ if (ret < 0)
> > +ÂÂÂÂÂÂÂ goto out_cfg_free;
> > +
> > ÂÂÂÂÂ ret = arm_smmu_master_alloc_smes(dev);
>
> Nit: it would be easier to just do the rpm_put here; then you don't 
> need to mess with the cleanup path.

Sure, will do that. It will be cleaner.

> > ÂÂÂÂÂ if (ret)
> > -ÂÂÂÂÂÂÂ goto out_cfg_free;
> > +ÂÂÂÂÂÂÂ goto out_rpm_put;
> > Â ÂÂÂÂÂ iommu_device_link(&smmu->iommu, dev);
> > Â +ÂÂÂ arm_smmu_rpm_put(smmu);
> > +
> > ÂÂÂÂÂ return 0;
> > Â +out_rpm_put:
> > +ÂÂÂ arm_smmu_rpm_put(smmu);
> > Â out_cfg_free:
> > ÂÂÂÂÂ kfree(cfg);
> > Â out_free:
> > @@ -1427,7 +1478,7 @@ static void arm_smmu_remove_device(struct 
> > device *dev)
> > ÂÂÂÂÂ struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > ÂÂÂÂÂ struct arm_smmu_master_cfg *cfg;
> > ÂÂÂÂÂ struct arm_smmu_device *smmu;
> > -
> > +ÂÂÂ int ret;
> > Â ÂÂÂÂÂ if (!fwspec || fwspec->ops != &arm_smmu_ops)
> > ÂÂÂÂÂÂÂÂÂ return;
> > @@ -1435,8 +1486,15 @@ static void arm_smmu_remove_device(struct 
> > device *dev)
> >  cfg = fwspec->iommu_priv;
> > ÂÂÂÂÂ smmu = cfg->smmu;
> > Â +ÂÂÂ ret = arm_smmu_rpm_get(smmu);
> > +ÂÂÂ if (ret < 0)
> > +ÂÂÂÂÂÂÂ return;
> > +
> > ÂÂÂÂÂ iommu_device_unlink(&smmu->iommu, dev);
> > ÂÂÂÂÂ arm_smmu_master_free_smes(fwspec);
> > +
> > +ÂÂÂ arm_smmu_rpm_put(smmu);
> > +
> > ÂÂÂÂÂ iommu_group_remove_device(dev);
> > ÂÂÂÂÂ kfree(fwspec->iommu_priv);
> > ÂÂÂÂÂ iommu_fwspec_free(dev);
> > @@ -2124,6 +2182,8 @@ static int arm_smmu_device_probe(struct 
> > platform_device *pdev)
> > ÂÂÂÂÂÂÂÂÂ smmu->irqs[i] = irq;
> > ÂÂÂÂÂ }
> > Â +ÂÂÂ platform_set_drvdata(pdev, smmu);
> > +
> > ÂÂÂÂÂ err = devm_clk_bulk_get(smmu->dev, smmu->num_clks, smmu->clks);
> > ÂÂÂÂÂ if (err)
> > ÂÂÂÂÂÂÂÂÂ return err;
> > @@ -2132,6 +2192,19 @@ static int arm_smmu_device_probe(struct 
> > platform_device *pdev)
> > ÂÂÂÂÂ if (err)
> > ÂÂÂÂÂÂÂÂÂ return err;
> > Â +ÂÂÂ /*
> > +ÂÂÂÂ * We want to avoid touching dev->power.lock in fastpaths unless
> > +ÂÂÂÂ * it's really going to do something useful - pm_runtime_enabled()
> > +ÂÂÂÂ * can serve as an ideal proxy for that decision. So, conditionally
> > +ÂÂÂÂ * enable pm_runtime.
> > +ÂÂÂÂ */
> > +ÂÂÂ if (dev->pm_domain)
> > +ÂÂÂÂÂÂÂ pm_runtime_enable(dev);
> > +
> > +ÂÂÂ err = arm_smmu_rpm_get(smmu);
> > +ÂÂÂ if (err < 0)
> > +ÂÂÂÂÂÂÂ return err;
> > +
> > ÂÂÂÂÂ err = arm_smmu_device_cfg_probe(smmu);
> > ÂÂÂÂÂ if (err)
> > ÂÂÂÂÂÂÂÂÂ return err;
> > @@ -2173,10 +2246,11 @@ static int arm_smmu_device_probe(struct 
> > platform_device *pdev)
> > ÂÂÂÂÂÂÂÂÂ return err;
> > ÂÂÂÂÂ }
> > Â -ÂÂÂ platform_set_drvdata(pdev, smmu);
> > ÂÂÂÂÂ arm_smmu_device_reset(smmu);
> > ÂÂÂÂÂ arm_smmu_test_smr_masks(smmu);
> > Â +ÂÂÂ arm_smmu_rpm_put(smmu);
> > +
> > ÂÂÂÂÂ /*
> > ÂÂÂÂÂÂ * For ACPI and generic DT bindings, an SMMU will be probed before
> > ÂÂÂÂÂÂ * any device which might need it, so we want the bus ops in place
> > @@ -2212,8 +2286,13 @@ static int arm_smmu_device_remove(struct 
> > platform_device *pdev)
> > ÂÂÂÂÂ if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
> > ÂÂÂÂÂÂÂÂÂ dev_err(&pdev->dev, "removing device with active domains!\n");
> > Â +ÂÂÂ arm_smmu_rpm_get(smmu);
> > ÂÂÂÂÂ /* Turn the thing off */
> > ÂÂÂÂÂ writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> > +ÂÂÂ arm_smmu_rpm_put(smmu);
> > +
> > +ÂÂÂ if (pm_runtime_enabled(smmu->dev))
> > +ÂÂÂÂÂÂÂ pm_runtime_disable(smmu->dev);
> > Â ÂÂÂÂÂ clk_bulk_unprepare(smmu->num_clks, smmu->clks);
>
> I don't know how runtime and system PM interact - does the reset in 
> arm_smmu_pm_resume need special treatment as well, or is the device 
> guaranteed to be powered up at that point by other means?

So, as Tomasz wrote, we should be okay with this.
Thanks.

regards
Vivek
> Robin.
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-arm-msm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html