[PATCH v2 0/2] serial: Add name field to uart_port

[PATCH v2 0/2] serial: Add name field to uart_port

[Vignesh R]

This series adds name attribute to uart_port struct inorder to store
name of the tty port that will help in identify different uart
instances.
This patch series is based on discussion here[1].

[1] https://www.spinics.net/lists/arm-kernel/msg569188.html
Previous version:  https://lkml.org/lkml/2017/3/21/328

Vignesh R (2):
  tty: serial_core: Add name field to uart_port struct
  serial: 8250: 8250_core: Fix irq name for 8250 serial IRQ

 drivers/tty/serial/8250/8250_core.c | 2 +-
 drivers/tty/serial/serial_core.c    | 7 +++++++
 include/linux/serial_core.h         | 1 +
 3 files changed, 9 insertions(+), 1 deletion(-)

-- 
2.11.0

[PATCH v2 1/2] tty: serial_core: Add name field to uart_port struct

[Vignesh R]

Introduce a field to store name of uart_port that can be used to easily
identify UART port instances on a system that has more than one UART
instance. The name is of the form ttyXN(eg. ttyS0, ttyAMA0,..) where N
is number that particular UART instance.
This field will be useful when printing debug info for a particular port
or in register IRQs with unique IRQ name. Port name is populated during
uart_add_one_port().

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

v2: 
use kasprintf() instead of snprintf()
Fix up commit message.

 drivers/tty/serial/serial_core.c | 7 +++++++
 include/linux/serial_core.h      | 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 0fb3f7cce62a..f5572e28d16a 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2744,6 +2744,12 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 	state->pm_state = UART_PM_STATE_UNDEFINED;
 	uport->cons = drv->cons;
 	uport->minor = drv->tty_driver->minor_start + uport->line;
+	uport->name = kasprintf(GFP_KERNEL, "%s%d", drv->dev_name,
+				drv->tty_driver->name_base + uport->line);
+	if (!uport->name) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	/*
 	 * If this port is a console, then the spinlock is already
@@ -2861,6 +2867,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
 	if (uport->type != PORT_UNKNOWN && uport->ops->release_port)
 		uport->ops->release_port(uport);
 	kfree(uport->tty_groups);
+	kfree(uport->name);
 
 	/*
 	 * Indicate that there isn't a port here anymore.
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 58484fb35cc8..60530678c633 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -247,6 +247,7 @@ struct uart_port {
 	unsigned char		suspended;
 	unsigned char		irq_wake;
 	unsigned char		unused[2];
+	char			*name;			/* port name */
 	struct attribute_group	*attr_group;		/* port specific attributes */
 	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
 	struct serial_rs485     rs485;
-- 
2.11.0

Re: [PATCH v2 1/2] tty: serial_core: Add name field to uart_port struct

[Andy Shevchenko]

On Fri, Mar 24, 2017 at 7:27 AM, Vignesh R <vigneshr@ti.com> wrote:
> Introduce a field to store name of uart_port that can be used to easily
> identify UART port instances on a system that has more than one UART
> instance. The name is of the form ttyXN(eg. ttyS0, ttyAMA0,..) where N
> is number that particular UART instance.
> This field will be useful when printing debug info for a particular port
> or in register IRQs with unique IRQ name. Port name is populated during
> uart_add_one_port().
>

Looks good to me:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Just in case, have you checked if there any possible scenarios where
memory will be leaked?

> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>
> v2:
> use kasprintf() instead of snprintf()
> Fix up commit message.
>
>  drivers/tty/serial/serial_core.c | 7 +++++++
>  include/linux/serial_core.h      | 1 +
>  2 files changed, 8 insertions(+)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 0fb3f7cce62a..f5572e28d16a 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2744,6 +2744,12 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>         state->pm_state = UART_PM_STATE_UNDEFINED;
>         uport->cons = drv->cons;
>         uport->minor = drv->tty_driver->minor_start + uport->line;
> +       uport->name = kasprintf(GFP_KERNEL, "%s%d", drv->dev_name,
> +                               drv->tty_driver->name_base + uport->line);
> +       if (!uport->name) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
>
>         /*
>          * If this port is a console, then the spinlock is already
> @@ -2861,6 +2867,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
>         if (uport->type != PORT_UNKNOWN && uport->ops->release_port)
>                 uport->ops->release_port(uport);
>         kfree(uport->tty_groups);
> +       kfree(uport->name);
>
>         /*
>          * Indicate that there isn't a port here anymore.
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 58484fb35cc8..60530678c633 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -247,6 +247,7 @@ struct uart_port {
>         unsigned char           suspended;
>         unsigned char           irq_wake;
>         unsigned char           unused[2];
> +       char                    *name;                  /* port name */
>         struct attribute_group  *attr_group;            /* port specific attributes */
>         const struct attribute_group **tty_groups;      /* all attributes (serial core use only) */
>         struct serial_rs485     rs485;
> --
> 2.11.0
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] tty: serial_core: Add name field to uart_port struct

[Vignesh R]

On 3/24/2017 5:16 PM, Andy Shevchenko wrote:
> On Fri, Mar 24, 2017 at 7:27 AM, Vignesh R <vigneshr@ti.com> wrote:
>> Introduce a field to store name of uart_port that can be used to easily
>> identify UART port instances on a system that has more than one UART
>> instance. The name is of the form ttyXN(eg. ttyS0, ttyAMA0,..) where N
>> is number that particular UART instance.
>> This field will be useful when printing debug info for a particular port
>> or in register IRQs with unique IRQ name. Port name is populated during
>> uart_add_one_port().
>>
> 
> Looks good to me:
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> Just in case, have you checked if there any possible scenarios where
> memory will be leaked?

I ran kmemleak scan after opening and closing a serial port and did not
see any memleak report. Also uport->tty_groups is allocated and
deallocated in the same functions uport->name.

> 
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>>
>> v2:
>> use kasprintf() instead of snprintf()
>> Fix up commit message.
>>
>>  drivers/tty/serial/serial_core.c | 7 +++++++
>>  include/linux/serial_core.h      | 1 +
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index 0fb3f7cce62a..f5572e28d16a 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -2744,6 +2744,12 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>>         state->pm_state = UART_PM_STATE_UNDEFINED;
>>         uport->cons = drv->cons;
>>         uport->minor = drv->tty_driver->minor_start + uport->line;
>> +       uport->name = kasprintf(GFP_KERNEL, "%s%d", drv->dev_name,
>> +                               drv->tty_driver->name_base + uport->line);
>> +       if (!uport->name) {
>> +               ret = -ENOMEM;
>> +               goto out;
>> +       }
>>
>>         /*
>>          * If this port is a console, then the spinlock is already
>> @@ -2861,6 +2867,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
>>         if (uport->type != PORT_UNKNOWN && uport->ops->release_port)
>>                 uport->ops->release_port(uport);
>>         kfree(uport->tty_groups);
>> +       kfree(uport->name);
>>
>>         /*
>>          * Indicate that there isn't a port here anymore.
>> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
>> index 58484fb35cc8..60530678c633 100644
>> --- a/include/linux/serial_core.h
>> +++ b/include/linux/serial_core.h
>> @@ -247,6 +247,7 @@ struct uart_port {
>>         unsigned char           suspended;
>>         unsigned char           irq_wake;
>>         unsigned char           unused[2];
>> +       char                    *name;                  /* port name */
>>         struct attribute_group  *attr_group;            /* port specific attributes */
>>         const struct attribute_group **tty_groups;      /* all attributes (serial core use only) */
>>         struct serial_rs485     rs485;
>> --
>> 2.11.0
>>
> 
> 
> 

[PATCH v2 2/2] 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 identify serial port IRQ using
uart_port's name field. This will help mapping IRQs to appropriate
ttySN(where N is the serial port index) instances.

Signed-off-by: Vignesh R <vigneshr@ti.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---

v2:
Pick up reviewed by
s/irq(s)/IRQ(s) in commit message

 drivers/tty/serial/8250/8250_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index f83b69f30987..48a07e2f617f 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -218,7 +218,7 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
 		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, up->port.name, i);
 		if (ret < 0)
 			serial_do_unlink(i, up);
 	}
-- 
2.11.0