Message-ID: <CAMV7Lq6ky1w5j4QJPpHh-aLTAqWUywnF2hVoOE4nesL+2HSUJA@mail.gmail.com>
Date: Thu, 7 Aug 2025 02:36:24 +0530
From: Abinash Singh <abinashsinghlalotra@...il.com>
To: Arnd Bergmann <arnd@...db.de>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby <jirislaby@...nel.org>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Sunil V L <sunilvl@...tanamicro.com>, 
	Uwe Kleine-König <u.kleine-koenig@...libre.com>, 
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org
Subject: Re: [RFC PATCH 1/2] tty: serial/8250: Fix build warning in serial8250_probe_acpi()

On Wed, Aug 6, 2025 at 12:32 PM Arnd Bergmann <arnd@...db.de> wrote:
> Hi Abinash,
Hi ,Arnd

> I've seen this one as well in some configurations, thanks
> for helping out addressing it!

You are welcome ..! ,
 I have a patch accepted in that
commit : (3df63fa8f2af) dma: dw-edma: Fix build warning in dw_edma_pcie_probe()

In general, using dynamic memory allocation in functions that are not in
performance-critical paths—like probe routines—is a reasonable approach
to reduce large stack usage without performance penalties.

> The same problem does show up in a lot of 8250 driver variants,
> plus a couple that have it in theory but don't run into the
> 1024 byte limit:

You are absolutely right. I compiled these drivers with
"-fstack-usage" flag which generated the stack usage of each function.

> drivers/char/mwave/mwavedd.c-static int register_serial_portandirq(unsigned int port, int irq)
> drivers/char/mwave/mwavedd.c-{
> drivers/char/mwave/mwavedd.c:   struct uart_8250_port uart;
>
// stack usage using -fstack-usage in bytes
drivers/char/mwave/mwavedd.c:123:13:mwave_ioctl 216 static
drivers/char/mwave/mwavedd.c:437:12:register_serial_portandirq 976 static

> drivers/ptp/ptp_ocp.c-ptp_ocp_serial_line(struct ptp_ocp *bp, struct ocp_resource *r)
> drivers/ptp/ptp_ocp.c-{
> drivers/ptp/ptp_ocp.c-  struct pci_dev *pdev = bp->pdev;
> drivers/ptp/ptp_ocp.c:  struct uart_8250_port uart;
>
// stack usage using -fstack-usage bytes
drivers/ptp/ptp_ocp.c:2273:1:ptp_ocp_serial_line.isra 984 static
drivers/ptp/ptp_ocp.c:2295:1:ptp_ocp_register_serial 32 static

similar results for other drivers mentioned.....

> In total, I see 34 drivers using this exact pattern for
> their probe function, and ideally we would to to fix them
> all to do it some other way.
>
Dynamic memory allocation is acceptable in probe functions,
as the introduced delay is minimal and these functions are
called infrequently.

> Note how serial8250_register_8250_port() ands up just copying
> individual members of the passed uart_8250_port structure into
> the global array of the same type, so one way of addressing
> this would be to use a structure for initialization that only
> contains a subset of the uart_8250_port members and can still
> be allocated on the stack, or possibly be constant.
>
This is a very good approach because it confines dynamic
memory allocation to the probe function, where it is acceptable.
In most other functions, dynamic allocation is generally not suitable.

I also reviewed a few functions and noticed that only a small number
of fields from the stack-allocated struct uart_8250_port are
actually used. Therefore, this method is well-suited for such cases.

> There is already a 'struct plat_serial8250_port', which some
> (mostly ancient) drivers use to register a 8250 compatible
> uart through the 8250_platform driver. That driver has
> a number of problems, so I don't really want to expand its
> usage, but it may be possible to use a single structure
> for both purposes.
>
To proceed, we can introduce a smaller struct containing only the
required fields.
In the probe function, the choice between dynamic allocation and using
this reduced
struct can be made based on the number of fields used in each specific scenario.

Thanks.

Abinash

Powered by blists - more mailing lists