| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2025071206-grip-overblown-bf53@gregkh>
Date: Sat, 12 Jul 2025 08:47:42 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Chaitanya Vadrevu <chaitanya.vadrevu@...rson.com>
Cc: jirislaby@...nel.org, linux-serial@...r.kernel.org,
linux-kernel@...r.kernel.org, kernel test robot <lkp@...el.com>,
Jason Smith <jason.smith@...rson.com>,
Gratian Crisan <gratian.crisan@...rson.com>
Subject: Re: [PATCH] serial: 8250_ni: Fix build warning
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@...el.com>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/202507030557.vIewJJQO-lkp@intel.com/
> > > Cc: Jason Smith <jason.smith@...rson.com>
> > > Cc: Gratian Crisan <gratian.crisan@...rson.com>
> > > Signed-off-by: Chaitanya Vadrevu <chaitanya.vadrevu@...rson.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
Powered by blists - more mailing lists