Re: [PATCH v2 2/3] serial: 8250_mxpcie: only unregister the ports that were registered

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver fails to initialize `up.port.port_id` for individual serial ports during probe, causing all successfully registered ports to incorrectly share `port_id = 0`. This leads to configuration meant for other ports silently overwriting port 0's hardware registers and software state.
--

commit 73e9e979a09f1fbc7bf6f2134b67c8afbd507c55
Author: Linmao Li <lilinmao@kylinos.cn>

    serial: 8250_mxpcie: only unregister the ports that were registered
    
    When serial8250_register_8250_port() fails the loop stops and the probe
    keeps the ports registered so far, like pciserial_init_ports() in
    8250_pci.c this driver was split from.  What the split lost is that
    function's priv->nr: mxpcie8250_remove() walks all num_ports entries,
    but the ones the loop never reached keep the zero devm_kzalloc() left
    there, and the one that failed keeps a negative error code.

> diff --git a/drivers/tty/serial/8250/8250_mxpcie.c b/drivers/tty/serial/8250/8250_mxpcie.c
> index c0e3517d4e4ec..ef2516ec16da0 100644
> --- a/drivers/tty/serial/8250/8250_mxpcie.c
> +++ b/drivers/tty/serial/8250/8250_mxpcie.c
[ ... ]
> @@ -564,7 +566,7 @@ static int mxpcie8250_probe(struct pci_dev *pdev, const struct pci_device_id *id
>  	up.port.handle_irq = mxpcie8250_handle_irq;
>  	up.port.break_ctl = mxpcie8250_break_ctl;
>  
> -	for (unsigned int i = 0; i < num_ports; i++) {
> +	for (i = 0; i < num_ports; i++) {
>  		mxpcie8250_setup_port(pdev, priv, &up, i);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but does mxpcie8250_setup_port() fail to set
up.port.port_id?

Because up.port.port_id is not initialized for individual serial ports during
probe, all successfully registered ports seem to incorrectly share port_id = 0.

Could this lead to configuration meant for other ports silently overwriting
port 0's hardware registers and software state?

For example, when setting RX trigger levels or handling ioctls for RS485
(TIOCSRS485) on any non-zero port, it looks like port->port_id will evaluate
to 0 in mxpcie8250_rs485_config():

drivers/tty/serial/8250/8250_mxpcie.c:mxpcie8250_rs485_config() {
	...
	mxpcie8250_set_interface(priv, port->port_id, mode);
	...
}

causing the driver to incorrectly read, modify, and write the configuration of
port 0 instead of the target port.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901041519.114737-1-lilinmao@kylinos.cn?part=2