[PATCH v2 0/3] serial: 8250_mxpcie: probe and console fixes

[PATCH v2 0/3] serial: 8250_mxpcie: probe and console fixes

[Linmao Li]

Three fixes for the Moxa PCIe serial driver, found by reading the code
that was split out of 8250_pci.c and extended during the 7.3 merge
window.  That code is in mainline now but has not been in a released
kernel, so the series is against tty-linus.

Patch 1 fixes a NULL pointer dereference during probe: the serial core
calls ->rs485_config() from within serial8250_register_8250_port(),
before pci_set_drvdata() has published the board.  It takes a board
without RS232 support and SER_RS485_ENABLED still set once
uart_get_rs485_mode() has run - on Crescent's ACPI machine the firmware
node clears that flag, which is why it does not crash there.

Patch 2 makes removal unregister only the ports that were registered.
v1 failed the probe instead; as Andy pointed out that is a behavioural
change, and keeping the ports that do work is what pciserial_init_ports()
in 8250_pci.c does, so v2 keeps it and restores only the missing count.

Patch 3 stops set_termios() from dereferencing port->state->port.tty,
which is NULL when the serial core sets the line up for a console or for
kgdboc.

I have no Moxa board here.  Crescent Hsieh tested patch 3 with kgdboc on
a CP-168EL-A, and looked for patch 1's crash on a CP-134EL-A; the rest is
compile-tested only, W=1 allmodconfig build of drivers/tty/serial/8250/
and checkpatch --strict, both clean.

v2:
- 1/3: no code change; the commit message now says the crash needs
  SER_RS485_ENABLED to survive uart_get_rs485_mode() (Crescent Hsieh).
- 2/3: keep the ports that registered successfully, record how many and
  unregister only those, instead of failing the probe (Andy Shevchenko).
  Retitled accordingly.
- 3/3: drop the cflag local and read new->c_cflag directly, and pick up
  Crescent Hsieh's Tested-by.

Link to v1:
https://lore.kernel.org/all/20260818093918.3190686-1-lilinmao@kylinos.cn/

Linmao Li (3):
  serial: 8250_mxpcie: set the driver data before registering ports
  serial: 8250_mxpcie: only unregister the ports that were registered
  serial: 8250_mxpcie: take the line settings from the new termios

 drivers/tty/serial/8250/8250_mxpcie.c | 30 +++++++++++++++------------
 1 file changed, 17 insertions(+), 13 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.25.1

[PATCH v2 1/3] serial: 8250_mxpcie: set the driver data before registering ports

[Linmao Li]

mxpcie8250_rs485_config() looks the board up with dev_get_drvdata() on
the PCI device, but pci_set_drvdata() only runs after the registration
loop.  Where mxpcie8250_setup_port() presets rs485.flags to
SER_RS485_ENABLED and the flag survives uart_get_rs485_mode() - the
device has no firmware node, or its node sets
linux,rs485-enabled-at-boot-time - uart_configure_port() calls
->rs485_config() from inside serial8250_register_8250_port(), and the
callback dereferences a NULL board pointer.

Publish the driver data before the first port is registered.

Fixes: d21a1509c623 ("serial: 8250_mxpcie: support serial interface mode switching")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
v2: no code change; the commit message now says the crash needs
    SER_RS485_ENABLED to survive uart_get_rs485_mode() (Crescent
    Hsieh).

 drivers/tty/serial/8250/8250_mxpcie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_mxpcie.c b/drivers/tty/serial/8250/8250_mxpcie.c
index ce873fbd62e9..c0e3517d4e4e 100644
--- a/drivers/tty/serial/8250/8250_mxpcie.c
+++ b/drivers/tty/serial/8250/8250_mxpcie.c
@@ -542,6 +542,8 @@ static int mxpcie8250_probe(struct pci_dev *pdev, const struct pci_device_id *id
 
 	mxpcie8250_init_board(pdev, priv);
 
+	pci_set_drvdata(pdev, priv);
+
 	up.port.dev = dev;
 	up.port.irq = pdev->irq;
 	up.port.uartclk = MOXA_PUART_BASE_BAUD * 16;
@@ -578,7 +580,6 @@ static int mxpcie8250_probe(struct pci_dev *pdev, const struct pci_device_id *id
 		}
 		priv->port[i].rx_trig_level = MOXA_PUART_RX_TRIG_DEFAULT;
 	}
-	pci_set_drvdata(pdev, priv);
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH v2 1/3] serial: 8250_mxpcie: set the driver data before registering ports

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing initialization of `up.port.port_id` causes all registered serial ports on a multi-port device to share ID 0 and corrupt each other's hardware state.
- [High] Unprotected read-modify-write on shared hardware register in `mxpcie8250_set_interface` causes a race condition between concurrent operations on adjacent ports.
--

commit 0dfacc505133057b054c4b8c8d42d557343c298b
Author: Linmao Li <lilinmao@kylinos.cn>

serial: 8250_mxpcie: set the driver data before registering ports

This commit fixes a NULL pointer dereference in the mxpcie8250_rs485_config
callback. It moves pci_set_drvdata() before port registration so the board
data is available when the callback is invoked during initialization.

> diff --git a/drivers/tty/serial/8250/8250_mxpcie.c b/drivers/tty/serial/8250/8250_mxpcie.c
> index ce873fbd62e93..c0e3517d4e4ec 100644
> --- a/drivers/tty/serial/8250/8250_mxpcie.c
> +++ b/drivers/tty/serial/8250/8250_mxpcie.c
> @@ -542,6 +542,8 @@ static int mxpcie8250_probe(struct pci_dev *pdev, const struct pci_device_id *id
>  
>  	mxpcie8250_init_board(pdev, priv);
>  
> +	pci_set_drvdata(pdev, priv);
> +
>  	up.port.dev = dev;
>  	up.port.irq = pdev->irq;
>  	up.port.uartclk = MOXA_PUART_BASE_BAUD * 16;
> @@ -578,7 +580,6 @@ static int mxpcie8250_probe(struct pci_dev *pdev, const struct pci_device_id *id
>  		}
>  		priv->port[i].rx_trig_level = MOXA_PUART_RX_TRIG_DEFAULT;
>  	}
> -	pci_set_drvdata(pdev, priv);
>  
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the port
registration loop, I noticed that mxpcie8250_setup_port() seems to leave
up.port.port_id initialized to 0 for all ports. Because the up structure
is zero-initialized, and up.port.port_id is never assigned the idx value,
it remains 0.

When this zeroed structure is passed into serial8250_register_8250_port(),
does this cause all registered ports on a multi-port device to share ID 0?
If so, hardware callbacks like mxpcie8250_rs485_config() and 
mxpcie8250_startup() that use port->port_id as an index might inadvertently
apply configuration changes to port 0 instead of the intended port.

[Severity: High]
This is also a pre-existing issue, but looking at mxpcie8250_set_interface(),
the shared hardware register controlling adjacent ports is updated using an
unprotected read-modify-write:

mxpcie8250_set_interface()
    ...
    cval = ioread8(uir_addr);

    if (port_idx % 2)
        FIELD_MODIFY(MOXA_ODD_RS_MASK, &cval, mode);
    else
        FIELD_MODIFY(MOXA_EVEN_RS_MASK, &cval, mode);

    iowrite8(cval, uir_addr);

Since this is called by mxpcie8250_rs485_config(), which is only protected
by the per-port uart_port_lock, could concurrent ioctls on adjacent ports
(like port 0 and port 1) race and corrupt this shared register byte?

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

Re: [PATCH v2 1/3] serial: 8250_mxpcie: set the driver data before registering ports

[Jiri Slaby]

Cc Crescent

Sashiko appears to be right.

On 01. 09. 26, 6:32, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Missing initialization of `up.port.port_id` causes all registered serial ports on a multi-port device to share ID 0 and corrupt each other's hardware state.
> - [High] Unprotected read-modify-write on shared hardware register in `mxpcie8250_set_interface` causes a race condition between concurrent operations on adjacent ports.
> --
> 
> commit 0dfacc505133057b054c4b8c8d42d557343c298b
> Author: Linmao Li <lilinmao@kylinos.cn>
> 
> serial: 8250_mxpcie: set the driver data before registering ports
> 
> This commit fixes a NULL pointer dereference in the mxpcie8250_rs485_config
> callback. It moves pci_set_drvdata() before port registration so the board
> data is available when the callback is invoked during initialization.
> 
>> diff --git a/drivers/tty/serial/8250/8250_mxpcie.c b/drivers/tty/serial/8250/8250_mxpcie.c
>> index ce873fbd62e93..c0e3517d4e4ec 100644
>> --- a/drivers/tty/serial/8250/8250_mxpcie.c
>> +++ b/drivers/tty/serial/8250/8250_mxpcie.c
>> @@ -542,6 +542,8 @@ static int mxpcie8250_probe(struct pci_dev *pdev, const struct pci_device_id *id
>>   
>>   	mxpcie8250_init_board(pdev, priv);
>>   
>> +	pci_set_drvdata(pdev, priv);
>> +
>>   	up.port.dev = dev;
>>   	up.port.irq = pdev->irq;
>>   	up.port.uartclk = MOXA_PUART_BASE_BAUD * 16;
>> @@ -578,7 +580,6 @@ static int mxpcie8250_probe(struct pci_dev *pdev, const struct pci_device_id *id
>>   		}
>>   		priv->port[i].rx_trig_level = MOXA_PUART_RX_TRIG_DEFAULT;
>>   	}
>> -	pci_set_drvdata(pdev, priv);
>>   
>>   	return 0;
>>   }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but while reviewing the port
> registration loop, I noticed that mxpcie8250_setup_port() seems to leave
> up.port.port_id initialized to 0 for all ports. Because the up structure
> is zero-initialized, and up.port.port_id is never assigned the idx value,
> it remains 0.
> 
> When this zeroed structure is passed into serial8250_register_8250_port(),
> does this cause all registered ports on a multi-port device to share ID 0?
> If so, hardware callbacks like mxpcie8250_rs485_config() and
> mxpcie8250_startup() that use port->port_id as an index might inadvertently
> apply configuration changes to port 0 instead of the intended port.
> 
> [Severity: High]
> This is also a pre-existing issue, but looking at mxpcie8250_set_interface(),
> the shared hardware register controlling adjacent ports is updated using an
> unprotected read-modify-write:
> 
> mxpcie8250_set_interface()
>      ...
>      cval = ioread8(uir_addr);
> 
>      if (port_idx % 2)
>          FIELD_MODIFY(MOXA_ODD_RS_MASK, &cval, mode);
>      else
>          FIELD_MODIFY(MOXA_EVEN_RS_MASK, &cval, mode);
> 
>      iowrite8(cval, uir_addr);
> 
> Since this is called by mxpcie8250_rs485_config(), which is only protected
> by the per-port uart_port_lock, could concurrent ioctls on adjacent ports
> (like port 0 and port 1) race and corrupt this shared register byte?
> 


-- 
js
suse labs

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

[Linmao Li]

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.

serial8250_unregister_port() checks neither, so removal unregisters
line 0 - a port this driver does not own - and indexes
serial8250_ports[] with a negative line number.

Record how many ports were registered and unregister only those.

Fixes: 0481a041e956 ("serial: 8250: split Moxa PCIe serial board support out of 8250_pci")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
v2: keep the ports that registered successfully and restore only the
    missing count; v1 failed the probe instead (Andy Shevchenko).

 drivers/tty/serial/8250/8250_mxpcie.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_mxpcie.c b/drivers/tty/serial/8250/8250_mxpcie.c
index c0e3517d4e4e..ef2516ec16da 100644
--- a/drivers/tty/serial/8250/8250_mxpcie.c
+++ b/drivers/tty/serial/8250/8250_mxpcie.c
@@ -109,6 +109,7 @@ struct mxpcie8250_port {
 struct mxpcie8250 {
 	unsigned int supp_rs;
 	unsigned int num_ports;
+	unsigned int nr;	/* ports actually registered */
 	void __iomem *bar1_base; /* UART registers (MMIO) */
 	void __iomem *bar2_base; /* UIR / GPIO / CPLD (IO) */
 	struct mxpcie8250_port port[] __counted_by(num_ports);
@@ -517,6 +518,7 @@ static int mxpcie8250_probe(struct pci_dev *pdev, const struct pci_device_id *id
 	struct mxpcie8250 *priv;
 	unsigned short device = pdev->device;
 	unsigned int num_ports;
+	unsigned int i;
 	int ret;
 
 	ret = pcim_enable_device(pdev);
@@ -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);
 
 		dev_dbg(dev, "Setup PCI port: port %lx, irq %d, type %d\n",
@@ -580,6 +582,7 @@ static int mxpcie8250_probe(struct pci_dev *pdev, const struct pci_device_id *id
 		}
 		priv->port[i].rx_trig_level = MOXA_PUART_RX_TRIG_DEFAULT;
 	}
+	priv->nr = i;
 
 	return 0;
 }
@@ -588,7 +591,7 @@ static void mxpcie8250_remove(struct pci_dev *pdev)
 {
 	struct mxpcie8250 *priv = pci_get_drvdata(pdev);
 
-	for (unsigned int i = 0; i < priv->num_ports; i++)
+	for (unsigned int i = 0; i < priv->nr; i++)
 		serial8250_unregister_port(priv->port[i].line);
 }
 
-- 
2.25.1

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

[sashiko-bot]

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

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

[Andy Shevchenko]

On Tue, Sep 01, 2026 at 12:15:18PM +0800, Linmao Li wrote:
> 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.
> 
> serial8250_unregister_port() checks neither, so removal unregisters
> line 0 - a port this driver does not own - and indexes
> serial8250_ports[] with a negative line number.
> 
> Record how many ports were registered and unregister only those.

...

>  struct mxpcie8250 {

>  	unsigned int supp_rs;
>  	unsigned int num_ports;
> +	unsigned int nr;	/* ports actually registered */

I would make it indented with the below comments.
But no need to resend for this. It's not critical at all.

>  	void __iomem *bar1_base; /* UART registers (MMIO) */
>  	void __iomem *bar2_base; /* UIR / GPIO / CPLD (IO) */
>  	struct mxpcie8250_port port[] __counted_by(num_ports);

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 3/3] serial: 8250_mxpcie: take the line settings from the new termios

[Linmao Li]

mxpcie8250_set_termios() reads the line settings out of
port->state->port.tty, which is only set once the port has been opened.

uart_set_options() builds a termios of its own and calls ->set_termios()
with no tty behind it, so using such a board as the console
(console=ttyS<n>) dereferences a NULL tty during console setup, as does
attaching kgdboc to it and resuming a suspended console from
uart_resume_port().

Read the settings from the termios the serial core passes in instead.
It holds the same values on the normal path - uart_change_line_settings()
passes &tty->termios - and it is what serial8250_do_set_termios() right
above already uses.

Fixes: 55edf8511f47 ("serial: 8250_mxpcie: enable automatic RTS/CTS flow control")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
Tested-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>
---
v2: drop the cflag local, read new->c_cflag directly (Crescent Hsieh).

 drivers/tty/serial/8250/8250_mxpcie.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_mxpcie.c b/drivers/tty/serial/8250/8250_mxpcie.c
index ef2516ec16da..19f1f51eec47 100644
--- a/drivers/tty/serial/8250/8250_mxpcie.c
+++ b/drivers/tty/serial/8250/8250_mxpcie.c
@@ -218,8 +218,6 @@ static void mxpcie8250_set_termios(struct uart_port *port,
 				   const struct ktermios *old)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
-	struct tty_struct *tty = port->state->port.tty;
-	unsigned int cflag = tty->termios.c_cflag;
 	u8 efr, val;
 
 	serial8250_do_set_termios(port, new, old);
@@ -229,23 +227,25 @@ static void mxpcie8250_set_termios(struct uart_port *port,
 	efr = serial_in(up, MOXA_PUART_EFR);
 	efr &= ~(MOXA_PUART_EFR_AUTO_RTS | MOXA_PUART_EFR_AUTO_CTS);
 
-	if (cflag & CRTSCTS) {
+	if (new->c_cflag & CRTSCTS) {
 		efr |= (MOXA_PUART_EFR_AUTO_RTS | MOXA_PUART_EFR_AUTO_CTS);
 		up->port.status |= (UPSTAT_AUTORTS | UPSTAT_AUTOCTS);
 	}
 	/* Set on-chip software flow control character */
-	serial_out(up, MOXA_PUART_XON1, START_CHAR(tty));
-	serial_out(up, MOXA_PUART_XON2, START_CHAR(tty));
-	serial_out(up, MOXA_PUART_XOFF1, STOP_CHAR(tty));
-	serial_out(up, MOXA_PUART_XOFF2, STOP_CHAR(tty));
+	serial_out(up, MOXA_PUART_XON1, new->c_cc[VSTART]);
+	serial_out(up, MOXA_PUART_XON2, new->c_cc[VSTART]);
+	serial_out(up, MOXA_PUART_XOFF1, new->c_cc[VSTOP]);
+	serial_out(up, MOXA_PUART_XOFF2, new->c_cc[VSTOP]);
 
-	val = I_IXON(tty) ? MOXA_PUART_EFR_RX_FLOW_XON1_XOFF1 : MOXA_PUART_EFR_RX_FLOW_DISABLED;
+	val = (new->c_iflag & IXON) ? MOXA_PUART_EFR_RX_FLOW_XON1_XOFF1 :
+				      MOXA_PUART_EFR_RX_FLOW_DISABLED;
 	FIELD_MODIFY(MOXA_PUART_EFR_RX_FLOW_MASK, &efr, val);
 
-	val = I_IXOFF(tty) ? MOXA_PUART_EFR_TX_FLOW_XON1_XOFF1 : MOXA_PUART_EFR_TX_FLOW_DISABLED;
+	val = (new->c_iflag & IXOFF) ? MOXA_PUART_EFR_TX_FLOW_XON1_XOFF1 :
+				       MOXA_PUART_EFR_TX_FLOW_DISABLED;
 	FIELD_MODIFY(MOXA_PUART_EFR_TX_FLOW_MASK, &efr, val);
 
-	if (I_IXOFF(tty))
+	if (new->c_iflag & IXOFF)
 		up->port.status |= UPSTAT_AUTOXOFF;
 
 	serial_out(up, MOXA_PUART_EFR, efr);
-- 
2.25.1

Re: [PATCH v2 3/3] serial: 8250_mxpcie: take the line settings from the new termios

[sashiko-bot]

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

Pre-existing issues:
- [High] Hardware registers and port status are modified without holding the port lock or a Runtime PM reference.
--

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

serial: 8250_mxpcie: take the line settings from the new termios

mxpcie8250_set_termios() reads the line settings out of port->state->port.tty,
which is only set once the port has been opened. This commit fixes a NULL
dereference during console setup or kgdboc attach by reading the settings
from the termios the serial core passes in instead.

> diff --git a/drivers/tty/serial/8250/8250_mxpcie.c b/drivers/tty/serial/8250/8250_mxpcie.c
> index ef2516ec16da0..19f1f51eec477 100644
> --- a/drivers/tty/serial/8250/8250_mxpcie.c
> +++ b/drivers/tty/serial/8250/8250_mxpcie.c
> @@ -218,8 +218,6 @@ static void mxpcie8250_set_termios(struct uart_port *port,
>  				   const struct ktermios *old)
>  {
>  	struct uart_8250_port *up = up_to_u8250p(port);
> -	struct tty_struct *tty = port->state->port.tty;
> -	unsigned int cflag = tty->termios.c_cflag;
>  	u8 efr, val;
>  
>  	serial8250_do_set_termios(port, new, old);
> @@ -229,23 +227,25 @@ static void mxpcie8250_set_termios(struct uart_port *port,
>  	efr = serial_in(up, MOXA_PUART_EFR);

[Severity: High]
This is a pre-existing issue, but is it safe to modify the port status and
access hardware registers here after serial8250_do_set_termios() has
already released the port lock and runtime PM reference?

Modifying up->port.status in mxpcie8250_set_termios() without holding the
port lock creates a data race that can corrupt flow control flags
(e.g., UPSTAT_AUTOCTS). This directly violates the locking contract
documented in include/linux/serial_core.h.

Furthermore, accessing UART hardware registers via serial_in() and
serial_out() without a port lock or a Runtime PM reference can lead to
concurrent register access races with IRQ handlers, or bus faults if the
device enters a suspended state. This sequence occurs every time
mxpcie8250_set_termios() is called, for instance when userspace calls
tcsetattr to change flow control on the TTY device.

>  	efr &= ~(MOXA_PUART_EFR_AUTO_RTS | MOXA_PUART_EFR_AUTO_CTS);
>  
> -	if (cflag & CRTSCTS) {
> +	if (new->c_cflag & CRTSCTS) {
>  		efr |= (MOXA_PUART_EFR_AUTO_RTS | MOXA_PUART_EFR_AUTO_CTS);
>  		up->port.status |= (UPSTAT_AUTORTS | UPSTAT_AUTOCTS);
>  	}

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

Re: [PATCH v2 3/3] serial: 8250_mxpcie: take the line settings from the new termios

[Jiri Slaby]

CC Crescent as well.

On 01. 09. 26, 6:23, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Hardware registers and port status are modified without holding the port lock or a Runtime PM reference.
> --
> 
> commit cc8d50f2957c7d540f5a4840cf6ed31ccd1bed47
> Author: Linmao Li <lilinmao@kylinos.cn>
> 
> serial: 8250_mxpcie: take the line settings from the new termios
> 
> mxpcie8250_set_termios() reads the line settings out of port->state->port.tty,
> which is only set once the port has been opened. This commit fixes a NULL
> dereference during console setup or kgdboc attach by reading the settings
> from the termios the serial core passes in instead.
> 
>> diff --git a/drivers/tty/serial/8250/8250_mxpcie.c b/drivers/tty/serial/8250/8250_mxpcie.c
>> index ef2516ec16da0..19f1f51eec477 100644
>> --- a/drivers/tty/serial/8250/8250_mxpcie.c
>> +++ b/drivers/tty/serial/8250/8250_mxpcie.c
>> @@ -218,8 +218,6 @@ static void mxpcie8250_set_termios(struct uart_port *port,
>>   				   const struct ktermios *old)
>>   {
>>   	struct uart_8250_port *up = up_to_u8250p(port);
>> -	struct tty_struct *tty = port->state->port.tty;
>> -	unsigned int cflag = tty->termios.c_cflag;
>>   	u8 efr, val;
>>   
>>   	serial8250_do_set_termios(port, new, old);
>> @@ -229,23 +227,25 @@ static void mxpcie8250_set_termios(struct uart_port *port,
>>   	efr = serial_in(up, MOXA_PUART_EFR);
> 
> [Severity: High]
> This is a pre-existing issue, but is it safe to modify the port status and
> access hardware registers here after serial8250_do_set_termios() has
> already released the port lock and runtime PM reference?
> 
> Modifying up->port.status in mxpcie8250_set_termios() without holding the
> port lock creates a data race that can corrupt flow control flags
> (e.g., UPSTAT_AUTOCTS). This directly violates the locking contract
> documented in include/linux/serial_core.h.
> 
> Furthermore, accessing UART hardware registers via serial_in() and
> serial_out() without a port lock or a Runtime PM reference can lead to
> concurrent register access races with IRQ handlers, or bus faults if the
> device enters a suspended state. This sequence occurs every time
> mxpcie8250_set_termios() is called, for instance when userspace calls
> tcsetattr to change flow control on the TTY device.
> 
>>   	efr &= ~(MOXA_PUART_EFR_AUTO_RTS | MOXA_PUART_EFR_AUTO_CTS);
>>   
>> -	if (cflag & CRTSCTS) {
>> +	if (new->c_cflag & CRTSCTS) {
>>   		efr |= (MOXA_PUART_EFR_AUTO_RTS | MOXA_PUART_EFR_AUTO_CTS);
>>   		up->port.status |= (UPSTAT_AUTORTS | UPSTAT_AUTOCTS);
>>   	}
> 


-- 
js
suse labs

Re: [PATCH v2 0/3] serial: 8250_mxpcie: probe and console fixes

[Andy Shevchenko]

On Tue, Sep 01, 2026 at 12:15:16PM +0800, Linmao Li wrote:
> Three fixes for the Moxa PCIe serial driver, found by reading the code
> that was split out of 8250_pci.c and extended during the 7.3 merge
> window.  That code is in mainline now but has not been in a released
> kernel, so the series is against tty-linus.
> 
> Patch 1 fixes a NULL pointer dereference during probe: the serial core
> calls ->rs485_config() from within serial8250_register_8250_port(),
> before pci_set_drvdata() has published the board.  It takes a board
> without RS232 support and SER_RS485_ENABLED still set once
> uart_get_rs485_mode() has run - on Crescent's ACPI machine the firmware
> node clears that flag, which is why it does not crash there.
> 
> Patch 2 makes removal unregister only the ports that were registered.
> v1 failed the probe instead; as Andy pointed out that is a behavioural
> change, and keeping the ports that do work is what pciserial_init_ports()
> in 8250_pci.c does, so v2 keeps it and restores only the missing count.
> 
> Patch 3 stops set_termios() from dereferencing port->state->port.tty,
> which is NULL when the serial core sets the line up for a console or for
> kgdboc.
> 
> I have no Moxa board here.  Crescent Hsieh tested patch 3 with kgdboc on
> a CP-168EL-A, and looked for patch 1's crash on a CP-134EL-A; the rest is
> compile-tested only, W=1 allmodconfig build of drivers/tty/serial/8250/
> and checkpatch --strict, both clean.

LGTM
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] serial: 8250_mxpcie: probe and console fixes

[Crescent Hsieh]

On Tue, Sep 01, 2026 at 12:15:16PM +0800, Linmao Li wrote:
> Three fixes for the Moxa PCIe serial driver, found by reading the code
> that was split out of 8250_pci.c and extended during the 7.3 merge
> window.  That code is in mainline now but has not been in a released
> kernel, so the series is against tty-linus.
> 
> Patch 1 fixes a NULL pointer dereference during probe: the serial core
> calls ->rs485_config() from within serial8250_register_8250_port(),
> before pci_set_drvdata() has published the board.  It takes a board
> without RS232 support and SER_RS485_ENABLED still set once
> uart_get_rs485_mode() has run - on Crescent's ACPI machine the firmware
> node clears that flag, which is why it does not crash there.
> 
> Patch 2 makes removal unregister only the ports that were registered.
> v1 failed the probe instead; as Andy pointed out that is a behavioural
> change, and keeping the ports that do work is what pciserial_init_ports()
> in 8250_pci.c does, so v2 keeps it and restores only the missing count.
> 
> Patch 3 stops set_termios() from dereferencing port->state->port.tty,
> which is NULL when the serial core sets the line up for a console or for
> kgdboc.
> 
> I have no Moxa board here.  Crescent Hsieh tested patch 3 with kgdboc on
> a CP-168EL-A, and looked for patch 1's crash on a CP-134EL-A; the rest is
> compile-tested only, W=1 allmodconfig build of drivers/tty/serial/8250/
> and checkpatch --strict, both clean.

The series looks good to me.

For the series:

Reviewed-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>

The additional pre-existing issues identified by Sashiko are independent
of this series. I will investigate them further and determine the
appropriate follow-up fixes.

--
Thanks,
Crescent Hsieh