[PATCH v2 0/6] iommu/exynos: Convert to a module

[PATCH v2 0/6] iommu/exynos: Convert to a module

[Sam Protsenko]

As exynos-iommu driver is not a critical platform driver, it can be
converted to a loadable module to avoid loading it on non-Exynos
platforms in order to improve the RAM footprint. This patch series
converts it to a module and does some related cleanups. IOMMU/DMA
specifics were taken into the account, so remove/exit methods weren't
added.

There are two drivers using CONFIG_EXYNOS_IOMMU in their code:
DRM_EXYNOS and S5P_MFC. Both were checked, and only a slight change was
needed for S5P_MFC driver (patch #6).

Changes in v2:
  - Extracted the "shutdown" method addition into a separate patch
  - Added MODULE_DEVICE_TABLE(of, ...) to support hot-plug loading
  - Added MODULE_ALIAS("platform:exynos-sysmmu")
  - Added fix for S5P_MFC driver to work correctly with EXYNOS_IOMMU=m
  - Fixed checkpatch coding style suggestion with "--strict" flag
  - Rebased on top of most recent joro/iommu.git:next

Sam Protsenko (6):
  iommu: Export iommu_group_default_domain() API
  iommu/exynos: Fix retval on getting clocks in probe
  iommu/exynos: Modularize the driver
  iommu/exynos: Implement shutdown driver method
  iommu/exynos: Rearrange the platform driver code
  media: platform: Use IS_ENABLED() to check EXYNOS_IOMMU in s5p_mfc

 drivers/iommu/Kconfig                         |   2 +-
 drivers/iommu/exynos-iommu.c                  | 355 +++++++++---------
 drivers/iommu/iommu.c                         |   1 +
 .../platform/samsung/s5p-mfc/s5p_mfc_iommu.h  |   4 +-
 4 files changed, 191 insertions(+), 171 deletions(-)

-- 
2.35.1

[PATCH v2 1/6] iommu: Export iommu_group_default_domain() API

[Sam Protsenko]

iommu_group_default_domain() may be needed for module users. E.g.
exynos-iommu driver is using it right now, and it's going to be
converted to a module soon.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - (none)

 drivers/iommu/iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6ca377f4fbf9..006a65411a28 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1675,6 +1675,7 @@ struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
 {
 	return group->default_domain;
 }
+EXPORT_SYMBOL_GPL(iommu_group_default_domain);
 
 static int probe_iommu_group(struct device *dev, void *data)
 {
-- 
2.35.1

[PATCH v2 2/6] iommu/exynos: Fix retval on getting clocks in probe

[Sam Protsenko]

checkpatch reports next warning for clock getting code in probe
function:

    WARNING: ENOSYS means 'invalid syscall nr' and nothing else

Replace it with -ENOINT to make checkpatch happy.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - (none)

 drivers/iommu/exynos-iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 45fd4850bacb..0d150b383d04 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -689,7 +689,7 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
 
 	if (!data->clk && (!data->aclk || !data->pclk)) {
 		dev_err(dev, "Failed to get device clock(s)!\n");
-		return -ENOSYS;
+		return -ENOENT;
 	}
 
 	data->clk_master = devm_clk_get(dev, "master");
-- 
2.35.1

Re: [PATCH v2 2/6] iommu/exynos: Fix retval on getting clocks in probe

[Krzysztof Kozlowski]

On 03/11/2022 15:51, Sam Protsenko wrote:
> checkpatch reports next warning for clock getting code in probe
> function:
> 
>     WARNING: ENOSYS means 'invalid syscall nr' and nothing else
> 
> Replace it with -ENOINT to make checkpatch happy.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v2 3/6] iommu/exynos: Modularize the driver

[Sam Protsenko]

Rework the driver so it can be built as a loadable module. That can be
useful as not all ARM64 platforms need it. And that's ok for it to be a
module because it's not a critical driver (platform can work when it's
disabled).

Remove method and module exit function are not implemented, as the
removal of IOMMUs cannot be done reliably. As Robin Murphy mentioned in
[1]:

    ...it's better not to even pretend that removing an IOMMU's driver
    while other drivers are using it (usually via DMA ops without even
    realising) is going to have anything other than catastrophic
    results.

[1] https://lore.kernel.org/lkml/20220702213724.3949-2-semen.protsenko@linaro.org/T/#md7e1e3f5b2c9e7fa5bc28fe33e818b6aa4a7237c

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - Add MODULE_DEVICE_TABLE(of, ...) to support hot-plug loading
  - Add MODULE_ALIAS() line
  - Extracted "shutdown" driver method adding into a separate patch

 drivers/iommu/Kconfig        | 2 +-
 drivers/iommu/exynos-iommu.c | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index dc5f7a156ff5..6f7055606679 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -259,7 +259,7 @@ config TEGRA_IOMMU_SMMU
 	  SoCs (Tegra30 up to Tegra210).
 
 config EXYNOS_IOMMU
-	bool "Exynos IOMMU Support"
+	tristate "Exynos IOMMU Support"
 	depends on ARCH_EXYNOS || COMPILE_TEST
 	depends on !CPU_BIG_ENDIAN # revisit driver if we can enable big-endian ptes
 	select IOMMU_API
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 0d150b383d04..aad845677bda 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -16,6 +16,7 @@
 #include <linux/interrupt.h>
 #include <linux/kmemleak.h>
 #include <linux/list.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
@@ -798,8 +799,9 @@ static const struct of_device_id sysmmu_of_match[] = {
 	{ .compatible	= "samsung,exynos-sysmmu", },
 	{ },
 };
+MODULE_DEVICE_TABLE(of, sysmmu_of_match);
 
-static struct platform_driver exynos_sysmmu_driver __refdata = {
+static struct platform_driver exynos_sysmmu_driver = {
 	.probe	= exynos_sysmmu_probe,
 	.driver	= {
 		.name		= "exynos-sysmmu",
@@ -1404,6 +1406,7 @@ static const struct iommu_ops exynos_iommu_ops = {
 	.release_device = exynos_iommu_release_device,
 	.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
 	.of_xlate = exynos_iommu_of_xlate,
+	.owner = THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= exynos_iommu_attach_device,
 		.detach_dev	= exynos_iommu_detach_device,
@@ -1454,3 +1457,7 @@ static int __init exynos_iommu_init(void)
 	return ret;
 }
 core_initcall(exynos_iommu_init);
+
+MODULE_DESCRIPTION("IOMMU driver for Exynos SoCs");
+MODULE_ALIAS("platform:exynos-sysmmu");
+MODULE_LICENSE("GPL");
-- 
2.35.1

Re: [PATCH v2 3/6] iommu/exynos: Modularize the driver

[Krzysztof Kozlowski]

On 03/11/2022 15:51, Sam Protsenko wrote:
> Rework the driver so it can be built as a loadable module. That can be
> useful as not all ARM64 platforms need it. And that's ok for it to be a
> module because it's not a critical driver (platform can work when it's
> disabled).
> 
> Remove method and module exit function are not implemented, as the
> removal of IOMMUs cannot be done reliably. As Robin Murphy mentioned in
> [1]:
> 
>     ...it's better not to even pretend that removing an IOMMU's driver
>     while other drivers are using it (usually via DMA ops without even
>     realising) is going to have anything other than catastrophic
>     results.
> 
> [1] https://lore.kernel.org/lkml/20220702213724.3949-2-semen.protsenko@linaro.org/T/#md7e1e3f5b2c9e7fa5bc28fe33e818b6aa4a7237c
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v2 4/6] iommu/exynos: Implement shutdown driver method

[Sam Protsenko]

While remove method shouldn't be implemented, as it can't be done
reliably, the shutdown method can be useful for performing a kexec.
That was inspired by other IOMMU drivers, see commit 1a4e90f25b2c
("iommu/rockchip: Perform a reset on shutdown") for example.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - New patch

 drivers/iommu/exynos-iommu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index aad845677bda..cd3f74e638f0 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -753,6 +753,16 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static void exynos_sysmmu_shutdown(struct platform_device *pdev)
+{
+	struct sysmmu_drvdata *data = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	int irq = platform_get_irq(pdev, 0);
+
+	devm_free_irq(dev, irq, data);
+	pm_runtime_force_suspend(dev);
+}
+
 static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
 {
 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
@@ -803,6 +813,7 @@ MODULE_DEVICE_TABLE(of, sysmmu_of_match);
 
 static struct platform_driver exynos_sysmmu_driver = {
 	.probe	= exynos_sysmmu_probe,
+	.shutdown = exynos_sysmmu_shutdown,
 	.driver	= {
 		.name		= "exynos-sysmmu",
 		.of_match_table	= sysmmu_of_match,
-- 
2.35.1

Re: [PATCH v2 4/6] iommu/exynos: Implement shutdown driver method

[Krzysztof Kozlowski]

On 03/11/2022 15:51, Sam Protsenko wrote:
> While remove method shouldn't be implemented, as it can't be done
> reliably, the shutdown method can be useful for performing a kexec.
> That was inspired by other IOMMU drivers, see commit 1a4e90f25b2c
> ("iommu/rockchip: Perform a reset on shutdown") for example.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v2 5/6] iommu/exynos: Rearrange the platform driver code

[Sam Protsenko]

Move the platform_driver code to the bottom of the driver, as it's a
canonical form for that. No functional change.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - Fixed checkpatch style suggestion with "--strict" flag

 drivers/iommu/exynos-iommu.c | 361 +++++++++++++++++------------------
 1 file changed, 180 insertions(+), 181 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index cd3f74e638f0..c995cf8294cf 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -641,187 +641,6 @@ static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
 	spin_unlock_irqrestore(&data->lock, flags);
 }
 
-static const struct iommu_ops exynos_iommu_ops;
-
-static int exynos_sysmmu_probe(struct platform_device *pdev)
-{
-	int irq, ret;
-	struct device *dev = &pdev->dev;
-	struct sysmmu_drvdata *data;
-	struct resource *res;
-
-	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	data->sfrbase = devm_ioremap_resource(dev, res);
-	if (IS_ERR(data->sfrbase))
-		return PTR_ERR(data->sfrbase);
-
-	irq = platform_get_irq(pdev, 0);
-	if (irq <= 0)
-		return irq;
-
-	ret = devm_request_irq(dev, irq, exynos_sysmmu_irq, 0,
-				dev_name(dev), data);
-	if (ret) {
-		dev_err(dev, "Unabled to register handler of irq %d\n", irq);
-		return ret;
-	}
-
-	data->clk = devm_clk_get(dev, "sysmmu");
-	if (PTR_ERR(data->clk) == -ENOENT)
-		data->clk = NULL;
-	else if (IS_ERR(data->clk))
-		return PTR_ERR(data->clk);
-
-	data->aclk = devm_clk_get(dev, "aclk");
-	if (PTR_ERR(data->aclk) == -ENOENT)
-		data->aclk = NULL;
-	else if (IS_ERR(data->aclk))
-		return PTR_ERR(data->aclk);
-
-	data->pclk = devm_clk_get(dev, "pclk");
-	if (PTR_ERR(data->pclk) == -ENOENT)
-		data->pclk = NULL;
-	else if (IS_ERR(data->pclk))
-		return PTR_ERR(data->pclk);
-
-	if (!data->clk && (!data->aclk || !data->pclk)) {
-		dev_err(dev, "Failed to get device clock(s)!\n");
-		return -ENOENT;
-	}
-
-	data->clk_master = devm_clk_get(dev, "master");
-	if (PTR_ERR(data->clk_master) == -ENOENT)
-		data->clk_master = NULL;
-	else if (IS_ERR(data->clk_master))
-		return PTR_ERR(data->clk_master);
-
-	data->sysmmu = dev;
-	spin_lock_init(&data->lock);
-
-	__sysmmu_get_version(data);
-
-	ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
-				     dev_name(data->sysmmu));
-	if (ret)
-		return ret;
-
-	ret = iommu_device_register(&data->iommu, &exynos_iommu_ops, dev);
-	if (ret)
-		goto err_iommu_register;
-
-	platform_set_drvdata(pdev, data);
-
-	if (PG_ENT_SHIFT < 0) {
-		if (MMU_MAJ_VER(data->version) < 5) {
-			PG_ENT_SHIFT = SYSMMU_PG_ENT_SHIFT;
-			LV1_PROT = SYSMMU_LV1_PROT;
-			LV2_PROT = SYSMMU_LV2_PROT;
-		} else {
-			PG_ENT_SHIFT = SYSMMU_V5_PG_ENT_SHIFT;
-			LV1_PROT = SYSMMU_V5_LV1_PROT;
-			LV2_PROT = SYSMMU_V5_LV2_PROT;
-		}
-	}
-
-	if (MMU_MAJ_VER(data->version) >= 5) {
-		ret = dma_set_mask(dev, DMA_BIT_MASK(36));
-		if (ret) {
-			dev_err(dev, "Unable to set DMA mask: %d\n", ret);
-			goto err_dma_set_mask;
-		}
-	}
-
-	/*
-	 * use the first registered sysmmu device for performing
-	 * dma mapping operations on iommu page tables (cpu cache flush)
-	 */
-	if (!dma_dev)
-		dma_dev = &pdev->dev;
-
-	pm_runtime_enable(dev);
-
-	return 0;
-
-err_dma_set_mask:
-	iommu_device_unregister(&data->iommu);
-err_iommu_register:
-	iommu_device_sysfs_remove(&data->iommu);
-	return ret;
-}
-
-static void exynos_sysmmu_shutdown(struct platform_device *pdev)
-{
-	struct sysmmu_drvdata *data = platform_get_drvdata(pdev);
-	struct device *dev = &pdev->dev;
-	int irq = platform_get_irq(pdev, 0);
-
-	devm_free_irq(dev, irq, data);
-	pm_runtime_force_suspend(dev);
-}
-
-static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
-{
-	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
-	struct device *master = data->master;
-
-	if (master) {
-		struct exynos_iommu_owner *owner = dev_iommu_priv_get(master);
-
-		mutex_lock(&owner->rpm_lock);
-		if (data->domain) {
-			dev_dbg(data->sysmmu, "saving state\n");
-			__sysmmu_disable(data);
-		}
-		mutex_unlock(&owner->rpm_lock);
-	}
-	return 0;
-}
-
-static int __maybe_unused exynos_sysmmu_resume(struct device *dev)
-{
-	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
-	struct device *master = data->master;
-
-	if (master) {
-		struct exynos_iommu_owner *owner = dev_iommu_priv_get(master);
-
-		mutex_lock(&owner->rpm_lock);
-		if (data->domain) {
-			dev_dbg(data->sysmmu, "restoring state\n");
-			__sysmmu_enable(data);
-		}
-		mutex_unlock(&owner->rpm_lock);
-	}
-	return 0;
-}
-
-static const struct dev_pm_ops sysmmu_pm_ops = {
-	SET_RUNTIME_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume, NULL)
-	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
-				pm_runtime_force_resume)
-};
-
-static const struct of_device_id sysmmu_of_match[] = {
-	{ .compatible	= "samsung,exynos-sysmmu", },
-	{ },
-};
-MODULE_DEVICE_TABLE(of, sysmmu_of_match);
-
-static struct platform_driver exynos_sysmmu_driver = {
-	.probe	= exynos_sysmmu_probe,
-	.shutdown = exynos_sysmmu_shutdown,
-	.driver	= {
-		.name		= "exynos-sysmmu",
-		.of_match_table	= sysmmu_of_match,
-		.pm		= &sysmmu_pm_ops,
-		.suppress_bind_attrs = true,
-	}
-};
-
 static inline void exynos_iommu_set_pte(sysmmu_pte_t *ent, sysmmu_pte_t val)
 {
 	dma_sync_single_for_cpu(dma_dev, virt_to_phys(ent), sizeof(*ent),
@@ -1428,6 +1247,185 @@ static const struct iommu_ops exynos_iommu_ops = {
 	}
 };
 
+static int exynos_sysmmu_probe(struct platform_device *pdev)
+{
+	int irq, ret;
+	struct device *dev = &pdev->dev;
+	struct sysmmu_drvdata *data;
+	struct resource *res;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->sfrbase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(data->sfrbase))
+		return PTR_ERR(data->sfrbase);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0)
+		return irq;
+
+	ret = devm_request_irq(dev, irq, exynos_sysmmu_irq, 0, dev_name(dev),
+			       data);
+	if (ret) {
+		dev_err(dev, "Unabled to register handler of irq %d\n", irq);
+		return ret;
+	}
+
+	data->clk = devm_clk_get(dev, "sysmmu");
+	if (PTR_ERR(data->clk) == -ENOENT)
+		data->clk = NULL;
+	else if (IS_ERR(data->clk))
+		return PTR_ERR(data->clk);
+
+	data->aclk = devm_clk_get(dev, "aclk");
+	if (PTR_ERR(data->aclk) == -ENOENT)
+		data->aclk = NULL;
+	else if (IS_ERR(data->aclk))
+		return PTR_ERR(data->aclk);
+
+	data->pclk = devm_clk_get(dev, "pclk");
+	if (PTR_ERR(data->pclk) == -ENOENT)
+		data->pclk = NULL;
+	else if (IS_ERR(data->pclk))
+		return PTR_ERR(data->pclk);
+
+	if (!data->clk && (!data->aclk || !data->pclk)) {
+		dev_err(dev, "Failed to get device clock(s)!\n");
+		return -ENOENT;
+	}
+
+	data->clk_master = devm_clk_get(dev, "master");
+	if (PTR_ERR(data->clk_master) == -ENOENT)
+		data->clk_master = NULL;
+	else if (IS_ERR(data->clk_master))
+		return PTR_ERR(data->clk_master);
+
+	data->sysmmu = dev;
+	spin_lock_init(&data->lock);
+
+	__sysmmu_get_version(data);
+
+	ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
+				     dev_name(data->sysmmu));
+	if (ret)
+		return ret;
+
+	ret = iommu_device_register(&data->iommu, &exynos_iommu_ops, dev);
+	if (ret)
+		goto err_iommu_register;
+
+	platform_set_drvdata(pdev, data);
+
+	if (PG_ENT_SHIFT < 0) {
+		if (MMU_MAJ_VER(data->version) < 5) {
+			PG_ENT_SHIFT = SYSMMU_PG_ENT_SHIFT;
+			LV1_PROT = SYSMMU_LV1_PROT;
+			LV2_PROT = SYSMMU_LV2_PROT;
+		} else {
+			PG_ENT_SHIFT = SYSMMU_V5_PG_ENT_SHIFT;
+			LV1_PROT = SYSMMU_V5_LV1_PROT;
+			LV2_PROT = SYSMMU_V5_LV2_PROT;
+		}
+	}
+
+	if (MMU_MAJ_VER(data->version) >= 5) {
+		ret = dma_set_mask(dev, DMA_BIT_MASK(36));
+		if (ret) {
+			dev_err(dev, "Unable to set DMA mask: %d\n", ret);
+			goto err_dma_set_mask;
+		}
+	}
+
+	/*
+	 * use the first registered sysmmu device for performing
+	 * dma mapping operations on iommu page tables (cpu cache flush)
+	 */
+	if (!dma_dev)
+		dma_dev = &pdev->dev;
+
+	pm_runtime_enable(dev);
+
+	return 0;
+
+err_dma_set_mask:
+	iommu_device_unregister(&data->iommu);
+err_iommu_register:
+	iommu_device_sysfs_remove(&data->iommu);
+	return ret;
+}
+
+static void exynos_sysmmu_shutdown(struct platform_device *pdev)
+{
+	struct sysmmu_drvdata *data = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	int irq = platform_get_irq(pdev, 0);
+
+	devm_free_irq(dev, irq, data);
+	pm_runtime_force_suspend(dev);
+}
+
+static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
+{
+	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
+	struct device *master = data->master;
+
+	if (master) {
+		struct exynos_iommu_owner *owner = dev_iommu_priv_get(master);
+
+		mutex_lock(&owner->rpm_lock);
+		if (data->domain) {
+			dev_dbg(data->sysmmu, "saving state\n");
+			__sysmmu_disable(data);
+		}
+		mutex_unlock(&owner->rpm_lock);
+	}
+	return 0;
+}
+
+static int __maybe_unused exynos_sysmmu_resume(struct device *dev)
+{
+	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
+	struct device *master = data->master;
+
+	if (master) {
+		struct exynos_iommu_owner *owner = dev_iommu_priv_get(master);
+
+		mutex_lock(&owner->rpm_lock);
+		if (data->domain) {
+			dev_dbg(data->sysmmu, "restoring state\n");
+			__sysmmu_enable(data);
+		}
+		mutex_unlock(&owner->rpm_lock);
+	}
+	return 0;
+}
+
+static const struct dev_pm_ops sysmmu_pm_ops = {
+	SET_RUNTIME_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+};
+
+static const struct of_device_id sysmmu_of_match[] = {
+	{ .compatible	= "samsung,exynos-sysmmu", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, sysmmu_of_match);
+
+static struct platform_driver exynos_sysmmu_driver = {
+	.probe	= exynos_sysmmu_probe,
+	.shutdown = exynos_sysmmu_shutdown,
+	.driver	= {
+		.name		= "exynos-sysmmu",
+		.of_match_table	= sysmmu_of_match,
+		.pm		= &sysmmu_pm_ops,
+		.suppress_bind_attrs = true,
+	}
+};
+
 static int __init exynos_iommu_init(void)
 {
 	struct device_node *np;
@@ -1461,6 +1459,7 @@ static int __init exynos_iommu_init(void)
 	}
 
 	return 0;
+
 err_zero_lv2:
 	platform_driver_unregister(&exynos_sysmmu_driver);
 err_reg_driver:
-- 
2.35.1

Re: [PATCH v2 5/6] iommu/exynos: Rearrange the platform driver code

[Krzysztof Kozlowski]

On 03/11/2022 15:51, Sam Protsenko wrote:
> Move the platform_driver code to the bottom of the driver, as it's a
> canonical form for that. No functional change.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - Fixed checkpatch style suggestion with "--strict" flag
> 
>  drivers/iommu/exynos-iommu.c | 361 +++++++++++++++++------------------


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v2 6/6] media: platform: Use IS_ENABLED() to check EXYNOS_IOMMU in s5p_mfc

[Sam Protsenko]

Now that CONFIG_EXYNOS_IOMMU can be built as a module, it's not correct
anymore to check whether it's enabled or not just with #ifdef. Use
proper IS_ENABLED() macro to handle both built-in and module cases.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - New patch

 drivers/media/platform/samsung/s5p-mfc/s5p_mfc_iommu.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_iommu.h b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_iommu.h
index 1a32266b7ddc..a8b48692d128 100644
--- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_iommu.h
+++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_iommu.h
@@ -7,7 +7,9 @@
 #ifndef S5P_MFC_IOMMU_H_
 #define S5P_MFC_IOMMU_H_
 
-#if defined(CONFIG_EXYNOS_IOMMU)
+#include <linux/kconfig.h>
+
+#if IS_ENABLED(CONFIG_EXYNOS_IOMMU)
 
 #include <linux/iommu.h>
 
-- 
2.35.1

Re: [PATCH v2 6/6] media: platform: Use IS_ENABLED() to check EXYNOS_IOMMU in s5p_mfc

[Krzysztof Kozlowski]

On 03/11/2022 15:51, Sam Protsenko wrote:
> Now that CONFIG_EXYNOS_IOMMU can be built as a module, it's not correct
> anymore to check whether it's enabled or not just with #ifdef. Use
> proper IS_ENABLED() macro to handle both built-in and module cases.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 0/6] iommu/exynos: Convert to a module

[Marek Szyprowski]

On 03.11.2022 20:51, Sam Protsenko wrote:
> As exynos-iommu driver is not a critical platform driver, it can be
> converted to a loadable module to avoid loading it on non-Exynos
> platforms in order to improve the RAM footprint. This patch series
> converts it to a module and does some related cleanups. IOMMU/DMA
> specifics were taken into the account, so remove/exit methods weren't
> added.
>
> There are two drivers using CONFIG_EXYNOS_IOMMU in their code:
> DRM_EXYNOS and S5P_MFC. Both were checked, and only a slight change was
> needed for S5P_MFC driver (patch #6).

Funny, compiling this driver as a module revealed an issue in the driver 
initialization sequence, here is a fix that need to be applied before 
this patchset:

https://lore.kernel.org/all/20221104115511.28256-1-m.szyprowski@samsung.com/

Besides that, the driver nukes with NULL pointer dereference in 
exynos_iommu_of_xlate() when compiled as a module on ARM 64bit 
Exynos5433 based TM2e board. ARM 32bit based board works fine. I'm 
checking this issue now.

> Changes in v2:
>    - Extracted the "shutdown" method addition into a separate patch
>    - Added MODULE_DEVICE_TABLE(of, ...) to support hot-plug loading
>    - Added MODULE_ALIAS("platform:exynos-sysmmu")
>    - Added fix for S5P_MFC driver to work correctly with EXYNOS_IOMMU=m
>    - Fixed checkpatch coding style suggestion with "--strict" flag
>    - Rebased on top of most recent joro/iommu.git:next
>
> Sam Protsenko (6):
>    iommu: Export iommu_group_default_domain() API
>    iommu/exynos: Fix retval on getting clocks in probe
>    iommu/exynos: Modularize the driver
>    iommu/exynos: Implement shutdown driver method
>    iommu/exynos: Rearrange the platform driver code
>    media: platform: Use IS_ENABLED() to check EXYNOS_IOMMU in s5p_mfc
>
>   drivers/iommu/Kconfig                         |   2 +-
>   drivers/iommu/exynos-iommu.c                  | 355 +++++++++---------
>   drivers/iommu/iommu.c                         |   1 +
>   .../platform/samsung/s5p-mfc/s5p_mfc_iommu.h  |   4 +-
>   4 files changed, 191 insertions(+), 171 deletions(-)
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 0/6] iommu/exynos: Convert to a module

[Sam Protsenko]

On Fri, 4 Nov 2022 at 13:10, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> On 03.11.2022 20:51, Sam Protsenko wrote:
> > As exynos-iommu driver is not a critical platform driver, it can be
> > converted to a loadable module to avoid loading it on non-Exynos
> > platforms in order to improve the RAM footprint. This patch series
> > converts it to a module and does some related cleanups. IOMMU/DMA
> > specifics were taken into the account, so remove/exit methods weren't
> > added.
> >
> > There are two drivers using CONFIG_EXYNOS_IOMMU in their code:
> > DRM_EXYNOS and S5P_MFC. Both were checked, and only a slight change was
> > needed for S5P_MFC driver (patch #6).
>
> Funny, compiling this driver as a module revealed an issue in the driver
> initialization sequence, here is a fix that need to be applied before
> this patchset:
>
> https://lore.kernel.org/all/20221104115511.28256-1-m.szyprowski@samsung.com/
>

I wonder why I didn't stumble upon that issue. Anyway, the fix looks
valid. I'll wait until you finish looking into that NULL pointer
dereference, and then we can figure out how to organize our patches.
Maybe I could send v3, including your patches in the beginning of the
series, etc.

> Besides that, the driver nukes with NULL pointer dereference in
> exynos_iommu_of_xlate() when compiled as a module on ARM 64bit
> Exynos5433 based TM2e board. ARM 32bit based board works fine. I'm
> checking this issue now.
>

Thanks for looking into that, Marek! I've tested the modularization
patch series using my emulated translation driver [1,2]. It works
fine, but exynos_iommu_of_xlate() is only executed once in my case,
when the test driver module is loaded. It's finished successfully
though. Not sure what is the cause of NULL pointer dereferences you're
seeing, alas I still don't have real users of SysMMU driver on my
platform, and my test driver is the only way for me to test it. Please
let me know if I can help in any way (e.g. I can trace some calls on
my board for you).

[1] https://github.com/joe-skb7/linux/blob/e850-96-mainline-iommu/drivers/iommu/samsung-iommu-test.c#L263
[2] https://github.com/joe-skb7/linux/blob/e850-96-mainline-iommu/arch/arm64/boot/dts/exynos/exynos850.dtsi#L477



> > Changes in v2:
> >    - Extracted the "shutdown" method addition into a separate patch
> >    - Added MODULE_DEVICE_TABLE(of, ...) to support hot-plug loading
> >    - Added MODULE_ALIAS("platform:exynos-sysmmu")
> >    - Added fix for S5P_MFC driver to work correctly with EXYNOS_IOMMU=m
> >    - Fixed checkpatch coding style suggestion with "--strict" flag
> >    - Rebased on top of most recent joro/iommu.git:next
> >
> > Sam Protsenko (6):
> >    iommu: Export iommu_group_default_domain() API
> >    iommu/exynos: Fix retval on getting clocks in probe
> >    iommu/exynos: Modularize the driver
> >    iommu/exynos: Implement shutdown driver method
> >    iommu/exynos: Rearrange the platform driver code
> >    media: platform: Use IS_ENABLED() to check EXYNOS_IOMMU in s5p_mfc
> >
> >   drivers/iommu/Kconfig                         |   2 +-
> >   drivers/iommu/exynos-iommu.c                  | 355 +++++++++---------
> >   drivers/iommu/iommu.c                         |   1 +
> >   .../platform/samsung/s5p-mfc/s5p_mfc_iommu.h  |   4 +-
> >   4 files changed, 191 insertions(+), 171 deletions(-)
> >
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Re: [PATCH v2 0/6] iommu/exynos: Convert to a module

[Marek Szyprowski]

On 04.11.2022 13:10, Marek Szyprowski wrote:
> On 03.11.2022 20:51, Sam Protsenko wrote:
>> As exynos-iommu driver is not a critical platform driver, it can be
>> converted to a loadable module to avoid loading it on non-Exynos
>> platforms in order to improve the RAM footprint. This patch series
>> converts it to a module and does some related cleanups. IOMMU/DMA
>> specifics were taken into the account, so remove/exit methods weren't
>> added.
>>
>> There are two drivers using CONFIG_EXYNOS_IOMMU in their code:
>> DRM_EXYNOS and S5P_MFC. Both were checked, and only a slight change was
>> needed for S5P_MFC driver (patch #6).
>
> Funny, compiling this driver as a module revealed an issue in the 
> driver initialization sequence, here is a fix that need to be applied 
> before this patchset:
>
> https://lore.kernel.org/all/20221104115511.28256-1-m.szyprowski@samsung.com/ 
>
>
> Besides that, the driver nukes with NULL pointer dereference in 
> exynos_iommu_of_xlate() when compiled as a module on ARM 64bit 
> Exynos5433 based TM2e board. ARM 32bit based board works fine. I'm 
> checking this issue now.
>
I've finally made Exynos IOMMU working as a module on Exynos5433 based 
TM2e board. It looks that this will be a bit longer journey that I've 
initially thought. I've posted a simple update of the fix for the driver 
initialization sequence, but the real problem is in the platform driver 
framework and OF helpers.

Basically to get it working as a module I had to apply the following 
changes:

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 3dda62503102..f6921f5fcab6 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -257,7 +257,7 @@ static int deferred_devs_show(struct seq_file *s, 
void *data)
  DEFINE_SHOW_ATTRIBUTE(deferred_devs);

  #ifdef CONFIG_MODULES
-int driver_deferred_probe_timeout = 10;
+int driver_deferred_probe_timeout = 30;
  #else
  int driver_deferred_probe_timeout;
  #endif
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 967f79b59016..e5df6672fee6 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1384,7 +1384,7 @@ static struct device_node *parse_interrupts(struct 
device_node *np,
  static const struct supplier_bindings of_supplier_bindings[] = {
         { .parse_prop = parse_clocks, },
         { .parse_prop = parse_interconnects, },
-       { .parse_prop = parse_iommus, .optional = true, },
+       { .parse_prop = parse_iommus, },
         { .parse_prop = parse_iommu_maps, .optional = true, },
         { .parse_prop = parse_mboxes, },
         { .parse_prop = parse_io_channels, },

Without that a really nasty things happened.

Initialization of the built-in drivers and loading modules takes time, 
so the default 10s deferred probe timeout is not enough to ensure that 
the built-in driver won't be probed before the Exynos IOMMU driver is 
loaded.

The second change fixes the problem that driver core probes Exynos IOMMU 
controllers in parallel to probing the master devices, what results in 
calling exynos_iommu_of_xlate() and exynos_iommu_probe_device() even on 
the partially initialized IOMMU controllers or initializing the dma_ops 
under the already probed and working master device. This was easy to 
observe especially on the master devices with multiple IOMMU 
controllers. I wasn't able to solve this concurrency/race issues inside 
the Exynos IOMMU driver.

Frankly speaking I don't know what is the rationale for making the 
'iommus' property optional, but this simply doesn't work well with IOMMU 
driver being a module. CCed Saravana and Rob for this.

Without fixing the above issues, I would add a warning that compiling 
the driver as a module leads to serious issues.


>> Changes in v2:
>>    - Extracted the "shutdown" method addition into a separate patch
>>    - Added MODULE_DEVICE_TABLE(of, ...) to support hot-plug loading
>>    - Added MODULE_ALIAS("platform:exynos-sysmmu")
>>    - Added fix for S5P_MFC driver to work correctly with EXYNOS_IOMMU=m
>>    - Fixed checkpatch coding style suggestion with "--strict" flag
>>    - Rebased on top of most recent joro/iommu.git:next
>>
>> Sam Protsenko (6):
>>    iommu: Export iommu_group_default_domain() API
>>    iommu/exynos: Fix retval on getting clocks in probe
>>    iommu/exynos: Modularize the driver
>>    iommu/exynos: Implement shutdown driver method
>>    iommu/exynos: Rearrange the platform driver code
>>    media: platform: Use IS_ENABLED() to check EXYNOS_IOMMU in s5p_mfc
>>
>>   drivers/iommu/Kconfig                         |   2 +-
>>   drivers/iommu/exynos-iommu.c                  | 355 +++++++++---------
>>   drivers/iommu/iommu.c                         |   1 +
>>   .../platform/samsung/s5p-mfc/s5p_mfc_iommu.h  |   4 +-
>>   4 files changed, 191 insertions(+), 171 deletions(-)
>>
> Best regards

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 0/6] iommu/exynos: Convert to a module

[Sam Protsenko]

On Thu, 10 Nov 2022 at 15:36, Marek Szyprowski <m.szyprowski@samsung.com> wrote:

[snip]

> I've finally made Exynos IOMMU working as a module on Exynos5433 based
> TM2e board. It looks that this will be a bit longer journey that I've
> initially thought. I've posted a simple update of the fix for the driver
> initialization sequence, but the real problem is in the platform driver
> framework and OF helpers.
>
> Basically to get it working as a module I had to apply the following
> changes:
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 3dda62503102..f6921f5fcab6 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -257,7 +257,7 @@ static int deferred_devs_show(struct seq_file *s,
> void *data)
>   DEFINE_SHOW_ATTRIBUTE(deferred_devs);
>
>   #ifdef CONFIG_MODULES
> -int driver_deferred_probe_timeout = 10;
> +int driver_deferred_probe_timeout = 30;
>   #else
>   int driver_deferred_probe_timeout;
>   #endif
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 967f79b59016..e5df6672fee6 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1384,7 +1384,7 @@ static struct device_node *parse_interrupts(struct
> device_node *np,
>   static const struct supplier_bindings of_supplier_bindings[] = {
>          { .parse_prop = parse_clocks, },
>          { .parse_prop = parse_interconnects, },
> -       { .parse_prop = parse_iommus, .optional = true, },
> +       { .parse_prop = parse_iommus, },
>          { .parse_prop = parse_iommu_maps, .optional = true, },
>          { .parse_prop = parse_mboxes, },
>          { .parse_prop = parse_io_channels, },
>
> Without that a really nasty things happened.
>
> Initialization of the built-in drivers and loading modules takes time,
> so the default 10s deferred probe timeout is not enough to ensure that
> the built-in driver won't be probed before the Exynos IOMMU driver is
> loaded.
>

Yeah, the whole time-based sync looks nasty... I remember coming
across the slides by Andrzej Hajda called "Deferred Problem" [1], but
I guess the proposed solution was never applied. Just hope that
increasing the timeout is upstreamable solution.

[1] https://events19.linuxfoundation.org/wp-content/uploads/2017/12/Deferred-Problem-Issues-With-Complex-Dependencies-Between-Devices-in-Linux-Kernel-Andrzej-Hajda-Samsung.pdf

> The second change fixes the problem that driver core probes Exynos IOMMU
> controllers in parallel to probing the master devices, what results in
> calling exynos_iommu_of_xlate() and exynos_iommu_probe_device() even on
> the partially initialized IOMMU controllers or initializing the dma_ops
> under the already probed and working master device. This was easy to
> observe especially on the master devices with multiple IOMMU
> controllers. I wasn't able to solve this concurrency/race issues inside
> the Exynos IOMMU driver.
>
> Frankly speaking I don't know what is the rationale for making the
> 'iommus' property optional, but this simply doesn't work well with IOMMU
> driver being a module. CCed Saravana and Rob for this.
>

The patch which makes 'iommus' optional doesn't provide much of
insight on reasons in commit message either.

> Without fixing the above issues, I would add a warning that compiling
> the driver as a module leads to serious issues.
>

Nice catch! It doesn't reproduce on my platform, alas. Can I expect
you to submit those patches? If so, I'll probably just wait for those
to be applied, and then re-send my modularization series on top of it.
Does that sounds reasonable?

[snip]

>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Re: [PATCH v2 0/6] iommu/exynos: Convert to a module

[Saravana Kannan]

On Fri, Nov 11, 2022 at 5:30 AM Sam Protsenko
<semen.protsenko@linaro.org> wrote:
>
> On Thu, 10 Nov 2022 at 15:36, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> [snip]

Hi Marek and Sam,

I'm replying to both of your comments in this email.

> > I've finally made Exynos IOMMU working as a module on Exynos5433 based
> > TM2e board. It looks that this will be a bit longer journey that I've
> > initially thought. I've posted a simple update of the fix for the driver
> > initialization sequence, but the real problem is in the platform driver
> > framework and OF helpers.
> >
> > Basically to get it working as a module I had to apply the following
> > changes:
> >
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index 3dda62503102..f6921f5fcab6 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -257,7 +257,7 @@ static int deferred_devs_show(struct seq_file *s,
> > void *data)
> >   DEFINE_SHOW_ATTRIBUTE(deferred_devs);
> >
> >   #ifdef CONFIG_MODULES
> > -int driver_deferred_probe_timeout = 10;
> > +int driver_deferred_probe_timeout = 30;
> >   #else
> >   int driver_deferred_probe_timeout;
> >   #endif
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 967f79b59016..e5df6672fee6 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1384,7 +1384,7 @@ static struct device_node *parse_interrupts(struct
> > device_node *np,
> >   static const struct supplier_bindings of_supplier_bindings[] = {
> >          { .parse_prop = parse_clocks, },
> >          { .parse_prop = parse_interconnects, },
> > -       { .parse_prop = parse_iommus, .optional = true, },
> > +       { .parse_prop = parse_iommus, },
> >          { .parse_prop = parse_iommu_maps, .optional = true, },
> >          { .parse_prop = parse_mboxes, },
> >          { .parse_prop = parse_io_channels, },
> >
> > Without that a really nasty things happened.

I have a command line option to do this without code changes. Use
fw_devlink.strict=1. That makes all optional properties into mandatory
ones.

I sent out a series[1] that tried to make fw_devlink.strict=1 the
default and then use the timeout behavior (more details) to handle
cases where iommu and dmas (or any other supplier) are optional on a
specific board. The cover letter of [1] should give some more context.

> > Initialization of the built-in drivers and loading modules takes time,
> > so the default 10s deferred probe timeout is not enough to ensure that
> > the built-in driver won't be probed before the Exynos IOMMU driver is
> > loaded.

The 10 second is the minimum delay from the time we hit late_initcall.
If a driver is registered before the 10s expires, then the timer will
be extended by another 10s. This behavior landed sometime around the
end of May 2022. So it should have been in your tree when you tested
this. I'm surprised this isn't sufficient for your case. Is there
really a 10s gap in your boot sequence where no module is being loaded
and then IOMMU modules get loaded later on? I'm kinda surprised by
this. Is it this long because some serial UART is enabled and it's
slowing down boot? Or something else?

I'm not saying your case isn't valid or we shouldn't extend the
timeout. I'm just trying to understand why the current timer behavior
wasn't able to cover your case.

> Yeah, the whole time-based sync looks nasty... I remember coming
> across the slides by Andrzej Hajda called "Deferred Problem" [1], but
> I guess the proposed solution was never applied. Just hope that
> increasing the timeout is upstreamable solution.
>
> [1] https://events19.linuxfoundation.org/wp-content/uploads/2017/12/Deferred-Problem-Issues-With-Complex-Dependencies-Between-Devices-in-Linux-Kernel-Andrzej-Hajda-Samsung.pdf

Sam, I kinda skimmed the slides right now. Looks like it talks about
device links and why they aren't sufficient and makes an alternate
proposal. fw_devlink is a solution that uses device links and I think
addresses a lot of the issues that were raised about device links.
There's still a bunch of TODOs left, but I think the end goal is the
same. I'm hoping to keep chipping away at it. For now, I've tried to
make the timer a bit more smart about detecting when modules are
getting loaded and extending the timer. fw_devlink also enables
something called sync_state() that's invaluable on a fully modular
system (search lore for references to that to get some idea).

The slides talk about a solution that will allow devices to probe with
limited functionality with whatever suppliers are available and then
reprobe as more suppliers are available. I'm not sure how well that'll
work across the board. It's going to be a bit weird if your phone
display goes off and then comes on again because an IOMMU driver got
loaded (and it can now do DRM playback). For now, I'm not going to
focus on that option because there are enough existing issues/TODOs to
work on for fw_devlink.

> > The second change fixes the problem that driver core probes Exynos IOMMU
> > controllers in parallel to probing the master devices, what results in
> > calling exynos_iommu_of_xlate() and exynos_iommu_probe_device() even on
> > the partially initialized IOMMU controllers or initializing the dma_ops
> > under the already probed and working master device. This was easy to
> > observe especially on the master devices with multiple IOMMU
> > controllers. I wasn't able to solve this concurrency/race issues inside
> > the Exynos IOMMU driver.
> >
> > Frankly speaking I don't know what is the rationale for making the
> > 'iommus' property optional, but this simply doesn't work well with IOMMU
> > driver being a module. CCed Saravana and Rob for this.
> >
>
> The patch which makes 'iommus' optional doesn't provide much of
> insight on reasons in commit message either.

This was the commit text:

    Not all DT bindings are mandatory bindings. Add support for optional DT
    bindings and mark iommus, iommu-map, dmas as optional DT bindings.

I thought it was obvious enough, but I guess I could have done better.
Geert convinced me that iommu's aren't always necessary and devices
could work perfectly well without them or dmas. And he has a bunch of
boards like that. So I went with adding optional and then introducing
fw_devlink.strict.

However, at this point in time, I believe none of them should be
marked as optional because technically any property can be optional
depending on what the firmware has set up and what the driver does. We
should figure this out at runtime on a board level -- which is what
[1] is trying to do. Yeah, not very pretty, but there hasn't been a
better solution that's not "have userspace tell us it's done loading
modules" (that's a "kernel depends on userspace to work correctly"
thing that no one likes). I've fixed some of the issues raised in [1]
in a fw_devlink improvement series[2] and I plan on continuing to work
on this until hopefully [1] can land.

> > Without fixing the above issues, I would add a warning that compiling
> > the driver as a module leads to serious issues.
> >
>
> Nice catch! It doesn't reproduce on my platform, alas. Can I expect
> you to submit those patches? If so, I'll probably just wait for those
> to be applied, and then re-send my modularization series on top of it.
> Does that sounds reasonable?

For now, maybe we could add a config to enable fw_devlink.strict=1 by
default and then select it if you make specific iommu drivers into
modules? And then Geert won't set it for his driver, but you can set
it for your driver?

Thanks,
Saravana

[1] - https://lore.kernel.org/lkml/20220601070707.3946847-1-saravanak@google.com/
[2] - https://lore.kernel.org/lkml/20230207014207.1678715-1-saravanak@google.com/

Re: [PATCH v2 0/6] iommu/exynos: Convert to a module

[Sam Protsenko]

On Mon, 6 Feb 2023 at 21:33, Saravana Kannan <saravanak@google.com> wrote:
>
> On Fri, Nov 11, 2022 at 5:30 AM Sam Protsenko
> <semen.protsenko@linaro.org> wrote:
> >
> > On Thu, 10 Nov 2022 at 15:36, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >
> > [snip]
>
> Hi Marek and Sam,
>
> I'm replying to both of your comments in this email.
>
> > > I've finally made Exynos IOMMU working as a module on Exynos5433 based
> > > TM2e board. It looks that this will be a bit longer journey that I've
> > > initially thought. I've posted a simple update of the fix for the driver
> > > initialization sequence, but the real problem is in the platform driver
> > > framework and OF helpers.
> > >
> > > Basically to get it working as a module I had to apply the following
> > > changes:
> > >
> > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > index 3dda62503102..f6921f5fcab6 100644
> > > --- a/drivers/base/dd.c
> > > +++ b/drivers/base/dd.c
> > > @@ -257,7 +257,7 @@ static int deferred_devs_show(struct seq_file *s,
> > > void *data)
> > >   DEFINE_SHOW_ATTRIBUTE(deferred_devs);
> > >
> > >   #ifdef CONFIG_MODULES
> > > -int driver_deferred_probe_timeout = 10;
> > > +int driver_deferred_probe_timeout = 30;
> > >   #else
> > >   int driver_deferred_probe_timeout;
> > >   #endif
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index 967f79b59016..e5df6672fee6 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -1384,7 +1384,7 @@ static struct device_node *parse_interrupts(struct
> > > device_node *np,
> > >   static const struct supplier_bindings of_supplier_bindings[] = {
> > >          { .parse_prop = parse_clocks, },
> > >          { .parse_prop = parse_interconnects, },
> > > -       { .parse_prop = parse_iommus, .optional = true, },
> > > +       { .parse_prop = parse_iommus, },
> > >          { .parse_prop = parse_iommu_maps, .optional = true, },
> > >          { .parse_prop = parse_mboxes, },
> > >          { .parse_prop = parse_io_channels, },
> > >
> > > Without that a really nasty things happened.
>
> I have a command line option to do this without code changes. Use
> fw_devlink.strict=1. That makes all optional properties into mandatory
> ones.
>
> I sent out a series[1] that tried to make fw_devlink.strict=1 the
> default and then use the timeout behavior (more details) to handle
> cases where iommu and dmas (or any other supplier) are optional on a
> specific board. The cover letter of [1] should give some more context.
>
> > > Initialization of the built-in drivers and loading modules takes time,
> > > so the default 10s deferred probe timeout is not enough to ensure that
> > > the built-in driver won't be probed before the Exynos IOMMU driver is
> > > loaded.
>
> The 10 second is the minimum delay from the time we hit late_initcall.
> If a driver is registered before the 10s expires, then the timer will
> be extended by another 10s. This behavior landed sometime around the
> end of May 2022. So it should have been in your tree when you tested
> this. I'm surprised this isn't sufficient for your case. Is there
> really a 10s gap in your boot sequence where no module is being loaded
> and then IOMMU modules get loaded later on? I'm kinda surprised by
> this. Is it this long because some serial UART is enabled and it's
> slowing down boot? Or something else?
>
> I'm not saying your case isn't valid or we shouldn't extend the
> timeout. I'm just trying to understand why the current timer behavior
> wasn't able to cover your case.
>
> > Yeah, the whole time-based sync looks nasty... I remember coming
> > across the slides by Andrzej Hajda called "Deferred Problem" [1], but
> > I guess the proposed solution was never applied. Just hope that
> > increasing the timeout is upstreamable solution.
> >
> > [1] https://events19.linuxfoundation.org/wp-content/uploads/2017/12/Deferred-Problem-Issues-With-Complex-Dependencies-Between-Devices-in-Linux-Kernel-Andrzej-Hajda-Samsung.pdf
>
> Sam, I kinda skimmed the slides right now. Looks like it talks about
> device links and why they aren't sufficient and makes an alternate
> proposal. fw_devlink is a solution that uses device links and I think
> addresses a lot of the issues that were raised about device links.
> There's still a bunch of TODOs left, but I think the end goal is the
> same. I'm hoping to keep chipping away at it. For now, I've tried to
> make the timer a bit more smart about detecting when modules are
> getting loaded and extending the timer. fw_devlink also enables
> something called sync_state() that's invaluable on a fully modular
> system (search lore for references to that to get some idea).
>
> The slides talk about a solution that will allow devices to probe with
> limited functionality with whatever suppliers are available and then
> reprobe as more suppliers are available. I'm not sure how well that'll
> work across the board. It's going to be a bit weird if your phone
> display goes off and then comes on again because an IOMMU driver got
> loaded (and it can now do DRM playback). For now, I'm not going to
> focus on that option because there are enough existing issues/TODOs to
> work on for fw_devlink.
>
> > > The second change fixes the problem that driver core probes Exynos IOMMU
> > > controllers in parallel to probing the master devices, what results in
> > > calling exynos_iommu_of_xlate() and exynos_iommu_probe_device() even on
> > > the partially initialized IOMMU controllers or initializing the dma_ops
> > > under the already probed and working master device. This was easy to
> > > observe especially on the master devices with multiple IOMMU
> > > controllers. I wasn't able to solve this concurrency/race issues inside
> > > the Exynos IOMMU driver.
> > >
> > > Frankly speaking I don't know what is the rationale for making the
> > > 'iommus' property optional, but this simply doesn't work well with IOMMU
> > > driver being a module. CCed Saravana and Rob for this.
> > >
> >
> > The patch which makes 'iommus' optional doesn't provide much of
> > insight on reasons in commit message either.
>
> This was the commit text:
>
>     Not all DT bindings are mandatory bindings. Add support for optional DT
>     bindings and mark iommus, iommu-map, dmas as optional DT bindings.
>
> I thought it was obvious enough, but I guess I could have done better.
> Geert convinced me that iommu's aren't always necessary and devices
> could work perfectly well without them or dmas. And he has a bunch of
> boards like that. So I went with adding optional and then introducing
> fw_devlink.strict.
>
> However, at this point in time, I believe none of them should be
> marked as optional because technically any property can be optional
> depending on what the firmware has set up and what the driver does. We
> should figure this out at runtime on a board level -- which is what
> [1] is trying to do. Yeah, not very pretty, but there hasn't been a
> better solution that's not "have userspace tell us it's done loading
> modules" (that's a "kernel depends on userspace to work correctly"
> thing that no one likes). I've fixed some of the issues raised in [1]
> in a fw_devlink improvement series[2] and I plan on continuing to work
> on this until hopefully [1] can land.
>
> > > Without fixing the above issues, I would add a warning that compiling
> > > the driver as a module leads to serious issues.
> > >
> >
> > Nice catch! It doesn't reproduce on my platform, alas. Can I expect
> > you to submit those patches? If so, I'll probably just wait for those
> > to be applied, and then re-send my modularization series on top of it.
> > Does that sounds reasonable?
>
> For now, maybe we could add a config to enable fw_devlink.strict=1 by
> default and then select it if you make specific iommu drivers into
> modules? And then Geert won't set it for his driver, but you can set
> it for your driver?
>

Marek, what is your take on this? I probably can't provide any useful
comments, as the issue is not even reproducible on my platform. Do you
think some easy solution exists to make modularized IOMMU driver work?
That series has been sitting for a while on ML now, so I wonder if we
can handle it somehow, w.r.t. Saravana's input.

> Thanks,
> Saravana
>
> [1] - https://lore.kernel.org/lkml/20220601070707.3946847-1-saravanak@google.com/
> [2] - https://lore.kernel.org/lkml/20230207014207.1678715-1-saravanak@google.com/

Re: [PATCH v2 0/6] iommu/exynos: Convert to a module

[Marek Szyprowski]

Hi Saravana,

I'm sorry for the delay again, but finally I managed to get back to this 
topic.

On 07.02.2023 04:32, Saravana Kannan wrote:
> On Fri, Nov 11, 2022 at 5:30 AM Sam Protsenko
> <semen.protsenko@linaro.org> wrote:
>> On Thu, 10 Nov 2022 at 15:36, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>>
>> [snip]
> Hi Marek and Sam,
>
> I'm replying to both of your comments in this email.
>
>>> I've finally made Exynos IOMMU working as a module on Exynos5433 based
>>> TM2e board. It looks that this will be a bit longer journey that I've
>>> initially thought. I've posted a simple update of the fix for the driver
>>> initialization sequence, but the real problem is in the platform driver
>>> framework and OF helpers.
>>>
>>> Basically to get it working as a module I had to apply the following
>>> changes:
>>>
>>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>>> index 3dda62503102..f6921f5fcab6 100644
>>> --- a/drivers/base/dd.c
>>> +++ b/drivers/base/dd.c
>>> @@ -257,7 +257,7 @@ static int deferred_devs_show(struct seq_file *s,
>>> void *data)
>>>    DEFINE_SHOW_ATTRIBUTE(deferred_devs);
>>>
>>>    #ifdef CONFIG_MODULES
>>> -int driver_deferred_probe_timeout = 10;
>>> +int driver_deferred_probe_timeout = 30;
>>>    #else
>>>    int driver_deferred_probe_timeout;
>>>    #endif
>>> diff --git a/drivers/of/property.c b/drivers/of/property.c
>>> index 967f79b59016..e5df6672fee6 100644
>>> --- a/drivers/of/property.c
>>> +++ b/drivers/of/property.c
>>> @@ -1384,7 +1384,7 @@ static struct device_node *parse_interrupts(struct
>>> device_node *np,
>>>    static const struct supplier_bindings of_supplier_bindings[] = {
>>>           { .parse_prop = parse_clocks, },
>>>           { .parse_prop = parse_interconnects, },
>>> -       { .parse_prop = parse_iommus, .optional = true, },
>>> +       { .parse_prop = parse_iommus, },
>>>           { .parse_prop = parse_iommu_maps, .optional = true, },
>>>           { .parse_prop = parse_mboxes, },
>>>           { .parse_prop = parse_io_channels, },
>>>
>>> Without that a really nasty things happened.
> I have a command line option to do this without code changes. Use
> fw_devlink.strict=1. That makes all optional properties into mandatory
> ones.
>
> I sent out a series[1] that tried to make fw_devlink.strict=1 the
> default and then use the timeout behavior (more details) to handle
> cases where iommu and dmas (or any other supplier) are optional on a
> specific board. The cover letter of [1] should give some more context.

I'm for removing the optional properties or making them dependent on the 
whole subsystem availability (see my next comments).


>>> Initialization of the built-in drivers and loading modules takes time,
>>> so the default 10s deferred probe timeout is not enough to ensure that
>>> the built-in driver won't be probed before the Exynos IOMMU driver is
>>> loaded.
> The 10 second is the minimum delay from the time we hit late_initcall.
> If a driver is registered before the 10s expires, then the timer will
> be extended by another 10s. This behavior landed sometime around the
> end of May 2022. So it should have been in your tree when you tested
> this. I'm surprised this isn't sufficient for your case. Is there
> really a 10s gap in your boot sequence where no module is being loaded
> and then IOMMU modules get loaded later on? I'm kinda surprised by
> this. Is it this long because some serial UART is enabled and it's
> slowing down boot? Or something else?
>
> I'm not saying your case isn't valid or we shouldn't extend the
> timeout. I'm just trying to understand why the current timer behavior
> wasn't able to cover your case.


Well, I have almost all possible CONFIG_DEBUG_* options enabled in 
exynos_defconfig, as well as kernel messages routed to UART console, so 
all my test system are really slow, but I do this intentionally. I 
suspect that over half of the issues I've reported were found because of 
such 'unusual' config. Loading modules also takes time in such case, so 
maybe the timeout should be extended also by each loaded module?


>> Yeah, the whole time-based sync looks nasty... I remember coming
>> across the slides by Andrzej Hajda called "Deferred Problem" [1], but
>> I guess the proposed solution was never applied. Just hope that
>> increasing the timeout is upstreamable solution.
>>
>> [1] https://events19.linuxfoundation.org/wp-content/uploads/2017/12/Deferred-Problem-Issues-With-Complex-Dependencies-Between-Devices-in-Linux-Kernel-Andrzej-Hajda-Samsung.pdf
> Sam, I kinda skimmed the slides right now. Looks like it talks about
> device links and why they aren't sufficient and makes an alternate
> proposal. fw_devlink is a solution that uses device links and I think
> addresses a lot of the issues that were raised about device links.
> There's still a bunch of TODOs left, but I think the end goal is the
> same. I'm hoping to keep chipping away at it. For now, I've tried to
> make the timer a bit more smart about detecting when modules are
> getting loaded and extending the timer. fw_devlink also enables
> something called sync_state() that's invaluable on a fully modular
> system (search lore for references to that to get some idea).
>
> The slides talk about a solution that will allow devices to probe with
> limited functionality with whatever suppliers are available and then
> reprobe as more suppliers are available. I'm not sure how well that'll
> work across the board. It's going to be a bit weird if your phone
> display goes off and then comes on again because an IOMMU driver got
> loaded (and it can now do DRM playback). For now, I'm not going to
> focus on that option because there are enough existing issues/TODOs to
> work on for fw_devlink.
>
>>> The second change fixes the problem that driver core probes Exynos IOMMU
>>> controllers in parallel to probing the master devices, what results in
>>> calling exynos_iommu_of_xlate() and exynos_iommu_probe_device() even on
>>> the partially initialized IOMMU controllers or initializing the dma_ops
>>> under the already probed and working master device. This was easy to
>>> observe especially on the master devices with multiple IOMMU
>>> controllers. I wasn't able to solve this concurrency/race issues inside
>>> the Exynos IOMMU driver.
>>>
>>> Frankly speaking I don't know what is the rationale for making the
>>> 'iommus' property optional, but this simply doesn't work well with IOMMU
>>> driver being a module. CCed Saravana and Rob for this.
>>>
>> The patch which makes 'iommus' optional doesn't provide much of
>> insight on reasons in commit message either.
> This was the commit text:
>
>      Not all DT bindings are mandatory bindings. Add support for optional DT
>      bindings and mark iommus, iommu-map, dmas as optional DT bindings.
>
> I thought it was obvious enough, but I guess I could have done better.
> Geert convinced me that iommu's aren't always necessary and devices
> could work perfectly well without them or dmas. And he has a bunch of
> boards like that. So I went with adding optional and then introducing
> fw_devlink.strict.

Well, that depends heavily on the kernel config and the intentions 
behind it. Indeed one might have IOMMU driver not selected in the config 
(for various reasons, like forcing drivers to use contiguous memory 
buffers for max performance) and system should boot fine. That time one 
didn't consider the IOMMUs being compiled as modules. The main problem 
is how to distinguish between the first case (intentionally no IOMMU 
driver or even no IOMMU support) and the modularized case (driver not 
yet loaded). I assume that one can skip waiting for IOMMUs only if the 
whole IOMMU subsystem is not compiled in, otherwise the IOMMU driver can 
still come in and it cannot properly start operating if client device 
drivers already started their operation.

DMAs, although similar a bit, are imho a bit different. Drivers usually 
have some kind of fallback if DMA is not yet available, but usually 
drivers can switch their operation on-fly between PIO and DMA (and some 
even do that intentionally and use PIO for very small transfers).

> However, at this point in time, I believe none of them should be
> marked as optional because technically any property can be optional
> depending on what the firmware has set up and what the driver does. We
> should figure this out at runtime on a board level -- which is what
> [1] is trying to do. Yeah, not very pretty, but there hasn't been a
> better solution that's not "have userspace tell us it's done loading
> modules" (that's a "kernel depends on userspace to work correctly"
> thing that no one likes). I've fixed some of the issues raised in [1]
> in a fw_devlink improvement series[2] and I plan on continuing to work
> on this until hopefully [1] can land.
>
>>> Without fixing the above issues, I would add a warning that compiling
>>> the driver as a module leads to serious issues.
>>>
>> Nice catch! It doesn't reproduce on my platform, alas. Can I expect
>> you to submit those patches? If so, I'll probably just wait for those
>> to be applied, and then re-send my modularization series on top of it.
>> Does that sounds reasonable?
> For now, maybe we could add a config to enable fw_devlink.strict=1 by
> default and then select it if you make specific iommu drivers into
> modules? And then Geert won't set it for his driver, but you can set
> it for your driver?

I think the best we can do now is to either add a kconfig option to 
force strict fw_devlinks (so Exynos IOMMU can be selected as a module 
only if that one is selected) or do a runtime check during the Exynos 
IOMMU initalization - and fail with appropriate error message if 
fw_devlink.strict!=1. What do you think?


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland