From: Hugo Villeneuve hvilleneuve@dimonoff.com
Hello, this patch series brings additional patches to fix some FIFO issues when backporting to linux-5.15.y for the sc16is7xx driver.
Commit ("serial: sc16is7xx: add missing support for rs485 devicetree properties") is required when using RS-485.
Commit ("serial: sc16is7xx: refactor FIFO access functions to increase commonality") is a prerequisite for commit ("serial: sc16is7xx: fix TX fifo corruption"). Altough it is not strictly necessary, it makes backporting easier.
I have tested the changes on a custom board with two SC16IS752 DUART over a SPI interface using a Variscite IMX8MN NANO SOM. The four UARTs are configured in RS-485 mode.
Thank you.
Hugo Villeneuve (4): serial: sc16is7xx: add missing support for rs485 devicetree properties serial: sc16is7xx: refactor FIFO access functions to increase commonality serial: sc16is7xx: fix TX fifo corruption serial: sc16is7xx: fix invalid FIFO access with special register set
drivers/tty/serial/sc16is7xx.c | 38 ++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 16 deletions(-)
From: Hugo Villeneuve hvilleneuve@dimonoff.com
Retrieve rs485 devicetree properties on registration of sc16is7xx ports in case they are attached to an rs485 transceiver.
Reworked to fix conflicts when backporting to linux-5.15.y.
Signed-off-by: Hugo Villeneuve hvilleneuve@dimonoff.com Reviewed-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Reviewed-by: Lech Perczak lech.perczak@camlingroup.com Tested-by: Lech Perczak lech.perczak@camlingroup.com Link: https://lore.kernel.org/r/20230807214556.540627-7-hugo@hugovil.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- drivers/tty/serial/sc16is7xx.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c index 692c14d7f7d1a..3d3f66563b73b 100644 --- a/drivers/tty/serial/sc16is7xx.c +++ b/drivers/tty/serial/sc16is7xx.c @@ -1307,6 +1307,10 @@ static int sc16is7xx_probe(struct device *dev,
mutex_init(&s->p[i].efr_lock);
+ ret = uart_get_rs485_mode(&s->p[i].port); + if (ret) + goto out_ports; + /* Disable all interrupts */ sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_IER_REG, 0); /* Disable TX/RX */
[ Sasha's backport helper bot ]
Hi,
Found matching upstream commit: b4a778303ea0fcabcaff974721477a5743e1f8ec
WARNING: Author mismatch between patch and found commit: Backport author: Hugo Villeneuve hugo@hugovil.com Commit author: Hugo Villeneuve hvilleneuve@dimonoff.com
Status in newer kernel trees: 6.12.y | Present (exact SHA1)
Note: The patch differs from the upstream commit: --- Failed to apply patch cleanly, falling back to interdiff...
interdiff error output: /home/sasha/stable/mailbot.sh: line 525: interdiff: command not found interdiff failed, falling back to standard diff... ---
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | stable/linux-6.12.y | Failed | N/A | | stable/linux-6.6.y | Failed | N/A | | stable/linux-6.1.y | Failed | N/A | | stable/linux-5.15.y | Failed | N/A | | stable/linux-5.10.y | Failed | N/A | | stable/linux-5.4.y | Failed | N/A |
On Mon, Dec 16, 2024 at 02:18:15PM -0500, Hugo Villeneuve wrote:
From: Hugo Villeneuve hvilleneuve@dimonoff.com
Retrieve rs485 devicetree properties on registration of sc16is7xx ports in case they are attached to an rs485 transceiver.
Reworked to fix conflicts when backporting to linux-5.15.y.
Signed-off-by: Hugo Villeneuve hvilleneuve@dimonoff.com Reviewed-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Reviewed-by: Lech Perczak lech.perczak@camlingroup.com Tested-by: Lech Perczak lech.perczak@camlingroup.com Link: https://lore.kernel.org/r/20230807214556.540627-7-hugo@hugovil.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
I do not see any commit ids on this series matching up with what is in Linus's tree. Please fix up and resend the series.
thanks,
greg k-h
On Tue, 17 Dec 2024 07:41:32 +0100 Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Mon, Dec 16, 2024 at 02:18:15PM -0500, Hugo Villeneuve wrote:
From: Hugo Villeneuve hvilleneuve@dimonoff.com
Retrieve rs485 devicetree properties on registration of sc16is7xx ports in case they are attached to an rs485 transceiver.
Reworked to fix conflicts when backporting to linux-5.15.y.
Signed-off-by: Hugo Villeneuve hvilleneuve@dimonoff.com Reviewed-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Reviewed-by: Lech Perczak lech.perczak@camlingroup.com Tested-by: Lech Perczak lech.perczak@camlingroup.com Link: https://lore.kernel.org/r/20230807214556.540627-7-hugo@hugovil.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
I do not see any commit ids on this series matching up with what is in Linus's tree. Please fix up and resend the series.
Ok will do. I will first submit only this patch to make sure my submission follows the stable guidelines.
Hugo.
From: Hugo Villeneuve hvilleneuve@dimonoff.com
Simplify FIFO access functions by avoiding to declare a struct sc16is7xx_port *s variable within each function.
This is mainly done to have more commonality between the max310x and sc16is7xx drivers.
Signed-off-by: Hugo Villeneuve hvilleneuve@dimonoff.com Link: https://lore.kernel.org/r/20231221231823.2327894-15-hugo@hugovil.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- drivers/tty/serial/sc16is7xx.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c index 3d3f66563b73b..591f97e78cdb7 100644 --- a/drivers/tty/serial/sc16is7xx.c +++ b/drivers/tty/serial/sc16is7xx.c @@ -361,17 +361,15 @@ static void sc16is7xx_port_write(struct uart_port *port, u8 reg, u8 val) regmap_write(one->regmap, reg, val); }
-static void sc16is7xx_fifo_read(struct uart_port *port, unsigned int rxlen) +static void sc16is7xx_fifo_read(struct uart_port *port, u8 *rxbuf, unsigned int rxlen) { - struct sc16is7xx_port *s = dev_get_drvdata(port->dev); struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
- regmap_noinc_read(one->regmap, SC16IS7XX_RHR_REG, s->buf, rxlen); + regmap_noinc_read(one->regmap, SC16IS7XX_RHR_REG, rxbuf, rxlen); }
-static void sc16is7xx_fifo_write(struct uart_port *port, u8 to_send) +static void sc16is7xx_fifo_write(struct uart_port *port, u8 *txbuf, u8 to_send) { - struct sc16is7xx_port *s = dev_get_drvdata(port->dev); struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
/* @@ -381,7 +379,7 @@ static void sc16is7xx_fifo_write(struct uart_port *port, u8 to_send) if (unlikely(!to_send)) return;
- regmap_noinc_write(one->regmap, SC16IS7XX_THR_REG, s->buf, to_send); + regmap_noinc_write(one->regmap, SC16IS7XX_THR_REG, txbuf, to_send); }
static void sc16is7xx_port_update(struct uart_port *port, u8 reg, @@ -583,7 +581,7 @@ static void sc16is7xx_handle_rx(struct uart_port *port, unsigned int rxlen, s->buf[0] = sc16is7xx_port_read(port, SC16IS7XX_RHR_REG); bytes_read = 1; } else { - sc16is7xx_fifo_read(port, rxlen); + sc16is7xx_fifo_read(port, s->buf, rxlen); bytes_read = rxlen; }
@@ -670,7 +668,7 @@ static void sc16is7xx_handle_tx(struct uart_port *port) xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); }
- sc16is7xx_fifo_write(port, to_send); + sc16is7xx_fifo_write(port, s->buf, to_send); }
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH 2/4] serial: sc16is7xx: refactor FIFO access functions to increase commonality Link: https://lore.kernel.org/stable/20241216191818.1553557-3-hugo%40hugovil.com
From: Hugo Villeneuve hvilleneuve@dimonoff.com
Sometimes, when a packet is received on channel A at almost the same time as a packet is about to be transmitted on channel B, we observe with a logic analyzer that the received packet on channel A is transmitted on channel B. In other words, the Tx buffer data on channel B is corrupted with data from channel A.
The problem appeared since commit 4409df5866b7 ("serial: sc16is7xx: change EFR lock to operate on each channels"), which changed the EFR locking to operate on each channel instead of chip-wise.
This commit has introduced a regression, because the EFR lock is used not only to protect the EFR registers access, but also, in a very obscure and undocumented way, to protect access to the data buffer, which is shared by the Tx and Rx handlers, but also by each channel of the IC.
Fix this regression first by switching to kfifo_out_linear_ptr() in sc16is7xx_handle_tx() to eliminate the need for a shared Rx/Tx buffer.
Secondly, replace the chip-wise Rx buffer with a separate Rx buffer for each channel.
Reworked to fix conflicts when backporting to linux-5.15.y.
Fixes: 4409df5866b7 ("serial: sc16is7xx: change EFR lock to operate on each channels") Cc: stable@vger.kernel.org Signed-off-by: Hugo Villeneuve hvilleneuve@dimonoff.com Link: https://lore.kernel.org/r/20240723125302.1305372-2-hugo@hugovil.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- drivers/tty/serial/sc16is7xx.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c index 591f97e78cdb7..928c701f0c35a 100644 --- a/drivers/tty/serial/sc16is7xx.c +++ b/drivers/tty/serial/sc16is7xx.c @@ -318,6 +318,7 @@ struct sc16is7xx_one { struct kthread_work tx_work; struct kthread_work reg_work; struct sc16is7xx_one_config config; + unsigned char buf[SC16IS7XX_FIFO_SIZE]; /* Rx buffer. */ bool irda_mode; };
@@ -327,7 +328,6 @@ struct sc16is7xx_port { #ifdef CONFIG_GPIOLIB struct gpio_chip gpio; #endif - unsigned char buf[SC16IS7XX_FIFO_SIZE]; struct kthread_worker kworker; struct task_struct *kworker_task; struct sc16is7xx_one p[]; @@ -555,17 +555,17 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud) static void sc16is7xx_handle_rx(struct uart_port *port, unsigned int rxlen, unsigned int iir) { - struct sc16is7xx_port *s = dev_get_drvdata(port->dev); + struct sc16is7xx_one *one = to_sc16is7xx_one(port, port); unsigned int lsr = 0, ch, flag, bytes_read, i; bool read_lsr = (iir == SC16IS7XX_IIR_RLSE_SRC) ? true : false;
- if (unlikely(rxlen >= sizeof(s->buf))) { + if (unlikely(rxlen >= sizeof(one->buf))) { dev_warn_ratelimited(port->dev, "ttySC%i: Possible RX FIFO overrun: %d\n", port->line, rxlen); port->icount.buf_overrun++; /* Ensure sanity of RX level */ - rxlen = sizeof(s->buf); + rxlen = sizeof(one->buf); }
while (rxlen) { @@ -578,10 +578,10 @@ static void sc16is7xx_handle_rx(struct uart_port *port, unsigned int rxlen, lsr = 0;
if (read_lsr) { - s->buf[0] = sc16is7xx_port_read(port, SC16IS7XX_RHR_REG); + one->buf[0] = sc16is7xx_port_read(port, SC16IS7XX_RHR_REG); bytes_read = 1; } else { - sc16is7xx_fifo_read(port, s->buf, rxlen); + sc16is7xx_fifo_read(port, one->buf, rxlen); bytes_read = rxlen; }
@@ -614,7 +614,7 @@ static void sc16is7xx_handle_rx(struct uart_port *port, unsigned int rxlen, }
for (i = 0; i < bytes_read; ++i) { - ch = s->buf[i]; + ch = one->buf[i]; if (uart_handle_sysrq_char(port, ch)) continue;
@@ -632,7 +632,7 @@ static void sc16is7xx_handle_rx(struct uart_port *port, unsigned int rxlen,
static void sc16is7xx_handle_tx(struct uart_port *port) { - struct sc16is7xx_port *s = dev_get_drvdata(port->dev); + struct sc16is7xx_one *one = to_sc16is7xx_one(port, port); struct circ_buf *xmit = &port->state->xmit; unsigned int txlen, to_send, i;
@@ -664,11 +664,11 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
/* Convert to linear buffer */ for (i = 0; i < to_send; ++i) { - s->buf[i] = xmit->buf[xmit->tail]; + one->buf[i] = xmit->buf[xmit->tail]; xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); }
- sc16is7xx_fifo_write(port, s->buf, to_send); + sc16is7xx_fifo_write(port, one->buf, to_send); }
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
From: Hugo Villeneuve hvilleneuve@dimonoff.com
When enabling access to the special register set, Receiver time-out and RHR interrupts can happen. In this case, the IRQ handler will try to read from the FIFO thru the RHR register at address 0x00, but address 0x00 is mapped to DLL register, resulting in erroneous FIFO reading.
Call graph example: sc16is7xx_startup(): entry sc16is7xx_ms_proc(): entry sc16is7xx_set_termios(): entry sc16is7xx_set_baud(): DLH/DLL = $009C --> access special register set sc16is7xx_port_irq() entry --> IIR is 0x0C sc16is7xx_handle_rx() entry sc16is7xx_fifo_read(): --> unable to access FIFO (RHR) because it is mapped to DLL (LCR=LCR_CONF_MODE_A) sc16is7xx_set_baud(): exit --> Restore access to general register set
Fix the problem by claiming the efr_lock mutex when accessing the Special register set.
Reworked to fix conflicts when backporting to linux-5.15.y.
Fixes: dfeae619d781 ("serial: sc16is7xx") Cc: stable@vger.kernel.org Signed-off-by: Hugo Villeneuve hvilleneuve@dimonoff.com Link: https://lore.kernel.org/r/20240723125302.1305372-3-hugo@hugovil.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- drivers/tty/serial/sc16is7xx.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c index 928c701f0c35a..3b7c238793ef4 100644 --- a/drivers/tty/serial/sc16is7xx.c +++ b/drivers/tty/serial/sc16is7xx.c @@ -537,6 +537,8 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud) prescaler == 1 ? 0 : SC16IS7XX_MCR_CLKSEL_BIT);
/* Open the LCR divisors for configuration */ + mutex_lock(&one->efr_lock); + sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, SC16IS7XX_LCR_CONF_MODE_A);
@@ -549,6 +551,8 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud) /* Put LCR back to the normal mode */ sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
+ mutex_unlock(&one->efr_lock); + return DIV_ROUND_CLOSEST((clk / prescaler) / 16, div); }
linux-stable-mirror@lists.linaro.org