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

[Bjorn Andersson]

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