* [PATCH 2/9] serial: sh-sci: Check if TX data was written to device in .tx_empty() [not found] <20241106120118.1719888-1-claudiu.beznea.uj@bp.renesas.com> @ 2024-11-06 12:01 ` Claudiu 2024-11-07 8:47 ` Greg KH 2024-11-06 12:01 ` [PATCH 3/9] serial: sh-sci: Clean sci_ports[0] after at earlycon exit Claudiu 1 sibling, 1 reply; 6+ messages in thread From: Claudiu @ 2024-11-06 12:01 UTC (permalink / raw) To: geert+renesas, magnus.damm, robh, krzk+dt, conor+dt, mturquette, sboyd, gregkh, jirislaby, p.zabel, lethal, g.liakhovetski, ysato, ulrich.hecht+renesas Cc: claudiu.beznea, linux-renesas-soc, devicetree, linux-kernel, linux-clk, linux-serial, Claudiu Beznea, stable From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> On the Renesas RZ/G3S, when doing suspend to RAM, the uart_suspend_port() is called. The uart_suspend_port() calls 3 times the struct uart_port::ops::tx_empty() before shutting down the port. According to the documentation, the struct uart_port::ops::tx_empty() API tests whether the transmitter FIFO and shifter for the port is empty. The Renesas RZ/G3S SCIFA IP reports the number of data units stored in the transmit FIFO through the FDR (FIFO Data Count Register). The data units in the FIFOs are written in the shift register and transmitted from there. The TEND bit in the Serial Status Register reports if the data was transmitted from the shift register. In the previous code, in the tx_empty() API implemented by the sh-sci driver, it is considered that the TX is empty if the hardware reports the TEND bit set and the number of data units in the FIFO is zero. According to the HW manual, the TEND bit has the following meaning: 0: Transmission is in the waiting state or in progress. 1: Transmission is completed. It has been noticed that when opening the serial device w/o using it and then switch to a power saving mode, the tx_empty() call in the uart_port_suspend() function fails, leading to the "Unable to drain transmitter" message being printed on the console. This is because the TEND=0 if nothing has been transmitted and the FIFOs are empty. As the TEND=0 has double meaning (waiting state, in progress) we can't determined the scenario described above. Add a software workaround for this. This sets a variable if any data has been sent on the serial console (when using PIO) or if the DMA callback has been called (meaning something has been transmitted). Fixes: 73a19e4c0301 ("serial: sh-sci: Add DMA support.") Cc: stable@vger.kernel.org Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> --- drivers/tty/serial/sh-sci.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index df523c744423..8e2d534401fa 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -153,6 +153,7 @@ struct sci_port { int rx_trigger; struct timer_list rx_fifo_timer; int rx_fifo_timeout; + atomic_t first_time_tx; u16 hscif_tot; bool has_rtscts; @@ -850,6 +851,7 @@ static void sci_transmit_chars(struct uart_port *port) { struct tty_port *tport = &port->state->port; unsigned int stopped = uart_tx_stopped(port); + struct sci_port *s = to_sci_port(port); unsigned short status; unsigned short ctrl; int count; @@ -885,6 +887,7 @@ static void sci_transmit_chars(struct uart_port *port) } sci_serial_out(port, SCxTDR, c); + atomic_set(&s->first_time_tx, 1); port->icount.tx++; } while (--count > 0); @@ -1241,6 +1244,8 @@ static void sci_dma_tx_complete(void *arg) if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS) uart_write_wakeup(port); + atomic_set(&s->first_time_tx, 1); + if (!kfifo_is_empty(&tport->xmit_fifo)) { s->cookie_tx = 0; schedule_work(&s->work_tx); @@ -2076,6 +2081,10 @@ static unsigned int sci_tx_empty(struct uart_port *port) { unsigned short status = sci_serial_in(port, SCxSR); unsigned short in_tx_fifo = sci_txfill(port); + struct sci_port *s = to_sci_port(port); + + if (!atomic_read(&s->first_time_tx)) + return TIOCSER_TEMT; return (status & SCxSR_TEND(port)) && !in_tx_fifo ? TIOCSER_TEMT : 0; } @@ -2247,6 +2256,7 @@ static int sci_startup(struct uart_port *port) dev_dbg(port->dev, "%s(%d)\n", __func__, port->line); + atomic_set(&s->first_time_tx, 0); sci_request_dma(port); ret = sci_request_irq(s); @@ -2267,6 +2277,7 @@ static void sci_shutdown(struct uart_port *port) dev_dbg(port->dev, "%s(%d)\n", __func__, port->line); s->autorts = false; + atomic_set(&s->first_time_tx, 0); mctrl_gpio_disable_ms(to_sci_port(port)->gpios); uart_port_lock_irqsave(port, &flags); -- 2.39.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/9] serial: sh-sci: Check if TX data was written to device in .tx_empty() 2024-11-06 12:01 ` [PATCH 2/9] serial: sh-sci: Check if TX data was written to device in .tx_empty() Claudiu @ 2024-11-07 8:47 ` Greg KH 2024-11-07 10:08 ` Claudiu Beznea 0 siblings, 1 reply; 6+ messages in thread From: Greg KH @ 2024-11-07 8:47 UTC (permalink / raw) To: Claudiu Cc: geert+renesas, magnus.damm, robh, krzk+dt, conor+dt, mturquette, sboyd, jirislaby, p.zabel, lethal, g.liakhovetski, ysato, ulrich.hecht+renesas, linux-renesas-soc, devicetree, linux-kernel, linux-clk, linux-serial, Claudiu Beznea, stable On Wed, Nov 06, 2024 at 02:01:11PM +0200, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > On the Renesas RZ/G3S, when doing suspend to RAM, the uart_suspend_port() > is called. The uart_suspend_port() calls 3 times the > struct uart_port::ops::tx_empty() before shutting down the port. > > According to the documentation, the struct uart_port::ops::tx_empty() > API tests whether the transmitter FIFO and shifter for the port is > empty. > > The Renesas RZ/G3S SCIFA IP reports the number of data units stored in the > transmit FIFO through the FDR (FIFO Data Count Register). The data units > in the FIFOs are written in the shift register and transmitted from there. > The TEND bit in the Serial Status Register reports if the data was > transmitted from the shift register. > > In the previous code, in the tx_empty() API implemented by the sh-sci > driver, it is considered that the TX is empty if the hardware reports the > TEND bit set and the number of data units in the FIFO is zero. > > According to the HW manual, the TEND bit has the following meaning: > > 0: Transmission is in the waiting state or in progress. > 1: Transmission is completed. > > It has been noticed that when opening the serial device w/o using it and > then switch to a power saving mode, the tx_empty() call in the > uart_port_suspend() function fails, leading to the "Unable to drain > transmitter" message being printed on the console. This is because the > TEND=0 if nothing has been transmitted and the FIFOs are empty. As the > TEND=0 has double meaning (waiting state, in progress) we can't > determined the scenario described above. > > Add a software workaround for this. This sets a variable if any data has > been sent on the serial console (when using PIO) or if the DMA callback has > been called (meaning something has been transmitted). > > Fixes: 73a19e4c0301 ("serial: sh-sci: Add DMA support.") > Cc: stable@vger.kernel.org > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > drivers/tty/serial/sh-sci.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c > index df523c744423..8e2d534401fa 100644 > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -153,6 +153,7 @@ struct sci_port { > int rx_trigger; > struct timer_list rx_fifo_timer; > int rx_fifo_timeout; > + atomic_t first_time_tx; Don't use an atomic variable for an informational thing like this, it is racy and doesn't work properly. Either use a real lock (because you care about the locking stuff here), or just use a boolean and live with any potential races. > u16 hscif_tot; > > bool has_rtscts; > @@ -850,6 +851,7 @@ static void sci_transmit_chars(struct uart_port *port) > { > struct tty_port *tport = &port->state->port; > unsigned int stopped = uart_tx_stopped(port); > + struct sci_port *s = to_sci_port(port); > unsigned short status; > unsigned short ctrl; > int count; > @@ -885,6 +887,7 @@ static void sci_transmit_chars(struct uart_port *port) > } > > sci_serial_out(port, SCxTDR, c); > + atomic_set(&s->first_time_tx, 1); > > port->icount.tx++; > } while (--count > 0); > @@ -1241,6 +1244,8 @@ static void sci_dma_tx_complete(void *arg) > if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS) > uart_write_wakeup(port); > > + atomic_set(&s->first_time_tx, 1); > + > if (!kfifo_is_empty(&tport->xmit_fifo)) { > s->cookie_tx = 0; > schedule_work(&s->work_tx); > @@ -2076,6 +2081,10 @@ static unsigned int sci_tx_empty(struct uart_port *port) > { > unsigned short status = sci_serial_in(port, SCxSR); > unsigned short in_tx_fifo = sci_txfill(port); > + struct sci_port *s = to_sci_port(port); > + > + if (!atomic_read(&s->first_time_tx)) > + return TIOCSER_TEMT; See, what happens here if the value changes right after you check it? Being an atomic doesn't mean anything :( thanks, greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/9] serial: sh-sci: Check if TX data was written to device in .tx_empty() 2024-11-07 8:47 ` Greg KH @ 2024-11-07 10:08 ` Claudiu Beznea 0 siblings, 0 replies; 6+ messages in thread From: Claudiu Beznea @ 2024-11-07 10:08 UTC (permalink / raw) To: Greg KH Cc: geert+renesas, magnus.damm, robh, krzk+dt, conor+dt, mturquette, sboyd, jirislaby, p.zabel, lethal, g.liakhovetski, ysato, ulrich.hecht+renesas, linux-renesas-soc, devicetree, linux-kernel, linux-clk, linux-serial, Claudiu Beznea, stable Hi, Greg, On 07.11.2024 10:47, Greg KH wrote: > On Wed, Nov 06, 2024 at 02:01:11PM +0200, Claudiu wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> On the Renesas RZ/G3S, when doing suspend to RAM, the uart_suspend_port() >> is called. The uart_suspend_port() calls 3 times the >> struct uart_port::ops::tx_empty() before shutting down the port. >> >> According to the documentation, the struct uart_port::ops::tx_empty() >> API tests whether the transmitter FIFO and shifter for the port is >> empty. >> >> The Renesas RZ/G3S SCIFA IP reports the number of data units stored in the >> transmit FIFO through the FDR (FIFO Data Count Register). The data units >> in the FIFOs are written in the shift register and transmitted from there. >> The TEND bit in the Serial Status Register reports if the data was >> transmitted from the shift register. >> >> In the previous code, in the tx_empty() API implemented by the sh-sci >> driver, it is considered that the TX is empty if the hardware reports the >> TEND bit set and the number of data units in the FIFO is zero. >> >> According to the HW manual, the TEND bit has the following meaning: >> >> 0: Transmission is in the waiting state or in progress. >> 1: Transmission is completed. >> >> It has been noticed that when opening the serial device w/o using it and >> then switch to a power saving mode, the tx_empty() call in the >> uart_port_suspend() function fails, leading to the "Unable to drain >> transmitter" message being printed on the console. This is because the >> TEND=0 if nothing has been transmitted and the FIFOs are empty. As the >> TEND=0 has double meaning (waiting state, in progress) we can't >> determined the scenario described above. >> >> Add a software workaround for this. This sets a variable if any data has >> been sent on the serial console (when using PIO) or if the DMA callback has >> been called (meaning something has been transmitted). >> >> Fixes: 73a19e4c0301 ("serial: sh-sci: Add DMA support.") >> Cc: stable@vger.kernel.org >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> drivers/tty/serial/sh-sci.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c >> index df523c744423..8e2d534401fa 100644 >> --- a/drivers/tty/serial/sh-sci.c >> +++ b/drivers/tty/serial/sh-sci.c >> @@ -153,6 +153,7 @@ struct sci_port { >> int rx_trigger; >> struct timer_list rx_fifo_timer; >> int rx_fifo_timeout; >> + atomic_t first_time_tx; > > Don't use an atomic variable for an informational thing like this, it is > racy and doesn't work properly. Either use a real lock (because you > care about the locking stuff here), or just use a boolean and live with > any potential races. OK, I'll drop it and use a boolean. > > > >> u16 hscif_tot; >> >> bool has_rtscts; >> @@ -850,6 +851,7 @@ static void sci_transmit_chars(struct uart_port *port) >> { >> struct tty_port *tport = &port->state->port; >> unsigned int stopped = uart_tx_stopped(port); >> + struct sci_port *s = to_sci_port(port); >> unsigned short status; >> unsigned short ctrl; >> int count; >> @@ -885,6 +887,7 @@ static void sci_transmit_chars(struct uart_port *port) >> } >> >> sci_serial_out(port, SCxTDR, c); >> + atomic_set(&s->first_time_tx, 1); >> >> port->icount.tx++; >> } while (--count > 0); >> @@ -1241,6 +1244,8 @@ static void sci_dma_tx_complete(void *arg) >> if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS) >> uart_write_wakeup(port); >> >> + atomic_set(&s->first_time_tx, 1); >> + >> if (!kfifo_is_empty(&tport->xmit_fifo)) { >> s->cookie_tx = 0; >> schedule_work(&s->work_tx); >> @@ -2076,6 +2081,10 @@ static unsigned int sci_tx_empty(struct uart_port *port) >> { >> unsigned short status = sci_serial_in(port, SCxSR); >> unsigned short in_tx_fifo = sci_txfill(port); >> + struct sci_port *s = to_sci_port(port); >> + >> + if (!atomic_read(&s->first_time_tx)) >> + return TIOCSER_TEMT; > > See, what happens here if the value changes right after you check it? I agree. I am aware if it. I chose this approach (w/o locking) as I noticed (as of my code checking) that this function is called in kernel through uart_ioctl(), uart_wait_until_sent(), uart_suspend_port(). The uart_wait_until_sent(), uart_suspend_port() are implementing a multiple try approach when checking the ops::tx_timeout() return value. I haven't checked any user space application but considered that it might work in a similar way. I will switch to a boolean in the next version. Thank you, Claudiu Beznea > Being an atomic doesn't mean anything :( > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/9] serial: sh-sci: Clean sci_ports[0] after at earlycon exit [not found] <20241106120118.1719888-1-claudiu.beznea.uj@bp.renesas.com> 2024-11-06 12:01 ` [PATCH 2/9] serial: sh-sci: Check if TX data was written to device in .tx_empty() Claudiu @ 2024-11-06 12:01 ` Claudiu 2024-11-27 16:28 ` Geert Uytterhoeven 1 sibling, 1 reply; 6+ messages in thread From: Claudiu @ 2024-11-06 12:01 UTC (permalink / raw) To: geert+renesas, magnus.damm, robh, krzk+dt, conor+dt, mturquette, sboyd, gregkh, jirislaby, p.zabel, lethal, g.liakhovetski, ysato, ulrich.hecht+renesas Cc: claudiu.beznea, linux-renesas-soc, devicetree, linux-kernel, linux-clk, linux-serial, Claudiu Beznea, stable From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> The early_console_setup() function initializes the sci_ports[0].port with an object of type struct uart_port obtained from the object of type struct earlycon_device received as argument by the early_console_setup(). It may happen that later, when the rest of the serial ports are probed, the serial port that was used as earlycon (e.g., port A) to be mapped to a different position in sci_ports[] and the slot 0 to be used by a different serial port (e.g., port B), as follows: sci_ports[0] = port A sci_ports[X] = port B In this case, the new port mapped at index zero will have associated data that was used for earlycon. In case this happens, after Linux boot, any access to the serial port that maps on sci_ports[0] (port A) will block the serial port that was used as earlycon (port B). To fix this, add early_console_exit() that clean the sci_ports[0] at earlycon exit time. Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support") Cc: stable@vger.kernel.org Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> --- drivers/tty/serial/sh-sci.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 8e2d534401fa..2f8188bdb251 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -3546,6 +3546,32 @@ sh_early_platform_init_buffer("earlyprintk", &sci_driver, #ifdef CONFIG_SERIAL_SH_SCI_EARLYCON static struct plat_sci_port port_cfg __initdata; +static int early_console_exit(struct console *co) +{ + struct sci_port *sci_port = &sci_ports[0]; + struct uart_port *port = &sci_port->port; + unsigned long flags; + int locked = 1; + + if (port->sysrq) + locked = 0; + else if (oops_in_progress) + locked = uart_port_trylock_irqsave(port, &flags); + else + uart_port_lock_irqsave(port, &flags); + + /* + * Clean the slot used by earlycon. A new SCI device might + * map to this slot. + */ + memset(sci_ports, 0, sizeof(*sci_port)); + + if (locked) + uart_port_unlock_irqrestore(port, flags); + + return 0; +} + static int __init early_console_setup(struct earlycon_device *device, int type) { @@ -3562,6 +3588,8 @@ static int __init early_console_setup(struct earlycon_device *device, SCSCR_RE | SCSCR_TE | port_cfg.scscr); device->con->write = serial_console_write; + device->con->exit = early_console_exit; + return 0; } static int __init sci_early_console_setup(struct earlycon_device *device, -- 2.39.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/9] serial: sh-sci: Clean sci_ports[0] after at earlycon exit 2024-11-06 12:01 ` [PATCH 3/9] serial: sh-sci: Clean sci_ports[0] after at earlycon exit Claudiu @ 2024-11-27 16:28 ` Geert Uytterhoeven 2024-11-27 17:33 ` Claudiu Beznea 0 siblings, 1 reply; 6+ messages in thread From: Geert Uytterhoeven @ 2024-11-27 16:28 UTC (permalink / raw) To: Claudiu Cc: magnus.damm, robh, krzk+dt, conor+dt, mturquette, sboyd, gregkh, jirislaby, p.zabel, lethal, g.liakhovetski, ysato, ulrich.hecht+renesas, linux-renesas-soc, devicetree, linux-kernel, linux-clk, linux-serial, Claudiu Beznea, stable, Wolfram Sang Hi Claudiu, Thanks for your patch, which is now commit 3791ea69a4858b81 ("serial: sh-sci: Clean sci_ports[0] after at earlycon exit") in tty/tty-next. On Wed, Nov 6, 2024 at 1:02 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > The early_console_setup() function initializes the sci_ports[0].port with > an object of type struct uart_port obtained from the object of type > struct earlycon_device received as argument by the early_console_setup(). > > It may happen that later, when the rest of the serial ports are probed, > the serial port that was used as earlycon (e.g., port A) to be mapped to a > different position in sci_ports[] and the slot 0 to be used by a different > serial port (e.g., port B), as follows: > > sci_ports[0] = port A > sci_ports[X] = port B Haven't you mixed A and B? > In this case, the new port mapped at index zero will have associated data > that was used for earlycon. Oops, do you have a simple reproducer for this? > In case this happens, after Linux boot, any access to the serial port that > maps on sci_ports[0] (port A) will block the serial port that was used as > earlycon (port B). Again, A <-> B? > To fix this, add early_console_exit() that clean the sci_ports[0] at > earlycon exit time. > > Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support") > Cc: stable@vger.kernel.org > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> This causes a crash (lock-up without any output) when CONFIG_DEBUG_SPINLOCK=y (e.g. CONFIG_PROVE_LOCKING=y). > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -3546,6 +3546,32 @@ sh_early_platform_init_buffer("earlyprintk", &sci_driver, > #ifdef CONFIG_SERIAL_SH_SCI_EARLYCON > static struct plat_sci_port port_cfg __initdata; > > +static int early_console_exit(struct console *co) > +{ > + struct sci_port *sci_port = &sci_ports[0]; > + struct uart_port *port = &sci_port->port; > + unsigned long flags; > + int locked = 1; > + > + if (port->sysrq) > + locked = 0; > + else if (oops_in_progress) > + locked = uart_port_trylock_irqsave(port, &flags); > + else > + uart_port_lock_irqsave(port, &flags); > + > + /* > + * Clean the slot used by earlycon. A new SCI device might > + * map to this slot. > + */ > + memset(sci_ports, 0, sizeof(*sci_port)); Nit: I'd rather use "*sci_port" instead of "sci_ports". > + > + if (locked) > + uart_port_unlock_irqrestore(port, flags); "BUG: spinlock bad magic", as you've just cleared the port, including the spinlock. I guess we can just remove all locking from this function to fix this? However, could it happen that the new device taking slot 0 is probed before the early console is terminated? In that case, its active sci_ports[] entry would be cleared when early_console_exit() is called. Also, what happens if "earlycon keep_bootcon" is passed on the kernel command line, and the new device takes slot 0? Thanks! > + > + return 0; > +} > + > static int __init early_console_setup(struct earlycon_device *device, > int type) > { > @@ -3562,6 +3588,8 @@ static int __init early_console_setup(struct earlycon_device *device, > SCSCR_RE | SCSCR_TE | port_cfg.scscr); > > device->con->write = serial_console_write; > + device->con->exit = early_console_exit; > + > return 0; > } > static int __init sci_early_console_setup(struct earlycon_device *device, Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/9] serial: sh-sci: Clean sci_ports[0] after at earlycon exit 2024-11-27 16:28 ` Geert Uytterhoeven @ 2024-11-27 17:33 ` Claudiu Beznea 0 siblings, 0 replies; 6+ messages in thread From: Claudiu Beznea @ 2024-11-27 17:33 UTC (permalink / raw) To: Geert Uytterhoeven Cc: magnus.damm, robh, krzk+dt, conor+dt, mturquette, sboyd, gregkh, jirislaby, p.zabel, lethal, g.liakhovetski, ysato, ulrich.hecht+renesas, linux-renesas-soc, devicetree, linux-kernel, linux-clk, linux-serial, Claudiu Beznea, stable, Wolfram Sang Hi, Geert, On 27.11.2024 18:28, Geert Uytterhoeven wrote: > Hi Claudiu, > > Thanks for your patch, which is now commit 3791ea69a4858b81 ("serial: > sh-sci: Clean sci_ports[0] after at earlycon exit") in tty/tty-next. > > On Wed, Nov 6, 2024 at 1:02 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> The early_console_setup() function initializes the sci_ports[0].port with >> an object of type struct uart_port obtained from the object of type >> struct earlycon_device received as argument by the early_console_setup(). >> >> It may happen that later, when the rest of the serial ports are probed, >> the serial port that was used as earlycon (e.g., port A) to be mapped to a >> different position in sci_ports[] and the slot 0 to be used by a different >> serial port (e.g., port B), as follows: >> >> sci_ports[0] = port A >> sci_ports[X] = port B > > Haven't you mixed A and B? > >> In this case, the new port mapped at index zero will have associated data >> that was used for earlycon. > > Oops, do you have a simple reproducer for this? It is reproducible with patches: - [PATCH 6/9] arm64: dts: renesas: rzg3s-smarc: Fix the debug serial alias - [PATCH 9/9] arm64: dts: renesas: r9a08g045s33-smarc-pmod: Add overlay for SCIF1 After boot, cat /dev/ttySC0 will lead to the issue described. > >> In case this happens, after Linux boot, any access to the serial port that >> maps on sci_ports[0] (port A) will block the serial port that was used as >> earlycon (port B). > > Again, A <-> B? > >> To fix this, add early_console_exit() that clean the sci_ports[0] at >> earlycon exit time. >> >> Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support") >> Cc: stable@vger.kernel.org >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > This causes a crash (lock-up without any output) when > CONFIG_DEBUG_SPINLOCK=y (e.g. CONFIG_PROVE_LOCKING=y). I missed to check this. Thank you for testing it. > >> --- a/drivers/tty/serial/sh-sci.c >> +++ b/drivers/tty/serial/sh-sci.c >> @@ -3546,6 +3546,32 @@ sh_early_platform_init_buffer("earlyprintk", &sci_driver, >> #ifdef CONFIG_SERIAL_SH_SCI_EARLYCON >> static struct plat_sci_port port_cfg __initdata; >> >> +static int early_console_exit(struct console *co) >> +{ >> + struct sci_port *sci_port = &sci_ports[0]; >> + struct uart_port *port = &sci_port->port; >> + unsigned long flags; >> + int locked = 1; >> + >> + if (port->sysrq) >> + locked = 0; >> + else if (oops_in_progress) >> + locked = uart_port_trylock_irqsave(port, &flags); >> + else >> + uart_port_lock_irqsave(port, &flags); >> + >> + /* >> + * Clean the slot used by earlycon. A new SCI device might >> + * map to this slot. >> + */ >> + memset(sci_ports, 0, sizeof(*sci_port)); > > Nit: I'd rather use "*sci_port" instead of "sci_ports". That would be better, indeed. > >> + >> + if (locked) >> + uart_port_unlock_irqrestore(port, flags); > > "BUG: spinlock bad magic", as you've just cleared the port, including > the spinlock. > > I guess we can just remove all locking from this function to fix this? I'll look to it. > > However, could it happen that the new device taking slot 0 is probed > before the early console is terminated? I don't know to answer this. In my testing I haven't encountered it. > In that case, its active > sci_ports[] entry would be cleared when early_console_exit() is called. > > Also, what happens if "earlycon keep_bootcon" is passed on the kernel > command line, and the new device takes slot 0? I checked it with earlycon and the serial device being on slot 0. In this case it was OK. > > Thanks! > >> + >> + return 0; >> +} >> + >> static int __init early_console_setup(struct earlycon_device *device, >> int type) >> { >> @@ -3562,6 +3588,8 @@ static int __init early_console_setup(struct earlycon_device *device, >> SCSCR_RE | SCSCR_TE | port_cfg.scscr); >> >> device->con->write = serial_console_write; >> + device->con->exit = early_console_exit; >> + >> return 0; >> } >> static int __init sci_early_console_setup(struct earlycon_device *device, > > Gr{oetje,eeting}s, > > Geert > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-11-27 17:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20241106120118.1719888-1-claudiu.beznea.uj@bp.renesas.com>
2024-11-06 12:01 ` [PATCH 2/9] serial: sh-sci: Check if TX data was written to device in .tx_empty() Claudiu
2024-11-07 8:47 ` Greg KH
2024-11-07 10:08 ` Claudiu Beznea
2024-11-06 12:01 ` [PATCH 3/9] serial: sh-sci: Clean sci_ports[0] after at earlycon exit Claudiu
2024-11-27 16:28 ` Geert Uytterhoeven
2024-11-27 17:33 ` Claudiu Beznea
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox