| Message ID | 20250411125423.1411061-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommu/io-pgtable-arm: dynamically allocate selftest device struct | expand |
On 11/04/2025 1:54 pm, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > In general a 'struct device' is way too large to be put on the kernel > stack. Apparently something just caused it to grow a slightly larger, > which pushed the arm_lpae_do_selftests() function over the warning > limit in some configurations: > > drivers/iommu/io-pgtable-arm.c:1423:19: error: stack frame size (1032) exceeds limit (1024) in 'arm_lpae_do_selftests' [-Werror,-Wframe-larger-than] > 1423 | static int __init arm_lpae_do_selftests(void) > | ^ > > Change the function to use a dynamically allocated platform_device > instead of the on-stack device structure. The device is not actually > registered with the system, but is initialized enough to be used here. > > Fixes: ca25ec247aad ("iommu/io-pgtable-arm: Remove iommu_dev==NULL special case") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/iommu/io-pgtable-arm.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c > index 7632c80edea6..9f3bf0b5e8da 100644 > --- a/drivers/iommu/io-pgtable-arm.c > +++ b/drivers/iommu/io-pgtable-arm.c > @@ -13,6 +13,7 @@ > #include <linux/bitops.h> > #include <linux/io-pgtable.h> > #include <linux/kernel.h> > +#include <linux/platform_device.h> > #include <linux/sizes.h> > #include <linux/slab.h> > #include <linux/types.h> > @@ -1433,15 +1434,17 @@ static int __init arm_lpae_do_selftests(void) > }; > > int i, j, k, pass = 0, fail = 0; > - struct device dev; Could we not simply make this static? Per the comment it's only here to serve a NUMA node lookup buried deep in the pagetable allocator (TBH my first thought was to just put an int on the stack and contrive a pointer as the inverse of dev_to_node(), but I decided that would probably be too contentious...) > + struct platform_device *pdev; > struct io_pgtable_cfg cfg = { > .tlb = &dummy_tlb_ops, > .coherent_walk = true, > - .iommu_dev = &dev, > }; > > - /* __arm_lpae_alloc_pages() merely needs dev_to_node() to work */ > - set_dev_node(&dev, NUMA_NO_NODE); > + pdev = platform_device_alloc("io-pgtable-test", 0); Otherwise, this would seem to be another perfect case for the new faux_device. Thanks, Robin. > + if (!pdev) > + return -ENOMEM; > + > + cfg.iommu_dev = &pdev->dev; > > for (i = 0; i < ARRAY_SIZE(pgsize); ++i) { > for (j = 0; j < ARRAY_SIZE(address_size); ++j) { > @@ -1461,6 +1464,8 @@ static int __init arm_lpae_do_selftests(void) > } > > pr_info("selftest: completed with %d PASS %d FAIL\n", pass, fail); > + platform_device_put(pdev); > + > return fail ? -EFAULT : 0; > } > subsys_initcall(arm_lpae_do_selftests);
On Fri, Apr 11, 2025, at 15:19, Robin Murphy wrote: > On 11/04/2025 1:54 pm, Arnd Bergmann wrote: >> @@ -1433,15 +1434,17 @@ static int __init arm_lpae_do_selftests(void) >> }; >> >> int i, j, k, pass = 0, fail = 0; >> - struct device dev; > > Could we not simply make this static? Per the comment it's only here to > serve a NUMA node lookup buried deep in the pagetable allocator (TBH my > first thought was to just put an int on the stack and contrive a pointer > as the inverse of dev_to_node(), but I decided that would probably be > too contentious...) A static device would work here, but that has other (small) downsides: - static devices are discouraged for any real purpose because of the problematic lifetime rules. I think Greg would still want to eliminate these entirely. - there is slightly more memory usage: the __init function gets eliminated after boot, while a static allocation says around. It could perhaps be made __initdata. - If we ever need anything beyond the NUMA node from it, the dynamic allocation is probably close enough to make that work. >> + struct platform_device *pdev; >> struct io_pgtable_cfg cfg = { >> .tlb = &dummy_tlb_ops, >> .coherent_walk = true, >> - .iommu_dev = &dev, >> }; >> >> - /* __arm_lpae_alloc_pages() merely needs dev_to_node() to work */ >> - set_dev_node(&dev, NUMA_NO_NODE); >> + pdev = platform_device_alloc("io-pgtable-test", 0); > > Otherwise, this would seem to be another perfect case for the new > faux_device. Good point, that is clearly better than platform_device in this case. Shall I send a new version with that? Arnd
On 11/04/2025 2:44 pm, Arnd Bergmann wrote: > On Fri, Apr 11, 2025, at 15:19, Robin Murphy wrote: >> On 11/04/2025 1:54 pm, Arnd Bergmann wrote: >>> @@ -1433,15 +1434,17 @@ static int __init arm_lpae_do_selftests(void) >>> }; >>> >>> int i, j, k, pass = 0, fail = 0; >>> - struct device dev; >> >> Could we not simply make this static? Per the comment it's only here to >> serve a NUMA node lookup buried deep in the pagetable allocator (TBH my >> first thought was to just put an int on the stack and contrive a pointer >> as the inverse of dev_to_node(), but I decided that would probably be >> too contentious...) > > A static device would work here, but that has other (small) > downsides: > > - static devices are discouraged for any real purpose because > of the problematic lifetime rules. I think Greg would still > want to eliminate these entirely. > > - there is slightly more memory usage: the __init function > gets eliminated after boot, while a static allocation says > around. It could perhaps be made __initdata. > > - If we ever need anything beyond the NUMA node from it, the > dynamic allocation is probably close enough to make that > work. > >>> + struct platform_device *pdev; >>> struct io_pgtable_cfg cfg = { >>> .tlb = &dummy_tlb_ops, >>> .coherent_walk = true, >>> - .iommu_dev = &dev, >>> }; >>> >>> - /* __arm_lpae_alloc_pages() merely needs dev_to_node() to work */ >>> - set_dev_node(&dev, NUMA_NO_NODE); >>> + pdev = platform_device_alloc("io-pgtable-test", 0); >> >> Otherwise, this would seem to be another perfect case for the new >> faux_device. > > Good point, that is clearly better than platform_device in this > case. Shall I send a new version with that? Sure, I'm happy to consciously err on the side of caution and robustness, just making sure :) Thanks, Robin.
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c index 7632c80edea6..9f3bf0b5e8da 100644 --- a/drivers/iommu/io-pgtable-arm.c +++ b/drivers/iommu/io-pgtable-arm.c @@ -13,6 +13,7 @@ #include <linux/bitops.h> #include <linux/io-pgtable.h> #include <linux/kernel.h> +#include <linux/platform_device.h> #include <linux/sizes.h> #include <linux/slab.h> #include <linux/types.h> @@ -1433,15 +1434,17 @@ static int __init arm_lpae_do_selftests(void) }; int i, j, k, pass = 0, fail = 0; - struct device dev; + struct platform_device *pdev; struct io_pgtable_cfg cfg = { .tlb = &dummy_tlb_ops, .coherent_walk = true, - .iommu_dev = &dev, }; - /* __arm_lpae_alloc_pages() merely needs dev_to_node() to work */ - set_dev_node(&dev, NUMA_NO_NODE); + pdev = platform_device_alloc("io-pgtable-test", 0); + if (!pdev) + return -ENOMEM; + + cfg.iommu_dev = &pdev->dev; for (i = 0; i < ARRAY_SIZE(pgsize); ++i) { for (j = 0; j < ARRAY_SIZE(address_size); ++j) { @@ -1461,6 +1464,8 @@ static int __init arm_lpae_do_selftests(void) } pr_info("selftest: completed with %d PASS %d FAIL\n", pass, fail); + platform_device_put(pdev); + return fail ? -EFAULT : 0; } subsys_initcall(arm_lpae_do_selftests);