[PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Vignesh R]

Using dev_name() as irq name during request_irq() might be misleading in
case of serial over PCI. Therefore use a better alternative name for
identifying serial port irqs as "serial" appended with serial_index of
the port. This ensures that "serial" string is always present in irq
name while port index will help in distinguishing b/w different ports.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---

Based on top of tty-next.

 drivers/tty/serial/8250/8250_core.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index f83b69f30987..78bf621d6827 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -213,12 +213,20 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
 
 		ret = 0;
 	} else {
+		char *irqname = kmalloc(20, GFP_KERNEL);
+		if (!irqname) {
+			spin_unlock_irq(&i->lock);
+			return -ENOMEM;
+		}
+		snprintf(irqname, 20,
+			 "serial%d", serial_index(&up->port));
+
 		INIT_LIST_HEAD(&up->list);
 		i->head = &up->list;
 		spin_unlock_irq(&i->lock);
 		irq_flags |= up->port.irqflags;
 		ret = request_irq(up->port.irq, serial8250_interrupt,
-				  irq_flags, dev_name(up->port.dev), i);
+				  irq_flags, irqname, i);
 		if (ret < 0)
 			serial_do_unlink(i, up);
 	}
-- 
2.11.0

[PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Vignesh R]

Using dev_name() as irq name during request_irq() might be misleading in
case of serial over PCI. Therefore use a better alternative name for
identifying serial port irqs as "serial" appended with serial_index of
the port. This ensures that "serial" string is always present in irq
name while port index will help in distinguishing b/w different ports.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---

Based on top of tty-next.

 drivers/tty/serial/8250/8250_core.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index f83b69f30987..78bf621d6827 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -213,12 +213,20 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
 
 		ret = 0;
 	} else {
+		char *irqname = kmalloc(20, GFP_KERNEL);
+		if (!irqname) {
+			spin_unlock_irq(&i->lock);
+			return -ENOMEM;
+		}
+		snprintf(irqname, 20,
+			 "serial%d", serial_index(&up->port));
+
 		INIT_LIST_HEAD(&up->list);
 		i->head = &up->list;
 		spin_unlock_irq(&i->lock);
 		irq_flags |= up->port.irqflags;
 		ret = request_irq(up->port.irq, serial8250_interrupt,
-				  irq_flags, dev_name(up->port.dev), i);
+				  irq_flags, irqname, i);
 		if (ret < 0)
 			serial_do_unlink(i, up);
 	}
-- 
2.11.0

[PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Vignesh R]

Using dev_name() as irq name during request_irq() might be misleading in
case of serial over PCI. Therefore use a better alternative name for
identifying serial port irqs as "serial" appended with serial_index of
the port. This ensures that "serial" string is always present in irq
name while port index will help in distinguishing b/w different ports.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---

Based on top of tty-next.

 drivers/tty/serial/8250/8250_core.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index f83b69f30987..78bf621d6827 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -213,12 +213,20 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
 
 		ret = 0;
 	} else {
+		char *irqname = kmalloc(20, GFP_KERNEL);
+		if (!irqname) {
+			spin_unlock_irq(&i->lock);
+			return -ENOMEM;
+		}
+		snprintf(irqname, 20,
+			 "serial%d", serial_index(&up->port));
+
 		INIT_LIST_HEAD(&up->list);
 		i->head = &up->list;
 		spin_unlock_irq(&i->lock);
 		irq_flags |= up->port.irqflags;
 		ret = request_irq(up->port.irq, serial8250_interrupt,
-				  irq_flags, dev_name(up->port.dev), i);
+				  irq_flags, irqname, i);
 		if (ret < 0)
 			serial_do_unlink(i, up);
 	}
-- 
2.11.0

Re: [PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Jiri Slaby]

On 03/16/2017, 01:26 PM, Vignesh R wrote:
> Using dev_name() as irq name during request_irq() might be misleading in
> case of serial over PCI. Therefore use a better alternative name for
> identifying serial port irqs as "serial" appended with serial_index of
> the port. This ensures that "serial" string is always present in irq
> name while port index will help in distinguishing b/w different ports.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
> 
> Based on top of tty-next.
> 
>  drivers/tty/serial/8250/8250_core.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index f83b69f30987..78bf621d6827 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -213,12 +213,20 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
>  
>  		ret = 0;
>  	} else {
> +		char *irqname = kmalloc(20, GFP_KERNEL);
> +		if (!irqname) {
> +			spin_unlock_irq(&i->lock);

GFP_KERNEL and spin_lock won't play well together.

-- 
js
suse labs

[PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Jiri Slaby]

On 03/16/2017, 01:26 PM, Vignesh R wrote:
> Using dev_name() as irq name during request_irq() might be misleading in
> case of serial over PCI. Therefore use a better alternative name for
> identifying serial port irqs as "serial" appended with serial_index of
> the port. This ensures that "serial" string is always present in irq
> name while port index will help in distinguishing b/w different ports.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
> 
> Based on top of tty-next.
> 
>  drivers/tty/serial/8250/8250_core.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index f83b69f30987..78bf621d6827 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -213,12 +213,20 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
>  
>  		ret = 0;
>  	} else {
> +		char *irqname = kmalloc(20, GFP_KERNEL);
> +		if (!irqname) {
> +			spin_unlock_irq(&i->lock);

GFP_KERNEL and spin_lock won't play well together.

-- 
js
suse labs

Re: [PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Jiri Slaby]

On 03/16/2017, 01:46 PM, Jiri Slaby wrote:
> On 03/16/2017, 01:26 PM, Vignesh R wrote:
>> Using dev_name() as irq name during request_irq() might be misleading in
>> case of serial over PCI. Therefore use a better alternative name for
>> identifying serial port irqs as "serial" appended with serial_index of
>> the port. This ensures that "serial" string is always present in irq
>> name while port index will help in distinguishing b/w different ports.
>>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>>
>> Based on top of tty-next.
>>
>>  drivers/tty/serial/8250/8250_core.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index f83b69f30987..78bf621d6827 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -213,12 +213,20 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
>>  
>>  		ret = 0;
>>  	} else {
>> +		char *irqname = kmalloc(20, GFP_KERNEL);
>> +		if (!irqname) {
>> +			spin_unlock_irq(&i->lock);
> 
> GFP_KERNEL and spin_lock won't play well together.

And you never free the memory. Given we have struct device, can we use
devm_kmalloc after spin_unlock_irq (you have to check if we cannot
allocate multiple times)?

thanks,
-- 
js
suse labs

[PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Jiri Slaby]

On 03/16/2017, 01:46 PM, Jiri Slaby wrote:
> On 03/16/2017, 01:26 PM, Vignesh R wrote:
>> Using dev_name() as irq name during request_irq() might be misleading in
>> case of serial over PCI. Therefore use a better alternative name for
>> identifying serial port irqs as "serial" appended with serial_index of
>> the port. This ensures that "serial" string is always present in irq
>> name while port index will help in distinguishing b/w different ports.
>>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>>
>> Based on top of tty-next.
>>
>>  drivers/tty/serial/8250/8250_core.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index f83b69f30987..78bf621d6827 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -213,12 +213,20 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
>>  
>>  		ret = 0;
>>  	} else {
>> +		char *irqname = kmalloc(20, GFP_KERNEL);
>> +		if (!irqname) {
>> +			spin_unlock_irq(&i->lock);
> 
> GFP_KERNEL and spin_lock won't play well together.

And you never free the memory. Given we have struct device, can we use
devm_kmalloc after spin_unlock_irq (you have to check if we cannot
allocate multiple times)?

thanks,
-- 
js
suse labs

Re: [PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Russell King - ARM Linux]

On Thu, Mar 16, 2017 at 05:56:53PM +0530, Vignesh R wrote:
> Using dev_name() as irq name during request_irq() might be misleading in
> case of serial over PCI. Therefore use a better alternative name for
> identifying serial port irqs as "serial" appended with serial_index of
> the port. This ensures that "serial" string is always present in irq
> name while port index will help in distinguishing b/w different ports.

Wouldn't it be better to use the device name (iow, ttySx) rather than
"serialx" ?

Maybe a helper function in serial_core.c to format the device name into
a supplied string, which can be re-used elsewhere, eg, uart_report_port()
and uart_suspend_port().  IOW:

const char *uart_port_name(char *buf, size_t n, struct uart_driver *drv,
			   struct uart_port *port)
{
	snprintf(buf, n, "%s%d", drv->dev_name,
		 drv->tty_driver->name_base + port->line);

	return buf;
}

which means you can do this:

	char name[16];

	request_irq(..., uart_port_name(name, sizeof(name), driver, port), ...)

which also avoids the allocation.  8250 device names are always "ttyS"
plus a number, so 16 characters (including NULL terminator) should be
more than sufficient, and that's most likely true of all serial drivers.
(The longest device name I'm aware of is ttyAMA plus a small integer
for PL011 ports.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Russell King - ARM Linux]

On Thu, Mar 16, 2017 at 05:56:53PM +0530, Vignesh R wrote:
> Using dev_name() as irq name during request_irq() might be misleading in
> case of serial over PCI. Therefore use a better alternative name for
> identifying serial port irqs as "serial" appended with serial_index of
> the port. This ensures that "serial" string is always present in irq
> name while port index will help in distinguishing b/w different ports.

Wouldn't it be better to use the device name (iow, ttySx) rather than
"serialx" ?

Maybe a helper function in serial_core.c to format the device name into
a supplied string, which can be re-used elsewhere, eg, uart_report_port()
and uart_suspend_port().  IOW:

const char *uart_port_name(char *buf, size_t n, struct uart_driver *drv,
			   struct uart_port *port)
{
	snprintf(buf, n, "%s%d", drv->dev_name,
		 drv->tty_driver->name_base + port->line);

	return buf;
}

which means you can do this:

	char name[16];

	request_irq(..., uart_port_name(name, sizeof(name), driver, port), ...)

which also avoids the allocation.  8250 device names are always "ttyS"
plus a number, so 16 characters (including NULL terminator) should be
more than sufficient, and that's most likely true of all serial drivers.
(The longest device name I'm aware of is ttyAMA plus a small integer
for PL011 ports.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Robin Murphy]

On 16/03/17 13:36, Russell King - ARM Linux wrote:
> On Thu, Mar 16, 2017 at 05:56:53PM +0530, Vignesh R wrote:
>> Using dev_name() as irq name during request_irq() might be misleading in
>> case of serial over PCI. Therefore use a better alternative name for
>> identifying serial port irqs as "serial" appended with serial_index of
>> the port. This ensures that "serial" string is always present in irq
>> name while port index will help in distinguishing b/w different ports.
> 
> Wouldn't it be better to use the device name (iow, ttySx) rather than
> "serialx" ?
> 
> Maybe a helper function in serial_core.c to format the device name into
> a supplied string, which can be re-used elsewhere, eg, uart_report_port()
> and uart_suspend_port().  IOW:
> 
> const char *uart_port_name(char *buf, size_t n, struct uart_driver *drv,
> 			   struct uart_port *port)
> {
> 	snprintf(buf, n, "%s%d", drv->dev_name,
> 		 drv->tty_driver->name_base + port->line);
> 
> 	return buf;
> }
> 
> which means you can do this:
> 
> 	char name[16];
> 
> 	request_irq(..., uart_port_name(name, sizeof(name), driver, port), ...)
> 
> which also avoids the allocation.

...and makes 'cat /proc/interrupts' particularly fun later:

  8:          0          GICv2  72 Level     �\x04�h	����V!

Unless a suitably long-lived string already exists somewhere else in the
serial core, the allocation is unavoidable, although kasprintf() (or its
devm_ variant) might make matters a little simpler.

Robin.

>  8250 device names are always "ttyS"
> plus a number, so 16 characters (including NULL terminator) should be
> more than sufficient, and that's most likely true of all serial drivers.
> (The longest device name I'm aware of is ttyAMA plus a small integer
> for PL011 ports.)
> 

Re: [PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Robin Murphy]

On 16/03/17 13:36, Russell King - ARM Linux wrote:
> On Thu, Mar 16, 2017 at 05:56:53PM +0530, Vignesh R wrote:
>> Using dev_name() as irq name during request_irq() might be misleading in
>> case of serial over PCI. Therefore use a better alternative name for
>> identifying serial port irqs as "serial" appended with serial_index of
>> the port. This ensures that "serial" string is always present in irq
>> name while port index will help in distinguishing b/w different ports.
> 
> Wouldn't it be better to use the device name (iow, ttySx) rather than
> "serialx" ?
> 
> Maybe a helper function in serial_core.c to format the device name into
> a supplied string, which can be re-used elsewhere, eg, uart_report_port()
> and uart_suspend_port().  IOW:
> 
> const char *uart_port_name(char *buf, size_t n, struct uart_driver *drv,
> 			   struct uart_port *port)
> {
> 	snprintf(buf, n, "%s%d", drv->dev_name,
> 		 drv->tty_driver->name_base + port->line);
> 
> 	return buf;
> }
> 
> which means you can do this:
> 
> 	char name[16];
> 
> 	request_irq(..., uart_port_name(name, sizeof(name), driver, port), ...)
> 
> which also avoids the allocation.

...and makes 'cat /proc/interrupts' particularly fun later:

  8:          0          GICv2  72 Level     �\x04�h	����V!

Unless a suitably long-lived string already exists somewhere else in the
serial core, the allocation is unavoidable, although kasprintf() (or its
devm_ variant) might make matters a little simpler.

Robin.

>  8250 device names are always "ttyS"
> plus a number, so 16 characters (including NULL terminator) should be
> more than sufficient, and that's most likely true of all serial drivers.
> (The longest device name I'm aware of is ttyAMA plus a small integer
> for PL011 ports.)
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Robin Murphy]

On 16/03/17 13:36, Russell King - ARM Linux wrote:
> On Thu, Mar 16, 2017 at 05:56:53PM +0530, Vignesh R wrote:
>> Using dev_name() as irq name during request_irq() might be misleading in
>> case of serial over PCI. Therefore use a better alternative name for
>> identifying serial port irqs as "serial" appended with serial_index of
>> the port. This ensures that "serial" string is always present in irq
>> name while port index will help in distinguishing b/w different ports.
> 
> Wouldn't it be better to use the device name (iow, ttySx) rather than
> "serialx" ?
> 
> Maybe a helper function in serial_core.c to format the device name into
> a supplied string, which can be re-used elsewhere, eg, uart_report_port()
> and uart_suspend_port().  IOW:
> 
> const char *uart_port_name(char *buf, size_t n, struct uart_driver *drv,
> 			   struct uart_port *port)
> {
> 	snprintf(buf, n, "%s%d", drv->dev_name,
> 		 drv->tty_driver->name_base + port->line);
> 
> 	return buf;
> }
> 
> which means you can do this:
> 
> 	char name[16];
> 
> 	request_irq(..., uart_port_name(name, sizeof(name), driver, port), ...)
> 
> which also avoids the allocation.

...and makes 'cat /proc/interrupts' particularly fun later:

  8:          0          GICv2  72 Level     ?\x04?h	????V!

Unless a suitably long-lived string already exists somewhere else in the
serial core, the allocation is unavoidable, although kasprintf() (or its
devm_ variant) might make matters a little simpler.

Robin.

>  8250 device names are always "ttyS"
> plus a number, so 16 characters (including NULL terminator) should be
> more than sufficient, and that's most likely true of all serial drivers.
> (The longest device name I'm aware of is ttyAMA plus a small integer
> for PL011 ports.)
> 

Re: [PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Andy Shevchenko]

On Thu, Mar 16, 2017 at 4:55 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 16/03/17 13:36, Russell King - ARM Linux wrote:
>> On Thu, Mar 16, 2017 at 05:56:53PM +0530, Vignesh R wrote:
>>> Using dev_name() as irq name during request_irq() might be misleading in
>>> case of serial over PCI. Therefore use a better alternative name for
>>> identifying serial port irqs as "serial" appended with serial_index of
>>> the port. This ensures that "serial" string is always present in irq
>>> name while port index will help in distinguishing b/w different ports.
>>
>> Wouldn't it be better to use the device name (iow, ttySx) rather than
>> "serialx" ?
>>
>> Maybe a helper function in serial_core.c to format the device name into
>> a supplied string, which can be re-used elsewhere, eg, uart_report_port()
>> and uart_suspend_port().  IOW:
>>
>> const char *uart_port_name(char *buf, size_t n, struct uart_driver *drv,
>>                          struct uart_port *port)
>> {
>>       snprintf(buf, n, "%s%d", drv->dev_name,
>>                drv->tty_driver->name_base + port->line);
>>
>>       return buf;
>> }
>>
>> which means you can do this:
>>
>>       char name[16];
>>
>>       request_irq(..., uart_port_name(name, sizeof(name), driver, port), ...)
>>
>> which also avoids the allocation.
>
> ...and makes 'cat /proc/interrupts' particularly fun later:
>
>   8:          0          GICv2  72 Level     � �h       ����V!
>
> Unless a suitably long-lived string already exists somewhere else in the
> serial core, the allocation is unavoidable, although kasprintf() (or its
> devm_ variant) might make matters a little simpler.

What prevents us to create a field in uart_port (uart8250_port?) where
we put the uart_port_name() for future use as long as uart_port is
alive?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Andy Shevchenko]

On Thu, Mar 16, 2017 at 4:55 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 16/03/17 13:36, Russell King - ARM Linux wrote:
>> On Thu, Mar 16, 2017 at 05:56:53PM +0530, Vignesh R wrote:
>>> Using dev_name() as irq name during request_irq() might be misleading in
>>> case of serial over PCI. Therefore use a better alternative name for
>>> identifying serial port irqs as "serial" appended with serial_index of
>>> the port. This ensures that "serial" string is always present in irq
>>> name while port index will help in distinguishing b/w different ports.
>>
>> Wouldn't it be better to use the device name (iow, ttySx) rather than
>> "serialx" ?
>>
>> Maybe a helper function in serial_core.c to format the device name into
>> a supplied string, which can be re-used elsewhere, eg, uart_report_port()
>> and uart_suspend_port().  IOW:
>>
>> const char *uart_port_name(char *buf, size_t n, struct uart_driver *drv,
>>                          struct uart_port *port)
>> {
>>       snprintf(buf, n, "%s%d", drv->dev_name,
>>                drv->tty_driver->name_base + port->line);
>>
>>       return buf;
>> }
>>
>> which means you can do this:
>>
>>       char name[16];
>>
>>       request_irq(..., uart_port_name(name, sizeof(name), driver, port), ...)
>>
>> which also avoids the allocation.
>
> ...and makes 'cat /proc/interrupts' particularly fun later:
>
>   8:          0          GICv2  72 Level     � �h       ����V!
>
> Unless a suitably long-lived string already exists somewhere else in the
> serial core, the allocation is unavoidable, although kasprintf() (or its
> devm_ variant) might make matters a little simpler.

What prevents us to create a field in uart_port (uart8250_port?) where
we put the uart_port_name() for future use as long as uart_port is
alive?


-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Andy Shevchenko]

On Thu, Mar 16, 2017 at 4:55 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 16/03/17 13:36, Russell King - ARM Linux wrote:
>> On Thu, Mar 16, 2017 at 05:56:53PM +0530, Vignesh R wrote:
>>> Using dev_name() as irq name during request_irq() might be misleading in
>>> case of serial over PCI. Therefore use a better alternative name for
>>> identifying serial port irqs as "serial" appended with serial_index of
>>> the port. This ensures that "serial" string is always present in irq
>>> name while port index will help in distinguishing b/w different ports.
>>
>> Wouldn't it be better to use the device name (iow, ttySx) rather than
>> "serialx" ?
>>
>> Maybe a helper function in serial_core.c to format the device name into
>> a supplied string, which can be re-used elsewhere, eg, uart_report_port()
>> and uart_suspend_port().  IOW:
>>
>> const char *uart_port_name(char *buf, size_t n, struct uart_driver *drv,
>>                          struct uart_port *port)
>> {
>>       snprintf(buf, n, "%s%d", drv->dev_name,
>>                drv->tty_driver->name_base + port->line);
>>
>>       return buf;
>> }
>>
>> which means you can do this:
>>
>>       char name[16];
>>
>>       request_irq(..., uart_port_name(name, sizeof(name), driver, port), ...)
>>
>> which also avoids the allocation.
>
> ...and makes 'cat /proc/interrupts' particularly fun later:
>
>   8:          0          GICv2  72 Level     ? ?h       ????V!
>
> Unless a suitably long-lived string already exists somewhere else in the
> serial core, the allocation is unavoidable, although kasprintf() (or its
> devm_ variant) might make matters a little simpler.

What prevents us to create a field in uart_port (uart8250_port?) where
we put the uart_port_name() for future use as long as uart_port is
alive?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Arnd Bergmann]

On Thursday, March 16, 2017 5:56:53 PM CET Vignesh R wrote:
> Using dev_name() as irq name during request_irq() might be misleading in
> case of serial over PCI. Therefore use a better alternative name for
> identifying serial port irqs as "serial" appended with serial_index of
> the port. This ensures that "serial" string is always present in irq
> name while port index will help in distinguishing b/w different ports.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
> 
> Based on top of tty-next.
> 
>  drivers/tty/serial/8250/8250_core.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index f83b69f30987..78bf621d6827 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -213,12 +213,20 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
>  
>  		ret = 0;
>  	} else {
> +		char *irqname = kmalloc(20, GFP_KERNEL);
> +		if (!irqname) {
> +			spin_unlock_irq(&i->lock);
> +			return -ENOMEM;
> +		}
> +		snprintf(irqname, 20,
> +			 "serial%d", serial_index(&up->port));

kasprintf()?

Also, you need to free the memory after free_irq().

	Arnd

Re: [PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Arnd Bergmann]

On Thursday, March 16, 2017 5:56:53 PM CET Vignesh R wrote:
> Using dev_name() as irq name during request_irq() might be misleading in
> case of serial over PCI. Therefore use a better alternative name for
> identifying serial port irqs as "serial" appended with serial_index of
> the port. This ensures that "serial" string is always present in irq
> name while port index will help in distinguishing b/w different ports.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
> 
> Based on top of tty-next.
> 
>  drivers/tty/serial/8250/8250_core.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index f83b69f30987..78bf621d6827 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -213,12 +213,20 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
>  
>  		ret = 0;
>  	} else {
> +		char *irqname = kmalloc(20, GFP_KERNEL);
> +		if (!irqname) {
> +			spin_unlock_irq(&i->lock);
> +			return -ENOMEM;
> +		}
> +		snprintf(irqname, 20,
> +			 "serial%d", serial_index(&up->port));

kasprintf()?

Also, you need to free the memory after free_irq().

	Arnd

[PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Arnd Bergmann]

On Thursday, March 16, 2017 5:56:53 PM CET Vignesh R wrote:
> Using dev_name() as irq name during request_irq() might be misleading in
> case of serial over PCI. Therefore use a better alternative name for
> identifying serial port irqs as "serial" appended with serial_index of
> the port. This ensures that "serial" string is always present in irq
> name while port index will help in distinguishing b/w different ports.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
> 
> Based on top of tty-next.
> 
>  drivers/tty/serial/8250/8250_core.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index f83b69f30987..78bf621d6827 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -213,12 +213,20 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
>  
>  		ret = 0;
>  	} else {
> +		char *irqname = kmalloc(20, GFP_KERNEL);
> +		if (!irqname) {
> +			spin_unlock_irq(&i->lock);
> +			return -ENOMEM;
> +		}
> +		snprintf(irqname, 20,
> +			 "serial%d", serial_index(&up->port));

kasprintf()?

Also, you need to free the memory after free_irq().

	Arnd

Re: [PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Vignesh R]

On Friday 17 March 2017 02:28 AM, Andy Shevchenko wrote:
> On Thu, Mar 16, 2017 at 4:55 PM, Robin Murphy <robin.murphy@arm.com> wrote:
>> On 16/03/17 13:36, Russell King - ARM Linux wrote:
>>> On Thu, Mar 16, 2017 at 05:56:53PM +0530, Vignesh R wrote:
>>>> Using dev_name() as irq name during request_irq() might be misleading in
>>>> case of serial over PCI. Therefore use a better alternative name for
>>>> identifying serial port irqs as "serial" appended with serial_index of
>>>> the port. This ensures that "serial" string is always present in irq
>>>> name while port index will help in distinguishing b/w different ports.
>>>
>>> Wouldn't it be better to use the device name (iow, ttySx) rather than
>>> "serialx" ?
>>>
>>> Maybe a helper function in serial_core.c to format the device name into
>>> a supplied string, which can be re-used elsewhere, eg, uart_report_port()
>>> and uart_suspend_port().  IOW:
>>>
>>> const char *uart_port_name(char *buf, size_t n, struct uart_driver *drv,
>>>                          struct uart_port *port)
>>> {
>>>       snprintf(buf, n, "%s%d", drv->dev_name,
>>>                drv->tty_driver->name_base + port->line);
>>>
>>>       return buf;
>>> }
>>>
>>> which means you can do this:
>>>
>>>       char name[16];
>>>
>>>       request_irq(..., uart_port_name(name, sizeof(name), driver, port), ...)
>>>
>>> which also avoids the allocation.
>>
>> ...and makes 'cat /proc/interrupts' particularly fun later:
>>
>>   8:          0          GICv2  72 Level     � �h       ����V!
>>
>> Unless a suitably long-lived string already exists somewhere else in the
>> serial core, the allocation is unavoidable, although kasprintf() (or its
>> devm_ variant) might make matters a little simpler.
> 
> What prevents us to create a field in uart_port (uart8250_port?) where
> we put the uart_port_name() for future use as long as uart_port is
> alive?
> 

Thanks for the suggestions. I will explore adding a field to uart_port
struct and provide a corresponding helper function uart_port_name() and
come up with a patch.


-- 
Regards
Vignesh

Re: [PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Vignesh R]

On Friday 17 March 2017 02:28 AM, Andy Shevchenko wrote:
> On Thu, Mar 16, 2017 at 4:55 PM, Robin Murphy <robin.murphy@arm.com> wrote:
>> On 16/03/17 13:36, Russell King - ARM Linux wrote:
>>> On Thu, Mar 16, 2017 at 05:56:53PM +0530, Vignesh R wrote:
>>>> Using dev_name() as irq name during request_irq() might be misleading in
>>>> case of serial over PCI. Therefore use a better alternative name for
>>>> identifying serial port irqs as "serial" appended with serial_index of
>>>> the port. This ensures that "serial" string is always present in irq
>>>> name while port index will help in distinguishing b/w different ports.
>>>
>>> Wouldn't it be better to use the device name (iow, ttySx) rather than
>>> "serialx" ?
>>>
>>> Maybe a helper function in serial_core.c to format the device name into
>>> a supplied string, which can be re-used elsewhere, eg, uart_report_port()
>>> and uart_suspend_port().  IOW:
>>>
>>> const char *uart_port_name(char *buf, size_t n, struct uart_driver *drv,
>>>                          struct uart_port *port)
>>> {
>>>       snprintf(buf, n, "%s%d", drv->dev_name,
>>>                drv->tty_driver->name_base + port->line);
>>>
>>>       return buf;
>>> }
>>>
>>> which means you can do this:
>>>
>>>       char name[16];
>>>
>>>       request_irq(..., uart_port_name(name, sizeof(name), driver, port), ...)
>>>
>>> which also avoids the allocation.
>>
>> ...and makes 'cat /proc/interrupts' particularly fun later:
>>
>>   8:          0          GICv2  72 Level     � �h       ����V!
>>
>> Unless a suitably long-lived string already exists somewhere else in the
>> serial core, the allocation is unavoidable, although kasprintf() (or its
>> devm_ variant) might make matters a little simpler.
> 
> What prevents us to create a field in uart_port (uart8250_port?) where
> we put the uart_port_name() for future use as long as uart_port is
> alive?
> 

Thanks for the suggestions. I will explore adding a field to uart_port
struct and provide a corresponding helper function uart_port_name() and
come up with a patch.


-- 
Regards
Vignesh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Vignesh R]

On Friday 17 March 2017 02:28 AM, Andy Shevchenko wrote:
> On Thu, Mar 16, 2017 at 4:55 PM, Robin Murphy <robin.murphy@arm.com> wrote:
>> On 16/03/17 13:36, Russell King - ARM Linux wrote:
>>> On Thu, Mar 16, 2017 at 05:56:53PM +0530, Vignesh R wrote:
>>>> Using dev_name() as irq name during request_irq() might be misleading in
>>>> case of serial over PCI. Therefore use a better alternative name for
>>>> identifying serial port irqs as "serial" appended with serial_index of
>>>> the port. This ensures that "serial" string is always present in irq
>>>> name while port index will help in distinguishing b/w different ports.
>>>
>>> Wouldn't it be better to use the device name (iow, ttySx) rather than
>>> "serialx" ?
>>>
>>> Maybe a helper function in serial_core.c to format the device name into
>>> a supplied string, which can be re-used elsewhere, eg, uart_report_port()
>>> and uart_suspend_port().  IOW:
>>>
>>> const char *uart_port_name(char *buf, size_t n, struct uart_driver *drv,
>>>                          struct uart_port *port)
>>> {
>>>       snprintf(buf, n, "%s%d", drv->dev_name,
>>>                drv->tty_driver->name_base + port->line);
>>>
>>>       return buf;
>>> }
>>>
>>> which means you can do this:
>>>
>>>       char name[16];
>>>
>>>       request_irq(..., uart_port_name(name, sizeof(name), driver, port), ...)
>>>
>>> which also avoids the allocation.
>>
>> ...and makes 'cat /proc/interrupts' particularly fun later:
>>
>>   8:          0          GICv2  72 Level     ? ?h       ????V!
>>
>> Unless a suitably long-lived string already exists somewhere else in the
>> serial core, the allocation is unavoidable, although kasprintf() (or its
>> devm_ variant) might make matters a little simpler.
> 
> What prevents us to create a field in uart_port (uart8250_port?) where
> we put the uart_port_name() for future use as long as uart_port is
> alive?
> 

Thanks for the suggestions. I will explore adding a field to uart_port
struct and provide a corresponding helper function uart_port_name() and
come up with a patch.


-- 
Regards
Vignesh

Re: [PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Russell King - ARM Linux]

On Thu, Mar 16, 2017 at 10:58:54PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 16, 2017 at 4:55 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> > On 16/03/17 13:36, Russell King - ARM Linux wrote:
> >> On Thu, Mar 16, 2017 at 05:56:53PM +0530, Vignesh R wrote:
> >>> Using dev_name() as irq name during request_irq() might be misleading in
> >>> case of serial over PCI. Therefore use a better alternative name for
> >>> identifying serial port irqs as "serial" appended with serial_index of
> >>> the port. This ensures that "serial" string is always present in irq
> >>> name while port index will help in distinguishing b/w different ports.
> >>
> >> Wouldn't it be better to use the device name (iow, ttySx) rather than
> >> "serialx" ?
> >>
> >> Maybe a helper function in serial_core.c to format the device name into
> >> a supplied string, which can be re-used elsewhere, eg, uart_report_port()
> >> and uart_suspend_port().  IOW:
> >>
> >> const char *uart_port_name(char *buf, size_t n, struct uart_driver *drv,
> >>                          struct uart_port *port)
> >> {
> >>       snprintf(buf, n, "%s%d", drv->dev_name,
> >>                drv->tty_driver->name_base + port->line);
> >>
> >>       return buf;
> >> }
> >>
> >> which means you can do this:
> >>
> >>       char name[16];
> >>
> >>       request_irq(..., uart_port_name(name, sizeof(name), driver, port), ...)
> >>
> >> which also avoids the allocation.
> >
> > ...and makes 'cat /proc/interrupts' particularly fun later:
> >
> >   8:          0          GICv2  72 Level     � �h       ����V!
> >
> > Unless a suitably long-lived string already exists somewhere else in the
> > serial core, the allocation is unavoidable, although kasprintf() (or its
> > devm_ variant) might make matters a little simpler.
> 
> What prevents us to create a field in uart_port (uart8250_port?) where
> we put the uart_port_name() for future use as long as uart_port is
> alive?

Probably a good idea - I didn't check whether request_irq() just uses
the pointer to the string or takes a copy of the string (I should've
done before making the suggestion...)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Russell King - ARM Linux]

On Thu, Mar 16, 2017 at 10:58:54PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 16, 2017 at 4:55 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> > On 16/03/17 13:36, Russell King - ARM Linux wrote:
> >> On Thu, Mar 16, 2017 at 05:56:53PM +0530, Vignesh R wrote:
> >>> Using dev_name() as irq name during request_irq() might be misleading in
> >>> case of serial over PCI. Therefore use a better alternative name for
> >>> identifying serial port irqs as "serial" appended with serial_index of
> >>> the port. This ensures that "serial" string is always present in irq
> >>> name while port index will help in distinguishing b/w different ports.
> >>
> >> Wouldn't it be better to use the device name (iow, ttySx) rather than
> >> "serialx" ?
> >>
> >> Maybe a helper function in serial_core.c to format the device name into
> >> a supplied string, which can be re-used elsewhere, eg, uart_report_port()
> >> and uart_suspend_port().  IOW:
> >>
> >> const char *uart_port_name(char *buf, size_t n, struct uart_driver *drv,
> >>                          struct uart_port *port)
> >> {
> >>       snprintf(buf, n, "%s%d", drv->dev_name,
> >>                drv->tty_driver->name_base + port->line);
> >>
> >>       return buf;
> >> }
> >>
> >> which means you can do this:
> >>
> >>       char name[16];
> >>
> >>       request_irq(..., uart_port_name(name, sizeof(name), driver, port), ...)
> >>
> >> which also avoids the allocation.
> >
> > ...and makes 'cat /proc/interrupts' particularly fun later:
> >
> >   8:          0          GICv2  72 Level     � �h       ����V!
> >
> > Unless a suitably long-lived string already exists somewhere else in the
> > serial core, the allocation is unavoidable, although kasprintf() (or its
> > devm_ variant) might make matters a little simpler.
> 
> What prevents us to create a field in uart_port (uart8250_port?) where
> we put the uart_port_name() for future use as long as uart_port is
> alive?

Probably a good idea - I didn't check whether request_irq() just uses
the pointer to the string or takes a copy of the string (I should've
done before making the suggestion...)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] serial: 8250: 8250_core: Fix irq name for 8250 serial irq

[Russell King - ARM Linux]

On Thu, Mar 16, 2017 at 10:58:54PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 16, 2017 at 4:55 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> > On 16/03/17 13:36, Russell King - ARM Linux wrote:
> >> On Thu, Mar 16, 2017 at 05:56:53PM +0530, Vignesh R wrote:
> >>> Using dev_name() as irq name during request_irq() might be misleading in
> >>> case of serial over PCI. Therefore use a better alternative name for
> >>> identifying serial port irqs as "serial" appended with serial_index of
> >>> the port. This ensures that "serial" string is always present in irq
> >>> name while port index will help in distinguishing b/w different ports.
> >>
> >> Wouldn't it be better to use the device name (iow, ttySx) rather than
> >> "serialx" ?
> >>
> >> Maybe a helper function in serial_core.c to format the device name into
> >> a supplied string, which can be re-used elsewhere, eg, uart_report_port()
> >> and uart_suspend_port().  IOW:
> >>
> >> const char *uart_port_name(char *buf, size_t n, struct uart_driver *drv,
> >>                          struct uart_port *port)
> >> {
> >>       snprintf(buf, n, "%s%d", drv->dev_name,
> >>                drv->tty_driver->name_base + port->line);
> >>
> >>       return buf;
> >> }
> >>
> >> which means you can do this:
> >>
> >>       char name[16];
> >>
> >>       request_irq(..., uart_port_name(name, sizeof(name), driver, port), ...)
> >>
> >> which also avoids the allocation.
> >
> > ...and makes 'cat /proc/interrupts' particularly fun later:
> >
> >   8:          0          GICv2  72 Level     ? ?h       ????V!
> >
> > Unless a suitably long-lived string already exists somewhere else in the
> > serial core, the allocation is unavoidable, although kasprintf() (or its
> > devm_ variant) might make matters a little simpler.
> 
> What prevents us to create a field in uart_port (uart8250_port?) where
> we put the uart_port_name() for future use as long as uart_port is
> alive?

Probably a good idea - I didn't check whether request_irq() just uses
the pointer to the string or takes a copy of the string (I should've
done before making the suggestion...)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.