linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage
@ 2020-06-17 22:48 Serge Semin
  2020-06-17 22:48 ` [PATCH v6 1/3] serial: 8250: Add 8250 port clock update method Serge Semin
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Serge Semin @ 2020-06-17 22:48 UTC (permalink / raw)
  To: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Maxim Kaurkin,
	Pavel Parkhomenko, Alexey Kolotnikov, Ramil Zaripov,
	Ekaterina Skachko, Vadim Vlasov, Arnd Bergmann, Andy Shevchenko,
	Maxime Ripard, Will Deacon, Russell King, linux-mips,
	linux-arm-kernel, linux-serial, linux-kernel

Greg, Jiri. We've missed the last merge window. It would be pity to miss
the next one. Please review/merge in the series.

Regarding the patchset. It might be dangerous if an UART port reference
clock rate is suddenly changed. In particular the 8250 port drivers
(and AFAICS most of the tty drivers using common clock framework clocks)
rely either on the exclusive reference clock utilization or on the ref
clock rate being always constant. Needless to say that it turns out not
true and if some other service suddenly changes the clock rate behind an
UART port driver back no good can happen. So the port might not only end
up with an invalid uartclk value saved, but may also experience a
distorted output/input data since such action will effectively update the
programmed baud-clock. We discovered such problem on Baikal-T1 SoC where
two DW 8250 ports have got a shared reference clock. Allwinner SoC is
equipped with an UART, which clock is derived from the CPU PLL clock
source, so the CPU frequency change might be propagated down up to the
serial port reference clock. This patchset provides a way to fix the
problem to the 8250 serial port controllers and mostly fixes it for the
DW 8250-compatible UART. I say mostly because due to not having a facility
to pause/stop and resume/restart on-going transfers we implemented the
UART clock rate update procedure executed post factum of the actual
reference clock rate change.

In addition the patchset includes a small optimization patch. It
simplifies the DW APB UART ref clock rate setting procedure a bit.

This patchset is rebased and tested on the mainline Linux kernel 5.7-rc4:
base-commit: 0e698dfa2822 ("Linux 5.7-rc4")
tag: v5.7-rc4

Changelog v3:
- Refactor the original patch to adjust the UART port divisor instead of
  requesting an exclusive ref clock utilization.

Changelog v4:
- Discard commit b426bf0fb085 ("serial: 8250: Fix max baud limit in generic
  8250 port") since Greg has already merged it into the tty-next branch.
- Use EXPORT_SYMBOL_GPL() for the serial8250_update_uartclk() method.

Changelog v5:
- Refactor dw8250_clk_work_cb() function cheking the clk_get_rate()
  return value for being erroneous and exit if it is.
- Don't update p->uartclk in the port startup. It will be updated later in
  the same procedure at the set_termios() function being invoked by the
  serial_core anyway.

Changelog v6:
- Resend

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru>
Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
Cc: Alexey Kolotnikov <Alexey.Kolotnikov@baikalelectronics.ru>
Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru>
Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru>
Cc: Alexey Kolotnikov <Alexey.Kolotnikov@baikalelectronics.ru>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Will Deacon <will@kernel.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: linux-mips@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-serial@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Serge Semin (3):
  serial: 8250: Add 8250 port clock update method
  serial: 8250_dw: Simplify the ref clock rate setting procedure
  serial: 8250_dw: Fix common clocks usage race condition

 drivers/tty/serial/8250/8250_dw.c   | 116 +++++++++++++++++++++++++---
 drivers/tty/serial/8250/8250_port.c |  38 +++++++++
 include/linux/serial_8250.h         |   2 +
 3 files changed, 144 insertions(+), 12 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v6 1/3] serial: 8250: Add 8250 port clock update method
  2020-06-17 22:48 [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage Serge Semin
@ 2020-06-17 22:48 ` Serge Semin
  2020-06-17 22:48 ` [PATCH v6 2/3] serial: 8250_dw: Simplify the ref clock rate setting procedure Serge Semin
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Serge Semin @ 2020-06-17 22:48 UTC (permalink / raw)
  To: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Arnd Bergmann,
	Andy Shevchenko, Maxime Ripard, Will Deacon, Russell King,
	linux-mips, linux-arm-kernel, linux-serial, linux-kernel

Some platforms can be designed in a way so the UART port reference clock
might be asynchronously changed at some point. In Baikal-T1 SoC this may
happen due to the reference clock being shared between two UART ports, on
the Allwinner SoC the reference clock is derived from the CPU clock, so
any CPU frequency change should get to be known/reflected by/in the UART
controller as well. But it's not enough to just update the
uart_port->uartclk field of the corresponding UART port, the 8250
controller reference clock divisor should be altered so to preserve
current baud rate setting. All of these things is done in a coherent
way by calling the serial8250_update_uartclk() method provided in this
patch. Though note that it isn't supposed to be called from within the
UART port callbacks because the locks using to the protect the UART port
data are already taken in there.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Will Deacon <will@kernel.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: linux-mips@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org

---

Changelog v4:
- Export serial8250_update_uartclk() symbol for GPL modules only.
---
 drivers/tty/serial/8250/8250_port.c | 38 +++++++++++++++++++++++++++++
 include/linux/serial_8250.h         |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 4d83c85a7389..5596868c8832 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2628,6 +2628,44 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port,
 				  (port->uartclk + tolerance) / 16);
 }
 
+/*
+ * Note in order to avoid the tty port mutex deadlock don't use the next method
+ * within the uart port callbacks. Primarily it's supposed to be utilized to
+ * handle a sudden reference clock rate change.
+ */
+void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	unsigned int baud, quot, frac = 0;
+	struct ktermios *termios;
+	unsigned long flags;
+
+	mutex_lock(&port->state->port.mutex);
+
+	if (port->uartclk == uartclk)
+		goto out_lock;
+
+	port->uartclk = uartclk;
+	termios = &port->state->port.tty->termios;
+
+	baud = serial8250_get_baud_rate(port, termios, NULL);
+	quot = serial8250_get_divisor(port, baud, &frac);
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	uart_update_timeout(port, termios->c_cflag, baud);
+
+	serial8250_set_divisor(port, baud, quot, frac);
+	serial_port_out(port, UART_LCR, up->lcr);
+	serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS);
+
+	spin_unlock_irqrestore(&port->lock, flags);
+
+out_lock:
+	mutex_unlock(&port->state->port.mutex);
+}
+EXPORT_SYMBOL_GPL(serial8250_update_uartclk);
+
 void
 serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 			  struct ktermios *old)
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 6545f8cfc8fa..2b70f736b091 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -155,6 +155,8 @@ extern int early_serial_setup(struct uart_port *port);
 
 extern int early_serial8250_setup(struct earlycon_device *device,
 					 const char *options);
+extern void serial8250_update_uartclk(struct uart_port *port,
+				      unsigned int uartclk);
 extern void serial8250_do_set_termios(struct uart_port *port,
 		struct ktermios *termios, struct ktermios *old);
 extern void serial8250_do_set_ldisc(struct uart_port *port,
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v6 2/3] serial: 8250_dw: Simplify the ref clock rate setting procedure
  2020-06-17 22:48 [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage Serge Semin
  2020-06-17 22:48 ` [PATCH v6 1/3] serial: 8250: Add 8250 port clock update method Serge Semin
@ 2020-06-17 22:48 ` Serge Semin
  2020-06-17 22:48 ` [PATCH v6 3/3] serial: 8250_dw: Fix common clocks usage race condition Serge Semin
  2020-06-18  8:17 ` [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage Andy Shevchenko
  3 siblings, 0 replies; 10+ messages in thread
From: Serge Semin @ 2020-06-17 22:48 UTC (permalink / raw)
  To: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Arnd Bergmann,
	Maxime Ripard, Will Deacon, Russell King, linux-mips,
	linux-arm-kernel, linux-serial, linux-kernel

Really instead of twice checking the clk_round_rate() return value
we could do it once, and if it isn't error the clock rate can be changed.
By doing so we decrease a number of ret-value tests and remove a weird
goto-based construction implemented in the dw8250_set_termios() method.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Will Deacon <will@kernel.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: linux-mips@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org

---

Changelog v3:
- This is a new patch.
---
 drivers/tty/serial/8250/8250_dw.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index aab3cccc6789..12866083731d 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -282,20 +282,13 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
 
 	clk_disable_unprepare(d->clk);
 	rate = clk_round_rate(d->clk, baud * 16);
-	if (rate < 0)
-		ret = rate;
-	else if (rate == 0)
-		ret = -ENOENT;
-	else
+	if (rate > 0) {
 		ret = clk_set_rate(d->clk, rate);
+		if (!ret)
+			p->uartclk = rate;
+	}
 	clk_prepare_enable(d->clk);
 
-	if (ret)
-		goto out;
-
-	p->uartclk = rate;
-
-out:
 	p->status &= ~UPSTAT_AUTOCTS;
 	if (termios->c_cflag & CRTSCTS)
 		p->status |= UPSTAT_AUTOCTS;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v6 3/3] serial: 8250_dw: Fix common clocks usage race condition
  2020-06-17 22:48 [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage Serge Semin
  2020-06-17 22:48 ` [PATCH v6 1/3] serial: 8250: Add 8250 port clock update method Serge Semin
  2020-06-17 22:48 ` [PATCH v6 2/3] serial: 8250_dw: Simplify the ref clock rate setting procedure Serge Semin
@ 2020-06-17 22:48 ` Serge Semin
  2020-06-18  8:29   ` Andy Shevchenko
  2020-06-18  8:17 ` [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage Andy Shevchenko
  3 siblings, 1 reply; 10+ messages in thread
From: Serge Semin @ 2020-06-17 22:48 UTC (permalink / raw)
  To: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Arnd Bergmann,
	Maxime Ripard, Will Deacon, Russell King, linux-mips,
	linux-arm-kernel, linux-serial, linux-kernel

The race condition may happen if the UART reference clock is shared with
some other device (on Baikal-T1 SoC it's another DW UART port). In this
case if that device changes the clock rate while serial console is using
it the DW 8250 UART port might not only end up with an invalid uartclk
value saved, but may also experience a distorted output data since
baud-clock could have been changed. In order to fix this lets at least
try to adjust the 8250 port setting like UART clock rate in case if the
reference clock rate change is discovered. The driver will call the new
method to update 8250 UART port clock rate settings. It's done by means of
the clock event notifier registered at the port startup and unregistered
in the shutdown callback method.

Note 1. In order to avoid deadlocks we had to execute the UART port update
method in a dedicated deferred work. This is due to (in my opinion
redundant) the clock update implemented in the dw8250_set_termios()
method.
Note 2. Before the ref clock is manually changed by the custom
set_termios() function we swap the port uartclk value with new rate
adjusted to be suitable for the requested baud. It is necessary in
order to effectively disable a functionality of the ref clock events
handler for the current UART port, since uartclk update will be done
a bit further in the generic serial8250_do_set_termios() function.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Will Deacon <will@kernel.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: linux-mips@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org

---

Changelog v2:
- Move exclusive ref clock lock/unlock precudures to the 8250 port
  startup/shutdown methods.
- The changelog message has also been slightly modified due to the
  alteration.
- Remove Alexey' SoB tag.
- Cc someone from ARM who might be concerned regarding this change.
- Cc someone from Clocks Framework to get their comments on this patch.

Changelog v3:
- Refactor the original patch to adjust the UART port divisor instead of
  requesting an exclusive ref clock utilization.

Changelog v5:
- Refactor dw8250_clk_work_cb() function cheking the clk_get_rate()
  return value for being erroneous and exit if it is.
- Don't update p->uartclk on the port startup. It will be updated later in
  the same procedure at the set_termios() function being invoked by the
  serial_core anyway.
---
 drivers/tty/serial/8250/8250_dw.c | 105 +++++++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 12866083731d..fa59c026270f 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -19,6 +19,8 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/workqueue.h>
+#include <linux/notifier.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
 #include <linux/clk.h>
@@ -43,6 +45,8 @@ struct dw8250_data {
 	int			msr_mask_off;
 	struct clk		*clk;
 	struct clk		*pclk;
+	struct notifier_block	clk_notifier;
+	struct work_struct	clk_work;
 	struct reset_control	*rst;
 
 	unsigned int		skip_autocfg:1;
@@ -54,6 +58,16 @@ static inline struct dw8250_data *to_dw8250_data(struct dw8250_port_data *data)
 	return container_of(data, struct dw8250_data, data);
 }
 
+static inline struct dw8250_data *clk_to_dw8250_data(struct notifier_block *nb)
+{
+	return container_of(nb, struct dw8250_data, clk_notifier);
+}
+
+static inline struct dw8250_data *work_to_dw8250_data(struct work_struct *work)
+{
+	return container_of(work, struct dw8250_data, clk_work);
+}
+
 static inline int dw8250_modify_msr(struct uart_port *p, int offset, int value)
 {
 	struct dw8250_data *d = to_dw8250_data(p->private_data);
@@ -260,6 +274,46 @@ static int dw8250_handle_irq(struct uart_port *p)
 	return 0;
 }
 
+static void dw8250_clk_work_cb(struct work_struct *work)
+{
+	struct dw8250_data *d = work_to_dw8250_data(work);
+	struct uart_8250_port *up;
+	unsigned long rate;
+
+	rate = clk_get_rate(d->clk);
+	if (rate <= 0)
+		return;
+
+	up = serial8250_get_port(d->data.line);
+
+	serial8250_update_uartclk(&up->port, rate);
+}
+
+static int dw8250_clk_notifier_cb(struct notifier_block *nb,
+				  unsigned long event, void *data)
+{
+	struct dw8250_data *d = clk_to_dw8250_data(nb);
+
+	/*
+	 * We have no choice but to defer the uartclk update due to two
+	 * deadlocks. First one is caused by a recursive mutex lock which
+	 * happens when clk_set_rate() is called from dw8250_set_termios().
+	 * Second deadlock is more tricky and is caused by an inverted order of
+	 * the clk and tty-port mutexes lock. It happens if clock rate change
+	 * is requested asynchronously while set_termios() is executed between
+	 * tty-port mutex lock and clk_set_rate() function invocation and
+	 * vise-versa. Anyway if we didn't have the reference clock alteration
+	 * in the dw8250_set_termios() method we wouldn't have needed this
+	 * deferred event handling complication.
+	 */
+	if (event == POST_RATE_CHANGE) {
+		queue_work(system_unbound_wq, &d->clk_work);
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
 static void
 dw8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old)
 {
@@ -283,9 +337,16 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
 	clk_disable_unprepare(d->clk);
 	rate = clk_round_rate(d->clk, baud * 16);
 	if (rate > 0) {
-		ret = clk_set_rate(d->clk, rate);
-		if (!ret)
-			p->uartclk = rate;
+		/*
+		 * Premilinary set the uartclk to the new clock rate so the
+		 * clock update event handler caused by the clk_set_rate()
+		 * calling wouldn't actually update the UART divisor since
+		 * we about to do this anyway.
+		 */
+		swap(p->uartclk, rate);
+		ret = clk_set_rate(d->clk, p->uartclk);
+		if (ret)
+			swap(p->uartclk, rate);
 	}
 	clk_prepare_enable(d->clk);
 
@@ -312,6 +373,39 @@ static void dw8250_set_ldisc(struct uart_port *p, struct ktermios *termios)
 	serial8250_do_set_ldisc(p, termios);
 }
 
+static int dw8250_startup(struct uart_port *p)
+{
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
+	int ret;
+
+	/*
+	 * Some platforms may provide a reference clock shared between several
+	 * devices. In this case before using the serial port first we have to
+	 * make sure that any clock state change is known to the UART port at
+	 * least post factum.
+	 */
+	if (d->clk) {
+		ret = clk_notifier_register(d->clk, &d->clk_notifier);
+		if (ret)
+			dev_warn(p->dev, "Failed to set the clock notifier\n");
+	}
+
+	return serial8250_do_startup(p);
+}
+
+static void dw8250_shutdown(struct uart_port *p)
+{
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
+
+	serial8250_do_shutdown(p);
+
+	if (d->clk) {
+		clk_notifier_unregister(d->clk, &d->clk_notifier);
+
+		flush_work(&d->clk_work);
+	}
+}
+
 /*
  * dw8250_fallback_dma_filter will prevent the UART from getting just any free
  * channel on platforms that have DMA engines, but don't have any channels
@@ -407,6 +501,8 @@ static int dw8250_probe(struct platform_device *pdev)
 	p->serial_out	= dw8250_serial_out;
 	p->set_ldisc	= dw8250_set_ldisc;
 	p->set_termios	= dw8250_set_termios;
+	p->startup	= dw8250_startup;
+	p->shutdown	= dw8250_shutdown;
 
 	p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
 	if (!p->membase)
@@ -468,6 +564,9 @@ static int dw8250_probe(struct platform_device *pdev)
 	if (IS_ERR(data->clk))
 		return PTR_ERR(data->clk);
 
+	INIT_WORK(&data->clk_work, dw8250_clk_work_cb);
+	data->clk_notifier.notifier_call = dw8250_clk_notifier_cb;
+
 	err = clk_prepare_enable(data->clk);
 	if (err)
 		dev_warn(dev, "could not enable optional baudclk: %d\n", err);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage
  2020-06-17 22:48 [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage Serge Semin
                   ` (2 preceding siblings ...)
  2020-06-17 22:48 ` [PATCH v6 3/3] serial: 8250_dw: Fix common clocks usage race condition Serge Semin
@ 2020-06-18  8:17 ` Andy Shevchenko
  2020-06-18  8:31   ` Andy Shevchenko
  2020-06-19 17:16   ` Serge Semin
  3 siblings, 2 replies; 10+ messages in thread
From: Andy Shevchenko @ 2020-06-18  8:17 UTC (permalink / raw)
  To: Serge Semin, Tony Lindgren
  Cc: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby, Serge Semin,
	Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko,
	Alexey Kolotnikov, Ramil Zaripov, Ekaterina Skachko,
	Vadim Vlasov, Arnd Bergmann, Andy Shevchenko, Maxime Ripard,
	Will Deacon, Russell King, linux-mips, linux-arm Mailing List,
	open list:SERIAL DRIVERS, Linux Kernel Mailing List

On Thu, Jun 18, 2020 at 1:52 AM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
>
> Greg, Jiri. We've missed the last merge window. It would be pity to miss
> the next one. Please review/merge in the series.
>
> Regarding the patchset. It might be dangerous if an UART port reference
> clock rate is suddenly changed. In particular the 8250 port drivers
> (and AFAICS most of the tty drivers using common clock framework clocks)
> rely either on the exclusive reference clock utilization or on the ref
> clock rate being always constant. Needless to say that it turns out not
> true and if some other service suddenly changes the clock rate behind an
> UART port driver back no good can happen. So the port might not only end
> up with an invalid uartclk value saved, but may also experience a
> distorted output/input data since such action will effectively update the
> programmed baud-clock. We discovered such problem on Baikal-T1 SoC where
> two DW 8250 ports have got a shared reference clock. Allwinner SoC is
> equipped with an UART, which clock is derived from the CPU PLL clock
> source, so the CPU frequency change might be propagated down up to the
> serial port reference clock. This patchset provides a way to fix the
> problem to the 8250 serial port controllers and mostly fixes it for the
> DW 8250-compatible UART. I say mostly because due to not having a facility
> to pause/stop and resume/restart on-going transfers we implemented the
> UART clock rate update procedure executed post factum of the actual
> reference clock rate change.
>
> In addition the patchset includes a small optimization patch. It
> simplifies the DW APB UART ref clock rate setting procedure a bit.
>
> This patchset is rebased and tested on the mainline Linux kernel 5.7-rc4:
> base-commit: 0e698dfa2822 ("Linux 5.7-rc4")
> tag: v5.7-rc4

I'm wondering how this will collaborate with runtime PM.

> Changelog v3:
> - Refactor the original patch to adjust the UART port divisor instead of
>   requesting an exclusive ref clock utilization.
>
> Changelog v4:
> - Discard commit b426bf0fb085 ("serial: 8250: Fix max baud limit in generic
>   8250 port") since Greg has already merged it into the tty-next branch.
> - Use EXPORT_SYMBOL_GPL() for the serial8250_update_uartclk() method.
>
> Changelog v5:
> - Refactor dw8250_clk_work_cb() function cheking the clk_get_rate()
>   return value for being erroneous and exit if it is.
> - Don't update p->uartclk in the port startup. It will be updated later in
>   the same procedure at the set_termios() function being invoked by the
>   serial_core anyway.
>
> Changelog v6:
> - Resend
>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru>
> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Cc: Alexey Kolotnikov <Alexey.Kolotnikov@baikalelectronics.ru>
> Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru>
> Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru>
> Cc: Alexey Kolotnikov <Alexey.Kolotnikov@baikalelectronics.ru>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: linux-mips@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-serial@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
>
> Serge Semin (3):
>   serial: 8250: Add 8250 port clock update method
>   serial: 8250_dw: Simplify the ref clock rate setting procedure
>   serial: 8250_dw: Fix common clocks usage race condition
>
>  drivers/tty/serial/8250/8250_dw.c   | 116 +++++++++++++++++++++++++---
>  drivers/tty/serial/8250/8250_port.c |  38 +++++++++
>  include/linux/serial_8250.h         |   2 +
>  3 files changed, 144 insertions(+), 12 deletions(-)
>
> --
> 2.26.2
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v6 3/3] serial: 8250_dw: Fix common clocks usage race condition
  2020-06-17 22:48 ` [PATCH v6 3/3] serial: 8250_dw: Fix common clocks usage race condition Serge Semin
@ 2020-06-18  8:29   ` Andy Shevchenko
  2020-06-19 17:47     ` Serge Semin
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2020-06-18  8:29 UTC (permalink / raw)
  To: Serge Semin, Tony Lindgren
  Cc: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby,
	Andy Shevchenko, Serge Semin, Alexey Malahov, Arnd Bergmann,
	Maxime Ripard, Will Deacon, Russell King, linux-mips,
	linux-arm Mailing List, open list:SERIAL DRIVERS,
	Linux Kernel Mailing List

On Thu, Jun 18, 2020 at 1:50 AM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
>
> The race condition may happen if the UART reference clock is shared with
> some other device (on Baikal-T1 SoC it's another DW UART port). In this
> case if that device changes the clock rate while serial console is using
> it the DW 8250 UART port might not only end up with an invalid uartclk
> value saved, but may also experience a distorted output data since
> baud-clock could have been changed. In order to fix this lets at least
> try to adjust the 8250 port setting like UART clock rate in case if the
> reference clock rate change is discovered. The driver will call the new
> method to update 8250 UART port clock rate settings. It's done by means of
> the clock event notifier registered at the port startup and unregistered
> in the shutdown callback method.
>
> Note 1. In order to avoid deadlocks we had to execute the UART port update
> method in a dedicated deferred work. This is due to (in my opinion
> redundant) the clock update implemented in the dw8250_set_termios()
> method.
> Note 2. Before the ref clock is manually changed by the custom
> set_termios() function we swap the port uartclk value with new rate
> adjusted to be suitable for the requested baud. It is necessary in
> order to effectively disable a functionality of the ref clock events
> handler for the current UART port, since uartclk update will be done
> a bit further in the generic serial8250_do_set_termios() function.

So, regarding runtime PM...

> +static void dw8250_clk_work_cb(struct work_struct *work)
> +{
> +       struct dw8250_data *d = work_to_dw8250_data(work);
> +       struct uart_8250_port *up;
> +       unsigned long rate;
> +
> +       rate = clk_get_rate(d->clk);
> +       if (rate <= 0)
> +               return;
> +

> +       up = serial8250_get_port(d->data.line);

(Btw, this can be done directly in the definition block above.)

> +       serial8250_update_uartclk(&up->port, rate);

This I think should require a device to be powered on. What in your
opinion is a better place to have it done?
To me it looks like serial8250_update_uartclk() misses it.

> +}

--
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage
  2020-06-18  8:17 ` [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage Andy Shevchenko
@ 2020-06-18  8:31   ` Andy Shevchenko
  2020-06-19 17:21     ` Serge Semin
  2020-06-19 17:16   ` Serge Semin
  1 sibling, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2020-06-18  8:31 UTC (permalink / raw)
  To: Serge Semin, Tony Lindgren
  Cc: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby, Serge Semin,
	Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko,
	Alexey Kolotnikov, Ramil Zaripov, Ekaterina Skachko,
	Vadim Vlasov, Arnd Bergmann, Andy Shevchenko, Maxime Ripard,
	Will Deacon, Russell King, linux-mips, linux-arm Mailing List,
	open list:SERIAL DRIVERS, Linux Kernel Mailing List

On Thu, Jun 18, 2020 at 11:17 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Jun 18, 2020 at 1:52 AM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:

> I'm wondering how this will collaborate with runtime PM.

Forgot to mention the WIP repository [1] for runtime PM rework  (it
doesn't mean my comment against patch 3 is not valid). Tony and I are
working on it, you actually may be interested as well.

[1]: https://gitlab.com/andy-shev/next

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage
  2020-06-18  8:17 ` [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage Andy Shevchenko
  2020-06-18  8:31   ` Andy Shevchenko
@ 2020-06-19 17:16   ` Serge Semin
  1 sibling, 0 replies; 10+ messages in thread
From: Serge Semin @ 2020-06-19 17:16 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Serge Semin, Tony Lindgren, Thomas Bogendoerfer,
	Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Maxim Kaurkin,
	Pavel Parkhomenko, Alexey Kolotnikov, Ramil Zaripov,
	Ekaterina Skachko, Vadim Vlasov, Arnd Bergmann, Andy Shevchenko,
	Maxime Ripard, Will Deacon, Russell King, linux-mips,
	linux-arm Mailing List, open list:SERIAL DRIVERS,
	Linux Kernel Mailing List

On Thu, Jun 18, 2020 at 11:17:47AM +0300, Andy Shevchenko wrote:
> On Thu, Jun 18, 2020 at 1:52 AM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
> >
> > Greg, Jiri. We've missed the last merge window. It would be pity to miss
> > the next one. Please review/merge in the series.
> >
> > Regarding the patchset. It might be dangerous if an UART port reference
> > clock rate is suddenly changed. In particular the 8250 port drivers
> > (and AFAICS most of the tty drivers using common clock framework clocks)
> > rely either on the exclusive reference clock utilization or on the ref
> > clock rate being always constant. Needless to say that it turns out not
> > true and if some other service suddenly changes the clock rate behind an
> > UART port driver back no good can happen. So the port might not only end
> > up with an invalid uartclk value saved, but may also experience a
> > distorted output/input data since such action will effectively update the
> > programmed baud-clock. We discovered such problem on Baikal-T1 SoC where
> > two DW 8250 ports have got a shared reference clock. Allwinner SoC is
> > equipped with an UART, which clock is derived from the CPU PLL clock
> > source, so the CPU frequency change might be propagated down up to the
> > serial port reference clock. This patchset provides a way to fix the
> > problem to the 8250 serial port controllers and mostly fixes it for the
> > DW 8250-compatible UART. I say mostly because due to not having a facility
> > to pause/stop and resume/restart on-going transfers we implemented the
> > UART clock rate update procedure executed post factum of the actual
> > reference clock rate change.
> >
> > In addition the patchset includes a small optimization patch. It
> > simplifies the DW APB UART ref clock rate setting procedure a bit.
> >
> > This patchset is rebased and tested on the mainline Linux kernel 5.7-rc4:
> > base-commit: 0e698dfa2822 ("Linux 5.7-rc4")
> > tag: v5.7-rc4
> 

> I'm wondering how this will collaborate with runtime PM.

Thanks for noticing this. You are right. PM should be taken into account.

-Sergey

> 
> > Changelog v3:
> > - Refactor the original patch to adjust the UART port divisor instead of
> >   requesting an exclusive ref clock utilization.
> >
> > Changelog v4:
> > - Discard commit b426bf0fb085 ("serial: 8250: Fix max baud limit in generic
> >   8250 port") since Greg has already merged it into the tty-next branch.
> > - Use EXPORT_SYMBOL_GPL() for the serial8250_update_uartclk() method.
> >
> > Changelog v5:
> > - Refactor dw8250_clk_work_cb() function cheking the clk_get_rate()
> >   return value for being erroneous and exit if it is.
> > - Don't update p->uartclk in the port startup. It will be updated later in
> >   the same procedure at the set_termios() function being invoked by the
> >   serial_core anyway.
> >
> > Changelog v6:
> > - Resend
> >
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru>
> > Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> > Cc: Alexey Kolotnikov <Alexey.Kolotnikov@baikalelectronics.ru>
> > Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> > Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru>
> > Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru>
> > Cc: Alexey Kolotnikov <Alexey.Kolotnikov@baikalelectronics.ru>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: linux-mips@vger.kernel.org
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-serial@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> >
> > Serge Semin (3):
> >   serial: 8250: Add 8250 port clock update method
> >   serial: 8250_dw: Simplify the ref clock rate setting procedure
> >   serial: 8250_dw: Fix common clocks usage race condition
> >
> >  drivers/tty/serial/8250/8250_dw.c   | 116 +++++++++++++++++++++++++---
> >  drivers/tty/serial/8250/8250_port.c |  38 +++++++++
> >  include/linux/serial_8250.h         |   2 +
> >  3 files changed, 144 insertions(+), 12 deletions(-)
> >
> > --
> > 2.26.2
> >
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage
  2020-06-18  8:31   ` Andy Shevchenko
@ 2020-06-19 17:21     ` Serge Semin
  0 siblings, 0 replies; 10+ messages in thread
From: Serge Semin @ 2020-06-19 17:21 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Serge Semin, Tony Lindgren, Thomas Bogendoerfer,
	Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Maxim Kaurkin,
	Pavel Parkhomenko, Alexey Kolotnikov, Ramil Zaripov,
	Ekaterina Skachko, Vadim Vlasov, Arnd Bergmann, Andy Shevchenko,
	Maxime Ripard, Will Deacon, Russell King, linux-mips,
	linux-arm Mailing List, open list:SERIAL DRIVERS,
	Linux Kernel Mailing List

On Thu, Jun 18, 2020 at 11:31:44AM +0300, Andy Shevchenko wrote:
> On Thu, Jun 18, 2020 at 11:17 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Thu, Jun 18, 2020 at 1:52 AM Serge Semin
> > <Sergey.Semin@baikalelectronics.ru> wrote:
> 
> > I'm wondering how this will collaborate with runtime PM.
> 

> Forgot to mention the WIP repository [1] for runtime PM rework  (it
> doesn't mean my comment against patch 3 is not valid). Tony and I are
> working on it, you actually may be interested as well.
> 
> [1]: https://gitlab.com/andy-shev/next

Thanks for the link. I'll take a look at the repo. Though our SoC doesn't
require the RPM moderation that critically.

-Sergey

> 
> -- 
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v6 3/3] serial: 8250_dw: Fix common clocks usage race condition
  2020-06-18  8:29   ` Andy Shevchenko
@ 2020-06-19 17:47     ` Serge Semin
  0 siblings, 0 replies; 10+ messages in thread
From: Serge Semin @ 2020-06-19 17:47 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Serge Semin, Tony Lindgren, Thomas Bogendoerfer,
	Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko, Alexey Malahov,
	Arnd Bergmann, Maxime Ripard, Will Deacon, Russell King,
	linux-mips, linux-arm Mailing List, open list:SERIAL DRIVERS,
	Linux Kernel Mailing List

On Thu, Jun 18, 2020 at 11:29:36AM +0300, Andy Shevchenko wrote:
> On Thu, Jun 18, 2020 at 1:50 AM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
> >
> > The race condition may happen if the UART reference clock is shared with
> > some other device (on Baikal-T1 SoC it's another DW UART port). In this
> > case if that device changes the clock rate while serial console is using
> > it the DW 8250 UART port might not only end up with an invalid uartclk
> > value saved, but may also experience a distorted output data since
> > baud-clock could have been changed. In order to fix this lets at least
> > try to adjust the 8250 port setting like UART clock rate in case if the
> > reference clock rate change is discovered. The driver will call the new
> > method to update 8250 UART port clock rate settings. It's done by means of
> > the clock event notifier registered at the port startup and unregistered
> > in the shutdown callback method.
> >
> > Note 1. In order to avoid deadlocks we had to execute the UART port update
> > method in a dedicated deferred work. This is due to (in my opinion
> > redundant) the clock update implemented in the dw8250_set_termios()
> > method.
> > Note 2. Before the ref clock is manually changed by the custom
> > set_termios() function we swap the port uartclk value with new rate
> > adjusted to be suitable for the requested baud. It is necessary in
> > order to effectively disable a functionality of the ref clock events
> > handler for the current UART port, since uartclk update will be done
> > a bit further in the generic serial8250_do_set_termios() function.
> 
> So, regarding runtime PM...
> 
> > +static void dw8250_clk_work_cb(struct work_struct *work)
> > +{
> > +       struct dw8250_data *d = work_to_dw8250_data(work);
> > +       struct uart_8250_port *up;
> > +       unsigned long rate;
> > +
> > +       rate = clk_get_rate(d->clk);
> > +       if (rate <= 0)
> > +               return;
> > +
> 
> > +       up = serial8250_get_port(d->data.line);
> 

> (Btw, this can be done directly in the definition block above.)

I would have done like you said if serial8250_get_port() was an inline or
macro. But since it's a normal exported function I'd leave the invocation
here, since calling it takes though small but still some time.

> 
> > +       serial8250_update_uartclk(&up->port, rate);
> 
> This I think should require a device to be powered on. What in your
> opinion is a better place to have it done?

> To me it looks like serial8250_update_uartclk() misses it.

Right. the PM thing should be there similarly to the rest of the serial8250
methods. I'll add the serial8250_rpm_get(up) and serial8250_rpm_put(up)
functions invocation around the divisor update clause, like it's done in the
serial8250_do_set_termios() method.

Thanks for noticing this.

-Sergey

> 
> > +}
> 
> --
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-06-19 17:47 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-17 22:48 [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage Serge Semin
2020-06-17 22:48 ` [PATCH v6 1/3] serial: 8250: Add 8250 port clock update method Serge Semin
2020-06-17 22:48 ` [PATCH v6 2/3] serial: 8250_dw: Simplify the ref clock rate setting procedure Serge Semin
2020-06-17 22:48 ` [PATCH v6 3/3] serial: 8250_dw: Fix common clocks usage race condition Serge Semin
2020-06-18  8:29   ` Andy Shevchenko
2020-06-19 17:47     ` Serge Semin
2020-06-18  8:17 ` [PATCH RESEND v6 0/3] serial: 8250_dw: Fix ref clock usage Andy Shevchenko
2020-06-18  8:31   ` Andy Shevchenko
2020-06-19 17:21     ` Serge Semin
2020-06-19 17:16   ` Serge Semin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).