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/[email protected]/T/#md7e1e3f5b2c9e7fa5bc28fe33e818b6aa4a7237c
Signed-off-by: Sam Protsenko <[email protected]>
---
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");
--
2.35.1
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/[email protected]/T/#md7e1e3f5b2c9e7fa5bc28fe33e818b6aa4a7237c
>
> Signed-off-by: Sam Protsenko <[email protected]>
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
Hi Marek,
[snip]
> 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.
>
Right, didn't think about hot-plug case. Will add
MODULE_DEVICE_TABLE() along with MODULE_ALIAS() in v2, thanks.
> 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.
>
Sure, I'll check all Kconfigs and defconfigs before sending out v2.
Btw, can you please also check my most recent reply [1] for the
"[PATCH 1/2] iommu/exynos: Abstract getting the fault info"?
[1] https://lore.kernel.org/lkml/CAPLW+4n-Lf6je61rxdJ9nJnX9h9F8F-y+qikG7eFF0avQpMV9Q@mail.gmail.com/
Thanks!
[snip]
On Thu, 3 Nov 2022 at 14:03, Sam Protsenko <[email protected]> wrote:
>
> Hi Marek,
>
> [snip]
>
> > 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.
> >
>
> Right, didn't think about hot-plug case. Will add
> MODULE_DEVICE_TABLE() along with MODULE_ALIAS() in v2, thanks.
>
> > 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.
> >
>
Did some research on that question:
- both DRM_EXYNOS and S5P_MFC drivers are designed to work fine even
when EXYNOS_IOMMU is disabled: they just take another code path in
that case
- DRM_EXYNOS already uses IS_ENABLE() macro, which covers EXYNOS_IOMMU=m case
- I'll provide a fix for S5P_MFC in v2 to do the same, as right now
it does the check with #ifdef CONFIG_EXYNOS_IOMMU, which only covers
=y case
- both DRM_EXYNOS and S5P_MFC don't use EXYNOS_IOMMU driver symbols,
as the latter doesn't export any
- the correct probe order (EXYNOS_IOMMU first, then its users) is
already ensured in device tree, by referencing sysmmu phandles in
"iommus = <&...>" properties
So I'm not sure if it makes sense to make those drivers depend on
EXYNOS_IOMMU in Kconfig? AFAIR, there could've been some issues if
those used some exported symbols from EXYNOS_IOMMU, but it's not the
case.
Please let me know if I'm missing something.
> Sure, I'll check all Kconfigs and defconfigs before sending out v2.
>
> Btw, can you please also check my most recent reply [1] for the
> "[PATCH 1/2] iommu/exynos: Abstract getting the fault info"?
>
> [1] https://lore.kernel.org/lkml/CAPLW+4n-Lf6je61rxdJ9nJnX9h9F8F-y+qikG7eFF0avQpMV9Q@mail.gmail.com/
>
> Thanks!
>
> [snip]