| 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
| ||
|
Message-ID: <X+MBcmzQn9iQWlVZ@chromium.org>
Date: Wed, 23 Dec 2020 17:36:02 +0900
From: Tomasz Figa <tfiga@...omium.org>
To: Yong Wu <yong.wu@...iatek.com>
Cc: Joerg Roedel <joro@...tes.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Rob Herring <robh+dt@...nel.org>,
Will Deacon <will@...nel.org>,
Robin Murphy <robin.murphy@....com>, youlin.pei@...iatek.com,
devicetree@...r.kernel.org,
Nicolas Boichat <drinkcat@...omium.org>,
srv_heupstream@...iatek.com, chao.hao@...iatek.com,
linux-kernel@...r.kernel.org, Evan Green <evgreen@...omium.org>,
Tomasz Figa <tfiga@...gle.com>,
iommu@...ts.linux-foundation.org,
linux-mediatek@...ts.infradead.org,
Krzysztof Kozlowski <krzk@...nel.org>, anan.sun@...iatek.com,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v5 18/27] iommu/mediatek: Add power-domain operation
On Wed, Dec 09, 2020 at 04:00:53PM +0800, Yong Wu wrote:
> In the previous SoC, the M4U HW is in the EMI power domain which is
> always on. the latest M4U is in the display power domain which may be
> turned on/off, thus we have to add pm_runtime interface for it.
>
> When the engine work, the engine always enable the power and clocks for
> smi-larb/smi-common, then the M4U's power will always be powered on
> automatically via the device link with smi-common.
>
> Note: we don't enable the M4U power in iommu_map/unmap for tlb flush.
> If its power already is on, of course it is ok. if the power is off,
> the main tlb will be reset while M4U power on, thus the tlb flush while
> m4u power off is unnecessary, just skip it.
>
> There will be one case that pm runctime status is not expected when tlb
> flush. After boot, the display may call dma_alloc_attrs before it call
> pm_runtime_get(disp-dev), then the m4u's pm status is not active inside
> the dma_alloc_attrs. Since it only happens after boot, the tlb is clean
> at that time, I also think this is ok.
>
> Signed-off-by: Yong Wu <yong.wu@...iatek.com>
> ---
> drivers/iommu/mtk_iommu.c | 41 +++++++++++++++++++++++++++++++++------
> 1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 6fe3ee2b2bf5..0e9c03cbab32 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -184,6 +184,8 @@ static void mtk_iommu_tlb_flush_all(void *cookie)
> struct mtk_iommu_data *data = cookie;
>
> for_each_m4u(data) {
> + if (!pm_runtime_active(data->dev))
> + continue;
Is it guaranteed that the status is active in the check above, but then
the process is preempted and it goes down here?
Shouldn't we do something like below?
ret = pm_runtime_get_if_active();
if (!ret)
continue;
if (ret < 0)
// handle error
// Flush
pm_runtime_put();
Similar comment to the other places being changed by this patch.
> writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0,
> data->base + data->plat_data->inv_sel_reg);
> writel_relaxed(F_ALL_INVLD, data->base + REG_MMU_INVALIDATE);
> @@ -200,6 +202,10 @@ static void mtk_iommu_tlb_flush_range_sync(unsigned long iova, size_t size,
> u32 tmp;
>
> for_each_m4u(data) {
> + /* skip tlb flush when pm is not active. */
> + if (!pm_runtime_active(data->dev))
> + continue;
> +
> spin_lock_irqsave(&data->tlb_lock, flags);
> writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0,
> data->base + data->plat_data->inv_sel_reg);
> @@ -384,6 +390,8 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain,
> {
> struct mtk_iommu_data *data = dev_iommu_priv_get(dev);
> struct mtk_iommu_domain *dom = to_mtk_domain(domain);
> + struct device *m4udev = data->dev;
> + bool pm_enabled = pm_runtime_enabled(m4udev);
> int ret;
>
> if (!data)
> @@ -391,12 +399,25 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain,
>
> /* Update the pgtable base address register of the M4U HW */
> if (!data->m4u_dom) {
> + if (pm_enabled) {
> + ret = pm_runtime_get_sync(m4udev);
> + if (ret < 0) {
> + pm_runtime_put_noidle(m4udev);
> + return ret;
> + }
> + }
> ret = mtk_iommu_hw_init(data);
> - if (ret)
> + if (ret) {
> + if (pm_enabled)
> + pm_runtime_put(m4udev);
> return ret;
> + }
> data->m4u_dom = dom;
> writel(dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK,
> data->base + REG_MMU_PT_BASE_ADDR);
> +
> + if (pm_enabled)
> + pm_runtime_put(m4udev);
> }
>
> mtk_iommu_config(data, dev, true);
> @@ -747,10 +768,13 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> if (dev->pm_domain) {
> struct device_link *link;
>
> + pm_runtime_enable(dev);
> +
> link = device_link_add(data->smicomm_dev, dev,
> DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
> if (!link) {
> dev_err(dev, "Unable link %s.\n", dev_name(data->smicomm_dev));
> + pm_runtime_disable(dev);
> return -EINVAL;
> }
> }
> @@ -785,8 +809,10 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> out_sysfs_remove:
> iommu_device_sysfs_remove(&data->iommu);
> out_link_remove:
> - if (dev->pm_domain)
> + if (dev->pm_domain) {
> device_link_remove(data->smicomm_dev, dev);
> + pm_runtime_disable(dev);
> + }
> return ret;
> }
>
> @@ -801,8 +827,10 @@ static int mtk_iommu_remove(struct platform_device *pdev)
> bus_set_iommu(&platform_bus_type, NULL);
>
> clk_disable_unprepare(data->bclk);
> - if (pdev->dev.pm_domain)
> + if (pdev->dev.pm_domain) {
> device_link_remove(data->smicomm_dev, &pdev->dev);
> + pm_runtime_disable(&pdev->dev);
> + }
> devm_free_irq(&pdev->dev, data->irq, data);
> component_master_del(&pdev->dev, &mtk_iommu_com_ops);
> return 0;
> @@ -834,6 +862,9 @@ static int __maybe_unused mtk_iommu_runtime_resume(struct device *dev)
> void __iomem *base = data->base;
> int ret;
>
> + /* Avoid first resume to affect the default value of registers below. */
> + if (!m4u_dom)
> + return 0;
> ret = clk_prepare_enable(data->bclk);
> if (ret) {
> dev_err(data->dev, "Failed to enable clk(%d) in resume\n", ret);
> @@ -847,9 +878,7 @@ static int __maybe_unused mtk_iommu_runtime_resume(struct device *dev)
> writel_relaxed(reg->int_main_control, base + REG_MMU_INT_MAIN_CONTROL);
> writel_relaxed(reg->ivrp_paddr, base + REG_MMU_IVRP_PADDR);
> writel_relaxed(reg->vld_pa_rng, base + REG_MMU_VLD_PA_RNG);
> - if (m4u_dom)
> - writel(m4u_dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK,
> - base + REG_MMU_PT_BASE_ADDR);
> + writel(m4u_dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK, base + REG_MMU_PT_BASE_ADDR);
> return 0;
> }
>
> --
> 2.18.0
>
> _______________________________________________
> iommu mailing list
> iommu@...ts.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
Powered by blists - more mailing lists