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

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

Posted by Viken Dadhaniya 11 months, 1 week ago

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.

Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.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);
+
 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];
+	}
 	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

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

Posted by kernel test robot 11 months, 1 week ago

Hi Viken,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on tty/tty-next tty/tty-linus usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.14-rc5 next-20250304]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Viken-Dadhaniya/serial-qcom-geni-Remove-alias-dependency-from-qcom-serial-driver/20250304-152222
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link:    https://lore.kernel.org/r/20250304071423.4033565-1-quic_vdadhani%40quicinc.com
patch subject: [PATCH v1] serial: qcom-geni: Remove alias dependency from qcom serial driver
config: x86_64-buildonly-randconfig-006-20250305 (https://download.01.org/0day-ci/archive/20250305/202503051821.tqFJ961p-lkp@intel.com/config)
compiler: clang version 19.1.7 (https://github.com/llvm/llvm-project cd708029e0b2869e80abe31ddb175f7c35361f90)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250305/202503051821.tqFJ961p-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503051821.tqFJ961p-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/tty/serial/qcom_geni_serial.c:267:40: warning: variable 'port' is uninitialized when used here [-Wuninitialized]
     267 |                         line = idr_alloc(&port_idr, (void *)port, max_alias_num + 1, nr_ports,
         |                                                             ^~~~
   drivers/tty/serial/qcom_geni_serial.c:255:36: note: initialize the variable 'port' to silence this warning
     255 |         struct qcom_geni_serial_port *port;
         |                                           ^
         |                                            = NULL
   1 warning generated.


vim +/port +267 drivers/tty/serial/qcom_geni_serial.c

   252	
   253	static struct qcom_geni_serial_port *get_port_from_line(int line, bool console)
   254	{
   255		struct qcom_geni_serial_port *port;
   256		int nr_ports = console ? GENI_UART_CONS_PORTS : GENI_UART_PORTS;
   257	
   258		if (console) {
   259			if (line < 0 || line >= nr_ports)
   260				return ERR_PTR(-ENXIO);
   261	
   262			port = &qcom_geni_console_port;
   263		} else {
   264			int max_alias_num = of_alias_get_highest_id("serial");
   265	
   266			if (line < 0 || line >= nr_ports)
 > 267				line = idr_alloc(&port_idr, (void *)port, max_alias_num + 1, nr_ports,
   268						 GFP_KERNEL);
   269			else
   270				line = idr_alloc(&port_idr, (void *)port, line, nr_ports, GFP_KERNEL);
   271	
   272			if (line < 0)
   273				return ERR_PTR(-ENXIO);
   274	
   275			port = &qcom_geni_uart_ports[line];
   276		}
   277		return port;
   278	}
   279	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

Posted by Bjorn Andersson 11 months, 1 week ago

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

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

Posted by Viken Dadhaniya 10 months, 2 weeks ago


On 3/4/2025 11:15 PM, Bjorn Andersson wrote:
> 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.

Sure, Updated in v2.

> 
>> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.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.

Use IDA in v2.

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

Yes, that's correct. We are also working on increasing the number of 
serial ports to support more ports on GENI hardware.

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

As per Greg's comment, used ida in v2.

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

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

Posted by Greg KH 11 months, 1 week ago

On Tue, Mar 04, 2025 at 11:45:53AM -0600, Bjorn Andersson wrote:
> 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@quicinc.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.

No, stick with an ida as that is what that is for and has the proper
locking built in, no need to "hand code" something as simple as a
numbering allocator with a bitmap.

thanks,

greg k-h