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

[sashiko-bot@kernel.org]

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