Re: [PATCH] serial: 8250_ni: Fix build warning

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Jul 11, 2025 at 02:27:48PM -0500, Chaitanya Vadrevu wrote:
> > > Allocate memory on heap instead of stack to fix following warning that
> > > clang version 20.1.2 produces on W=1 build.
> > > 
> > > drivers/tty/serial/8250/8250_ni.c:277:12: warning: stack frame size (1072) exceeds limit (1024) in 'ni16550_probe' [-Wframe-larger-than]
> > >   277 | static int ni16550_probe(struct platform_device *pdev)
> > >       |            ^
> > >   1 warning generated.
> > > 
> > > Also, reorder variable declarations to follow reverse Christmas tree
> > > style.
> > 
> > When you say "also", that's usually a hint this should be a separate
> > patch :(
> 
> Sure, I'll split this and send a v2.
> 
> > > 
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/202507030557.vIewJJQO-lkp@intel.com/
> > > Cc: Jason Smith <jason.smith@emerson.com>
> > > Cc: Gratian Crisan <gratian.crisan@emerson.com>
> > > Signed-off-by: Chaitanya Vadrevu <chaitanya.vadrevu@emerson.com>
> > > ---
> > >  drivers/tty/serial/8250/8250_ni.c | 56 +++++++++++++++++--------------
> > >  1 file changed, 30 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/8250/8250_ni.c b/drivers/tty/serial/8250/8250_ni.c
> > > index b0e44fb00b3a4..cb5b42b3609c9 100644
> > > --- a/drivers/tty/serial/8250/8250_ni.c
> > > +++ b/drivers/tty/serial/8250/8250_ni.c
> > > @@ -275,76 +275,80 @@ static void ni16550_set_mctrl(struct uart_port *port, unsigned int mctrl)
> > >  
> > >  static int ni16550_probe(struct platform_device *pdev)
> > >  {
> > > +	struct uart_8250_port *uart __free(kfree) = NULL;
> > >  	const struct ni16550_device_info *info;
> > >  	struct device *dev = &pdev->dev;
> > > -	struct uart_8250_port uart = {};
> > >  	unsigned int txfifosz, rxfifosz;
> > > -	unsigned int prescaler;
> > >  	struct ni16550_data *data;
> > > +	unsigned int prescaler;
> > >  	const char *portmode;
> > >  	bool rs232_property;
> > >  	int ret;
> > >  
> > > +	uart = kzalloc(sizeof(*uart), GFP_KERNEL);
> > > +	if (!uart)
> > > +		return -ENOMEM;
> > > +
> > >  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > >  	if (!data)
> > >  		return -ENOMEM;
> > >  
> > > -	spin_lock_init(&uart.port.lock);
> > > +	spin_lock_init(&uart->port.lock);
> > >  
> > > -	ret = ni16550_get_regs(pdev, &uart.port);
> > > +	ret = ni16550_get_regs(pdev, &uart->port);
> > >  	if (ret < 0)
> > >  		return ret;
> > >  
> > >  	/* early setup so that serial_in()/serial_out() work */
> > > -	serial8250_set_defaults(&uart);
> > > +	serial8250_set_defaults(uart);
> > >  
> > >  	info = device_get_match_data(dev);
> > >  
> > > -	uart.port.dev		= dev;
> > > -	uart.port.flags		= UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_FIXED_TYPE;
> > > -	uart.port.startup	= ni16550_port_startup;
> > > -	uart.port.shutdown	= ni16550_port_shutdown;
> > > +	uart->port.dev		= dev;
> > > +	uart->port.flags	= UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_FIXED_TYPE;
> > > +	uart->port.startup	= ni16550_port_startup;
> > > +	uart->port.shutdown	= ni16550_port_shutdown;
> > >  
> > >  	/*
> > >  	 * Hardware instantiation of FIFO sizes are held in registers.
> > >  	 */
> > > -	txfifosz = ni16550_read_fifo_size(&uart, NI16550_TFS_OFFSET);
> > > -	rxfifosz = ni16550_read_fifo_size(&uart, NI16550_RFS_OFFSET);
> > > +	txfifosz = ni16550_read_fifo_size(uart, NI16550_TFS_OFFSET);
> > > +	rxfifosz = ni16550_read_fifo_size(uart, NI16550_RFS_OFFSET);
> > >  
> > >  	dev_dbg(dev, "NI 16550 has TX FIFO size %u, RX FIFO size %u\n",
> > >  		txfifosz, rxfifosz);
> > >  
> > > -	uart.port.type		= PORT_16550A;
> > > -	uart.port.fifosize	= txfifosz;
> > > -	uart.tx_loadsz		= txfifosz;
> > > -	uart.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10;
> > > -	uart.capabilities	= UART_CAP_FIFO | UART_CAP_AFE | UART_CAP_EFR;
> > > +	uart->port.type		= PORT_16550A;
> > > +	uart->port.fifosize	= txfifosz;
> > > +	uart->tx_loadsz		= txfifosz;
> > > +	uart->fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10;
> > > +	uart->capabilities	= UART_CAP_FIFO | UART_CAP_AFE | UART_CAP_EFR;
> > >  
> > >  	/*
> > >  	 * Declaration of the base clock frequency can come from one of:
> > >  	 * - static declaration in this driver (for older ACPI IDs)
> > >  	 * - a "clock-frequency" ACPI
> > >  	 */
> > > -	uart.port.uartclk = info->uartclk;
> > > +	uart->port.uartclk = info->uartclk;
> > >  
> > > -	ret = uart_read_port_properties(&uart.port);
> > > +	ret = uart_read_port_properties(&uart->port);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	if (!uart.port.uartclk) {
> > > +	if (!uart->port.uartclk) {
> > >  		data->clk = devm_clk_get_enabled(dev, NULL);
> > >  		if (!IS_ERR(data->clk))
> > > -			uart.port.uartclk = clk_get_rate(data->clk);
> > > +			uart->port.uartclk = clk_get_rate(data->clk);
> > >  	}
> > >  
> > > -	if (!uart.port.uartclk)
> > > +	if (!uart->port.uartclk)
> > >  		return dev_err_probe(dev, -ENODEV, "unable to determine clock frequency!\n");
> > >  
> > >  	prescaler = info->prescaler;
> > >  	device_property_read_u32(dev, "clock-prescaler", &prescaler);
> > >  	if (prescaler) {
> > > -		uart.port.set_mctrl = ni16550_set_mctrl;
> > > -		ni16550_config_prescaler(&uart, (u8)prescaler);
> > > +		uart->port.set_mctrl = ni16550_set_mctrl;
> > > +		ni16550_config_prescaler(uart, (u8)prescaler);
> > >  	}
> > >  
> > >  	/*
> > > @@ -362,7 +366,7 @@ static int ni16550_probe(struct platform_device *pdev)
> > >  		dev_dbg(dev, "port is in %s mode (via device property)\n",
> > >  			rs232_property ? "RS-232" : "RS-485");
> > >  	} else if (info->flags & NI_HAS_PMR) {
> > > -		rs232_property = is_pmr_rs232_mode(&uart);
> > > +		rs232_property = is_pmr_rs232_mode(uart);
> > >  
> > >  		dev_dbg(dev, "port is in %s mode (via PMR)\n",
> > >  			rs232_property ? "RS-232" : "RS-485");
> > > @@ -377,10 +381,10 @@ static int ni16550_probe(struct platform_device *pdev)
> > >  		 * Neither the 'transceiver' property nor the PMR indicate
> > >  		 * that this is an RS-232 port, so it must be an RS-485 one.
> > >  		 */
> > > -		ni16550_rs485_setup(&uart.port);
> > > +		ni16550_rs485_setup(&uart->port);
> > >  	}
> > >  
> > > -	ret = serial8250_register_8250_port(&uart);
> > > +	ret = serial8250_register_8250_port(uart);
> > 
> > So uart is freed after this and that's ok?  Are you sure?
> 
> Before this patch, uart was defined on stack where it would also be
> freed at the end of the function.
> I see similar pattern being used in other 8250 drivers where they also
> define uart_8250_port on stack. So, I don't believe this is an issue.

Ah, you are right, sorry, I always get that one wrong.  I'll just take
this as-is, no need to redo it.

Also, if other drivers put this on the stack, they should get the same
fixup as well.

thanks,

greg k-h