| Message ID | 20240320180333.151043-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc: ps3: mark ps3_notification_device static for stack usage | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
On 3/21/24 03:03, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The device is way too large to be on the stack, causing a warning for > an allmodconfig build with clang: > > arch/powerpc/platforms/ps3/device-init.c:771:12: error: stack frame size (2064) exceeds limit (2048) in 'ps3_probe_thread' [-Werror,-Wframe-larger-than] > 771 | static int ps3_probe_thread(void *data) > > There is only a single thread that ever accesses this, so it's fine to > make it static, which avoids the warning. > > Fixes: b4cb2941f855 ("[POWERPC] PS3: Use the HVs storage device notification mechanism properly") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > arch/powerpc/platforms/ps3/device-init.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/platforms/ps3/device-init.c b/arch/powerpc/platforms/ps3/device-init.c > index 878bc160246e..ce99f06698a9 100644 > --- a/arch/powerpc/platforms/ps3/device-init.c > +++ b/arch/powerpc/platforms/ps3/device-init.c > @@ -770,7 +770,7 @@ static struct task_struct *probe_task; > > static int ps3_probe_thread(void *data) > { > - struct ps3_notification_device dev; > + static struct ps3_notification_device dev; > int res; > unsigned int irq; > u64 lpar; Seems fine. Acked-by: Geoff Levand <geoff@infradead.org>
Hi Arnd, On Wed, Mar 20, 2024 at 7:03 PM Arnd Bergmann <arnd@kernel.org> wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The device is way too large to be on the stack, causing a warning for > an allmodconfig build with clang: > > arch/powerpc/platforms/ps3/device-init.c:771:12: error: stack frame size (2064) exceeds limit (2048) in 'ps3_probe_thread' [-Werror,-Wframe-larger-than] > 771 | static int ps3_probe_thread(void *data) > > There is only a single thread that ever accesses this, so it's fine to > make it static, which avoids the warning. > > Fixes: b4cb2941f855 ("[POWERPC] PS3: Use the HVs storage device notification mechanism properly") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Thanks for your patch! > --- a/arch/powerpc/platforms/ps3/device-init.c > +++ b/arch/powerpc/platforms/ps3/device-init.c > @@ -770,7 +770,7 @@ static struct task_struct *probe_task; > > static int ps3_probe_thread(void *data) > { > - struct ps3_notification_device dev; > + static struct ps3_notification_device dev; > int res; > unsigned int irq; > u64 lpar; Making it static increases kernel size for everyone. So I'd rather allocate it dynamically. The thread already allocates a buffer, which can be replaced at no cost by allocating a structure containing both the ps3_notification_device and the buffer. Gr{oetje,eeting}s, Geert
Hi Geert, On 3/21/24 17:32, Geert Uytterhoeven wrote: >> static int ps3_probe_thread(void *data) >> { >> - struct ps3_notification_device dev; >> + static struct ps3_notification_device dev; >> int res; >> unsigned int irq; >> u64 lpar; > > Making it static increases kernel size for everyone. So I'd rather > allocate it dynamically. The thread already allocates a buffer, which > can be replaced at no cost by allocating a structure containing both > the ps3_notification_device and the buffer. This seems like a much better solution. -Geoff
On 3/21/24 17:32, Geert Uytterhoeven wrote: > --- a/arch/powerpc/platforms/ps3/device-init.c >> +++ b/arch/powerpc/platforms/ps3/device-init.c >> @@ -770,7 +770,7 @@ static struct task_struct *probe_task; >> >> static int ps3_probe_thread(void *data) >> { >> - struct ps3_notification_device dev; >> + static struct ps3_notification_device dev; >> int res; >> unsigned int irq; >> u64 lpar; > > Making it static increases kernel size for everyone. So I'd rather > allocate it dynamically. The thread already allocates a buffer, which > can be replaced at no cost by allocating a structure containing both > the ps3_notification_device and the buffer. Here's what I came up with. It builds for me without warnings. I haven't tested it yet. A review would be appreciated. diff --git a/arch/powerpc/platforms/ps3/device-init.c b/arch/powerpc/platforms/ps3/device-init.c index 878bc160246e..9bb44a6ccdaf 100644 --- a/arch/powerpc/platforms/ps3/device-init.c +++ b/arch/powerpc/platforms/ps3/device-init.c @@ -770,37 +770,48 @@ static struct task_struct *probe_task; static int ps3_probe_thread(void *data) { - struct ps3_notification_device dev; + struct ps3_probe_thread_local { + struct ps3_notification_device dev; + union { + char buf[512]; + struct ps3_notify_cmd notify_cmd; + struct ps3_notify_event notify_event; + }; + }; + struct ps3_probe_thread_local *local; + struct ps3_notification_device *dev; + struct ps3_notify_cmd *notify_cmd; + struct ps3_notify_event *notify_event; int res; unsigned int irq; u64 lpar; - void *buf; - struct ps3_notify_cmd *notify_cmd; - struct ps3_notify_event *notify_event; pr_debug(" -> %s:%u: kthread started\n", __func__, __LINE__); - buf = kzalloc(512, GFP_KERNEL); - if (!buf) + local = kzalloc(sizeof(local), GFP_KERNEL); + + if (!local) return -ENOMEM; - lpar = ps3_mm_phys_to_lpar(__pa(buf)); - notify_cmd = buf; - notify_event = buf; + dev = &local->dev; + notify_cmd = &local->notify_cmd; + notify_event = &local->notify_event; + + lpar = ps3_mm_phys_to_lpar(__pa(&local->notify_cmd)); /* dummy system bus device */ - dev.sbd.bus_id = (u64)data; - dev.sbd.dev_id = PS3_NOTIFICATION_DEV_ID; - dev.sbd.interrupt_id = PS3_NOTIFICATION_INTERRUPT_ID; + dev->sbd.bus_id = (u64)data; + dev->sbd.dev_id = PS3_NOTIFICATION_DEV_ID; + dev->sbd.interrupt_id = PS3_NOTIFICATION_INTERRUPT_ID; - res = lv1_open_device(dev.sbd.bus_id, dev.sbd.dev_id, 0); + res = lv1_open_device(dev->sbd.bus_id, dev->sbd.dev_id, 0); if (res) { pr_err("%s:%u: lv1_open_device failed %s\n", __func__, __LINE__, ps3_result(res)); goto fail_free; } - res = ps3_sb_event_receive_port_setup(&dev.sbd, PS3_BINDING_CPU_ANY, + res = ps3_sb_event_receive_port_setup(&dev->sbd, PS3_BINDING_CPU_ANY, &irq); if (res) { pr_err("%s:%u: ps3_sb_event_receive_port_setup failed %d\n", @@ -808,11 +819,11 @@ static int ps3_probe_thread(void *data) goto fail_close_device; } - spin_lock_init(&dev.lock); - rcuwait_init(&dev.wait); + spin_lock_init(&dev->lock); + rcuwait_init(&dev->wait); res = request_irq(irq, ps3_notification_interrupt, 0, - "ps3_notification", &dev); + "ps3_notification", &local->dev); if (res) { pr_err("%s:%u: request_irq failed %d\n", __func__, __LINE__, res); @@ -823,7 +834,7 @@ static int ps3_probe_thread(void *data) notify_cmd->operation_code = 0; /* must be zero */ notify_cmd->event_mask = 1UL << notify_region_probe; - res = ps3_notification_read_write(&dev, lpar, 1); + res = ps3_notification_read_write(&local->dev, lpar, 1); if (res) goto fail_free_irq; @@ -834,36 +845,36 @@ static int ps3_probe_thread(void *data) memset(notify_event, 0, sizeof(*notify_event)); - res = ps3_notification_read_write(&dev, lpar, 0); + res = ps3_notification_read_write(&local->dev, lpar, 0); if (res) break; pr_debug("%s:%u: notify event type 0x%llx bus id %llu dev id %llu" " type %llu port %llu\n", __func__, __LINE__, - notify_event->event_type, notify_event->bus_id, - notify_event->dev_id, notify_event->dev_type, - notify_event->dev_port); + notify_event->event_type, notify_event->bus_id, + notify_event->dev_id, notify_event->dev_type, + notify_event->dev_port); if (notify_event->event_type != notify_region_probe || - notify_event->bus_id != dev.sbd.bus_id) { + notify_event->bus_id != dev->sbd.bus_id) { pr_warn("%s:%u: bad notify_event: event %llu, dev_id %llu, dev_type %llu\n", __func__, __LINE__, notify_event->event_type, notify_event->dev_id, notify_event->dev_type); continue; } - ps3_find_and_add_device(dev.sbd.bus_id, notify_event->dev_id); + ps3_find_and_add_device(dev->sbd.bus_id, notify_event->dev_id); } while (!kthread_should_stop()); fail_free_irq: - free_irq(irq, &dev); + free_irq(irq, &local->dev); fail_sb_event_receive_port_destroy: - ps3_sb_event_receive_port_destroy(&dev.sbd, irq); + ps3_sb_event_receive_port_destroy(&dev->sbd, irq); fail_close_device: - lv1_close_device(dev.sbd.bus_id, dev.sbd.dev_id); + lv1_close_device(dev->sbd.bus_id, dev->sbd.dev_id); fail_free: - kfree(buf); + kfree(local); probe_task = NULL;
On Fri, Mar 22, 2024, at 09:34, Geoff Levand wrote: > On 3/21/24 17:32, Geert Uytterhoeven wrote: >> --- a/arch/powerpc/platforms/ps3/device-init.c >>> +++ b/arch/powerpc/platforms/ps3/device-init.c >>> @@ -770,7 +770,7 @@ static struct task_struct *probe_task; >>> >>> static int ps3_probe_thread(void *data) >>> { >>> - struct ps3_notification_device dev; >>> + static struct ps3_notification_device dev; >>> int res; >>> unsigned int irq; >>> u64 lpar; >> >> Making it static increases kernel size for everyone. So I'd rather >> allocate it dynamically. The thread already allocates a buffer, which >> can be replaced at no cost by allocating a structure containing both >> the ps3_notification_device and the buffer. I didn't think it mattered much, given that you would rarely have a kernel with PS3 support along with other platforms. I suppose it does increase the size for a PS3-only kernel as well, while your version makes it smaller. > Here's what I came up with. It builds for me without warnings. > I haven't tested it yet. A review would be appreciated. It seems a little complicated but looks all correct to me and reduces both stack and .data size, so Acked-by: Arnd Bergmann <arnd@arndb.de>
On 3/23/24 05:24, Arnd Bergmann wrote: > On Fri, Mar 22, 2024, at 09:34, Geoff Levand wrote: >> On 3/21/24 17:32, Geert Uytterhoeven wrote: >>> --- a/arch/powerpc/platforms/ps3/device-init.c >>>> +++ b/arch/powerpc/platforms/ps3/device-init.c >>>> @@ -770,7 +770,7 @@ static struct task_struct *probe_task; >>>> >>>> static int ps3_probe_thread(void *data) >>>> { >>>> - struct ps3_notification_device dev; >>>> + static struct ps3_notification_device dev; >>>> int res; >>>> unsigned int irq; >>>> u64 lpar; >>> >>> Making it static increases kernel size for everyone. So I'd rather >>> allocate it dynamically. The thread already allocates a buffer, which >>> can be replaced at no cost by allocating a structure containing both >>> the ps3_notification_device and the buffer. > > I didn't think it mattered much, given that you would rarely > have a kernel with PS3 support along with other platforms. > > I suppose it does increase the size for a PS3-only kernel > as well, while your version makes it smaller. > >> Here's what I came up with. It builds for me without warnings. >> I haven't tested it yet. A review would be appreciated. > > It seems a little complicated but looks all correct to > me and reduces both stack and .data size, so > > Acked-by: Arnd Bergmann <arnd@arndb.de> Thanks Arnd. I also thought it was a bit too much. I made a simpler version that I'll post as a separate message. -Geoff
diff --git a/arch/powerpc/platforms/ps3/device-init.c b/arch/powerpc/platforms/ps3/device-init.c index 878bc160246e..ce99f06698a9 100644 --- a/arch/powerpc/platforms/ps3/device-init.c +++ b/arch/powerpc/platforms/ps3/device-init.c @@ -770,7 +770,7 @@ static struct task_struct *probe_task; static int ps3_probe_thread(void *data) { - struct ps3_notification_device dev; + static struct ps3_notification_device dev; int res; unsigned int irq; u64 lpar;