[PATCH 0/4] rs485 bus termination GPIO

[PATCH 0/4] rs485 bus termination GPIO

[Lukas Wunner]

Commit e8759ad17d41 ("serial: uapi: Add support for bus termination"),
allowed user space to change rs485 bus termination through a flag in
struct serial_rs485.  But so far only a single driver, 8250_exar.c,
supports the flag:  It hardcodes a GPIO specific to Siemens IOT2040
products.

Provide for a more generic solution:  Define a device tree binding
for an rs485 bus termination GPIO (patch [3/4]), amend the serial core
to retrieve the GPIO from the device tree and amend the default
->rs485_config() callback for 8250 drivers to change the GPIO on
request from user space (patch [4/4]).

Retrieving the GPIO from the device tree may fail, so allow
uart_get_rs485_mode() to return an errno and change all callers
to check for failure (patch [2/4]).

Testing has exposed a bug in the 8250 core if retrieval of the GPIO
initially fails with -EPROBE_DEFER and is later retried.  That bug
is fixed by patch [1/4].


Lukas Wunner (4):
  serial: 8250: Avoid error message on reprobe
  serial: Allow uart_get_rs485_mode() to return errno
  dt-bindings: serial: Add binding for rs485 bus termination GPIO
  serial: 8250: Support rs485 bus termination GPIO

 .../devicetree/bindings/serial/rs485.yaml     |  4 +++
 drivers/tty/serial/8250/8250_core.c           | 16 +++++++---
 drivers/tty/serial/8250/8250_port.c           |  4 +++
 drivers/tty/serial/ar933x_uart.c              |  6 ++--
 drivers/tty/serial/atmel_serial.c             |  6 ++--
 drivers/tty/serial/fsl_lpuart.c               |  5 +++-
 drivers/tty/serial/imx.c                      |  6 +++-
 drivers/tty/serial/omap-serial.c              |  4 ++-
 drivers/tty/serial/serial_core.c              | 30 ++++++++++++++++++-
 drivers/tty/serial/stm32-usart.c              |  8 ++---
 include/linux/serial_core.h                   |  4 ++-
 11 files changed, 76 insertions(+), 17 deletions(-)

-- 
2.26.2

[PATCH 1/4] serial: 8250: Avoid error message on reprobe

[Lukas Wunner]

If the call to uart_add_one_port() in serial8250_register_8250_port()
fails, a half-initialized entry in the serial_8250ports[] array is left
behind.

A subsequent reprobe of the same serial port causes that entry to be
reused.  Because uart->port.dev is set, uart_remove_one_port() is called
for the half-initialized entry and bails out with an error message:

bcm2835-aux-uart 3f215040.serial: Removing wrong port: (null) != (ptrval)

The same happens on failure of mctrl_gpio_init() since commit
4a96895f74c9 ("tty/serial/8250: use mctrl_gpio helpers").

Fix by zeroing the uart->port.dev pointer in the probe error path.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v2.6.10+
---
 drivers/tty/serial/8250/8250_core.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 45d9117cab68..14abade589da 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1040,7 +1040,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 			gpios = mctrl_gpio_init(&uart->port, 0);
 			if (IS_ERR(gpios)) {
 				ret = PTR_ERR(gpios);
-				goto out_unlock;
+				goto err;
 			} else {
 				uart->gpios = gpios;
 			}
@@ -1089,7 +1089,9 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 			serial8250_apply_quirks(uart);
 			ret = uart_add_one_port(&serial8250_reg,
 						&uart->port);
-			if (ret == 0)
+			if (ret)
+				goto err;
+			else
 				ret = uart->port.line;
 		} else {
 			dev_info(uart->port.dev,
@@ -1112,10 +1114,14 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 		}
 	}
 
-out_unlock:
 	mutex_unlock(&serial_mutex);
 
 	return ret;
+
+err:
+	uart->port.dev = NULL;
+	mutex_unlock(&serial_mutex);
+	return ret;
 }
 EXPORT_SYMBOL(serial8250_register_8250_port);
 
-- 
2.26.2

[PATCH 2/4] serial: Allow uart_get_rs485_mode() to return errno

[Lukas Wunner]

We're about to amend uart_get_rs485_mode() to support a GPIO pin for
rs485 bus termination.  Retrieving the GPIO descriptor may fail, so
allow uart_get_rs485_mode() to return an errno and change all callers
to check for failure.

The GPIO descriptor is going to be stored in struct uart_port.  Pass
that struct to uart_get_rs485_mode() in lieu of a struct device and
struct serial_rs485, both of which are directly accessible from struct
uart_port.

A few drivers call uart_get_rs485_mode() before setting the struct
device pointer in struct uart_port.  Shuffle those calls around where
necessary.

[Heiko Stuebner did the ar933x_uart.c portion, hence his Signed-off-by.]

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/tty/serial/8250/8250_core.c | 4 +++-
 drivers/tty/serial/ar933x_uart.c    | 6 ++++--
 drivers/tty/serial/atmel_serial.c   | 6 ++++--
 drivers/tty/serial/fsl_lpuart.c     | 5 ++++-
 drivers/tty/serial/imx.c            | 6 +++++-
 drivers/tty/serial/omap-serial.c    | 4 +++-
 drivers/tty/serial/serial_core.c    | 6 +++++-
 drivers/tty/serial/stm32-usart.c    | 8 ++++----
 include/linux/serial_core.h         | 2 +-
 9 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 14abade589da..43121973ce02 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1026,7 +1026,9 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 
 		if (up->port.dev) {
 			uart->port.dev = up->port.dev;
-			uart_get_rs485_mode(uart->port.dev, &uart->port.rs485);
+			ret = uart_get_rs485_mode(&uart->port);
+			if (ret)
+				goto err;
 		}
 
 		if (up->port.flags & UPF_FIXED_TYPE)
diff --git a/drivers/tty/serial/ar933x_uart.c b/drivers/tty/serial/ar933x_uart.c
index 7e7f1398019f..0c80a79d7442 100644
--- a/drivers/tty/serial/ar933x_uart.c
+++ b/drivers/tty/serial/ar933x_uart.c
@@ -766,8 +766,6 @@ static int ar933x_uart_probe(struct platform_device *pdev)
 		goto err_disable_clk;
 	}
 
-	uart_get_rs485_mode(&pdev->dev, &port->rs485);
-
 	port->mapbase = mem_res->start;
 	port->line = id;
 	port->irq = irq_res->start;
@@ -786,6 +784,10 @@ static int ar933x_uart_probe(struct platform_device *pdev)
 	baud = ar933x_uart_get_baud(port->uartclk, 0, AR933X_UART_MAX_STEP);
 	up->max_baud = min_t(unsigned int, baud, AR933X_UART_MAX_BAUD);
 
+	ret = uart_get_rs485_mode(port);
+	if (ret)
+		goto err_disable_clk;
+
 	up->gpios = mctrl_gpio_init(port, 0);
 	if (IS_ERR(up->gpios) && PTR_ERR(up->gpios) != -ENOSYS)
 		return PTR_ERR(up->gpios);
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 8d7080efad9b..e43471b33710 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2491,8 +2491,6 @@ static int atmel_init_port(struct atmel_uart_port *atmel_port,
 	atmel_init_property(atmel_port, pdev);
 	atmel_set_ops(port);
 
-	uart_get_rs485_mode(&mpdev->dev, &port->rs485);
-
 	port->iotype		= UPIO_MEM;
 	port->flags		= UPF_BOOT_AUTOCONF | UPF_IOREMAP;
 	port->ops		= &atmel_pops;
@@ -2506,6 +2504,10 @@ static int atmel_init_port(struct atmel_uart_port *atmel_port,
 
 	memset(&atmel_port->rx_ring, 0, sizeof(atmel_port->rx_ring));
 
+	ret = uart_get_rs485_mode(port);
+	if (ret)
+		return ret;
+
 	/* for console, the clock could already be configured */
 	if (!atmel_port->clk) {
 		atmel_port->clk = clk_get(&mpdev->dev, "usart");
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 6a9909e56449..029324c77cd7 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -2619,7 +2619,9 @@ static int lpuart_probe(struct platform_device *pdev)
 	if (ret)
 		goto failed_attach_port;
 
-	uart_get_rs485_mode(&pdev->dev, &sport->port.rs485);
+	ret = uart_get_rs485_mode(&sport->port);
+	if (ret)
+		goto failed_get_rs485;
 
 	if (sport->port.rs485.flags & SER_RS485_RX_DURING_TX)
 		dev_err(&pdev->dev, "driver doesn't support RX during TX\n");
@@ -2632,6 +2634,7 @@ static int lpuart_probe(struct platform_device *pdev)
 
 	return 0;
 
+failed_get_rs485:
 failed_attach_port:
 failed_irq_request:
 	lpuart_disable_clks(sport);
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index f4d68109bc8b..91f3910d6c44 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2302,7 +2302,11 @@ static int imx_uart_probe(struct platform_device *pdev)
 	sport->ucr4 = readl(sport->port.membase + UCR4);
 	sport->ufcr = readl(sport->port.membase + UFCR);
 
-	uart_get_rs485_mode(&pdev->dev, &sport->port.rs485);
+	ret = uart_get_rs485_mode(&sport->port);
+	if (ret) {
+		clk_disable_unprepare(sport->clk_ipg);
+		return ret;
+	}
 
 	if (sport->port.rs485.flags & SER_RS485_ENABLED &&
 	    (!sport->have_rtscts && !sport->have_rtsgpio))
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index c71c1a2266dc..8573fc9cb0cd 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1608,7 +1608,9 @@ static int serial_omap_probe_rs485(struct uart_omap_port *up,
 	if (!np)
 		return 0;
 
-	uart_get_rs485_mode(up->dev, rs485conf);
+	ret = uart_get_rs485_mode(&up->port);
+	if (ret)
+		return ret;
 
 	if (of_property_read_bool(np, "rs485-rts-active-high")) {
 		rs485conf->flags |= SER_RS485_RTS_ON_SEND;
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 66a5e2faf57e..43b6682877d5 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3295,8 +3295,10 @@ EXPORT_SYMBOL(uart_remove_one_port);
  * This function implements the device tree binding described in
  * Documentation/devicetree/bindings/serial/rs485.txt.
  */
-void uart_get_rs485_mode(struct device *dev, struct serial_rs485 *rs485conf)
+int uart_get_rs485_mode(struct uart_port *port)
 {
+	struct serial_rs485 *rs485conf = &port->rs485;
+	struct device *dev = port->dev;
 	u32 rs485_delay[2];
 	int ret;
 
@@ -3328,6 +3330,8 @@ void uart_get_rs485_mode(struct device *dev, struct serial_rs485 *rs485conf)
 		rs485conf->flags &= ~SER_RS485_RTS_ON_SEND;
 		rs485conf->flags |= SER_RS485_RTS_AFTER_SEND;
 	}
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(uart_get_rs485_mode);
 
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index 5e93e8d40f59..e3db54398159 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -158,9 +158,7 @@ static int stm32_init_rs485(struct uart_port *port,
 	if (!pdev->dev.of_node)
 		return -ENODEV;
 
-	uart_get_rs485_mode(&pdev->dev, rs485conf);
-
-	return 0;
+	return uart_get_rs485_mode(port);
 }
 
 static int stm32_pending_rx(struct uart_port *port, u32 *sr, int *last_res,
@@ -931,7 +929,9 @@ static int stm32_init_port(struct stm32_port *stm32port,
 
 	port->rs485_config = stm32_config_rs485;
 
-	stm32_init_rs485(port, pdev);
+	ret = stm32_init_rs485(port, pdev);
+	if (ret)
+		return ret;
 
 	if (stm32port->info->cfg.has_wakeup) {
 		stm32port->wakeirq = platform_get_irq(pdev, 1);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 92f5eba86052..b649a2b894e7 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -472,5 +472,5 @@ extern int uart_handle_break(struct uart_port *port);
 					 (cflag) & CRTSCTS || \
 					 !((cflag) & CLOCAL))
 
-void uart_get_rs485_mode(struct device *dev, struct serial_rs485 *rs485conf);
+int uart_get_rs485_mode(struct uart_port *port);
 #endif /* LINUX_SERIAL_CORE_H */
-- 
2.26.2

[PATCH 3/4] dt-bindings: serial: Add binding for rs485 bus termination GPIO

[Lukas Wunner]

Commit e8759ad17d41 ("serial: uapi: Add support for bus termination")
introduced the ability to enable rs485 bus termination from user space.
So far the feature is only used by a single driver, 8250_exar.c, using a
hardcoded GPIO pin specific to Siemens IOT2040 products.

Provide for a more generic solution by allowing specification of an
rs485 bus termination GPIO pin in the device tree.  An upcoming commit
implements support for this pin for any 8250 driver.  The binding is
used in device trees of the "Revolution Pi" PLCs offered by KUNBUS.

[Heiko Stuebner converted the binding to YAML, hence his Signed-off-by.]

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
---
 Documentation/devicetree/bindings/serial/rs485.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/rs485.yaml b/Documentation/devicetree/bindings/serial/rs485.yaml
index d4beaf11222d..a9ad17864889 100644
--- a/Documentation/devicetree/bindings/serial/rs485.yaml
+++ b/Documentation/devicetree/bindings/serial/rs485.yaml
@@ -43,3 +43,7 @@ properties:
   rs485-rx-during-tx:
    description: enables the receiving of data even while sending data.
    $ref: /schemas/types.yaml#/definitions/flag
+
+  rs485-term-gpios:
+    description: GPIO pin to enable RS485 bus termination.
+    maxItems: 1
-- 
2.26.2

[PATCH 4/4] serial: 8250: Support rs485 bus termination GPIO

[Lukas Wunner]

Commit e8759ad17d41 ("serial: uapi: Add support for bus termination")
introduced the ability to enable rs485 bus termination from user space.
So far the feature is only used by a single driver, 8250_exar.c, using a
hardcoded GPIO pin specific to Siemens IOT2040 products.

Provide for a more generic solution by allowing specification of an
rs485 bus termination GPIO pin in the device tree:  Amend the serial
core to retrieve the GPIO from the device tree (or ACPI table) and amend
the default ->rs485_config() callback for 8250 drivers to change the
GPIO on request from user space.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/tty/serial/8250/8250_port.c |  4 ++++
 drivers/tty/serial/serial_core.c    | 24 ++++++++++++++++++++++++
 include/linux/serial_core.h         |  2 ++
 3 files changed, 30 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index f77bf820b7a3..b5b630d02110 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -681,6 +681,10 @@ int serial8250_em485_config(struct uart_port *port, struct serial_rs485 *rs485)
 	memset(rs485->padding, 0, sizeof(rs485->padding));
 	port->rs485 = *rs485;
 
+	if (port->rs485_term_gpio)
+		gpiod_set_value(port->rs485_term_gpio,
+				rs485->flags & SER_RS485_TERMINATE_BUS);
+
 	/*
 	 * Both serial8250_em485_init() and serial8250_em485_destroy()
 	 * are idempotent.
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 43b6682877d5..7c929aad066e 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3317,6 +3317,7 @@ int uart_get_rs485_mode(struct uart_port *port)
 	 * to get to a defined state with the following properties:
 	 */
 	rs485conf->flags &= ~(SER_RS485_RX_DURING_TX | SER_RS485_ENABLED |
+			      SER_RS485_TERMINATE_BUS |
 			      SER_RS485_RTS_AFTER_SEND);
 	rs485conf->flags |= SER_RS485_RTS_ON_SEND;
 
@@ -3331,6 +3332,29 @@ int uart_get_rs485_mode(struct uart_port *port)
 		rs485conf->flags |= SER_RS485_RTS_AFTER_SEND;
 	}
 
+	if (port->rs485_term_gpio)
+		devm_gpiod_put(dev, port->rs485_term_gpio);
+
+	port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term",
+		GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT);
+	if (IS_ERR(port->rs485_term_gpio)) {
+		ret = PTR_ERR(port->rs485_term_gpio);
+		port->rs485_term_gpio = NULL;
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Cannot get rs485-term-gpios\n");
+		return ret;
+	}
+
+	if (port->rs485_term_gpio) {
+		ret = gpiod_get_value(port->rs485_term_gpio);
+		if (ret < 0) {
+			dev_err(dev, "Cannot get rs485-term-gpios value\n");
+			return ret;
+		}
+		if (ret)
+			rs485conf->flags |= SER_RS485_TERMINATE_BUS;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(uart_get_rs485_mode);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index b649a2b894e7..9fd550e7946a 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -10,6 +10,7 @@
 #include <linux/bitops.h>
 #include <linux/compiler.h>
 #include <linux/console.h>
+#include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/circ_buf.h>
 #include <linux/spinlock.h>
@@ -251,6 +252,7 @@ struct uart_port {
 	struct attribute_group	*attr_group;		/* port specific attributes */
 	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
 	struct serial_rs485     rs485;
+	struct gpio_desc	*rs485_term_gpio;	/* enable RS485 bus termination */
 	struct serial_iso7816   iso7816;
 	void			*private_data;		/* generic platform data pointer */
 };
-- 
2.26.2

Re: [PATCH 1/4] serial: 8250: Avoid error message on reprobe

[Andy Shevchenko]

On Tue, May 05, 2020 at 04:42:01PM +0200, Lukas Wunner wrote:
> If the call to uart_add_one_port() in serial8250_register_8250_port()
> fails, a half-initialized entry in the serial_8250ports[] array is left
> behind.
> 
> A subsequent reprobe of the same serial port causes that entry to be
> reused.  Because uart->port.dev is set, uart_remove_one_port() is called
> for the half-initialized entry and bails out with an error message:
> 
> bcm2835-aux-uart 3f215040.serial: Removing wrong port: (null) != (ptrval)
> 
> The same happens on failure of mctrl_gpio_init() since commit
> 4a96895f74c9 ("tty/serial/8250: use mctrl_gpio helpers").
> 
> Fix by zeroing the uart->port.dev pointer in the probe error path.

Fixes tag?

> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v2.6.10+

...

>  			ret = uart_add_one_port(&serial8250_reg,
>  						&uart->port);
> -			if (ret == 0)
> +			if (ret)
> +				goto err;

> +			else

Redundant.

>  				ret = uart->port.line;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] serial: 8250: Support rs485 bus termination GPIO

[Andy Shevchenko]

On Tue, May 05, 2020 at 04:42:04PM +0200, Lukas Wunner wrote:
> Commit e8759ad17d41 ("serial: uapi: Add support for bus termination")
> introduced the ability to enable rs485 bus termination from user space.
> So far the feature is only used by a single driver, 8250_exar.c, using a
> hardcoded GPIO pin specific to Siemens IOT2040 products.
> 
> Provide for a more generic solution by allowing specification of an
> rs485 bus termination GPIO pin in the device tree:  Amend the serial
> core to retrieve the GPIO from the device tree (or ACPI table) and amend
> the default ->rs485_config() callback for 8250 drivers to change the
> GPIO on request from user space.

...

> @@ -3331,6 +3332,29 @@ int uart_get_rs485_mode(struct uart_port *port)

> +		devm_gpiod_put(dev, port->rs485_term_gpio);

> +	port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term",

Using devm_*() in uart_get_rs485_mode() seems not right.
Why do you need this?

> +		GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT);

Parameter has a specific macro GPIOD_OUT_HIGH.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] serial: 8250: Avoid error message on reprobe

[Lukas Wunner]

On Tue, May 05, 2020 at 07:01:01PM +0300, Andy Shevchenko wrote:
> On Tue, May 05, 2020 at 04:42:01PM +0200, Lukas Wunner wrote:
> > If the call to uart_add_one_port() in serial8250_register_8250_port()
> > fails, a half-initialized entry in the serial_8250ports[] array is left
> > behind.
> > 
> > A subsequent reprobe of the same serial port causes that entry to be
> > reused.  Because uart->port.dev is set, uart_remove_one_port() is called
> > for the half-initialized entry and bails out with an error message:
> > 
> > bcm2835-aux-uart 3f215040.serial: Removing wrong port: (null) != (ptrval)
> > 
> > The same happens on failure of mctrl_gpio_init() since commit
> > 4a96895f74c9 ("tty/serial/8250: use mctrl_gpio helpers").
> > 
> > Fix by zeroing the uart->port.dev pointer in the probe error path.
> >  
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > Cc: stable@vger.kernel.org # v2.6.10+
> 
> Fixes tag?

The bug was introduced in the pre-git era, so I can't provide a Fixes tag:

https://git.kernel.org/tglx/history/c/befff6f5bf5f

This commit, which went into v2.6.10, added an unconditional
uart_remove_one_port() in serial8250_register_port().

In 2012, commit 835d844d1a28 ("8250_pnp: do pnp probe before legacy probe")
made the call to uart_remove_one_port() conditional on uart->port.dev
being non-NULL and that allows me to fix the issue by setting that
pointer to NULL in the error path.  The commit went into v3.7, so it
should be possible to fix the problem going back to v3.7 with my patch.
And before that one needs to additionally make the call to
uart_remove_one_port() conditional.

However, according to www.kernel.org the oldest LTS kernel is v3.16.
So I've given you the nitty-gritty details but it's all fairly
irrelevant and the Cc: stable tag I've put into the commit seems the
best I can do in this case.

As for your other comment:

> >  			ret = uart_add_one_port(&serial8250_reg,
> >  						&uart->port);
> > -			if (ret == 0)
> > +			if (ret)
> > +				goto err;
> 
> > +			else
> 
> Redundant.
> 
> >  				ret = uart->port.line;

Sure, I can change that.

Thanks!

Lukas

Re: [PATCH 4/4] serial: 8250: Support rs485 bus termination GPIO

[Lukas Wunner]

On Tue, May 05, 2020 at 07:10:35PM +0300, Andy Shevchenko wrote:
> On Tue, May 05, 2020 at 04:42:04PM +0200, Lukas Wunner wrote:
> > Commit e8759ad17d41 ("serial: uapi: Add support for bus termination")
> > introduced the ability to enable rs485 bus termination from user space.
> > So far the feature is only used by a single driver, 8250_exar.c, using a
> > hardcoded GPIO pin specific to Siemens IOT2040 products.
> > 
> > Provide for a more generic solution by allowing specification of an
> > rs485 bus termination GPIO pin in the device tree:  Amend the serial
> > core to retrieve the GPIO from the device tree (or ACPI table) and amend
> > the default ->rs485_config() callback for 8250 drivers to change the
> > GPIO on request from user space.
> 
> ...
> 
> > @@ -3331,6 +3332,29 @@ int uart_get_rs485_mode(struct uart_port *port)
> 
> > +		devm_gpiod_put(dev, port->rs485_term_gpio);
> 
> > +	port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term",
> 
> Using devm_*() in uart_get_rs485_mode() seems not right.
> Why do you need this?

uart_get_rs485_mode() is called from a driver's ->probe() hook and we
do not have a corresponding function that is called from a ->remove()
hook where we'd be able to relinquish rs485 resources we've acquired
on probe.

Of course I could add that but it would be more heavy-weight compared
to simply using devm_*().  Do you disagree?

devm_gpiod_put() isn't strictly necessary here.  It is only necessary
if one of the drivers would invoke uart_get_rs485_mode() multiple
times, which none of them does AFAICS.  It's just a safety measure.
I can drop it if that is preferred.


> > +		GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT);
> 
> Parameter has a specific macro GPIOD_OUT_HIGH.

Good point.  It's also occurred to me now that reading the GPIO's
value after changing its direction to output is nonsense.  If anything
it ought to be read *before* changing the direction to output.
That would make sense in case the board has a pullup or pulldown on
the Termination Enable pin.  In other cases the pin may just float
and the value will be unpredictable.  However if I do not read the
pin, I'd have to choose either high or low as initial state.  Hm.
Let me check back with our hardware engineers today and see what they
recommend.

Thanks,

Lukas

Re: [PATCH 1/4] serial: 8250: Avoid error message on reprobe

[Andy Shevchenko]

On Wed, May 06, 2020 at 08:06:23AM +0200, Lukas Wunner wrote:
> On Tue, May 05, 2020 at 07:01:01PM +0300, Andy Shevchenko wrote:
> > On Tue, May 05, 2020 at 04:42:01PM +0200, Lukas Wunner wrote:
> > > If the call to uart_add_one_port() in serial8250_register_8250_port()
> > > fails, a half-initialized entry in the serial_8250ports[] array is left
> > > behind.
> > > 
> > > A subsequent reprobe of the same serial port causes that entry to be
> > > reused.  Because uart->port.dev is set, uart_remove_one_port() is called
> > > for the half-initialized entry and bails out with an error message:
> > > 
> > > bcm2835-aux-uart 3f215040.serial: Removing wrong port: (null) != (ptrval)
> > > 
> > > The same happens on failure of mctrl_gpio_init() since commit
> > > 4a96895f74c9 ("tty/serial/8250: use mctrl_gpio helpers").
> > > 
> > > Fix by zeroing the uart->port.dev pointer in the probe error path.
> > >  
> > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > > Cc: stable@vger.kernel.org # v2.6.10+
> > 
> > Fixes tag?
> 
> The bug was introduced in the pre-git era, so I can't provide a Fixes tag:
> 
> https://git.kernel.org/tglx/history/c/befff6f5bf5f
> 
> This commit, which went into v2.6.10, added an unconditional
> uart_remove_one_port() in serial8250_register_port().

We have history tree, but I heard you.

> In 2012, commit 835d844d1a28 ("8250_pnp: do pnp probe before legacy probe")
> made the call to uart_remove_one_port() conditional on uart->port.dev
> being non-NULL and that allows me to fix the issue by setting that
> pointer to NULL in the error path.  The commit went into v3.7, so it
> should be possible to fix the problem going back to v3.7 with my patch.
> And before that one needs to additionally make the call to
> uart_remove_one_port() conditional.

Perhaps it will be the best candidate.

Fixes, AFAIU, helps scripts to find whatever trees it might been applied
against. If somebody keeps private stable tree on unusual base it will be
helpful for them as well (but I understand that there is little care of a
such).

> However, according to www.kernel.org the oldest LTS kernel is v3.16.
> So I've given you the nitty-gritty details but it's all fairly
> irrelevant and the Cc: stable tag I've put into the commit seems the
> best I can do in this case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] serial: 8250: Support rs485 bus termination GPIO

[Andy Shevchenko]

On Wed, May 06, 2020 at 08:29:43AM +0200, Lukas Wunner wrote:
> On Tue, May 05, 2020 at 07:10:35PM +0300, Andy Shevchenko wrote:
> > On Tue, May 05, 2020 at 04:42:04PM +0200, Lukas Wunner wrote:

...

> > > +		devm_gpiod_put(dev, port->rs485_term_gpio);
> > 
> > > +	port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term",
> > 
> > Using devm_*() in uart_get_rs485_mode() seems not right.
> > Why do you need this?
> 
> uart_get_rs485_mode() is called from a driver's ->probe() hook and we
> do not have a corresponding function that is called from a ->remove()
> hook where we'd be able to relinquish rs485 resources we've acquired
> on probe.
> 
> Of course I could add that but it would be more heavy-weight compared
> to simply using devm_*().  Do you disagree?
> 
> devm_gpiod_put() isn't strictly necessary here.  It is only necessary
> if one of the drivers would invoke uart_get_rs485_mode() multiple
> times, which none of them does AFAICS.  It's just a safety measure.
> I can drop it if that is preferred.

I think putting and re-requesting here is also racy. Somebody can request the
very same GPIO in between (for example crazy user space tool).

Setting the same value many times won't hurt.

> > > +		GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT);
> > 
> > Parameter has a specific macro GPIOD_OUT_HIGH.
> 
> Good point.  It's also occurred to me now that reading the GPIO's
> value after changing its direction to output is nonsense.  If anything
> it ought to be read *before* changing the direction to output.

It's not a complete nonsense, depends what you actually want to achieve here.

> That would make sense in case the board has a pullup or pulldown on
> the Termination Enable pin.  In other cases the pin may just float
> and the value will be unpredictable.  However if I do not read the
> pin, I'd have to choose either high or low as initial state.  Hm.
> Let me check back with our hardware engineers today and see what they
> recommend.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] serial: 8250: Avoid error message on reprobe

[Lukas Wunner]

On Wed, May 06, 2020 at 01:01:57PM +0300, Andy Shevchenko wrote:
> On Wed, May 06, 2020 at 08:06:23AM +0200, Lukas Wunner wrote:
> > On Tue, May 05, 2020 at 07:01:01PM +0300, Andy Shevchenko wrote:
> > > On Tue, May 05, 2020 at 04:42:01PM +0200, Lukas Wunner wrote:
> > > > Cc: stable@vger.kernel.org # v2.6.10+
> > > 
> > > Fixes tag?
> > 
> > The bug was introduced in the pre-git era, so I can't provide a Fixes tag:
> > 
> > https://git.kernel.org/tglx/history/c/befff6f5bf5f
> > 
> > This commit, which went into v2.6.10, added an unconditional
> > uart_remove_one_port() in serial8250_register_port().
> 
> We have history tree, but I heard you.
> 
> > In 2012, commit 835d844d1a28 ("8250_pnp: do pnp probe before legacy probe")
> > made the call to uart_remove_one_port() conditional on uart->port.dev
> > being non-NULL and that allows me to fix the issue by setting that
> > pointer to NULL in the error path.  The commit went into v3.7, so it
> > should be possible to fix the problem going back to v3.7 with my patch.
> > And before that one needs to additionally make the call to
> > uart_remove_one_port() conditional.
> 
> Perhaps it will be the best candidate.

There's a syntax to specify prerequisites which is documented in:
Documentation/process/stable-kernel-rules.rst

So I intend to do the following:

Cc: stable@vger.kernel.org # v2.6.10
Cc: stable@vger.kernel.org # v2.6.10: 835d844d1a28: 8250_pnp: do pnp probe before legacy

And it probably doesn't hurt to include the explanation above
(which historic commit I'm fixing and so on) in the commit message.

Thanks,

Lukas