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

[Chaitanya Vadrevu <chaitanya.vadrevu@emerson.com>]

> > 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.

Thanks,
Chaitanya