| Message ID | 20221028191243.31721-4-semen.protsenko@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommu/exynos: Convert to module | expand |
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
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 <semen.protsenko@linaro.org> 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]
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");
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> --- drivers/iommu/Kconfig | 2 +- drivers/iommu/exynos-iommu.c | 18 +++++++++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-)