[PATCH 0/4] MIPS: SiByte: Fix serial device regressions

[PATCH 0/4] MIPS: SiByte: Fix serial device regressions

[Maciej W. Rozycki]

Hi,

 Starting from commit 84a9582fd203 ("serial: core: Start managing serial 
controllers to enable runtime PM") the driver for the SiByte onchip DUART 
has stopped working due to a null pointer dereference in the serial core, 
which means a kernel crash at bootstrap if the driver has been enabled, as 
is usually the case for the system console.

 This patch series addresses the issue by switching the driver away from 
legacy probing to using platform devices.  A notable consequence of this 
is the serial console is only switched from the bootconsole handler that 
uses firmware calls over to our serial driver at the time the driver is 
being properly brought up.  This causes messages to be produced to the 
console between the device reset and console setup, which in turn causes 
the firmware still being called via the bootconsole handler to loop 
forever owing to the transmitter having been disabled.

 Therefore introductory change 2/4 is included to fix the issue by doing a 
rudimentary device setup right after reset, using parameters compatible 
with those used by the firmware (115200n8).  There is auxiliary change 1/4 
included as well that prevents message corruption from happening at reset, 
which becomes more prominent due to the change in timing, with the driver 
switch to the platform device, of messages produced to the console.

 Additionally there is a revert included as change 4/4 for a regression 
introduced by an earlier change that caused previously good code to fail 
requesting the control register resource shared between DUART channels, 
and issue a warning to the kernel log.  Not to be backported as strictly 
speaking the driver works just fine despite the missing reservation.

 See individual change descriptions for details.  Verified with a SWARM
(BCM91250A) system.

 Please apply.

  Maciej

[PATCH 1/4] MIPS: SiByte: Fix console message clobbering at channel resets

[Maciej W. Rozycki]

Ensure any characters outstanding have been sent before issuing channel 
resets so as to prevent messages issued to the bootconsole from getting 
clobbered.

Contrary to device documentation at the time the transmitter empty bit 
is set only the transmit FIFO has been drained and there is still data 
outstanding in the transmitter shift register, so wait an extra amount 
of time for that register to drain too.  This also prevents subsequent 
messages produced to the console from getting clobbered, owing to what
seems a transmitter synchronisation issue.

When called from sbd_serial_console_init() it is too early for fsleep() 
to work and even before lpj has been calculated and therefore the delay 
is actually not sufficient for the transmitter to drain and is merely a 
placeholder now.  This will be addressed in a follow-up change.

Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Cc: stable@vger.kernel.org # v6.5+
---
 drivers/tty/serial/sb1250-duart.c |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

linux-serial-sb1250-duart-reset-drain.diff
Index: linux-macro/drivers/tty/serial/sb1250-duart.c
===================================================================
--- linux-macro.orig/drivers/tty/serial/sb1250-duart.c
+++ linux-macro/drivers/tty/serial/sb1250-duart.c
@@ -516,6 +516,28 @@ static void sbd_init_port(struct sbd_por
 
 	if (sport->initialised)
 		return;
+	/*
+	 * Contrary to documentation, which says that the transmitter
+	 * empty bit is set when "there are no characters to send and
+	 * the transmitter is idle," the bit is already set by hardware
+	 * once the transmit FIFO has been drained only and while the
+	 * transmitter shift register still holds data being supplied
+	 * to the line.  Consequently issuing a transmitter reset at
+	 * this point causes the final character outstanding to be lost.
+	 *
+	 * Moreover, resetting the transmitter while transmission is
+	 * in progress appears to make the transmitter go out of sync
+	 * and subsequent characters issued after the transmitter has
+	 * been reprogrammed and re-enabled are sent corrupted or with
+	 * their bit patterns shifted.
+	 *
+	 * So once the transmitter empty bit has been set wait an extra
+	 * amount of time, sufficient for the transmitter shift register
+	 * to drain at 115200bps, which is the baud rate setting used by
+	 * a standard CFE firmware compilation.
+	 */
+	sbd_line_drain(sport);
+	udelay(100);
 
 	/* There is no DUART reset feature, so just set some sane defaults.  */
 	write_sbdchn(sport, R_DUART_CMD, V_DUART_MISC_CMD_RESET_TX);

[PATCH 2/4] MIPS: SiByte: Fix bootconsole handover lockup

[Maciej W. Rozycki]

Calling sbd_init_port() in the course of setting up the serial device 
causes line parameters to be messed up and the transmitter disabled.  
We've been lucky in that no message is usually produced to the kernel 
log between this call and the later call to uart_set_options() in the 
course of console setup done by sbd_serial_console_init(), or the system 
would hang as the console output handler in CFE tried to access a port 
whose transmitter has been disabled and line parameters messed up.

It'll change with the next change to the driver, so fix sbd_init_port() 
such that line parameters are set for 115200n8 console operation as with 
the CFE firmware and the transmitter re-enabled after reset.

Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Cc: stable@vger.kernel.org # v6.5+
---
 drivers/tty/serial/sb1250-duart.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

linux-serial-sb1250-duart-prom-console.diff
Index: linux-macro/drivers/tty/serial/sb1250-duart.c
===================================================================
--- linux-macro.orig/drivers/tty/serial/sb1250-duart.c
+++ linux-macro/drivers/tty/serial/sb1250-duart.c
@@ -542,14 +542,19 @@ static void sbd_init_port(struct sbd_por
 	/* There is no DUART reset feature, so just set some sane defaults.  */
 	write_sbdchn(sport, R_DUART_CMD, V_DUART_MISC_CMD_RESET_TX);
 	write_sbdchn(sport, R_DUART_CMD, V_DUART_MISC_CMD_RESET_RX);
-	write_sbdchn(sport, R_DUART_MODE_REG_1, V_DUART_BITS_PER_CHAR_8);
+	write_sbdchn(sport, R_DUART_MODE_REG_1,
+		     V_DUART_PARITY_MODE_NONE | V_DUART_BITS_PER_CHAR_8);
 	write_sbdchn(sport, R_DUART_MODE_REG_2, 0);
+	write_sbdchn(sport, R_DUART_CLK_SEL, V_DUART_BAUD_RATE(115200));
 	write_sbdchn(sport, R_DUART_FULL_CTL,
 		     V_DUART_INT_TIME(0) | V_DUART_SIG_FULL(15));
 	write_sbdchn(sport, R_DUART_OPCR_X, 0);
 	write_sbdchn(sport, R_DUART_AUXCTL_X, 0);
 	write_sbdshr(sport, R_DUART_IMRREG((uport->line) % 2), 0);
 
+	/* Re-enable transmission for the initial PROM-based console.  */
+	write_sbdchn(sport, R_DUART_CMD, M_DUART_TX_EN);
+
 	sport->initialised = 1;
 }
 

[PATCH 3/4] MIPS: SiByte: Convert to use a platform device

[Maciej W. Rozycki]

Prevent a crash from happening as the first serial port is initialised:

  pata-swarm: PATA interface at GenBus slot 4
  workingset: timestamp_bits=62 max_order=18 bucket_order=0
  Block layer SCSI generic (bsg) driver version 0.4 loaded (major 253)
  CPU 1 Unable to handle kernel paging request at virtual address 0000000000000208, epc == ffffffff8067f8f8, ra == ffffffff80666330
  Oops[#1]:
  CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.19.0-dirty #27 NONE 
  $ 0   : 0000000000000000 0000000014001fe0 0000000000000020 ffffffff80666130
  $ 4   : 0000000000000000 a800000100e6f118 ffffffff8112cbc0 0000000000000000
  $ 8   : 0000000000000002 0000000000000000 0000000000000000 0000000001a80000
  $12   : 0000000000000000 ffffffff809fd488 000000004ddf14dd ffffffff00000000
  $16   : a800000100e6f000 0000000000000000 ffffffff8112c1d0 a800000100e6f000
  $20   : 0000000000000000 00000000000004d0 0000000000000004 ffffffff8112c1d0
  $24   : 0000000000000001 0000000000000003                                  
  $28   : a80000010007c000 a80000010007fcb0 00000000000000ef ffffffff80666330
  Hi    : fffffffffffffdb9
  Lo    : 0000000000000035
  epc   : ffffffff8067f8f8 __dev_fwnode+0x0/0x8
  ra    : ffffffff80666330 serial_base_ctrl_add+0xb8/0x180
  Status: 14001fe3	KX SX UX KERNEL EXL IE 
  Cause : 80800008 (ExcCode 02)
  BadVA : 0000000000000208
  PrId  : 03040102 (SiByte SB1)
  Process swapper/0 (pid: 1, threadinfo=(____ptrval____), task=(____ptrval____), tls=0000000000000000)
  Stack : 0000000000000000 ffffffff80cd5178 ffffffff80cd0000 ffffffff8112c1c8
          0000000000000260 ffffffff806655c4 a800000100275bc0 ffffffff8064ac88
          004000408112c000 0000000000000002 0000000000000000 ffffffff801965d0
          a800000100786ba0 ffffffff80cd5178 a800000100786ba0 0000000000000004
          a800000100275bc0 0000000000000000 0000000000000000 ffffffff80cd5178
          0000000000000000 ffffffff8112c1c8 0000000000000260 00000000000004d0
          0000000000000004 ffffffff80bf0000 00000000000000ef ffffffff80d171dc
          ffffffff80d17120 ffffffff80d25658 0000000000000000 ffffffff80d50000
          ffffffff80d2f928 ffffffff80d50000 ffffffff80d25698 ffffffff80cfcecc
          00ffffff80b84428 0000000000000000 0000000000000006 0000000000000006
          ...
  Call Trace:
  [<ffffffff8067f8f8>] __dev_fwnode+0x0/0x8
  [<ffffffff80666330>] serial_base_ctrl_add+0xb8/0x180
  [<ffffffff806655c4>] serial_core_register_port+0x174/0x8e0
  [<ffffffff80d171dc>] sbd_init+0xbc/0xf4
  [<ffffffff80cfcecc>] do_one_initcall+0x64/0x150
  [<ffffffff80cfd284>] kernel_init_freeable+0x25c/0x30c
  [<ffffffff809ff6e4>] kernel_init+0x24/0x118
  [<ffffffff80112d20>] ret_from_kernel_thread+0x14/0x1c
  
  Code: 67bd0010  03e00008  2402ffea <03e00008> dc820208  67bdffa0  ffbe0050  ffb70048  ffb60040 
  
  ---[ end trace 0000000000000000 ]---

-- where a pointer is dereferenced that has been derived from a null
pointer to the port's parent device.

Since no device is available with legacy probing and it's not anymore a
preferable way to discover devices anyway, switch the driver to using a
platform device and use it as the port's parent device.

Use platform_driver_probe() because SB1250 DUART devices are embedded 
onchip the SoC and therefore not that straightforward to remove.

An unfortunate consequence of the switch to a platform device is we now 
hand the console over from the bootconsole much later in the bootstrap. 
The CFE console handler appears good enough though to work so late and 
in particular with interrupts enabled.

Conversely only starting the console port so late lets the reset code
fully utilise our delay handlers, so switch from udelay() to fsleep()
for transmitter draining so as to avoid busy-waiting for an excessive
amount of time.

Since there is one way only remaining to reach sbd_init_port() now, drop 
the port initialisation marker as no longer needed and go through the 
channel resets unconditionally.

Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Cc: stable@vger.kernel.org # needs to use .remove_new for <= 6.10
---
 arch/mips/sibyte/swarm/platform.c |   97 +++++++++++++++++++++++-
 drivers/tty/serial/sb1250-duart.c |  148 ++++++++++++++------------------------
 2 files changed, 146 insertions(+), 99 deletions(-)

linux-serial-sb1250-duart-platform.diff
Index: linux-macro/arch/mips/sibyte/swarm/platform.c
===================================================================
--- linux-macro.orig/arch/mips/sibyte/swarm/platform.c
+++ linux-macro/arch/mips/sibyte/swarm/platform.c
@@ -8,7 +8,13 @@
 
 #include <asm/sibyte/board.h>
 #include <asm/sibyte/sb1250_genbus.h>
+#if defined(CONFIG_SIBYTE_BCM1x80)
+#include <asm/sibyte/bcm1480_regs.h>
+#include <asm/sibyte/bcm1480_int.h>
+#else
 #include <asm/sibyte/sb1250_regs.h>
+#include <asm/sibyte/sb1250_int.h>
+#endif
 
 #if defined(CONFIG_SIBYTE_SWARM) || defined(CONFIG_SIBYTE_LITTLESUR)
 
@@ -85,6 +91,82 @@ device_initcall(swarm_pata_init);
 
 #endif /* defined(CONFIG_SIBYTE_SWARM) || defined(CONFIG_SIBYTE_LITTLESUR) */
 
+#if defined(CONFIG_SIBYTE_BCM1x80)
+static struct resource sb1250_duart_resources[][2] = {
+	{
+		{
+			.name = "sb1250-duart0",
+			.flags = IORESOURCE_MEM,
+			.start = A_BCM1480_DUART0,
+			.end = (A_BCM1480_DUART0 +
+				4 * BCM1480_DUART_CHANREG_SPACING - 1),
+		},
+		{
+			.name = "sb1250-duart0",
+			.flags = IORESOURCE_IRQ,
+			.start = K_BCM1480_INT_UART_0,
+			.end = K_BCM1480_INT_UART_1,
+		},
+	},
+	{
+		{
+			.name = "sb1250-duart1",
+			.flags = IORESOURCE_MEM,
+			.start = A_BCM1480_DUART1,
+			.end = (A_BCM1480_DUART1 +
+				4 * BCM1480_DUART_CHANREG_SPACING - 1),
+		},
+		{
+			.name = "sb1250-duart1",
+			.flags = IORESOURCE_IRQ,
+			.start = K_BCM1480_INT_UART_2,
+			.end = K_BCM1480_INT_UART_3,
+		},
+	},
+};
+#else /* !defined(CONFIG_SIBYTE_BCM1x80) */
+static struct resource sb1250_duart_resources[][2] = {
+	{
+		{
+			.name = "sb1250-duart0",
+			.flags = IORESOURCE_MEM,
+			.start = A_DUART,
+			.end = A_DUART + 4 * DUART_CHANREG_SPACING - 1,
+		},
+		{
+			.name = "sb1250-duart0",
+			.flags = IORESOURCE_IRQ,
+			.start = K_INT_UART_0,
+			.end = K_INT_UART_1,
+		},
+	},
+};
+#endif /* !defined(CONFIG_SIBYTE_BCM1x80) */
+
+static struct platform_device sb1250_duart_device[] = {
+	{
+		.name = "sb1250-duart",
+		.id = 0,
+		.resource = sb1250_duart_resources[0],
+		.num_resources = ARRAY_SIZE(sb1250_duart_resources[0]),
+	},
+#if defined(CONFIG_SIBYTE_BCM1x80)
+	{
+		.name = "sb1250-duart",
+		.id = 1,
+		.resource = sb1250_duart_resources[1],
+		.num_resources = ARRAY_SIZE(sb1250_duart_resources[1]),
+	},
+#endif
+};
+
+static struct platform_device *sb1250_duart_devices[] __initdata = {
+	&sb1250_duart_device[0],
+#if defined(CONFIG_SIBYTE_BCM1x80)
+	&sb1250_duart_device[1],
+#endif
+};
+
 #define sb1250_dev_struct(num) \
 	static struct resource sb1250_res##num = {		\
 		.name = "SB1250 MAC " __stringify(num),		\
@@ -113,28 +195,31 @@ static struct platform_device *sb1250_de
 
 static int __init sb1250_device_init(void)
 {
-	int ret;
+	int ret1, ret2;
+
+	ret1 = platform_add_devices(sb1250_duart_devices,
+				    ARRAY_SIZE(sb1250_duart_devices));
 
 	/* Set the number of available units based on the SOC type.  */
 	switch (soc_type) {
 	case K_SYS_SOC_TYPE_BCM1250:
 	case K_SYS_SOC_TYPE_BCM1250_ALT:
-		ret = platform_add_devices(sb1250_devs, 3);
+		ret2 = platform_add_devices(sb1250_devs, 3);
 		break;
 	case K_SYS_SOC_TYPE_BCM1120:
 	case K_SYS_SOC_TYPE_BCM1125:
 	case K_SYS_SOC_TYPE_BCM1125H:
 	case K_SYS_SOC_TYPE_BCM1250_ALT2:	/* Hybrid */
-		ret = platform_add_devices(sb1250_devs, 2);
+		ret2 = platform_add_devices(sb1250_devs, 2);
 		break;
 	case K_SYS_SOC_TYPE_BCM1x55:
 	case K_SYS_SOC_TYPE_BCM1x80:
-		ret = platform_add_devices(sb1250_devs, 4);
+		ret2 = platform_add_devices(sb1250_devs, 4);
 		break;
 	default:
-		ret = -ENODEV;
+		ret2 = 0;
 		break;
 	}
-	return ret;
+	return ret1 ? ret1 : ret2;
 }
 device_initcall(sb1250_device_init);
Index: linux-macro/drivers/tty/serial/sb1250-duart.c
===================================================================
--- linux-macro.orig/drivers/tty/serial/sb1250-duart.c
+++ linux-macro/drivers/tty/serial/sb1250-duart.c
@@ -3,7 +3,7 @@
  *	Support for the asynchronous serial interface (DUART) included
  *	in the BCM1250 and derived System-On-a-Chip (SOC) devices.
  *
- *	Copyright (c) 2007  Maciej W. Rozycki
+ *	Copyright (c) 2007, 2026  Maciej W. Rozycki
  *
  *	Derived from drivers/char/sb1250_duart.c for which the following
  *	copyright applies:
@@ -25,6 +25,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/major.h>
+#include <linux/platform_device.h>
 #include <linux/serial.h>
 #include <linux/serial_core.h>
 #include <linux/spinlock.h>
@@ -45,10 +46,6 @@
 #include <asm/sibyte/bcm1480_regs.h>
 #include <asm/sibyte/bcm1480_int.h>
 
-#define SBD_CHANREGS(line)	A_BCM1480_DUART_CHANREG((line), 0)
-#define SBD_CTRLREGS(line)	A_BCM1480_DUART_CTRLREG((line), 0)
-#define SBD_INT(line)		(K_BCM1480_INT_UART_0 + (line))
-
 #define DUART_CHANREG_SPACING	BCM1480_DUART_CHANREG_SPACING
 
 #define R_DUART_IMRREG(line)	R_BCM1480_DUART_IMRREG(line)
@@ -59,10 +56,6 @@
 #include <asm/sibyte/sb1250_regs.h>
 #include <asm/sibyte/sb1250_int.h>
 
-#define SBD_CHANREGS(line)	A_DUART_CHANREG((line), 0)
-#define SBD_CTRLREGS(line)	A_DUART_CTRLREG(0)
-#define SBD_INT(line)		(K_INT_UART_0 + (line))
-
 #else
 #error invalid SB1250 UART configuration
 
@@ -85,7 +78,6 @@ struct sbd_port {
 	struct uart_port	port;
 	unsigned char __iomem	*memctrl;
 	int			tx_stopped;
-	int			initialised;
 };
 
 /*
@@ -100,6 +92,7 @@ struct sbd_duart {
 #define to_sport(uport) container_of(uport, struct sbd_port, port)
 
 static struct sbd_duart sbd_duarts[DUART_MAX_CHIP];
+static struct uart_driver sbd_reg;
 
 
 /*
@@ -514,8 +507,6 @@ static void sbd_init_port(struct sbd_por
 {
 	struct uart_port *uport = &sport->port;
 
-	if (sport->initialised)
-		return;
 	/*
 	 * Contrary to documentation, which says that the transmitter
 	 * empty bit is set when "there are no characters to send and
@@ -537,7 +528,7 @@ static void sbd_init_port(struct sbd_por
 	 * a standard CFE firmware compilation.
 	 */
 	sbd_line_drain(sport);
-	udelay(100);
+	fsleep(100);
 
 	/* There is no DUART reset feature, so just set some sane defaults.  */
 	write_sbdchn(sport, R_DUART_CMD, V_DUART_MISC_CMD_RESET_TX);
@@ -554,8 +545,6 @@ static void sbd_init_port(struct sbd_por
 
 	/* Re-enable transmission for the initial PROM-based console.  */
 	write_sbdchn(sport, R_DUART_CMD, M_DUART_TX_EN);
-
-	sport->initialised = 1;
 }
 
 static void sbd_set_termios(struct uart_port *uport, struct ktermios *termios,
@@ -794,50 +783,55 @@ static const struct uart_ops sbd_ops = {
 };
 
 /* Initialize SB1250 DUART port structures.  */
-static void __init sbd_probe_duarts(void)
+static int __init sbd_probe(struct platform_device *pdev)
 {
-	static int probed;
+	struct resource *mem_resource, *irq_resource;
 	int chip, side;
-	int max_lines, line;
+	int i;
 
-	if (probed)
-		return;
+	mem_resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	irq_resource = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!mem_resource || !irq_resource)
+		return -ENODEV;
 
-	/* Set the number of available units based on the SOC type.  */
-	switch (soc_type) {
-	case K_SYS_SOC_TYPE_BCM1x55:
-	case K_SYS_SOC_TYPE_BCM1x80:
-		max_lines = 4;
-		break;
-	default:
-		/* Assume at least two serial ports at the normal address.  */
-		max_lines = 2;
-		break;
-	}
+	chip = pdev->id;
+	sbd_duarts[chip].mapctrl = mem_resource->start +
+				   DUART_CHANREG_SPACING * 3;
+	for (side = 0; side < DUART_MAX_SIDE; side++) {
+		struct sbd_port *sport = &sbd_duarts[chip].sport[side];
+		struct uart_port *uport = &sport->port;
 
-	probed = 1;
+		sport->duart	= &sbd_duarts[chip];
 
-	for (chip = 0, line = 0; chip < DUART_MAX_CHIP && line < max_lines;
-	     chip++) {
-		sbd_duarts[chip].mapctrl = SBD_CTRLREGS(line);
+		uport->dev	= &pdev->dev;
+		uport->irq	= irq_resource->start + side;
+		uport->uartclk	= 100000000 / 20 * 16;
+		uport->fifosize	= 16;
+		uport->iotype	= UPIO_MEM;
+		uport->flags	= UPF_BOOT_AUTOCONF;
+		uport->ops	= &sbd_ops;
+		uport->line	= chip * DUART_MAX_SIDE + side;
+		uport->mapbase	= mem_resource->start +
+				  DUART_CHANREG_SPACING * (side + 1);
+		uport->has_sysrq = IS_ENABLED(CONFIG_SERIAL_SB1250_DUART_CONSOLE);
+		if (uart_add_one_port(&sbd_reg, uport))
+			uport->dev = NULL;
+	}
 
-		for (side = 0; side < DUART_MAX_SIDE && line < max_lines;
-		     side++, line++) {
-			struct sbd_port *sport = &sbd_duarts[chip].sport[side];
-			struct uart_port *uport = &sport->port;
+	return 0;
+}
 
-			sport->duart	= &sbd_duarts[chip];
+static void __exit sbd_remove(struct platform_device *pdev)
+{
+	int chip, side;
 
-			uport->irq	= SBD_INT(line);
-			uport->uartclk	= 100000000 / 20 * 16;
-			uport->fifosize	= 16;
-			uport->iotype	= UPIO_MEM;
-			uport->flags	= UPF_BOOT_AUTOCONF;
-			uport->ops	= &sbd_ops;
-			uport->line	= line;
-			uport->mapbase	= SBD_CHANREGS(line);
-			uport->has_sysrq = IS_ENABLED(CONFIG_SERIAL_SB1250_DUART_CONSOLE);
-		}
+	chip = pdev->id;
+	for (side = DUART_MAX_SIDE - 1; side >= 0; side--) {
+		struct sbd_port *sport = &sbd_duarts[chip].sport[side];
+		struct uart_port *uport = &sport->port;
+
+		if (uport->dev)
+			uart_remove_one_port(&sbd_reg, uport);
 	}
 }
 
@@ -895,23 +889,14 @@ static int __init sbd_console_setup(stru
 	int bits = 8;
 	int parity = 'n';
 	int flow = 'n';
-	int ret;
 
 	if (!sport->duart)
 		return -ENXIO;
-
-	ret = sbd_map_port(uport);
-	if (ret)
-		return ret;
-
-	sbd_init_port(sport);
-
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
 	return uart_set_options(uport, co, baud, parity, bits, flow);
 }
 
-static struct uart_driver sbd_reg;
 static struct console sbd_console = {
 	.name	= "duart",
 	.write	= sbd_console_write,
@@ -922,16 +907,6 @@ static struct console sbd_console = {
 	.data	= &sbd_reg
 };
 
-static int __init sbd_serial_console_init(void)
-{
-	sbd_probe_duarts();
-	register_console(&sbd_console);
-
-	return 0;
-}
-
-console_initcall(sbd_serial_console_init);
-
 #define SERIAL_SB1250_DUART_CONSOLE	&sbd_console
 #else
 #define SERIAL_SB1250_DUART_CONSOLE	NULL
@@ -948,43 +923,30 @@ static struct uart_driver sbd_reg = {
 	.cons		= SERIAL_SB1250_DUART_CONSOLE,
 };
 
+static struct platform_driver sbd_driver = {
+	.remove = __exit_p(sbd_remove),
+	.driver = { .name = "sb1250-duart" },
+};
+
 /* Set up the driver and register it.  */
 static int __init sbd_init(void)
 {
-	int i, ret;
-
-	sbd_probe_duarts();
+	int ret;
 
 	ret = uart_register_driver(&sbd_reg);
 	if (ret)
 		return ret;
+	ret = platform_driver_probe(&sbd_driver, sbd_probe);
+	if (ret)
+		uart_unregister_driver(&sbd_reg);
 
-	for (i = 0; i < DUART_MAX_CHIP * DUART_MAX_SIDE; i++) {
-		struct sbd_duart *duart = &sbd_duarts[i / DUART_MAX_SIDE];
-		struct sbd_port *sport = &duart->sport[i % DUART_MAX_SIDE];
-		struct uart_port *uport = &sport->port;
-
-		if (sport->duart)
-			uart_add_one_port(&sbd_reg, uport);
-	}
-
-	return 0;
+	return ret;
 }
 
 /* Unload the driver.  Unregister stuff, get ready to go away.  */
 static void __exit sbd_exit(void)
 {
-	int i;
-
-	for (i = DUART_MAX_CHIP * DUART_MAX_SIDE - 1; i >= 0; i--) {
-		struct sbd_duart *duart = &sbd_duarts[i / DUART_MAX_SIDE];
-		struct sbd_port *sport = &duart->sport[i % DUART_MAX_SIDE];
-		struct uart_port *uport = &sport->port;
-
-		if (sport->duart)
-			uart_remove_one_port(&sbd_reg, uport);
-	}
-
+	platform_driver_unregister(&sbd_driver);
 	uart_unregister_driver(&sbd_reg);
 }
 

[PATCH 4/4] Revert "drivers: convert sbd_duart.map_guard from atomic_t to refcount_t"

[Maciej W. Rozycki]

Revert commit 22a33651a56f ("drivers: convert sbd_duart.map_guard from
atomic_t to refcount_t"), which broke perfectly valid code:

  ------------[ cut here ]------------
  WARNING: CPU: 1 PID: 1 at lib/refcount.c:114 sbd_request_port+0x54/0x140
  refcount_t: increment on 0; use-after-free.
  CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.11.0-rc2+ #34
  Stack : 0000000014001fe0 0000000000000000 ffffffff80830000 0000000000000000
          ffffffff8127bc7a ffffffff8016fe08 ffffffff808d0000 ffffffff808d0000
          ffffffff807aa828 ffffffff80822337 ffffffff808ce188 a8000001860b0000
          0000000000000001 0000000000000001 00000000000001c8 ffffffff808a3090
          00000000000000bb ffffffff801b09d4 a80000018609bb68 ffffffff801231cc
          ffffffff812a0000 ffffffff80171388 0000000000001000 ffffffff807aa828
          0000000000000001 0000000000000001 0000000000000000 0000000000000000
          0000000000000000 a80000018609bab0 0000000000000000 ffffffff803c47cc
          0000000000000000 0000000000000000 0000000000000000 0000000000000000
          ffffffff807cb648 ffffffff8010bff8 0000000014001fe1 ffffffff803c47cc
          ...
  Call Trace:
  [<ffffffff8010bff8>] show_stack+0x28/0x88
  [<ffffffff803c47cc>] dump_stack+0x8c/0xc0
  [<ffffffff801aff5c>] __warn+0xe0/0x114
  [<ffffffff801233f0>] warn_slowpath_fmt+0x40/0x50
  [<ffffffff80455bcc>] sbd_request_port+0x54/0x140
  [<ffffffff804563a4>] sbd_config_port+0x2c/0x68
  ---[ end trace f666d696412caa3e ]---

(report at the offending commit) -- sbd_request_port() is called twice
per DUART instance, to reserve a resource holding the control register
block shared between the two channels, so there's no slightest chance
for an overflow.  Also this doesn't stop the driver from working and
it's just the reservation that is missing as a result, i.e.:

10060100-100601ff : sb1250-duart
10060200-100602ff : sb1250-duart

as from the offending change, vs:

10060100-100601ff : sb1250-duart
10060200-100602ff : sb1250-duart
10060300-100603ff : sb1250-duart

beforehand, which is surely why the breakage has gone so long unnoticed.

"If it ain't broke, don't fix it," so just revert the broken commit.

Fixes: 22a33651a56f ("drivers: convert sbd_duart.map_guard from atomic_t to refcount_t")
Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
 drivers/tty/serial/sb1250-duart.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

linux-serial-sb1250-duart-map-guard-atomic.diff
Index: linux-macro/drivers/tty/serial/sb1250-duart.c
===================================================================
--- linux-macro.orig/drivers/tty/serial/sb1250-duart.c
+++ linux-macro/drivers/tty/serial/sb1250-duart.c
@@ -34,8 +34,8 @@
 #include <linux/tty_flip.h>
 #include <linux/types.h>
 
-#include <linux/refcount.h>
-#include <linux/io.h>
+#include <linux/atomic.h>
+#include <asm/io.h>
 
 #include <asm/sibyte/sb1250.h>
 #include <asm/sibyte/sb1250_uart.h>
@@ -86,7 +86,7 @@ struct sbd_port {
 struct sbd_duart {
 	struct sbd_port		sport[2];
 	unsigned long		mapctrl;
-	refcount_t		map_guard;
+	atomic_t		map_guard;
 };
 
 #define to_sport(uport) container_of(uport, struct sbd_port, port)
@@ -662,13 +662,15 @@ static void sbd_release_port(struct uart
 {
 	struct sbd_port *sport = to_sport(uport);
 	struct sbd_duart *duart = sport->duart;
+	int map_guard;
 
 	iounmap(sport->memctrl);
 	sport->memctrl = NULL;
 	iounmap(uport->membase);
 	uport->membase = NULL;
 
-	if(refcount_dec_and_test(&duart->map_guard))
+	map_guard = atomic_add_return(-1, &duart->map_guard);
+	if (!map_guard)
 		release_mem_region(duart->mapctrl, DUART_CHANREG_SPACING);
 	release_mem_region(uport->mapbase, DUART_CHANREG_SPACING);
 }
@@ -704,6 +706,7 @@ static int sbd_request_port(struct uart_
 {
 	const char *err = KERN_ERR "sbd: Unable to reserve MMIO resource\n";
 	struct sbd_duart *duart = to_sport(uport)->duart;
+	int map_guard;
 	int ret = 0;
 
 	if (!request_mem_region(uport->mapbase, DUART_CHANREG_SPACING,
@@ -711,11 +714,11 @@ static int sbd_request_port(struct uart_
 		printk(err);
 		return -EBUSY;
 	}
-	refcount_inc(&duart->map_guard);
-	if (refcount_read(&duart->map_guard) == 1) {
+	map_guard = atomic_add_return(1, &duart->map_guard);
+	if (map_guard == 1) {
 		if (!request_mem_region(duart->mapctrl, DUART_CHANREG_SPACING,
 					"sb1250-duart")) {
-			refcount_dec(&duart->map_guard);
+			atomic_add(-1, &duart->map_guard);
 			printk(err);
 			ret = -EBUSY;
 		}
@@ -723,7 +726,8 @@ static int sbd_request_port(struct uart_
 	if (!ret) {
 		ret = sbd_map_port(uport);
 		if (ret) {
-			if (refcount_dec_and_test(&duart->map_guard))
+			map_guard = atomic_add_return(-1, &duart->map_guard);
+			if (!map_guard)
 				release_mem_region(duart->mapctrl,
 						   DUART_CHANREG_SPACING);
 		}

Re: [PATCH 4/4] Revert "drivers: convert sbd_duart.map_guard from atomic_t to refcount_t"

[Greg Kroah-Hartman]

On Mon, Apr 13, 2026 at 04:28:53AM +0100, Maciej W. Rozycki wrote:
> Revert commit 22a33651a56f ("drivers: convert sbd_duart.map_guard from
> atomic_t to refcount_t"), which broke perfectly valid code:
> 
>   ------------[ cut here ]------------
>   WARNING: CPU: 1 PID: 1 at lib/refcount.c:114 sbd_request_port+0x54/0x140
>   refcount_t: increment on 0; use-after-free.
>   CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.11.0-rc2+ #34
>   Stack : 0000000014001fe0 0000000000000000 ffffffff80830000 0000000000000000
>           ffffffff8127bc7a ffffffff8016fe08 ffffffff808d0000 ffffffff808d0000
>           ffffffff807aa828 ffffffff80822337 ffffffff808ce188 a8000001860b0000
>           0000000000000001 0000000000000001 00000000000001c8 ffffffff808a3090
>           00000000000000bb ffffffff801b09d4 a80000018609bb68 ffffffff801231cc
>           ffffffff812a0000 ffffffff80171388 0000000000001000 ffffffff807aa828
>           0000000000000001 0000000000000001 0000000000000000 0000000000000000
>           0000000000000000 a80000018609bab0 0000000000000000 ffffffff803c47cc
>           0000000000000000 0000000000000000 0000000000000000 0000000000000000
>           ffffffff807cb648 ffffffff8010bff8 0000000014001fe1 ffffffff803c47cc
>           ...
>   Call Trace:
>   [<ffffffff8010bff8>] show_stack+0x28/0x88
>   [<ffffffff803c47cc>] dump_stack+0x8c/0xc0
>   [<ffffffff801aff5c>] __warn+0xe0/0x114
>   [<ffffffff801233f0>] warn_slowpath_fmt+0x40/0x50
>   [<ffffffff80455bcc>] sbd_request_port+0x54/0x140
>   [<ffffffff804563a4>] sbd_config_port+0x2c/0x68
>   ---[ end trace f666d696412caa3e ]---
> 
> (report at the offending commit) -- sbd_request_port() is called twice
> per DUART instance, to reserve a resource holding the control register
> block shared between the two channels, so there's no slightest chance
> for an overflow.  Also this doesn't stop the driver from working and
> it's just the reservation that is missing as a result, i.e.:
> 
> 10060100-100601ff : sb1250-duart
> 10060200-100602ff : sb1250-duart
> 
> as from the offending change, vs:
> 
> 10060100-100601ff : sb1250-duart
> 10060200-100602ff : sb1250-duart
> 10060300-100603ff : sb1250-duart
> 
> beforehand, which is surely why the breakage has gone so long unnoticed.
> 
> "If it ain't broke, don't fix it," so just revert the broken commit.

How about fix this up to work properly with a refcount?  having "open
coded" atomic variables like this is ripe for problems, like it seems
this driver is abusing.

thanks,

greg k-h