[PATCH] serial: sc16is7xx: declare MCR and IER as volatile registers

[PATCH] serial: sc16is7xx: declare MCR and IER as volatile registers

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

MCR register has the same address as XON1 register.
IER register has the same address as DLH register.

Which register is accessed depends on bits configuration in some
other registers.

To prevent potential access errors, declare MCR and IER as volatile
registers.

Fixes: dfeae619d781 ("serial: sc16is7xx")
Cc: stable@vger.kernel.org
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 296079c16fdb9..d6b3b67d17d55 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -501,7 +501,9 @@ static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
 	case SC16IS7XX_RHR_REG: /* Shared address space with THR & DLL */
+	case SC16IS7XX_IER_REG: /* Shared address space with DLH */
 	case SC16IS7XX_IIR_REG: /* Shared address space with FCR & EFR */
+	case SC16IS7XX_MCR_REG: /* Shared address space with XON1 */
 	case SC16IS7XX_LSR_REG: /* Shared address space with XON2 */
 	case SC16IS7XX_MSR_REG: /* Shared address space with TCR & XOFF1 */
 	case SC16IS7XX_SPR_REG: /* Shared address space with TLR & XOFF2 */

base-commit: 36844ea19656fb41278799ef3d5cd52120d89149
-- 
2.47.3

Re: [PATCH] serial: sc16is7xx: declare MCR and IER as volatile registers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tty: sc16is7xx: making MCR/IER volatile fails to fix bank-switch race
--

commit d56a719b3706ed6ed6267916f8234030f616e7b3
Author: Hugo Villeneuve <hvilleneuve@dimonoff.com>

serial: sc16is7xx: declare MCR and IER as volatile registers

This commit declares MCR and IER as volatile in the regmap configuration
to prevent potential access errors, as they share addresses with XON1 and
DLH.

> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 296079c16fdb9..d6b3b67d17d55 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -501,7 +501,9 @@ static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
>  {
>  	switch (reg) {
>  	case SC16IS7XX_RHR_REG: /* Shared address space with THR & DLL */
> +	case SC16IS7XX_IER_REG: /* Shared address space with DLH */
>  	case SC16IS7XX_IIR_REG: /* Shared address space with FCR & EFR */
> +	case SC16IS7XX_MCR_REG: /* Shared address space with XON1 */

[Severity: High]
Does making these registers volatile actually fix the underlying bank-switch
race?

If sc16is7xx_set_termios() holds one->lock and changes LCR to 0xBF to access
enhanced registers (like XON1), and sc16is7xx_reg_proc() concurrently updates
MCR without holding one->lock:

drivers/tty/serial/sc16is7xx.c:sc16is7xx_reg_proc() {
	...
		sc16is7xx_port_update(&one->port, SC16IS7XX_MCR_REG,
				      SC16IS7XX_MCR_RTS_BIT |
				      SC16IS7XX_MCR_DTR_BIT |
				      SC16IS7XX_MCR_LOOP_BIT,
				      mcr);
	...
}

Won't the forced physical hardware read-modify-write still hit XON1 instead
of MCR since LCR is currently switched?

This same pattern appears in sc16is7xx_tx_empty() where it reads LSR without
holding one->lock, racing with LCR bank switching:

drivers/tty/serial/sc16is7xx.c:sc16is7xx_tx_empty() {
	...
	lsr = sc16is7xx_port_read(port, SC16IS7XX_LSR_REG);
	...
}

Does this approach fail to fix the underlying locking race while adding
unnecessary I2C/SPI overhead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006192540.234402-1-hugo@hugovil.com?part=1