[v1] serial: 8250_ni: Fix build warning

[PATCH] serial: 8250_ni: Fix build warning

Posted by Chaitanya Vadrevu 2 months, 4 weeks ago

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.

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);
 	if (ret < 0)
 		return ret;
 	data->line = ret;
-- 
2.43.0

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

Posted by Greg KH 2 months, 4 weeks ago

On Thu, Jul 10, 2025 at 05:38:37PM -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 :(

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

thanks,

greg k-h

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

Posted by Chaitanya Vadrevu 2 months, 3 weeks ago

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

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

Posted by Greg KH 2 months, 3 weeks ago

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

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

Posted by Greg KH 2 months, 3 weeks ago

On Sat, Jul 12, 2025 at 08:47:42AM +0200, Greg KH wrote:
> Ah, you are right, sorry, I always get that one wrong.  I'll just take
> this as-is, no need to redo it.

You already sent v2, thanks, I'll take that.

greg k-h

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

Posted by Jiri Slaby 2 months, 4 weeks ago

On 11. 07. 25, 0:38, 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]

Hmm, OK:
$ pahole -sy uart_8250_port drivers/tty/serial/8250/8250_port.o
uart_8250_port  784     4

That's quite a few. I am not sure why we did not see this earlier? 
Perhaps because CONFIG_FRAME_WARN is set higher for most use/compilation 
cases.

Should we switch all probes?

I have a patchset to move ops to a separate struct (pointer), but that's 
far from finished and it does not save that much:
uart_8250_port  624     4

Another question is if we can introduce some lightweight struct for 
register instead of full uart_8250_port which is copied from (and only 
small part of it).

So perhaps:
Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

thanks,
-- 
js
suse labs