[PATCH] sc16is7xx: Fix for incorrect data being transmitted

[PATCH] sc16is7xx: Fix for incorrect data being transmitted

[Phil Elwell]

UART drivers are meant to use the port spinlock within certain
methods, to protect against reentrancy. The sc16is7xx driver does
very little locking, presumably because when added it triggers
"scheduling while atomic" errors. This is due to the use of mutexes
within the regmap abstraction layer, and the mutex implementation's
habit of sleeping the current thread while waiting for access.
Unfortunately this lack of interlocking can lead to corruption of
outbound data, which occurs when the buffer used for I2C transmission
is used simultaneously by two threads - a work queue thread running
sc16is7xx_tx_proc, and an IRQ thread in sc16is7xx_port_irq, both
of which can call sc16is7xx_handle_tx.

An earlier patch added efr_lock, a mutex that controls access to the
EFR register. This mutex is already claimed in the IRQ handler, and
all that is required is to claim the same mutex in sc16is7xx_tx_proc.

See: https://github.com/raspberrypi/linux/issues/4885

Signed-off-by: Phil Elwell <phil@raspberrypi.com>
---
 drivers/tty/serial/sc16is7xx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 25d67b8c4db7..3a6c68e19c80 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -734,12 +734,15 @@ static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
 static void sc16is7xx_tx_proc(struct kthread_work *ws)
 {
 	struct uart_port *port = &(to_sc16is7xx_one(ws, tx_work)->port);
+	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
 
 	if ((port->rs485.flags & SER_RS485_ENABLED) &&
 	    (port->rs485.delay_rts_before_send > 0))
 		msleep(port->rs485.delay_rts_before_send);
 
+	mutex_lock(&s->efr_lock);
 	sc16is7xx_handle_tx(port);
+	mutex_unlock(&s->efr_lock);
 }
 
 static void sc16is7xx_reconf_rs485(struct uart_port *port)
-- 
2.25.1

Re: [PATCH] sc16is7xx: Fix for incorrect data being transmitted

[Greg Kroah-Hartman]

On Wed, Feb 16, 2022 at 03:08:58PM +0000, Phil Elwell wrote:
> UART drivers are meant to use the port spinlock within certain
> methods, to protect against reentrancy. The sc16is7xx driver does
> very little locking, presumably because when added it triggers
> "scheduling while atomic" errors. This is due to the use of mutexes
> within the regmap abstraction layer, and the mutex implementation's
> habit of sleeping the current thread while waiting for access.
> Unfortunately this lack of interlocking can lead to corruption of
> outbound data, which occurs when the buffer used for I2C transmission
> is used simultaneously by two threads - a work queue thread running
> sc16is7xx_tx_proc, and an IRQ thread in sc16is7xx_port_irq, both
> of which can call sc16is7xx_handle_tx.
> 
> An earlier patch added efr_lock, a mutex that controls access to the
> EFR register. This mutex is already claimed in the IRQ handler, and
> all that is required is to claim the same mutex in sc16is7xx_tx_proc.
> 
> See: https://github.com/raspberrypi/linux/issues/4885
> 
> Signed-off-by: Phil Elwell <phil@raspberrypi.com>
> ---
>  drivers/tty/serial/sc16is7xx.c | 3 +++
>  1 file changed, 3 insertions(+)

What commit id does this fix?

thanks,

greg k-h

Re: [PATCH] sc16is7xx: Fix for incorrect data being transmitted

[Phil Elwell]

On 16/02/2022 15:20, Greg Kroah-Hartman wrote:
> On Wed, Feb 16, 2022 at 03:08:58PM +0000, Phil Elwell wrote:
>> UART drivers are meant to use the port spinlock within certain
>> methods, to protect against reentrancy. The sc16is7xx driver does
>> very little locking, presumably because when added it triggers
>> "scheduling while atomic" errors. This is due to the use of mutexes
>> within the regmap abstraction layer, and the mutex implementation's
>> habit of sleeping the current thread while waiting for access.
>> Unfortunately this lack of interlocking can lead to corruption of
>> outbound data, which occurs when the buffer used for I2C transmission
>> is used simultaneously by two threads - a work queue thread running
>> sc16is7xx_tx_proc, and an IRQ thread in sc16is7xx_port_irq, both
>> of which can call sc16is7xx_handle_tx.
>>
>> An earlier patch added efr_lock, a mutex that controls access to the
>> EFR register. This mutex is already claimed in the IRQ handler, and
>> all that is required is to claim the same mutex in sc16is7xx_tx_proc.
>>
>> See: https://github.com/raspberrypi/linux/issues/4885
>>
>> Signed-off-by: Phil Elwell <phil@raspberrypi.com>
>> ---
>>   drivers/tty/serial/sc16is7xx.c | 3 +++
>>   1 file changed, 3 insertions(+)
> 
> What commit id does this fix?

That's a good question, and one you shouldn't have to ask. I was going to say 
[1] because it deleted a previous mutex, but I think it's use of a single 
workqueue for both IRQ and TX tasks made it safe. That leaves [2], which
replaces the IRQ work queue with a threaded IRQ, removing the reentrancy protection.

If you're in agreement I'll send a v2 with the appropriate Fixes: tag.

[1] commit 9e6f0ca3e567 ("sc16is7xx: use kthread_worker for tx_work and irq")
[2] commit 6393ff1c4435 ("sc16is7xx: Use threaded IRQ")

Phil