[PATCH 01/18] serial: sc16is7xx: fix segfault when removing driver

[PATCH 01/18] serial: sc16is7xx: fix segfault when removing driver

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

If a problem with a device occurs during probing, and we subsequently
try to remove the driver, we get the following error:

$ rmmod sc16is7xx
[   62.783166] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000040
[   62.994226] Call trace:
[   62.996672]  serial_core_unregister_port+0x58/0x2b0
[   63.001558]  serial_ctrl_unregister_port+0x18/0x30
[   63.006354]  uart_remove_one_port+0x18/0x30
[   63.010542]  sc16is7xx_spi_remove+0xc0/0x190 [sc16is7xx]
Segmentation fault

Tested on a custom board with two SC16IS742 ICs, and by simulating a fault
when probing channel (port) B of the second device.

The reason is that uart_remove_one_port() has already been called during
probe in the out_ports: error path, and is called again in
sc16is7xx_remove().

Fix the problem by clearing the device drvdata in probe error path to
indicate that all resources have been deallocated. And only deallocate
resources in sc16is7xx_remove() if device drvdata is non-null.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index e40e4a99277e..b585663c1e6e 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1663,6 +1663,8 @@ static int sc16is7xx_probe(struct device *dev,
 out_clk:
 	clk_disable_unprepare(s->clk);
 
+	dev_set_drvdata(dev, NULL);
+
 	return ret;
 }
 
@@ -1671,6 +1673,9 @@ static void sc16is7xx_remove(struct device *dev)
 	struct sc16is7xx_port *s = dev_get_drvdata(dev);
 	int i;
 
+	if (!s)
+		return;
+
 #ifdef CONFIG_GPIOLIB
 	if (s->gpio_valid_mask)
 		gpiochip_remove(&s->gpio);
-- 
2.39.2

[PATCH 02/18] serial: sc16is7xx: fix invalid sc16is7xx_lines bitfield in case of probe error

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

If an error occurs during probing, the sc16is7xx_lines bitfield may be left
in a state that doesn't represent the correct state of lines allocation.

For example, in a system with two SC16 devices, if an error occurs only
during probing of channel (port) B of the second device, sc16is7xx_lines
final state will be 00001011b instead of the expected 00000011b.

This is caused in part because of the "i--" in the for/loop located in
the out_ports: error path.

Fix this by checking the return value of uart_add_one_port() and set line
allocation bit only if this was successful. This allows the refactor of
the obfuscated for(i--...) loop in the error path, and properly call
uart_remove_one_port() only when needed, and properly unset line allocation
bits.

Also use same mechanism in remove() when calling uart_remove_one_port().

Fixes: c64349722d14 ("sc16is7xx: support multiple devices")
Cc: stable@vger.kernel.org
Cc: Yury Norov <yury.norov@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
There is already a patch by Yury Norov <yury.norov@gmail.com> to simplify
sc16is7xx_alloc_line():
https://lore.kernel.org/all/20231212022749.625238-30-yury.norov@gmail.com/

Since my patch gets rid of sc16is7xx_alloc_line() entirely, it would make
Yury's patch unnecessary.
---
 drivers/tty/serial/sc16is7xx.c | 44 ++++++++++++++--------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index b585663c1e6e..b92fd01cfeec 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -407,19 +407,6 @@ static void sc16is7xx_port_update(struct uart_port *port, u8 reg,
 	regmap_update_bits(one->regmap, reg, mask, val);
 }
 
-static int sc16is7xx_alloc_line(void)
-{
-	int i;
-
-	BUILD_BUG_ON(SC16IS7XX_MAX_DEVS > BITS_PER_LONG);
-
-	for (i = 0; i < SC16IS7XX_MAX_DEVS; i++)
-		if (!test_and_set_bit(i, &sc16is7xx_lines))
-			break;
-
-	return i;
-}
-
 static void sc16is7xx_power(struct uart_port *port, int on)
 {
 	sc16is7xx_port_update(port, SC16IS7XX_IER_REG,
@@ -1550,6 +1537,13 @@ static int sc16is7xx_probe(struct device *dev,
 		     SC16IS7XX_IOCONTROL_SRESET_BIT);
 
 	for (i = 0; i < devtype->nr_uart; ++i) {
+		s->p[i].port.line = find_first_zero_bit(&sc16is7xx_lines,
+							SC16IS7XX_MAX_DEVS);
+		if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
+			ret = -ERANGE;
+			goto out_ports;
+		}
+
 		/* Initialize port data */
 		s->p[i].port.dev	= dev;
 		s->p[i].port.irq	= irq;
@@ -1569,14 +1563,8 @@ static int sc16is7xx_probe(struct device *dev,
 		s->p[i].port.rs485_supported = sc16is7xx_rs485_supported;
 		s->p[i].port.ops	= &sc16is7xx_ops;
 		s->p[i].old_mctrl	= 0;
-		s->p[i].port.line	= sc16is7xx_alloc_line();
 		s->p[i].regmap		= regmaps[i];
 
-		if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
-			ret = -ENOMEM;
-			goto out_ports;
-		}
-
 		mutex_init(&s->p[i].efr_lock);
 
 		ret = uart_get_rs485_mode(&s->p[i].port);
@@ -1594,8 +1582,13 @@ static int sc16is7xx_probe(struct device *dev,
 		kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
 		kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
 		kthread_init_delayed_work(&s->p[i].ms_work, sc16is7xx_ms_proc);
+
 		/* Register port */
-		uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
+		ret = uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
+		if (ret)
+			goto out_ports;
+
+		set_bit(s->p[i].port.line, &sc16is7xx_lines);
 
 		/* Enable EFR */
 		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
@@ -1653,10 +1646,9 @@ static int sc16is7xx_probe(struct device *dev,
 #endif
 
 out_ports:
-	for (i--; i >= 0; i--) {
-		uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
-		clear_bit(s->p[i].port.line, &sc16is7xx_lines);
-	}
+	for (i = 0; i < devtype->nr_uart; i++)
+		if (test_and_clear_bit(s->p[i].port.line, &sc16is7xx_lines))
+			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
 
 	kthread_stop(s->kworker_task);
 
@@ -1683,8 +1675,8 @@ static void sc16is7xx_remove(struct device *dev)
 
 	for (i = 0; i < s->devtype->nr_uart; i++) {
 		kthread_cancel_delayed_work_sync(&s->p[i].ms_work);
-		uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
-		clear_bit(s->p[i].port.line, &sc16is7xx_lines);
+		if (test_and_clear_bit(s->p[i].port.line, &sc16is7xx_lines))
+			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
 		sc16is7xx_power(&s->p[i].port, 0);
 	}
 
-- 
2.39.2

[PATCH 03/18] serial: sc16is7xx: remove obsolete loop in sc16is7xx_port_irq()

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Commit 834449872105 ("sc16is7xx: Fix for multi-channel stall") changed
sc16is7xx_port_irq() from looping multiple times when there was still
interrupts to serve. It simply changed the do {} while(1) loop to a
do {} while(0) loop, which makes the loop itself now obsolete.

Clean the code by removing this obsolete do {} while(0) loop.

Fixes: 834449872105 ("sc16is7xx: Fix for multi-channel stall")
Cc: stable@vger.kernel.org
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 85 ++++++++++++++++------------------
 1 file changed, 41 insertions(+), 44 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index b92fd01cfeec..b2d0f6d307bd 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -724,58 +724,55 @@ static void sc16is7xx_update_mlines(struct sc16is7xx_one *one)
 static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
 {
 	bool rc = true;
+	unsigned int iir, rxlen;
 	struct uart_port *port = &s->p[portno].port;
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 
 	mutex_lock(&one->efr_lock);
 
-	do {
-		unsigned int iir, rxlen;
+	iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
+	if (iir & SC16IS7XX_IIR_NO_INT_BIT) {
+		rc = false;
+		goto out_port_irq;
+	}
 
-		iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
-		if (iir & SC16IS7XX_IIR_NO_INT_BIT) {
-			rc = false;
-			goto out_port_irq;
-		}
+	iir &= SC16IS7XX_IIR_ID_MASK;
 
-		iir &= SC16IS7XX_IIR_ID_MASK;
-
-		switch (iir) {
-		case SC16IS7XX_IIR_RDI_SRC:
-		case SC16IS7XX_IIR_RLSE_SRC:
-		case SC16IS7XX_IIR_RTOI_SRC:
-		case SC16IS7XX_IIR_XOFFI_SRC:
-			rxlen = sc16is7xx_port_read(port, SC16IS7XX_RXLVL_REG);
-
-			/*
-			 * There is a silicon bug that makes the chip report a
-			 * time-out interrupt but no data in the FIFO. This is
-			 * described in errata section 18.1.4.
-			 *
-			 * When this happens, read one byte from the FIFO to
-			 * clear the interrupt.
-			 */
-			if (iir == SC16IS7XX_IIR_RTOI_SRC && !rxlen)
-				rxlen = 1;
-
-			if (rxlen)
-				sc16is7xx_handle_rx(port, rxlen, iir);
-			break;
+	switch (iir) {
+	case SC16IS7XX_IIR_RDI_SRC:
+	case SC16IS7XX_IIR_RLSE_SRC:
+	case SC16IS7XX_IIR_RTOI_SRC:
+	case SC16IS7XX_IIR_XOFFI_SRC:
+		rxlen = sc16is7xx_port_read(port, SC16IS7XX_RXLVL_REG);
+
+		/*
+		 * There is a silicon bug that makes the chip report a
+		 * time-out interrupt but no data in the FIFO. This is
+		 * described in errata section 18.1.4.
+		 *
+		 * When this happens, read one byte from the FIFO to
+		 * clear the interrupt.
+		 */
+		if (iir == SC16IS7XX_IIR_RTOI_SRC && !rxlen)
+			rxlen = 1;
+
+		if (rxlen)
+			sc16is7xx_handle_rx(port, rxlen, iir);
+		break;
 		/* CTSRTS interrupt comes only when CTS goes inactive */
-		case SC16IS7XX_IIR_CTSRTS_SRC:
-		case SC16IS7XX_IIR_MSI_SRC:
-			sc16is7xx_update_mlines(one);
-			break;
-		case SC16IS7XX_IIR_THRI_SRC:
-			sc16is7xx_handle_tx(port);
-			break;
-		default:
-			dev_err_ratelimited(port->dev,
-					    "ttySC%i: Unexpected interrupt: %x",
-					    port->line, iir);
-			break;
-		}
-	} while (0);
+	case SC16IS7XX_IIR_CTSRTS_SRC:
+	case SC16IS7XX_IIR_MSI_SRC:
+		sc16is7xx_update_mlines(one);
+		break;
+	case SC16IS7XX_IIR_THRI_SRC:
+		sc16is7xx_handle_tx(port);
+		break;
+	default:
+		dev_err_ratelimited(port->dev,
+				    "ttySC%i: Unexpected interrupt: %x",
+				    port->line, iir);
+		break;
+	}
 
 out_port_irq:
 	mutex_unlock(&one->efr_lock);
-- 
2.39.2

[PATCH 04/18] serial: sc16is7xx: improve do/while loop in sc16is7xx_irq()

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Simplify and improve readability by replacing while(1) loop with
do {} while, and by using the keep_polling variable as the exit
condition, making it more explicit.

Fixes: 834449872105 ("sc16is7xx: Fix for multi-channel stall")
Cc: stable@vger.kernel.org
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index b2d0f6d307bd..8a038a9be09e 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -782,17 +782,17 @@ static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
 
 static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
 {
+	bool keep_polling;
+
 	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
 
-	while (1) {
-		bool keep_polling = false;
+	do {
+		keep_polling = false;
 		int i;
 
 		for (i = 0; i < s->devtype->nr_uart; ++i)
 			keep_polling |= sc16is7xx_port_irq(s, i);
-		if (!keep_polling)
-			break;
-	}
+	} while (keep_polling);
 
 	return IRQ_HANDLED;
 }
-- 
2.39.2

[PATCH 15/18] serial: sc16is7xx: pass R/W buffer in FIFO functions

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

To simplify function by avoiding cast.

This is similar to what is done in max310x driver.

Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
If deemed appropriate for stable kernel backporting:
Fixes: dec273ecc116 ("sc16is7xx: fix FIFO address of secondary UART")
Cc: stable@vger.kernel.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 a9e44e5ef713..3322507ab18e 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -380,17 +380,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);
 
 	/*
@@ -400,7 +398,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,
@@ -576,7 +574,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;
 		}
 
@@ -665,7 +663,7 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
 			uart_xmit_advance(port, 1);
 		}
 
-		sc16is7xx_fifo_write(port, to_send);
+		sc16is7xx_fifo_write(port, s->buf, to_send);
 	}
 
 	uart_port_lock_irqsave(port, &flags);
-- 
2.39.2

Re: [PATCH 01/18] serial: sc16is7xx: fix segfault when removing driver

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:45PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> If a problem with a device occurs during probing, and we subsequently
> try to remove the driver, we get the following error:
> 
> $ rmmod sc16is7xx
> [   62.783166] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000040
> [   62.994226] Call trace:
> [   62.996672]  serial_core_unregister_port+0x58/0x2b0
> [   63.001558]  serial_ctrl_unregister_port+0x18/0x30
> [   63.006354]  uart_remove_one_port+0x18/0x30
> [   63.010542]  sc16is7xx_spi_remove+0xc0/0x190 [sc16is7xx]
> Segmentation fault
> 
> Tested on a custom board with two SC16IS742 ICs, and by simulating a fault
> when probing channel (port) B of the second device.
> 
> The reason is that uart_remove_one_port() has already been called during
> probe in the out_ports: error path, and is called again in
> sc16is7xx_remove().
> 
> Fix the problem by clearing the device drvdata in probe error path to
> indicate that all resources have been deallocated. And only deallocate
> resources in sc16is7xx_remove() if device drvdata is non-null.

...

> +	dev_set_drvdata(dev, NULL);

I believe this is wrong approach to fix the issue as this one is prone
to be cleaned up in the future as we don't do this call explicitly for
the past ~15 years.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 01/18] serial: sc16is7xx: fix segfault when removing driver

[Andy Shevchenko]

On Wed, Dec 20, 2023 at 05:34:29PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 19, 2023 at 12:18:45PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

...

> > +	dev_set_drvdata(dev, NULL);
> 
> I believe this is wrong approach to fix the issue as this one is prone
> to be cleaned up in the future as we don't do this call explicitly for
> the past ~15 years.

On top of that the ->remove() is not the only uart_remove_one_port() call.
It has a lot of other stuff to go with.

It seems that ->remove() doesn't check the bit in &sc16is7xx_lines, that
might be the proper fix for the issue you have.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 02/18] serial: sc16is7xx: fix invalid sc16is7xx_lines bitfield in case of probe error

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:46PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> If an error occurs during probing, the sc16is7xx_lines bitfield may be left
> in a state that doesn't represent the correct state of lines allocation.
> 
> For example, in a system with two SC16 devices, if an error occurs only
> during probing of channel (port) B of the second device, sc16is7xx_lines
> final state will be 00001011b instead of the expected 00000011b.
> 
> This is caused in part because of the "i--" in the for/loop located in
> the out_ports: error path.
> 
> Fix this by checking the return value of uart_add_one_port() and set line
> allocation bit only if this was successful. This allows the refactor of
> the obfuscated for(i--...) loop in the error path, and properly call
> uart_remove_one_port() only when needed, and properly unset line allocation
> bits.
> 
> Also use same mechanism in remove() when calling uart_remove_one_port().

Yes, this seems to be the correct one to fix the problem described in
the patch 1. I dunno why the patch 1 even exists.

As for Yury's patch, you are doing fixes, so your stuff has priority on his.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/18] serial: sc16is7xx: remove obsolete loop in sc16is7xx_port_irq()

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:47PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Commit 834449872105 ("sc16is7xx: Fix for multi-channel stall") changed
> sc16is7xx_port_irq() from looping multiple times when there was still
> interrupts to serve. It simply changed the do {} while(1) loop to a
> do {} while(0) loop, which makes the loop itself now obsolete.
> 
> Clean the code by removing this obsolete do {} while(0) loop.

I'm just wondering if you used --histogram diff algo when prepared the patches.
If no, use that by default.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 04/18] serial: sc16is7xx: improve do/while loop in sc16is7xx_irq()

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:48PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Simplify and improve readability by replacing while(1) loop with
> do {} while, and by using the keep_polling variable as the exit
> condition, making it more explicit.

...

> +	bool keep_polling;

> +

Stray blank line. Otherwise LGTM.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 15/18] serial: sc16is7xx: pass R/W buffer in FIFO functions

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:59PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> To simplify function by avoiding cast.
> 
> This is similar to what is done in max310x driver.

...

> ---
> If deemed appropriate for stable kernel backporting:

I don't think it's eligible.

> ---

I don't see the necessity of the change, OTOH it's harmless.
The problem is that commit message is basically "Yeah, we
do cargo cult." Because I haven't seen what casting you are
talking about.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 04/18] serial: sc16is7xx: improve do/while loop in sc16is7xx_irq()

[Hugo Villeneuve]

On Wed, 20 Dec 2023 17:42:42 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Dec 19, 2023 at 12:18:48PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Simplify and improve readability by replacing while(1) loop with
> > do {} while, and by using the keep_polling variable as the exit
> > condition, making it more explicit.
> 
> ...
> 
> > +	bool keep_polling;
> 
> > +
> 
> Stray blank line. Otherwise LGTM.

Yes, and I just realized I should also change:

    do {
        keep_polling = false;
        int i;
        ...

to:

    do {
        int i;

        keep_polling = false;
        ...

Hugo Villeneuve

Re: [PATCH 03/18] serial: sc16is7xx: remove obsolete loop in sc16is7xx_port_irq()

[Hugo Villeneuve]

On Wed, 20 Dec 2023 17:41:31 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Dec 19, 2023 at 12:18:47PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Commit 834449872105 ("sc16is7xx: Fix for multi-channel stall") changed
> > sc16is7xx_port_irq() from looping multiple times when there was still
> > interrupts to serve. It simply changed the do {} while(1) loop to a
> > do {} while(0) loop, which makes the loop itself now obsolete.
> > 
> > Clean the code by removing this obsolete do {} while(0) loop.
> 
> I'm just wondering if you used --histogram diff algo when prepared the patches.
> If no, use that by default.

Hi,
I did not, and effectively it makes the patch easier to follow.

I will use it as default from now on.

Hugo Villeneuve

Re: [PATCH 02/18] serial: sc16is7xx: fix invalid sc16is7xx_lines bitfield in case of probe error

[Hugo Villeneuve]

On Wed, 20 Dec 2023 17:40:42 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Dec 19, 2023 at 12:18:46PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > If an error occurs during probing, the sc16is7xx_lines bitfield may be left
> > in a state that doesn't represent the correct state of lines allocation.
> > 
> > For example, in a system with two SC16 devices, if an error occurs only
> > during probing of channel (port) B of the second device, sc16is7xx_lines
> > final state will be 00001011b instead of the expected 00000011b.
> > 
> > This is caused in part because of the "i--" in the for/loop located in
> > the out_ports: error path.
> > 
> > Fix this by checking the return value of uart_add_one_port() and set line
> > allocation bit only if this was successful. This allows the refactor of
> > the obfuscated for(i--...) loop in the error path, and properly call
> > uart_remove_one_port() only when needed, and properly unset line allocation
> > bits.
> > 
> > Also use same mechanism in remove() when calling uart_remove_one_port().
> 
> Yes, this seems to be the correct one to fix the problem described in
> the patch 1. I dunno why the patch 1 even exists.

Hi,
this will indeed fix the problem described in patch 1.

However, if I remove patch 1, and I simulate the same probe error as
described in patch 1, now we get stuck forever when trying to 
remove the driver. This is something that I observed before and
that patch 1 also corrected.

The problem is caused in sc16is7xx_remove() when calling this function

    kthread_flush_worker(&s->kworker);

I am not sure how best to handle that without patch 1.

Hugo Villeneuve

Re: [PATCH 02/18] serial: sc16is7xx: fix invalid sc16is7xx_lines bitfield in case of probe error

[Andy Shevchenko]

On Thu, Dec 21, 2023 at 10:56:39AM -0500, Hugo Villeneuve wrote:
> On Wed, 20 Dec 2023 17:40:42 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Tue, Dec 19, 2023 at 12:18:46PM -0500, Hugo Villeneuve wrote:

...

> > Yes, this seems to be the correct one to fix the problem described in
> > the patch 1. I dunno why the patch 1 even exists.
> 
> Hi,
> this will indeed fix the problem described in patch 1.
> 
> However, if I remove patch 1, and I simulate the same probe error as
> described in patch 1, now we get stuck forever when trying to 
> remove the driver. This is something that I observed before and
> that patch 1 also corrected.
> 
> The problem is caused in sc16is7xx_remove() when calling this function
> 
>     kthread_flush_worker(&s->kworker);
> 
> I am not sure how best to handle that without patch 1.

So, it means we need to root cause this issue. Because patch 1 looks
really bogus.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 02/18] serial: sc16is7xx: fix invalid sc16is7xx_lines bitfield in case of probe error

[Hugo Villeneuve]

On Thu, 21 Dec 2023 10:56:39 -0500
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Wed, 20 Dec 2023 17:40:42 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> 
> > On Tue, Dec 19, 2023 at 12:18:46PM -0500, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > 
> > > If an error occurs during probing, the sc16is7xx_lines bitfield may be left
> > > in a state that doesn't represent the correct state of lines allocation.
> > > 
> > > For example, in a system with two SC16 devices, if an error occurs only
> > > during probing of channel (port) B of the second device, sc16is7xx_lines
> > > final state will be 00001011b instead of the expected 00000011b.
> > > 
> > > This is caused in part because of the "i--" in the for/loop located in
> > > the out_ports: error path.
> > > 
> > > Fix this by checking the return value of uart_add_one_port() and set line
> > > allocation bit only if this was successful. This allows the refactor of
> > > the obfuscated for(i--...) loop in the error path, and properly call
> > > uart_remove_one_port() only when needed, and properly unset line allocation
> > > bits.
> > > 
> > > Also use same mechanism in remove() when calling uart_remove_one_port().
> > 
> > Yes, this seems to be the correct one to fix the problem described in
> > the patch 1. I dunno why the patch 1 even exists.
> 
> Hi,
> this will indeed fix the problem described in patch 1.
> 
> However, if I remove patch 1, and I simulate the same probe error as
> described in patch 1, now we get stuck forever when trying to 
> remove the driver. This is something that I observed before and
> that patch 1 also corrected.
> 
> The problem is caused in sc16is7xx_remove() when calling this function
> 
>     kthread_flush_worker(&s->kworker);
> 
> I am not sure how best to handle that without patch 1.

Also, if we manage to get past kthread_flush_worker() and 
kthread_stop() (commented out for testing purposes), we get another bug:

# rmmod sc16is7xx
...
crystal-duart-24m already disabled
WARNING: CPU: 2 PID: 340 at drivers/clk/clk.c:1090
clk_core_disable+0x1b0/0x1e0
...
Call trace:
clk_core_disable+0x1b0/0x1e0
clk_disable+0x38/0x60
sc16is7xx_remove+0x1e4/0x240 [sc16is7xx]

This one is caused by calling clk_disable_unprepare(). But
clk_disable_unprepare() has already been called in probe error handling
code. Patch 1 also fixed this...

Hugo Villeneuve

Re: [PATCH 02/18] serial: sc16is7xx: fix invalid sc16is7xx_lines bitfield in case of probe error

[Andy Shevchenko]

On Thu, Dec 21, 2023 at 11:13:37AM -0500, Hugo Villeneuve wrote:
> On Thu, 21 Dec 2023 10:56:39 -0500
> Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Wed, 20 Dec 2023 17:40:42 +0200
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

...

> > this will indeed fix the problem described in patch 1.
> > 
> > However, if I remove patch 1, and I simulate the same probe error as
> > described in patch 1, now we get stuck forever when trying to 
> > remove the driver. This is something that I observed before and
> > that patch 1 also corrected.
> > 
> > The problem is caused in sc16is7xx_remove() when calling this function
> > 
> >     kthread_flush_worker(&s->kworker);
> > 
> > I am not sure how best to handle that without patch 1.
> 
> Also, if we manage to get past kthread_flush_worker() and 
> kthread_stop() (commented out for testing purposes), we get another bug:
> 
> # rmmod sc16is7xx
> ...
> crystal-duart-24m already disabled
> WARNING: CPU: 2 PID: 340 at drivers/clk/clk.c:1090
> clk_core_disable+0x1b0/0x1e0
> ...
> Call trace:
> clk_core_disable+0x1b0/0x1e0
> clk_disable+0x38/0x60
> sc16is7xx_remove+0x1e4/0x240 [sc16is7xx]
> 
> This one is caused by calling clk_disable_unprepare(). But
> clk_disable_unprepare() has already been called in probe error handling
> code. Patch 1 also fixed this...

Word "fixed" is incorrect. "Papered over" is what it did.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 02/18] serial: sc16is7xx: fix invalid sc16is7xx_lines bitfield in case of probe error

[Hugo Villeneuve]

On Thu, 21 Dec 2023 18:16:40 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Thu, Dec 21, 2023 at 11:13:37AM -0500, Hugo Villeneuve wrote:
> > On Thu, 21 Dec 2023 10:56:39 -0500
> > Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > On Wed, 20 Dec 2023 17:40:42 +0200
> > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> 
> ...
> 
> > > this will indeed fix the problem described in patch 1.
> > > 
> > > However, if I remove patch 1, and I simulate the same probe error as
> > > described in patch 1, now we get stuck forever when trying to 
> > > remove the driver. This is something that I observed before and
> > > that patch 1 also corrected.
> > > 
> > > The problem is caused in sc16is7xx_remove() when calling this function
> > > 
> > >     kthread_flush_worker(&s->kworker);
> > > 
> > > I am not sure how best to handle that without patch 1.
> > 
> > Also, if we manage to get past kthread_flush_worker() and 
> > kthread_stop() (commented out for testing purposes), we get another bug:
> > 
> > # rmmod sc16is7xx
> > ...
> > crystal-duart-24m already disabled
> > WARNING: CPU: 2 PID: 340 at drivers/clk/clk.c:1090
> > clk_core_disable+0x1b0/0x1e0
> > ...
> > Call trace:
> > clk_core_disable+0x1b0/0x1e0
> > clk_disable+0x38/0x60
> > sc16is7xx_remove+0x1e4/0x240 [sc16is7xx]
> > 
> > This one is caused by calling clk_disable_unprepare(). But
> > clk_disable_unprepare() has already been called in probe error handling
> > code. Patch 1 also fixed this...
> 
> Word "fixed" is incorrect. "Papered over" is what it did.

Hi,
I just found the problem, and it was in my bug simulation, not the
driver itself. When I simulated the bug, I forgot to set "ret" to an
error code, and thus I returned 0 at the end of sc16is7xx_probe(). This
is why sc16is7xx_remove() was called when unloading driver, but
shouldn't have.

If I simulate my probe error and return "-EINVAL" at the end of
sc16is7xx_probe(), sc16is7xx_remove() is not called when
unloading the driver.

Sorry for the noise, so I will drop patch 1 and leave patch "fix invalid
sc16is7xx_lines bitfield in case of probe error" as it is, and
simply remove comments about Yury's patch.

Hugo.

Re: [PATCH 15/18] serial: sc16is7xx: pass R/W buffer in FIFO functions

[Hugo Villeneuve]

On Wed, 20 Dec 2023 17:57:59 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Dec 19, 2023 at 12:18:59PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > To simplify function by avoiding cast.
> > 
> > This is similar to what is done in max310x driver.
> 
> ...
> 
> > ---
> > If deemed appropriate for stable kernel backporting:
> 
> I don't think it's eligible.

No problem.


> > ---
> 
> I don't see the necessity of the change, OTOH it's harmless.
> The problem is that commit message is basically "Yeah, we
> do cargo cult." Because I haven't seen what casting you are
> talking about.

I'll try to reword the commit message.

And replace cast with something like "... to remove additional struct
sc16is7xx_port variable..."

Hugo Villeneuve