[PATCH v4 0/2] serial: sifive: Convert sifive console to nbcon

[PATCH v4 0/2] serial: sifive: Convert sifive console to nbcon

[Ryo Takakura]

Hi!

This is v4 of series converting sifive console to nbcon.

The first patch fixes the issue which was pointed out by John [0] 
that the driver has been accessing SIFIVE_SERIAL_IE_OFFS register 
on its ->startup() and ->shutdown() without port lock synchronization 
against ->write().

The fix on the first patch still applies to the second patch which 
converts the console to nbcon as ->write_thread() holds port lock
and ->write_atomic() checks for the console ownership.

Sincerely,
Ryo Takakura

[0] https://lore.kernel.org/lkml/84sen2fo4b.fsf@jogness.linutronix.de/

---

Changes since v1:
[1] https://lore.kernel.org/lkml/20250323060603.388621-1-ryotkkr98@gmail.com/

- Thank you John for the feedback!
- Add a patch for synchronizing startup()/shutdown() vs write(). 
- Add <Reviewed-by> by John.

Changes since v2:
[2] https://lore.kernel.org/all/20250330003058.386447-1-ryotkkr98@gmail.com/ 

- Add Cc stable for the first patch.

Changes since v3:
[3] https://lore.kernel.org/all/20250330110957.392460-1-ryotkkr98@gmail.com/

- Avoid using return statement on return-void function. Thanks Sebastian 
  for pointing out!

---

Ryo Takakura (2):
  serial: sifive: lock port in startup()/shutdown() callbacks
  serial: sifive: Switch to nbcon console

 drivers/tty/serial/sifive.c | 93 +++++++++++++++++++++++++++++++------
 1 file changed, 80 insertions(+), 13 deletions(-)

-- 
2.34.1

[PATCH v4 1/2] serial: sifive: lock port in startup()/shutdown() callbacks

[Ryo Takakura]

startup()/shutdown() callbacks access SIFIVE_SERIAL_IE_OFFS.
The register is also accessed from write() callback.

If console were printing and startup()/shutdown() callback
gets called, its access to the register could be overwritten.

Add port->lock to startup()/shutdown() callbacks to make sure
their access to SIFIVE_SERIAL_IE_OFFS is synchronized against
write() callback.

Signed-off-by: Ryo Takakura <ryotkkr98@gmail.com>
Cc: stable@vger.kernel.org
---
 drivers/tty/serial/sifive.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c
index 5904a2d4c..054a8e630 100644
--- a/drivers/tty/serial/sifive.c
+++ b/drivers/tty/serial/sifive.c
@@ -563,8 +563,11 @@ static void sifive_serial_break_ctl(struct uart_port *port, int break_state)
 static int sifive_serial_startup(struct uart_port *port)
 {
 	struct sifive_serial_port *ssp = port_to_sifive_serial_port(port);
+	unsigned long flags;
 
+	uart_port_lock_irqsave(&ssp->port, &flags);
 	__ssp_enable_rxwm(ssp);
+	uart_port_unlock_irqrestore(&ssp->port, flags);
 
 	return 0;
 }
@@ -572,9 +575,12 @@ static int sifive_serial_startup(struct uart_port *port)
 static void sifive_serial_shutdown(struct uart_port *port)
 {
 	struct sifive_serial_port *ssp = port_to_sifive_serial_port(port);
+	unsigned long flags;
 
+	uart_port_lock_irqsave(&ssp->port, &flags);
 	__ssp_disable_rxwm(ssp);
 	__ssp_disable_txwm(ssp);
+	uart_port_unlock_irqrestore(&ssp->port, flags);
 }
 
 /**
-- 
2.34.1

Re: [PATCH v4 1/2] serial: sifive: lock port in startup()/shutdown() callbacks

[Greg KH]

On Sat, Apr 05, 2025 at 01:43:38PM +0900, Ryo Takakura wrote:
> startup()/shutdown() callbacks access SIFIVE_SERIAL_IE_OFFS.
> The register is also accessed from write() callback.
> 
> If console were printing and startup()/shutdown() callback
> gets called, its access to the register could be overwritten.
> 
> Add port->lock to startup()/shutdown() callbacks to make sure
> their access to SIFIVE_SERIAL_IE_OFFS is synchronized against
> write() callback.
> 
> Signed-off-by: Ryo Takakura <ryotkkr98@gmail.com>
> Cc: stable@vger.kernel.org

What commit id does this fix?

Why does patch 1/2 need to go to stable, but patch 2/2 does not?  Please
do not mix changes like this in the same series, otherwise we have to
split them up manually when we apply them to the different branches,
right?

thanks,

greg k-h

Re: [PATCH v4 1/2] serial: sifive: lock port in startup()/shutdown() callbacks

[Ryo Takakura]

Hi Greg, thanks for the comments!

On Sat, 5 Apr 2025 08:35:44 +0100, Greg KH wrote:
>On Sat, Apr 05, 2025 at 01:43:38PM +0900, Ryo Takakura wrote:
>> startup()/shutdown() callbacks access SIFIVE_SERIAL_IE_OFFS.
>> The register is also accessed from write() callback.
>> 
>> If console were printing and startup()/shutdown() callback
>> gets called, its access to the register could be overwritten.
>> 
>> Add port->lock to startup()/shutdown() callbacks to make sure
>> their access to SIFIVE_SERIAL_IE_OFFS is synchronized against
>> write() callback.
>> 
>> Signed-off-by: Ryo Takakura <ryotkkr98@gmail.com>
>> Cc: stable@vger.kernel.org
>
>What commit id does this fix?

I believe the issue existed ever since the driver was added by commit 
45c054d0815b ("tty: serial: add driver for the SiFive UART").

>Why does patch 1/2 need to go to stable, but patch 2/2 does not?  Please

The patch 2/2 has nothing to do with existing issue and its only the 
patch 1/2 that needs to go to stable as discussed [0].

>do not mix changes like this in the same series, otherwise we have to
>split them up manually when we apply them to the different branches,
>right?

I see, I'll keep this in mind.
Let me resend the two separately with 'Fixes:' tag for the patch 1/2. 

Sincerely,
Ryo Takakura

>thanks,
>
>greg k-h

[0] https://lore.kernel.org/lkml/84sen2fo4b.fsf@jogness.linutronix.de/

Re: [PATCH v4 1/2] serial: sifive: lock port in startup()/shutdown() callbacks

[Vlastimil Babka]

On 4/5/25 09:35, Greg KH wrote:
> On Sat, Apr 05, 2025 at 01:43:38PM +0900, Ryo Takakura wrote:
>> startup()/shutdown() callbacks access SIFIVE_SERIAL_IE_OFFS.
>> The register is also accessed from write() callback.
>> 
>> If console were printing and startup()/shutdown() callback
>> gets called, its access to the register could be overwritten.
>> 
>> Add port->lock to startup()/shutdown() callbacks to make sure
>> their access to SIFIVE_SERIAL_IE_OFFS is synchronized against
>> write() callback.
>> 
>> Signed-off-by: Ryo Takakura <ryotkkr98@gmail.com>
>> Cc: stable@vger.kernel.org
> 
> What commit id does this fix?

> Why does patch 1/2 need to go to stable, but patch 2/2 does not?  Please
> do not mix changes like this in the same series, otherwise we have to
> split them up manually when we apply them to the different branches,
> right?

I admit it's surprising to see such a request as AFAIK it's normally done to
mix stable fixes and new features in the same series (especially when the
patches depend on each other), and ordering the fixes first and marking only
them as stable should be sufficient. We do that all the time in -mm. I
thought that stable works with stable marked commits primarily, not series?

Also since the patches are AFAIU dependent on each other, sending them
separately makes the mainline development process more difficult, as
evidenced by the later revisions having to add notes in the diffstat area
etc. This would go against the goal that stable process does not add extra
burden to the mainline process, no?

Thanks,
Vlastimil

> thanks,
> 
> greg k-h

Re: [PATCH v4 1/2] serial: sifive: lock port in startup()/shutdown() callbacks

[Greg KH]

On Tue, Apr 22, 2025 at 12:20:42PM +0200, Vlastimil Babka wrote:
> On 4/5/25 09:35, Greg KH wrote:
> > On Sat, Apr 05, 2025 at 01:43:38PM +0900, Ryo Takakura wrote:
> >> startup()/shutdown() callbacks access SIFIVE_SERIAL_IE_OFFS.
> >> The register is also accessed from write() callback.
> >> 
> >> If console were printing and startup()/shutdown() callback
> >> gets called, its access to the register could be overwritten.
> >> 
> >> Add port->lock to startup()/shutdown() callbacks to make sure
> >> their access to SIFIVE_SERIAL_IE_OFFS is synchronized against
> >> write() callback.
> >> 
> >> Signed-off-by: Ryo Takakura <ryotkkr98@gmail.com>
> >> Cc: stable@vger.kernel.org
> > 
> > What commit id does this fix?
> 
> > Why does patch 1/2 need to go to stable, but patch 2/2 does not?  Please
> > do not mix changes like this in the same series, otherwise we have to
> > split them up manually when we apply them to the different branches,
> > right?
> 
> I admit it's surprising to see such a request as AFAIK it's normally done to
> mix stable fixes and new features in the same series (especially when the
> patches depend on each other), and ordering the fixes first and marking only
> them as stable should be sufficient. We do that all the time in -mm. I
> thought that stable works with stable marked commits primarily, not series?

Yes, but when picking which "branch" to apply a series to, what would
you do if you have some "fix some bugs, then add some new features" in a
single patch series?  The one to go to -final or the one for the next
-rc1?

I see a lot of bugfixes delayed until -rc1 because of this issue, and
it's really not a good idea at all.

> Also since the patches are AFAIU dependent on each other, sending them
> separately makes the mainline development process more difficult, as
> evidenced by the later revisions having to add notes in the diffstat area
> etc. This would go against the goal that stable process does not add extra
> burden to the mainline process, no?

If they are dependent on each other, that's the creator's issue, not the
maintainer's issue, no?  :)

Submit the bug fixes, get them merged, and then submit the new features.

thanks,

greg k-h

Re: [PATCH v4 1/2] serial: sifive: lock port in startup()/shutdown() callbacks

[Vlastimil Babka]

On 4/22/25 12:50, Greg KH wrote:
> On Tue, Apr 22, 2025 at 12:20:42PM +0200, Vlastimil Babka wrote:
>> 
>> I admit it's surprising to see such a request as AFAIK it's normally done to
>> mix stable fixes and new features in the same series (especially when the
>> patches depend on each other), and ordering the fixes first and marking only
>> them as stable should be sufficient. We do that all the time in -mm. I
>> thought that stable works with stable marked commits primarily, not series?
> 
> Yes, but when picking which "branch" to apply a series to, what would
> you do if you have some "fix some bugs, then add some new features" in a
> single patch series?  The one to go to -final or the one for the next
> -rc1?

As a maintainer I could split it myself.

> I see a lot of bugfixes delayed until -rc1 because of this issue, and
> it's really not a good idea at all.

In my experience, most of the time these fixes are created because a dev:

- works on the code to implement the feature part
- while working at the code, spots an existing bug
- the bug can be old (Fixes: commit a number of releases ago)
- wants to be helpful so isolates the fix separately as an early patch of
the series and marks stable because the bug can be serious enough in theory
- at the same time there are no known reports of the bug being hit in the wild

In that case I don't see the urgency to fix it ASAP (unless it's e.g.
something obviously dangerously exploitable) so it might not be such a bad
idea just to put everything towards next rc1.

This very thread seems to be a good example of the above. I see the later
version added a
Fixes: 45c054d0815b ("tty: serial: add driver for the SiFive UART")
which is a v5.2 commit.

Thanks,
Vlastimil

>> Also since the patches are AFAIU dependent on each other, sending them
>> separately makes the mainline development process more difficult, as
>> evidenced by the later revisions having to add notes in the diffstat area
>> etc. This would go against the goal that stable process does not add extra
>> burden to the mainline process, no?
> 
> If they are dependent on each other, that's the creator's issue, not the
> maintainer's issue, no?  :)
> 
> Submit the bug fixes, get them merged, and then submit the new features.
> 
> thanks,
> 
> greg k-h

Re: [PATCH v4 1/2] serial: sifive: lock port in startup()/shutdown() callbacks

[Greg KH]

On Tue, Apr 22, 2025 at 03:07:54PM +0200, Vlastimil Babka wrote:
> On 4/22/25 12:50, Greg KH wrote:
> > On Tue, Apr 22, 2025 at 12:20:42PM +0200, Vlastimil Babka wrote:
> >> 
> >> I admit it's surprising to see such a request as AFAIK it's normally done to
> >> mix stable fixes and new features in the same series (especially when the
> >> patches depend on each other), and ordering the fixes first and marking only
> >> them as stable should be sufficient. We do that all the time in -mm. I
> >> thought that stable works with stable marked commits primarily, not series?
> > 
> > Yes, but when picking which "branch" to apply a series to, what would
> > you do if you have some "fix some bugs, then add some new features" in a
> > single patch series?  The one to go to -final or the one for the next
> > -rc1?
> 
> As a maintainer I could split it myself.

You must not have that many patches to review, remember, some of us get
a few more than others ;)

> > I see a lot of bugfixes delayed until -rc1 because of this issue, and
> > it's really not a good idea at all.
> 
> In my experience, most of the time these fixes are created because a dev:
> 
> - works on the code to implement the feature part
> - while working at the code, spots an existing bug
> - the bug can be old (Fixes: commit a number of releases ago)
> - wants to be helpful so isolates the fix separately as an early patch of
> the series and marks stable because the bug can be serious enough in theory
> - at the same time there are no known reports of the bug being hit in the wild
> 
> In that case I don't see the urgency to fix it ASAP (unless it's e.g.
> something obviously dangerously exploitable) so it might not be such a bad
> idea just to put everything towards next rc1.

Yes, but then look at the huge number of "bugfixes" that land in -rc1.
Is that ok or not?  I don't know...

> This very thread seems to be a good example of the above. I see the later
> version added a
> Fixes: 45c054d0815b ("tty: serial: add driver for the SiFive UART")
> which is a v5.2 commit.

Agreed, but delaying a known-fix for weeks/months feels bad to me.

thanks,

greg k-h

Re: [PATCH v4 1/2] serial: sifive: lock port in startup()/shutdown() callbacks

[Petr Mladek]

On Tue 2025-04-22 15:16:21, Greg KH wrote:
> On Tue, Apr 22, 2025 at 03:07:54PM +0200, Vlastimil Babka wrote:
> > On 4/22/25 12:50, Greg KH wrote:
> > > On Tue, Apr 22, 2025 at 12:20:42PM +0200, Vlastimil Babka wrote:
> > >> 
> > >> I admit it's surprising to see such a request as AFAIK it's normally done to
> > >> mix stable fixes and new features in the same series (especially when the
> > >> patches depend on each other), and ordering the fixes first and marking only
> > >> them as stable should be sufficient. We do that all the time in -mm. I
> > >> thought that stable works with stable marked commits primarily, not series?
> > > 
> > > Yes, but when picking which "branch" to apply a series to, what would
> > > you do if you have some "fix some bugs, then add some new features" in a
> > > single patch series?  The one to go to -final or the one for the next
> > > -rc1?
> > 
> > As a maintainer I could split it myself.
> 
> You must not have that many patches to review, remember, some of us get
> a few more than others ;)
> 
> > > I see a lot of bugfixes delayed until -rc1 because of this issue, and
> > > it's really not a good idea at all.
> > 
> > In my experience, most of the time these fixes are created because a dev:
> > 
> > - works on the code to implement the feature part
> > - while working at the code, spots an existing bug
> > - the bug can be old (Fixes: commit a number of releases ago)
> > - wants to be helpful so isolates the fix separately as an early patch of
> > the series and marks stable because the bug can be serious enough in theory
> > - at the same time there are no known reports of the bug being hit in the wild
> > 
> > In that case I don't see the urgency to fix it ASAP (unless it's e.g.
> > something obviously dangerously exploitable) so it might not be such a bad
> > idea just to put everything towards next rc1.
> 
> Yes, but then look at the huge number of "bugfixes" that land in -rc1.
> Is that ok or not?  I don't know...
>
> > This very thread seems to be a good example of the above. I see the later
> > version added a
> > Fixes: 45c054d0815b ("tty: serial: add driver for the SiFive UART")
> > which is a v5.2 commit.
> 
> Agreed, but delaying a known-fix for weeks/months feels bad to me.

I personally push rc1 regression fixes ASAP. But it has a cost.
I do extra careful review, testing, and still I am nervous of causing
a regression which might leak to a stable release.

IMHO, it is perfectly fine to delay fixes for bugs which
were there for months or years. For example, this patch fixes
a bug which has been in the driver since the beginning (2019).

I think that the root of the problem is in the view of the stable release
process. I am pretty conservative. My experience is that some problems
are caught only in the -rc phase when the kernel gets more testing.
I am not sure if stable -rc kernels get the same amount of testing.

Best Regards,
Petr

[PATCH v4 2/2] serial: sifive: Switch to nbcon console

[Ryo Takakura]

Add the necessary callbacks(write_atomic, write_thread, device_lock
and device_unlock) and CON_NBCON flag to switch the sifive console
driver to perform as nbcon console.

Both ->write_atomic() and ->write_thread() will check for console
ownership whenever they are accessing registers.

The ->device_lock()/unlock() will provide the additional serilization
necessary for ->write_thread() which is called from dedicated printing
thread.

Signed-off-by: Ryo Takakura <ryotkkr98@gmail.com>
Reviewed-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/sifive.c | 87 +++++++++++++++++++++++++++++++------
 1 file changed, 74 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c
index 054a8e630..8f7f770ec 100644
--- a/drivers/tty/serial/sifive.c
+++ b/drivers/tty/serial/sifive.c
@@ -151,6 +151,7 @@ struct sifive_serial_port {
 	unsigned long		baud_rate;
 	struct clk		*clk;
 	struct notifier_block	clk_notifier;
+	bool			console_line_ended;
 };
 
 /*
@@ -785,33 +786,88 @@ static void sifive_serial_console_putchar(struct uart_port *port, unsigned char
 
 	__ssp_wait_for_xmitr(ssp);
 	__ssp_transmit_char(ssp, ch);
+
+	ssp->console_line_ended = (ch == '\n');
+}
+
+static void sifive_serial_device_lock(struct console *co, unsigned long *flags)
+{
+	struct uart_port *up = &sifive_serial_console_ports[co->index]->port;
+
+	__uart_port_lock_irqsave(up, flags);
+}
+
+static void sifive_serial_device_unlock(struct console *co, unsigned long flags)
+{
+	struct uart_port *up = &sifive_serial_console_ports[co->index]->port;
+
+	__uart_port_unlock_irqrestore(up, flags);
 }
 
-static void sifive_serial_console_write(struct console *co, const char *s,
-					unsigned int count)
+static void sifive_serial_console_write_atomic(struct console *co,
+					       struct nbcon_write_context *wctxt)
 {
 	struct sifive_serial_port *ssp = sifive_serial_console_ports[co->index];
-	unsigned long flags;
+	struct uart_port *port = &ssp->port;
 	unsigned int ier;
-	int locked = 1;
 
 	if (!ssp)
 		return;
 
-	if (oops_in_progress)
-		locked = uart_port_trylock_irqsave(&ssp->port, &flags);
-	else
-		uart_port_lock_irqsave(&ssp->port, &flags);
+	if (!nbcon_enter_unsafe(wctxt))
+		return;
 
 	ier = __ssp_readl(ssp, SIFIVE_SERIAL_IE_OFFS);
 	__ssp_writel(0, SIFIVE_SERIAL_IE_OFFS, ssp);
 
-	uart_console_write(&ssp->port, s, count, sifive_serial_console_putchar);
+	if (!ssp->console_line_ended)
+		uart_console_write(port, "\n", 1, sifive_serial_console_putchar);
+	uart_console_write(port, wctxt->outbuf, wctxt->len,
+			   sifive_serial_console_putchar);
 
 	__ssp_writel(ier, SIFIVE_SERIAL_IE_OFFS, ssp);
 
-	if (locked)
-		uart_port_unlock_irqrestore(&ssp->port, flags);
+	nbcon_exit_unsafe(wctxt);
+}
+
+static void sifive_serial_console_write_thread(struct console *co,
+					       struct nbcon_write_context *wctxt)
+{
+	struct sifive_serial_port *ssp = sifive_serial_console_ports[co->index];
+	struct uart_port *port = &ssp->port;
+	unsigned int ier;
+
+	if (!ssp)
+		return;
+
+	if (!nbcon_enter_unsafe(wctxt))
+		return;
+
+	ier = __ssp_readl(ssp, SIFIVE_SERIAL_IE_OFFS);
+	__ssp_writel(0, SIFIVE_SERIAL_IE_OFFS, ssp);
+
+	if (nbcon_exit_unsafe(wctxt)) {
+		int len = READ_ONCE(wctxt->len);
+		int i;
+
+		for (i = 0; i < len; i++) {
+			if (!nbcon_enter_unsafe(wctxt))
+				break;
+
+			uart_console_write(port, wctxt->outbuf + i, 1,
+					   sifive_serial_console_putchar);
+
+			if (!nbcon_exit_unsafe(wctxt))
+				break;
+		}
+	}
+
+	while (!nbcon_enter_unsafe(wctxt))
+		nbcon_reacquire_nobuf(wctxt);
+
+	__ssp_writel(ier, SIFIVE_SERIAL_IE_OFFS, ssp);
+
+	nbcon_exit_unsafe(wctxt);
 }
 
 static int sifive_serial_console_setup(struct console *co, char *options)
@@ -829,6 +885,8 @@ static int sifive_serial_console_setup(struct console *co, char *options)
 	if (!ssp)
 		return -ENODEV;
 
+	ssp->console_line_ended = true;
+
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
 
@@ -839,10 +897,13 @@ static struct uart_driver sifive_serial_uart_driver;
 
 static struct console sifive_serial_console = {
 	.name		= SIFIVE_TTY_PREFIX,
-	.write		= sifive_serial_console_write,
+	.write_atomic	= sifive_serial_console_write_atomic,
+	.write_thread	= sifive_serial_console_write_thread,
+	.device_lock	= sifive_serial_device_lock,
+	.device_unlock	= sifive_serial_device_unlock,
 	.device		= uart_console_device,
 	.setup		= sifive_serial_console_setup,
-	.flags		= CON_PRINTBUFFER,
+	.flags		= CON_PRINTBUFFER | CON_NBCON,
 	.index		= -1,
 	.data		= &sifive_serial_uart_driver,
 };
-- 
2.34.1