linux-kernel - Re: [PATCH v1] serial: qcom-geni: Remove alias dependency from qcom serial driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <tn6czifetdf2mg5gl3mhfpwcb6q7dkn5r4kgqln6evm4qdsjvi@ehpl3qj3axvw>
Date: Tue, 4 Mar 2025 11:45:53 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Viken Dadhaniya <quic_vdadhani@...cinc.com>
Cc: gregkh@...uxfoundation.org, jirislaby@...nel.org, 
	johan+linaro@...nel.org, dianders@...omium.org, konradybcio@...nel.org, 
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org, 
	quic_msavaliy@...cinc.com, quic_anupkulk@...cinc.com
Subject: Re: [PATCH v1] serial: qcom-geni: Remove alias dependency from qcom
 serial driver

On Tue, Mar 04, 2025 at 12:44:23PM +0530, Viken Dadhaniya wrote:
> Remove the dependency on aliases in the device tree configuration for the
> qcom serial driver. Currently, the absence of an alias results in an
> invalid line number, causing the driver probe to fail for geni serial.
> 
> To prevent probe failures, implement logic to dynamically assign line
> numbers if an alias is not present in the device tree for non-console
> ports.
> 

Please read and follow https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

Start with your problem description, then a description of your proposed
solution.

> Signed-off-by: Viken Dadhaniya <quic_vdadhani@...cinc.com>
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index a80ce7aaf309..2457f39dfc84 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -98,6 +98,8 @@
>  
>  #define DMA_RX_BUF_SIZE		2048
>  
> +static DEFINE_IDR(port_idr);

You're just looking for a index allocator, so DEFINE_IDA() is probably
what you want to use.


That said, theoretically I think we could get 24 GENI serial instances
in a system. Making this a huge waste of memory and CPU cycles.

An empty idr takes 88 bytes, if you then allocate 1 entry it grows with
at least one xa_array node which is 576 bytes.

> +
>  struct qcom_geni_device_data {
>  	bool console;
>  	enum geni_se_xfer_mode mode;
> @@ -253,10 +255,25 @@ static struct qcom_geni_serial_port *get_port_from_line(int line, bool console)
>  	struct qcom_geni_serial_port *port;
>  	int nr_ports = console ? GENI_UART_CONS_PORTS : GENI_UART_PORTS;
>  
> -	if (line < 0 || line >= nr_ports)
> -		return ERR_PTR(-ENXIO);
> +	if (console) {
> +		if (line < 0 || line >= nr_ports)
> +			return ERR_PTR(-ENXIO);
> +
> +		port = &qcom_geni_console_port;
> +	} else {
> +		int max_alias_num = of_alias_get_highest_id("serial");
> +
> +		if (line < 0 || line >= nr_ports)
> +			line = idr_alloc(&port_idr, (void *)port, max_alias_num + 1, nr_ports,
> +					 GFP_KERNEL);
> +		else
> +			line = idr_alloc(&port_idr, (void *)port, line, nr_ports, GFP_KERNEL);
> +
> +		if (line < 0)
> +			return ERR_PTR(-ENXIO);
>  
> -	port = console ? &qcom_geni_console_port : &qcom_geni_uart_ports[line];
> +		port = &qcom_geni_uart_ports[line];

Plus qcom_geni_uart_ports[] is GENI_UART_PORTS long. So you will
actually only have a maximum of 3 ports.


I like the change, but please replace port_idr with a u32 and use
linux/bitmap.h to implement the port allocation scheme.

Regards,
Bjorn

> +	}
>  	return port;
>  }
>  
> @@ -1761,6 +1778,7 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  						port->wakeup_irq);
>  		if (ret) {
>  			device_init_wakeup(&pdev->dev, false);
> +			idr_remove(&port_idr, uport->line);
>  			uart_remove_one_port(drv, uport);
>  			return ret;
>  		}
> @@ -1772,10 +1790,12 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  static void qcom_geni_serial_remove(struct platform_device *pdev)
>  {
>  	struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> +	struct uart_port *uport = &port->uport;
>  	struct uart_driver *drv = port->private_data.drv;
>  
>  	dev_pm_clear_wake_irq(&pdev->dev);
>  	device_init_wakeup(&pdev->dev, false);
> +	idr_remove(&port_idr, uport->line);
>  	uart_remove_one_port(drv, &port->uport);
>  }
>  
> -- 
> 2.34.1
> 
>