[PATCH 0/5] serial: mcf: add devicetree support

[PATCH 0/5] serial: mcf: add devicetree support

[Greg Ungerer]

This patch series is designed to add devicetree support to the ColdFire
serial driver (mcf.c). The m68k/ColdFire arch support is not devicetree
capable itself yet, but this is an important first step.

The first patch is a cleanup, the second is about normalizing the existing
platform device setup. The third changes the driver to always use dynamic
memory allocation for devices, the forth and fifth patches add the
devicetree support itself.

Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
---
 b/Documentation/devicetree/bindings/serial/fsl,mcf-uart.yaml |   43 ++
 b/arch/m68k/coldfire/device.c                                |  179 +++++++----
 b/arch/m68k/include/asm/mcfuart.h                            |   10 
 b/drivers/tty/serial/mcf.c                                   |    4 
 drivers/tty/serial/mcf.c                                     |   93 +++--
 5 files changed, 235 insertions(+), 94 deletions(-)

[PATCH 1/5] serial: mcf: update author email address

[Greg Ungerer]

The copyright and author email address of gerg@uclinux.org is no longer
valid. Change to a current valid email address, gerg@linux-m68k.org.

Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
---
 drivers/tty/serial/mcf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/mcf.c b/drivers/tty/serial/mcf.c
index 93e7dda4d39a..2b862dfe39ab 100644
--- a/drivers/tty/serial/mcf.c
+++ b/drivers/tty/serial/mcf.c
@@ -4,7 +4,7 @@
 /*
  *	mcf.c -- Freescale ColdFire UART driver
  *
- *	(C) Copyright 2003-2007, Greg Ungerer <gerg@uclinux.org>
+ *	(C) Copyright 2003-2007, Greg Ungerer <gerg@linux-m68k.org>
  */
 
 /****************************************************************************/
@@ -654,7 +654,7 @@ static void __exit mcf_exit(void)
 module_init(mcf_init);
 module_exit(mcf_exit);
 
-MODULE_AUTHOR("Greg Ungerer <gerg@uclinux.org>");
+MODULE_AUTHOR("Greg Ungerer <gerg@linux-m68k.org>");
 MODULE_DESCRIPTION("Freescale ColdFire UART driver");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("platform:mcfuart");
-- 
2.55.0

Re: [PATCH 1/5] serial: mcf: update author email address

[sashiko-bot]

> The copyright and author email address of gerg@uclinux.org is no longer
> valid. Change to a current valid email address, gerg@linux-m68k.org.
> 
> Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013120831.1046590-1-gerg@linux-m68k.org?part=1

[PATCH 2/5] serial: mcf: convert to use proper platform resources

[Greg Ungerer]

The ColdFire SoC device setup code supplies a non-standard array of
hardware address information (memory address and IRQ) for initializing
the UARTS - which it does as a single blob. The mcf serial probe
function processes the whole set in one go from a single platform entry.

Convert this setup to use the proper resource structures, with one for
each UART present on a platform. Modify the probe function to process
a single platform device per call, and use the proper platform resource
API to get the memory and IRQ addresses. This results in the proper
entries now being present in /proc/iomem.

Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
---
 arch/m68k/coldfire/device.c     | 179 +++++++++++++++++++++++---------
 arch/m68k/include/asm/mcfuart.h |  10 --
 drivers/tty/serial/mcf.c        |  45 ++++----
 3 files changed, 154 insertions(+), 80 deletions(-)

diff --git a/arch/m68k/coldfire/device.c b/arch/m68k/coldfire/device.c
index 9a0258acd998..e32bc3322c97 100644
--- a/arch/m68k/coldfire/device.c
+++ b/arch/m68k/coldfire/device.c
@@ -18,8 +18,8 @@
 #include <asm/traps.h>
 #include <asm/coldfire.h>
 #include <asm/mcfsim.h>
-#include <asm/mcfuart.h>
 #include <asm/mcfqspi.h>
+#include <linux/platform_device.h>
 #include <linux/platform_data/edma.h>
 #include <linux/platform_data/dma-mcf-edma.h>
 #include <linux/platform_data/mmc-esdhc-mcf.h>
@@ -27,71 +27,125 @@
 /*
  *	All current ColdFire parts contain from 2, 3, 4 or 10 UARTS.
  */
-static struct mcf_platform_uart mcf_uart_platform_data[] = {
-	{
-		.mapbase	= MCFUART_BASE0,
-		.irq		= MCF_IRQ_UART0,
-	},
-	{
-		.mapbase	= MCFUART_BASE1,
-		.irq		= MCF_IRQ_UART1,
-	},
+static struct resource mcf_uart0_resources[] = {
+	DEFINE_RES_MEM(MCFUART_BASE0, 0x80),
+	DEFINE_RES_IRQ(MCF_IRQ_UART0),
+};
+static struct platform_device mcf_uart0 = {
+	.name			= "mcfuart",
+	.id			= 0,
+	.num_resources		= ARRAY_SIZE(mcf_uart0_resources),
+	.resource		= mcf_uart0_resources,
+};
+
+#ifdef MCFUART_BASE1
+static struct resource mcf_uart1_resources[] = {
+	DEFINE_RES_MEM(MCFUART_BASE1, 0x80),
+	DEFINE_RES_IRQ(MCF_IRQ_UART1),
+};
+static struct platform_device mcf_uart1 = {
+	.name			= "mcfuart",
+	.id			= 1,
+	.num_resources		= ARRAY_SIZE(mcf_uart1_resources),
+	.resource		= mcf_uart1_resources,
+};
+#endif
 #ifdef MCFUART_BASE2
-	{
-		.mapbase	= MCFUART_BASE2,
-		.irq		= MCF_IRQ_UART2,
-	},
+static struct resource mcf_uart2_resources[] = {
+	DEFINE_RES_MEM(MCFUART_BASE2, 0x80),
+	DEFINE_RES_IRQ(MCF_IRQ_UART2),
+};
+static struct platform_device mcf_uart2 = {
+	.name			= "mcfuart",
+	.id			= 2,
+	.num_resources		= ARRAY_SIZE(mcf_uart2_resources),
+	.resource		= mcf_uart2_resources,
+};
 #endif
 #ifdef MCFUART_BASE3
-	{
-		.mapbase	= MCFUART_BASE3,
-		.irq		= MCF_IRQ_UART3,
-	},
+static struct resource mcf_uart3_resources[] = {
+	DEFINE_RES_MEM(MCFUART_BASE3, 0x80),
+	DEFINE_RES_IRQ(MCF_IRQ_UART3),
+};
+static struct platform_device mcf_uart3 = {
+	.name			= "mcfuart",
+	.id			= 3,
+	.num_resources		= ARRAY_SIZE(mcf_uart3_resources),
+	.resource		= mcf_uart3_resources,
+};
 #endif
 #ifdef MCFUART_BASE4
-	{
-		.mapbase	= MCFUART_BASE4,
-		.irq		= MCF_IRQ_UART4,
-	},
+static struct resource mcf_uart4_resources[] = {
+	DEFINE_RES_MEM(MCFUART_BASE4, 0x80),
+	DEFINE_RES_IRQ(MCF_IRQ_UART4),
+};
+static struct platform_device mcf_uart4 = {
+	.name			= "mcfuart",
+	.id			= 4,
+	.num_resources		= ARRAY_SIZE(mcf_uart4_resources),
+	.resource		= mcf_uart4_resources,
+};
 #endif
 #ifdef MCFUART_BASE5
-	{
-		.mapbase	= MCFUART_BASE5,
-		.irq		= MCF_IRQ_UART5,
-	},
+static struct resource mcf_uart5_resources[] = {
+	DEFINE_RES_MEM(MCFUART_BASE5, 0x80),
+	DEFINE_RES_IRQ(MCF_IRQ_UART5),
+};
+static struct platform_device mcf_uart5 = {
+	.name			= "mcfuart",
+	.id			= 5,
+	.num_resources		= ARRAY_SIZE(mcf_uart5_resources),
+	.resource		= mcf_uart5_resources,
+};
 #endif
 #ifdef MCFUART_BASE6
-	{
-		.mapbase	= MCFUART_BASE6,
-		.irq		= MCF_IRQ_UART6,
-	},
+static struct resource mcf_uart6_resources[] = {
+	DEFINE_RES_MEM(MCFUART_BASE6, 0x80),
+	DEFINE_RES_IRQ(MCF_IRQ_UART6),
+};
+static struct platform_device mcf_uart6 = {
+	.name			= "mcfuart",
+	.id			= 6,
+	.num_resources		= ARRAY_SIZE(mcf_uart6_resources),
+	.resource		= mcf_uart6_resources,
+};
 #endif
 #ifdef MCFUART_BASE7
-	{
-		.mapbase	= MCFUART_BASE7,
-		.irq		= MCF_IRQ_UART7,
-	},
+static struct resource mcf_uart7_resources[] = {
+	DEFINE_RES_MEM(MCFUART_BASE7, 0x80),
+	DEFINE_RES_IRQ(MCF_IRQ_UART7),
+};
+static struct platform_device mcf_uart7 = {
+	.name			= "mcfuart",
+	.id			= 7,
+	.num_resources		= ARRAY_SIZE(mcf_uart7_resources),
+	.resource		= mcf_uart7_resources,
+};
 #endif
 #ifdef MCFUART_BASE8
-	{
-		.mapbase	= MCFUART_BASE8,
-		.irq		= MCF_IRQ_UART8,
-	},
+static struct resource mcf_uart8_resources[] = {
+	DEFINE_RES_MEM(MCFUART_BASE8, 0x80),
+	DEFINE_RES_IRQ(MCF_IRQ_UART8),
+};
+static struct platform_device mcf_uart8 = {
+	.name			= "mcfuart",
+	.id			= 8,
+	.num_resources		= ARRAY_SIZE(mcf_uart8_resources),
+	.resource		= mcf_uart8_resources,
+};
 #endif
 #ifdef MCFUART_BASE9
-	{
-		.mapbase	= MCFUART_BASE9,
-		.irq		= MCF_IRQ_UART9,
-	},
-#endif
-	{ },
+static struct resource mcf_uart9_resources[] = {
+	DEFINE_RES_MEM(MCFUART_BASE9, 0x80),
+	DEFINE_RES_IRQ(MCF_IRQ_UART9),
 };
-
-static struct platform_device mcf_uart = {
+static struct platform_device mcf_uart9 = {
 	.name			= "mcfuart",
-	.id			= 0,
-	.dev.platform_data	= mcf_uart_platform_data,
+	.id			= 9,
+	.num_resources		= ARRAY_SIZE(mcf_uart9_resources),
+	.resource		= mcf_uart9_resources,
 };
+#endif
 
 #ifdef MCFFEC_BASE0
 
@@ -623,7 +677,34 @@ static struct platform_device mcf_flexcan0 = {
 #endif /* MCFFLEXCAN_SIZE */
 
 static struct platform_device *mcf_devices[] __initdata = {
-	&mcf_uart,
+	&mcf_uart0,
+#ifdef MCFUART_BASE1
+	&mcf_uart1,
+#endif
+#ifdef MCFUART_BASE2
+	&mcf_uart2,
+#endif
+#ifdef MCFUART_BASE3
+	&mcf_uart3,
+#endif
+#ifdef MCFUART_BASE4
+	&mcf_uart4,
+#endif
+#ifdef MCFUART_BASE5
+	&mcf_uart5,
+#endif
+#ifdef MCFUART_BASE6
+	&mcf_uart6,
+#endif
+#ifdef MCFUART_BASE7
+	&mcf_uart7,
+#endif
+#ifdef MCFUART_BASE8
+	&mcf_uart8,
+#endif
+#ifdef MCFUART_BASE9
+	&mcf_uart9,
+#endif
 #ifdef MCFFEC_BASE0
 	&mcf_fec0,
 #endif
diff --git a/arch/m68k/include/asm/mcfuart.h b/arch/m68k/include/asm/mcfuart.h
index a1f35352f328..c80b022c1ebb 100644
--- a/arch/m68k/include/asm/mcfuart.h
+++ b/arch/m68k/include/asm/mcfuart.h
@@ -13,16 +13,6 @@
 #define	mcfuart_h
 /****************************************************************************/
 
-#include <linux/serial_core.h>
-#include <linux/platform_device.h>
-
-struct mcf_platform_uart {
-	unsigned long	mapbase;	/* Physical address base */
-	void __iomem	*membase;	/* Virtual address if mapped */
-	unsigned int	irq;		/* Interrupt vector */
-	unsigned int	uartclk;	/* UART clock rate */
-};
-
 /*
  *	Define the ColdFire UART register set addresses.
  */
diff --git a/drivers/tty/serial/mcf.c b/drivers/tty/serial/mcf.c
index 2b862dfe39ab..3651910dff99 100644
--- a/drivers/tty/serial/mcf.c
+++ b/drivers/tty/serial/mcf.c
@@ -570,31 +570,34 @@ static struct uart_driver mcf_driver = {
 
 static int mcf_probe(struct platform_device *pdev)
 {
-	struct mcf_platform_uart *platp = dev_get_platdata(&pdev->dev);
 	struct uart_port *port;
-	int i;
+	struct resource *res;
 
-	for (i = 0; ((i < MCF_MAXPORTS) && (platp[i].mapbase)); i++) {
-		port = &mcf_ports[i].port;
+	if (pdev->id >= MCF_MAXPORTS)
+		return -ENODEV;
+	port = &mcf_ports[pdev->id].port;
 
-		port->line = i;
-		port->type = PORT_MCF;
-		port->mapbase = platp[i].mapbase;
-		port->membase = (platp[i].membase) ? platp[i].membase :
-			(unsigned char __iomem *) platp[i].mapbase;
-		port->dev = &pdev->dev;
-		port->iotype = SERIAL_IO_MEM;
-		port->irq = platp[i].irq;
-		port->uartclk = MCF_BUSCLK;
-		port->ops = &mcf_uart_ops;
-		port->flags = UPF_BOOT_AUTOCONF;
-		port->rs485_config = mcf_config_rs485;
-		port->rs485_supported = mcf_rs485_supported;
-		port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_MCF_CONSOLE);
-
-		uart_add_one_port(&mcf_driver, port);
-	}
+	port->membase = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(port->membase))
+		return PTR_ERR(port->membase);
+	port->mapbase = res->start;
+
+	port->irq = platform_get_irq(pdev, 0);
+	if (port->irq < 0)
+		return port->irq;
 
+	port->line = pdev->id;
+	port->type = PORT_MCF;
+	port->dev = &pdev->dev;
+	port->iotype = SERIAL_IO_MEM;
+	port->uartclk = MCF_BUSCLK;
+	port->ops = &mcf_uart_ops;
+	port->flags = UPF_BOOT_AUTOCONF;
+	port->rs485_config = mcf_config_rs485;
+	port->rs485_supported = mcf_rs485_supported;
+	port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_MCF_CONSOLE);
+
+	uart_add_one_port(&mcf_driver, port);
 	return 0;
 }
 
-- 
2.55.0

Re: [PATCH 2/5] serial: mcf: convert to use proper platform resources

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] tty: mcf: unconditional removal of all UART ports in mcf_remove()
- [High] tty: mcf: ERR_PTR stored in membase bypasses uninitialized check
- [Medium] tty: mcf: tautological error check on platform_get_irq()
--

commit c5cb7916e270b19e0758d11e193eb23c71763eca
Author: Greg Ungerer <gerg@linux-m68k.org>

serial: mcf: convert to use proper platform resources

The ColdFire SoC device setup code supplies a non-standard array of
hardware address information (memory address and IRQ) for initializing
the UARTS - which it does as a single blob. The mcf serial probe
function processes the whole set in one go from a single platform entry.

[ ... ]
> diff --git a/drivers/tty/serial/mcf.c b/drivers/tty/serial/mcf.c
> index 2b862dfe39ab3..3651910dff99e 100644
> --- a/drivers/tty/serial/mcf.c
> +++ b/drivers/tty/serial/mcf.c
[ ... ]
> @@ -570,31 +570,34 @@ static struct uart_driver mcf_driver = {
>  
>  static int mcf_probe(struct platform_device *pdev)
>  {
> -	struct mcf_platform_uart *platp = dev_get_platdata(&pdev->dev);
>  	struct uart_port *port;
> -	int i;
> +	struct resource *res;
>  
> -	for (i = 0; ((i < MCF_MAXPORTS) && (platp[i].mapbase)); i++) {
> -		port = &mcf_ports[i].port;
> +	if (pdev->id >= MCF_MAXPORTS)
> +		return -ENODEV;
> +	port = &mcf_ports[pdev->id].port;
>  
> -		port->line = i;
> -		port->type = PORT_MCF;
> -		port->mapbase = platp[i].mapbase;
> -		port->membase = (platp[i].membase) ? platp[i].membase :
> -			(unsigned char __iomem *) platp[i].mapbase;
> -		port->dev = &pdev->dev;
> -		port->iotype = SERIAL_IO_MEM;
> -		port->irq = platp[i].irq;
> -		port->uartclk = MCF_BUSCLK;
> -		port->ops = &mcf_uart_ops;
> -		port->flags = UPF_BOOT_AUTOCONF;
> -		port->rs485_config = mcf_config_rs485;
> -		port->rs485_supported = mcf_rs485_supported;
> -		port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_MCF_CONSOLE);
> -
> -		uart_add_one_port(&mcf_driver, port);
> -	}
> +	port->membase = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(port->membase))
> +		return PTR_ERR(port->membase);

[Severity: High]
Since port is a pointer to the global mcf_ports array, assigning an ERR_PTR
to port->membase leaves the error pointer in the global state if
devm_platform_get_and_ioremap_resource() fails.

Later, when the early console is initialized, mcf_console_setup() checks if
membase is valid using:

    if (port->membase == 0)
        return -ENODEV;

Since an ERR_PTR is not zero, this check will pass, which could cause a
kernel panic when mcf_console_write() dereferences the ERR_PTR during a
printk.

Could we store the mapped address in a local variable and only assign it to
port->membase on success?

> +	port->mapbase = res->start;
> +
> +	port->irq = platform_get_irq(pdev, 0);
> +	if (port->irq < 0)
> +		return port->irq;

[Severity: Medium]
In struct uart_port, irq is defined as an unsigned int. Does the check
(port->irq < 0) undergo unsigned comparison and become tautologically false?

If platform_get_irq() fails and returns a negative error code like -ENXIO,
it seems this error would be swallowed, and the port would be registered
with a bogus, very large IRQ number.

> +	port->line = pdev->id;
> +	port->type = PORT_MCF;
> +	port->dev = &pdev->dev;
> +	port->iotype = SERIAL_IO_MEM;
> +	port->uartclk = MCF_BUSCLK;
> +	port->ops = &mcf_uart_ops;
> +	port->flags = UPF_BOOT_AUTOCONF;
> +	port->rs485_config = mcf_config_rs485;
> +	port->rs485_supported = mcf_rs485_supported;
> +	port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_MCF_CONSOLE);
> +
> +	uart_add_one_port(&mcf_driver, port);
>  	return 0;
>  }

[Severity: High]
With the conversion of mcf_probe() to process a single platform device per
call instead of a single blob, does mcf_remove() also need to be updated?

Looking at mcf_remove() in drivers/tty/serial/mcf.c:

static void mcf_remove(struct platform_device *pdev)
{
	struct uart_port *port;
	int i;

	for (i = 0; (i < MCF_MAXPORTS); i++) {
		port = &mcf_ports[i].port;
		if (port)
			uart_remove_one_port(&mcf_driver, port);
	}
}

It appears this loop still unconditionally unregisters all 10 ports,
regardless of which pdev is being removed. If a single device is unbound
via sysfs, will this disrupt other devices or lead to list corruption by
calling uart_remove_one_port() on uninitialized structs or multiple times
on the same port?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013120831.1046590-1-gerg@linux-m68k.org?part=2

[PATCH 3/5] serial: mcf: dynamically allocate memory

[Greg Ungerer]

Modify the ColdFire serial driver to dynamically allocate the local uart
port structures as required at init time. Currently it uses a fixed
size array - a little wasteful of memory on most parts with only a
couple of UARTs.

Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
---
 drivers/tty/serial/mcf.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/mcf.c b/drivers/tty/serial/mcf.c
index 3651910dff99..7cbe43cbe3a0 100644
--- a/drivers/tty/serial/mcf.c
+++ b/drivers/tty/serial/mcf.c
@@ -55,6 +55,14 @@ struct mcf_uart {
 	unsigned char		imr;		/* Local IMR mirror */
 };
 
+/*
+ * Define maximum number of UARTs this driver will support.
+ * This is set to maximum number on any known ColdFire SoC.
+ */
+#define MCF_MAXPORTS            10
+
+static struct mcf_uart *mcf_ports[MCF_MAXPORTS];
+
 /****************************************************************************/
 
 static unsigned int mcf_tx_empty(struct uart_port *port)
@@ -462,19 +470,19 @@ static const struct uart_ops mcf_uart_ops = {
 	.verify_port	= mcf_verify_port,
 };
 
-static struct mcf_uart mcf_ports[10];
-
-#define	MCF_MAXPORTS	ARRAY_SIZE(mcf_ports)
-
 /****************************************************************************/
 #if defined(CONFIG_SERIAL_MCF_CONSOLE)
 /****************************************************************************/
 
 static void mcf_console_putc(struct console *co, const char c)
 {
-	struct uart_port *port = &(mcf_ports + co->index)->port;
+	struct uart_port *port;
 	int i;
 
+	if ((co->index < 0) || (co->index >= MCF_MAXPORTS))
+		co->index = 0;
+	port = &mcf_ports[co->index]->port;
+
 	for (i = 0; (i < 0x10000); i++) {
 		if (readb(port->membase + MCFUART_USR) & MCFUART_USR_TXREADY)
 			break;
@@ -509,9 +517,9 @@ static int __init mcf_console_setup(struct console *co, char *options)
 
 	if ((co->index < 0) || (co->index >= MCF_MAXPORTS))
 		co->index = 0;
-	port = &mcf_ports[co->index].port;
-	if (port->membase == 0)
+	if (mcf_ports[co->index] == NULL)
 		return -ENODEV;
+	port = &mcf_ports[co->index]->port;
 
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
@@ -572,10 +580,16 @@ static int mcf_probe(struct platform_device *pdev)
 {
 	struct uart_port *port;
 	struct resource *res;
+	struct mcf_uart *mp;
 
 	if (pdev->id >= MCF_MAXPORTS)
 		return -ENODEV;
-	port = &mcf_ports[pdev->id].port;
+
+	mp = devm_kzalloc(&pdev->dev, sizeof(*mp), GFP_KERNEL);
+	if (mp == NULL)
+		return -ENOMEM;
+	mcf_ports[pdev->id] = mp;
+	port = &mp->port;
 
 	port->membase = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
 	if (IS_ERR(port->membase))
@@ -605,13 +619,11 @@ static int mcf_probe(struct platform_device *pdev)
 
 static void mcf_remove(struct platform_device *pdev)
 {
-	struct uart_port *port;
 	int i;
 
 	for (i = 0; (i < MCF_MAXPORTS); i++) {
-		port = &mcf_ports[i].port;
-		if (port)
-			uart_remove_one_port(&mcf_driver, port);
+		if (mcf_ports[i])
+			uart_remove_one_port(&mcf_driver, &mcf_ports[i]->port);
 	}
 }
 
-- 
2.55.0

Re: [PATCH 3/5] serial: mcf: dynamically allocate memory

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] tty: mcf: Use-After-Free and double-unregister due to dangling pointers and global array loop
- [High] tty: mcf: Premature console publication causes NULL pointer dereference
- [High] tty: serial: mcf: Use-After-Free on probe error and driver removal
--

commit cc5afe1f9a1d1092ba4a245994a51d452fb2379c
Author: Greg Ungerer <gerg@linux-m68k.org>

serial: mcf: dynamically allocate memory

Modify the ColdFire serial driver to dynamically allocate the local uart
port structures as required at init time. Currently it uses a fixed
size array - a little wasteful of memory on most parts with only a
couple of UARTs.

> diff --git a/drivers/tty/serial/mcf.c b/drivers/tty/serial/mcf.c
> index 3651910dff99e..7cbe43cbe3a0e 100644
> --- a/drivers/tty/serial/mcf.c
> +++ b/drivers/tty/serial/mcf.c

[ ... ]

> @@ -509,9 +517,9 @@ static int __init mcf_console_setup(struct console *co, char *options)
>  
>  	if ((co->index < 0) || (co->index >= MCF_MAXPORTS))
>  		co->index = 0;
> -	port = &mcf_ports[co->index].port;
> -	if (port->membase == 0)
> +	if (mcf_ports[co->index] == NULL)
>  		return -ENODEV;
> +	port = &mcf_ports[co->index]->port;

[Severity: High]
Could this change lead to a NULL pointer dereference if console setup runs
concurrently with device probe?

In mcf_probe(), the new port structure is allocated and assigned to the
global mcf_ports[] array before port->membase is initialized via ioremap.
Since mcf_console_setup() no longer checks if port->membase is zero, it can
successfully complete before the mapping is done. If a printk occurs in
this window, mcf_console_putc() will dereference the NULL membase when
attempting to read the UART status.

[ ... ]

> @@ -572,10 +580,16 @@ static int mcf_probe(struct platform_device *pdev)
>  {
>  	struct uart_port *port;
>  	struct resource *res;
> +	struct mcf_uart *mp;
>  
>  	if (pdev->id >= MCF_MAXPORTS)
>  		return -ENODEV;
> -	port = &mcf_ports[pdev->id].port;
> +
> +	mp = devm_kzalloc(&pdev->dev, sizeof(*mp), GFP_KERNEL);
> +	if (mp == NULL)
> +		return -ENOMEM;
> +	mcf_ports[pdev->id] = mp;
> +	port = &mp->port;
>  
>  	port->membase = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>  	if (IS_ERR(port->membase))

[Severity: High]
Does this leak a dangling pointer into the global mcf_ports array if probe
fails?

The devm_kzalloc() pointer is assigned to mcf_ports[pdev->id] before
devm_platform_get_and_ioremap_resource() or platform_get_irq() are called.
If either of those functions fail, mcf_probe() returns an error, and the
devres framework will free the memory. However, mcf_ports[pdev->id] is
never cleared, leaving a dangling pointer that can be dereferenced later.

[ ... ]

> @@ -605,13 +619,11 @@ static int mcf_probe(struct platform_device *pdev)
>  
>  static void mcf_remove(struct platform_device *pdev)
>  {
> -	struct uart_port *port;
>  	int i;
>  
>  	for (i = 0; (i < MCF_MAXPORTS); i++) {
> -		port = &mcf_ports[i].port;
> -		if (port)
> -			uart_remove_one_port(&mcf_driver, port);
> +		if (mcf_ports[i])
> +			uart_remove_one_port(&mcf_driver, &mcf_ports[i]->port);
>  	}
>  }

[Severity: High]
Can this loop cause a use-after-free and improperly unregister all ports
when a single device is removed?

During mcf_remove(), the code iterates through the entire mcf_ports array
up to MCF_MAXPORTS and calls uart_remove_one_port() on every non-NULL
entry, rather than just the port associated with the pdev being removed.

Additionally, it does not clear mcf_ports[i] back to NULL. When
mcf_remove() finishes, devres will free the memory associated with the
removed pdev. If another UART device is subsequently unbound, mcf_remove()
will run again, iterate over the array, and call uart_remove_one_port()
using the dangling pointer from the previously freed device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013120831.1046590-1-gerg@linux-m68k.org?part=3

[PATCH 4/5] dt-bindings: serial: create binding for mcf-uart

[Greg Ungerer]

Create documentation for a new binding to support the Freescale ColdFire
UART hardware block. This is found on pretty much all variants of the
M68K derived Freescale ColdFire family of System-on-Chip devices.

It is not compatible with any other known UARTs.

Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
---
 .../bindings/serial/fsl,mcf-uart.yaml         | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/fsl,mcf-uart.yaml

diff --git a/Documentation/devicetree/bindings/serial/fsl,mcf-uart.yaml b/Documentation/devicetree/bindings/serial/fsl,mcf-uart.yaml
new file mode 100644
index 000000000000..d1dba518f67a
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/fsl,mcf-uart.yaml
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/serial/fsl-mcf-uart.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale ColdFire UART
+
+maintainers:
+  - Greg Ungerer <gerg@linux-m68k.org>
+
+allof:
+  - $ref: serial.yaml#
+
+properties:
+  compatible:
+    const: fsl,mcf-uart
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    uart0: serial@fc060000 {
+        compatible = "fsl,mcf-uart";
+        reg = <0xfc060000 0x80>;
+        interrupts = <90>;
+        clocks = <&coreclk>;
+    };
-- 
2.55.0

Re: [PATCH 4/5] dt-bindings: serial: create binding for mcf-uart

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: $id path mismatch breaks schema cross-referencing
- [Medium] dt-bindings: schema keyword typo allof prevents base serial schema evaluation
--

commit c5bd98e903d69d54479e4a7058d0ad59884cfb54
Author: Greg Ungerer <gerg@linux-m68k.org>

dt-bindings: serial: create binding for mcf-uart

This commit creates documentation for a new binding to support the Freescale
ColdFire UART hardware block. This hardware block is found on M68K derived
Freescale ColdFire family of SoC devices.

> diff --git a/Documentation/devicetree/bindings/serial/fsl,mcf-uart.yaml b/Documentation/devicetree/bindings/serial/fsl,mcf-uart.yaml
> new file mode 100644
> index 0000000000000..d1dba518f67af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/fsl,mcf-uart.yaml
> @@ -0,0 +1,43 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/serial/fsl-mcf-uart.yaml#

[Severity: Medium]
Will this schema mismatch cause a regression by breaking cross referencing?

In Documentation/devicetree/bindings/serial/fsl,mcf-uart.yaml, the file is
named with a comma, but the id is defined with a hyphen.

The device tree bindings subsystem guidelines require that the path in the id
field exactly match the file path relative to
Documentation/devicetree/bindings/. The dt_binding_check tooling will
encounter this mismatch and silently skip or fail validation because the
paths do not correspond.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale ColdFire UART
> +
> +maintainers:
> +  - Greg Ungerer <gerg@linux-m68k.org>
> +
> +allof:
> +  - $ref: serial.yaml#

[Severity: Medium]
Could this typo cause a regression where the base serial schema is not
evaluated?

JSON Schema keywords are case-sensitive. The binding uses allof instead of
allOf, meaning the reference directive is silently ignored. 

When running dt_binding_check on device trees utilizing this compatible
string, it will not enforce necessary properties or allow standard UART
properties because it fails to inherit the base serial schema.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013120831.1046590-1-gerg@linux-m68k.org?part=4

[PATCH 5/5] serial: mcf: add devicetree support

[Greg Ungerer]

Add basic support for probing the ColdFire UARTs via a devicetree.

Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
---
 drivers/tty/serial/mcf.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/tty/serial/mcf.c b/drivers/tty/serial/mcf.c
index 7cbe43cbe3a0..01b3ab6e7dba 100644
--- a/drivers/tty/serial/mcf.c
+++ b/drivers/tty/serial/mcf.c
@@ -21,6 +21,7 @@
 #include <linux/io.h>
 #include <linux/uaccess.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
 #include <asm/coldfire.h>
 #include <asm/mcfsim.h>
 #include <asm/mcfuart.h>
@@ -62,6 +63,7 @@ struct mcf_uart {
 #define MCF_MAXPORTS            10
 
 static struct mcf_uart *mcf_ports[MCF_MAXPORTS];
+static int mcf_numports;
 
 /****************************************************************************/
 
@@ -582,6 +584,8 @@ static int mcf_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct mcf_uart *mp;
 
+	if (pdev->id == -1)
+		pdev->id = mcf_numports;
 	if (pdev->id >= MCF_MAXPORTS)
 		return -ENODEV;
 
@@ -612,6 +616,8 @@ static int mcf_probe(struct platform_device *pdev)
 	port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_MCF_CONSOLE);
 
 	uart_add_one_port(&mcf_driver, port);
+
+	mcf_numports++;
 	return 0;
 }
 
@@ -629,11 +635,17 @@ static void mcf_remove(struct platform_device *pdev)
 
 /****************************************************************************/
 
+static const struct of_device_id mcf_uart_dt_ids[] = {
+	{ .compatible = "fsl,mcf-uart", },
+	{ }
+};
+
 static struct platform_driver mcf_platform_driver = {
 	.probe		= mcf_probe,
 	.remove		= mcf_remove,
 	.driver		= {
 		.name	= "mcfuart",
+		.of_match_table = mcf_uart_dt_ids,
 	},
 };
 
-- 
2.55.0

Re: [PATCH 5/5] serial: mcf: add devicetree support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] tty: serial: mcf: ID collision due to persistent pdev->id mutation on probe deferral
- [High] tty: serial: mcf: dangling pointer in global mcf_ports array on probe error
- [High] tty: serial: mcf: mcf_remove unregisters all ports system-wide instead of unbound device
- [High] serial: mcf: broken port numbering and API violations in mcf_probe
- [Medium] tty: serial: mcf: missing MODULE_DEVICE_TABLE prevents module autoloading
- [Medium] tty: serial: mcf: missing error check for uart_add_one_port
--

commit 61f92ee3964b5b13fd6350da88b718b7350512cd
Author: Greg Ungerer <gerg@linux-m68k.org>

serial: mcf: add devicetree support
    
Add basic support for probing the ColdFire UARTs via a devicetree.

> diff --git a/drivers/tty/serial/mcf.c b/drivers/tty/serial/mcf.c
> index 7cbe43cbe3a0e..01b3ab6e7dba9 100644
> --- a/drivers/tty/serial/mcf.c
> +++ b/drivers/tty/serial/mcf.c
> @@ -582,6 +584,8 @@ static int mcf_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	struct mcf_uart *mp;
>  
> +	if (pdev->id == -1)
> +		pdev->id = mcf_numports;

[Severity: High]
Does modifying pdev->id persistently break deferred probe or unbind/rebind
cycles?

If the device is unbound and rebound, or if probe defers, it will retain
the old ID on the next probe attempt. Since mcf_numports is incremented on
success, this can lead to ID exhaustion over multiple unbind/rebind cycles.

Additionally, should of_alias_get_id(pdev->dev.of_node, "serial") be used
here to respect standard device tree aliases instead of an auto-incrementing
counter?

[Severity: High]
Does this code leave a dangling pointer in the global mcf_ports array if
probe fails later?

If a subsequent call (like devm_platform_get_and_ioremap_resource() or
platform_get_irq()) fails, the devres subsystem will automatically free 'mp'
allocated via devm_kzalloc(), but mcf_ports[pdev->id] is never cleared.

>  	if (pdev->id >= MCF_MAXPORTS)
>  		return -ENODEV;
>  

[ ... ]

> @@ -612,6 +616,8 @@ static int mcf_probe(struct platform_device *pdev)
>  	port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_MCF_CONSOLE);
>  
>  	uart_add_one_port(&mcf_driver, port);

[Severity: Medium]
Is it safe to ignore the return value of uart_add_one_port() here?

If registration fails (e.g., out of memory or line conflicts), mcf_numports
is still incremented and probe falsely reports success, leaving a partially
initialized state.

> +
> +	mcf_numports++;
>  	return 0;
>  }
>  

[ ... ]

> @@ -629,11 +635,17 @@ static void mcf_remove(struct platform_device *pdev)
>  
>  /****************************************************************************/
>  

[Severity: High]
Will mcf_remove() still unregister all ports system-wide when a single
device is unbound?

It appears that mcf_remove() unconditionally iterates over all MCF_MAXPORTS
and removes every active port, rather than only removing the specific port
associated with the unbound device.

> +static const struct of_device_id mcf_uart_dt_ids[] = {
> +	{ .compatible = "fsl,mcf-uart", },
> +	{ }
> +};
> +

[Severity: Medium]
Does the absence of MODULE_DEVICE_TABLE(of, mcf_uart_dt_ids) prevent module
autoloading when the hardware is present in the device tree?

>  static struct platform_driver mcf_platform_driver = {
>  	.probe		= mcf_probe,
>  	.remove		= mcf_remove,
>  	.driver		= {
>  		.name	= "mcfuart",
> +		.of_match_table = mcf_uart_dt_ids,
>  	},
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013120831.1046590-1-gerg@linux-m68k.org?part=5