Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7ed8c884-cee1-7cb0-f2ce-f7ec156cd841@samsung.com>
Date:   Wed, 2 Nov 2022 23:07:15 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0)
        Gecko/20100101 Thunderbird/102.4.1
Subject: Re: [PATCH 3/4] iommu/exynos: Modularize the driver
Content-Language: en-US
To:     Sam Protsenko <semen.protsenko@linaro.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        Janghyuck Kim <janghyuck.kim@samsung.com>,
        Cho KyongHo <pullip.cho@samsung.com>,
        Daniel Mentz <danielmentz@google.com>,
        David Virag <virag.david003@gmail.com>, iommu@lists.linux.dev,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-samsung-soc@vger.kernel.org
From:   Marek Szyprowski <m.szyprowski@samsung.com>
In-Reply-To: <20221028191243.31721-4-semen.protsenko@linaro.org>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20221028191243.31721-1-semen.protsenko@linaro.org>
        <CGME20221028191256eucas1p1a4fd95cbcc44959fa968956a92738f03@eucas1p1.samsung.com>
        <20221028191243.31721-4-semen.protsenko@linaro.org>
Precedence: bulk

Hi Sam,

On 28.10.2022 21:12, 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).
>
> Also add the shutdown driver method, while at it. That was inspired by
> other IOMMU drivers, and can be useful e.g. for performing a kexec. See
> commit 1a4e90f25b2c ("iommu/rockchip: Perform a reset on shutdown") for
> example.
>
> 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>

MODULE_DEVICE_TABLE(of, sysmmu_of_match); is missing, so the driver 
won't be automatically loaded, what breaks its operation if compiled as 
module.

Also Exynos DRM and S5P-MFC drivers rely on the Exynos IOMMU being 
built-in, so they need to be adjusted for modularized builds too imho, 
at least in the Kconfig dependency.

> ---
>   drivers/iommu/Kconfig        |  2 +-
>   drivers/iommu/exynos-iommu.c | 18 +++++++++++++++++-
>   2 files changed, 18 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..57492db877e2 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>
> @@ -752,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);
> @@ -799,8 +810,9 @@ static const struct of_device_id sysmmu_of_match[] = {
>   	{ },
>   };
>   
> -static struct platform_driver exynos_sysmmu_driver __refdata = {
> +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,
> @@ -1404,6 +1416,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 +1467,6 @@ static int __init exynos_iommu_init(void)
>   	return ret;
>   }
>   core_initcall(exynos_iommu_init);
> +
> +MODULE_DESCRIPTION("IOMMU driver for Exynos SoCs");
> +MODULE_LICENSE("GPL");

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