Message ID | 20170316122653.19930-1-vigneshr@ti.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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.
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,
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.)
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. 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.) >
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?
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
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.
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...)
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); }
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(-)