Greg, Jiri, Andy. 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.8-rc5:
base-commit: 11ba468877bb ("Linux 5.8-rc5")
tag: v5.8-rc5
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
Link: https://lore.kernel.org/linux-serial/[email protected]
Changelog v7:
- Wake the device up on the serial port divider update.
Link: https://lore.kernel.org/linux-serial/[email protected]
Changelog v8:
- Add a new patch:
"serial: 8250_dw: Pass the same rate to the clk round and set rate methods"
Link: https://lore.kernel.org/linux-serial/[email protected]
Changelog v9:
- Resend
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Pavel Parkhomenko <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Maxime Ripard <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Russell King <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Serge Semin (4):
serial: 8250: Add 8250 port clock update method
serial: 8250_dw: Simplify the ref clock rate setting procedure
serial: 8250_dw: Pass the same rate to the clk round and set rate
methods
serial: 8250_dw: Fix common clocks usage race condition
drivers/tty/serial/8250/8250_dw.c | 120 ++++++++++++++++++++++++----
drivers/tty/serial/8250/8250_port.c | 40 ++++++++++
include/linux/serial_8250.h | 2 +
3 files changed, 148 insertions(+), 14 deletions(-)
--
2.26.2
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 <[email protected]>
---
Changelog v4:
- Export serial8250_update_uartclk() symbol for GPL modules only.
Changelog v7:
- Wake the device up on the serial port divider update.
---
drivers/tty/serial/8250/8250_port.c | 40 +++++++++++++++++++++++++++++
include/linux/serial_8250.h | 2 ++
2 files changed, 42 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 1632f7d25acc..f19757ef4999 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2631,6 +2631,46 @@ 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);
+
+ serial8250_rpm_get(up);
+ 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);
+ serial8250_rpm_put(up);
+
+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
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 <[email protected]>
---
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 | 103 +++++++++++++++++++++++++++++-
1 file changed, 101 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index c1511f9244bb..87f450b7c177 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, newrate);
if (rate > 0) {
+ /*
+ * 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, newrate);
- if (!ret)
- p->uartclk = rate;
+ 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
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 <[email protected]>
---
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
Indeed according to the clk API if clk_round_rate() has successfully
accepted a rate, then in order setup the clock with value returned by the
clk_round_rate() the clk_set_rate() method must be called with the
original rate value.
Suggested-by: Russell King <[email protected]>
Signed-off-by: Serge Semin <[email protected]>
---
Changelog v8:
- This is a new patch created by the Russell King suggestion.
---
drivers/tty/serial/8250/8250_dw.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 12866083731d..c1511f9244bb 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -275,15 +275,15 @@ dw8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old)
static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
struct ktermios *old)
{
- unsigned int baud = tty_termios_baud_rate(termios);
+ unsigned long newrate = tty_termios_baud_rate(termios) * 16;
struct dw8250_data *d = to_dw8250_data(p->private_data);
long rate;
int ret;
clk_disable_unprepare(d->clk);
- rate = clk_round_rate(d->clk, baud * 16);
+ rate = clk_round_rate(d->clk, newrate);
if (rate > 0) {
- ret = clk_set_rate(d->clk, rate);
+ ret = clk_set_rate(d->clk, newrate);
if (!ret)
p->uartclk = rate;
}
--
2.26.2
On Thu, Jul 23, 2020 at 03:33:54AM +0300, Serge Semin wrote:
> 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.
..
> +/*
> + * 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);
> +
> + serial8250_rpm_get(up);
> + 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);
> + serial8250_rpm_put(up);
> +
> +out_lock:
> + mutex_unlock(&port->state->port.mutex);
While looking for something else I have stumbled over this function.
My Q is, since it has some duplications with
serial8250_do_set_termios(), can we actually call the latter (or
derevative that can be called in both) in the above code instead of
duplicating some lines?
if (port UART clock has to be updated)
call (unlocked version of) serial8250_do_set_termios()
Serge, what do you think?
> +}
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Wed, Feb 14, 2024 at 03:45:16PM +0200, Andy Shevchenko wrote:
> On Thu, Jul 23, 2020 at 03:33:54AM +0300, Serge Semin wrote:
> > 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.
>
> ...
>
> > +/*
> > + * 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);
> > +
> > + serial8250_rpm_get(up);
> > + 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);
> > + serial8250_rpm_put(up);
> > +
> > +out_lock:
> > + mutex_unlock(&port->state->port.mutex);
>
> While looking for something else I have stumbled over this function.
> My Q is, since it has some duplications with
> serial8250_do_set_termios(), can we actually call the latter (or
> derevative that can be called in both) in the above code instead of
> duplicating some lines?
>
> if (port UART clock has to be updated)
> call (unlocked version of) serial8250_do_set_termios()
>
> Serge, what do you think?
What an old thread you've digged out.)
Well, AFAIR I didn't create a common baud-rate/clock-update method
because the baud-rate change was just a two stages action:
1. calculate divisor+quot couple based on the new clock,
2. update the divisor+quot (+ update the timeout).
The first stage didn't need to have the IRQsafe lock being held and
the runtime-PM being enabled, meanwhile the later one needed those.
So unless the nested locking or try-lock-based pattern is implemented
each stage required dedicated function introduced, which would have
been an overkill for that. But even if I got to implement the
try-lock-based solution with a single function containing both stages
I still couldn't avoid having the serial8250_get_baud_rate() and
serial8250_get_divisor() methods executed in the atomic context, which
isn't required for them and which would needlessly pro-long the CPU
executing with the IRQs disabled. As you well know it's better to
speed up the atomic context execution as much as possible.
Secondly I didn't know much about the tty/serial subsystem internals
back then. So I was afraid to break some parts I didn't aware of if
the baud-rate/ref-clock change code had some implicit dependencies
from the surrounding code and vice-versa (like the LCR DLAB flag
state).
Finally frankly it didn't seem like that much worth bothering about.
Basically AFAICS there were only four methods which invocation I
would have needed to move to a separate function:
serial8250_get_baud_rate();
serial8250_get_divisor();
// spin-lock
uart_update_timeout(port, termios->c_cflag, baud);
serial8250_set_divisor(port, baud, quot, frac);
// spin-unlock
So I decided to take a simplest and safest path, and created a
dedicated method for the just the ref-clock updates case leaving the
baud-rate change task implemented in the framework of the standard
serial8250_do_set_termios() method.
Regarding doing vise-versa and calling the serial8250_do_set_termios()
method from serial8250_update_uartclk() instead. To be honest I didn't
consider that option. That might work though, but AFAICS the
serial8250_do_set_termios() function will do much more than it's
required in case if the ref-clock has changed.
-Serge(y)
>
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Thu, Feb 15, 2024 at 10:32:18PM +0300, Serge Semin wrote:
> On Wed, Feb 14, 2024 at 03:45:16PM +0200, Andy Shevchenko wrote:
> > On Thu, Jul 23, 2020 at 03:33:54AM +0300, Serge Semin wrote:
> > > 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.
..
> > > +/*
> > > + * 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);
> > > +
> > > + serial8250_rpm_get(up);
> > > + 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);
> > > + serial8250_rpm_put(up);
> > > +
> > > +out_lock:
> > > + mutex_unlock(&port->state->port.mutex);
> >
>
> > While looking for something else I have stumbled over this function.
> > My Q is, since it has some duplications with
> > serial8250_do_set_termios(), can we actually call the latter (or
> > derevative that can be called in both) in the above code instead of
> > duplicating some lines?
> >
> > if (port UART clock has to be updated)
> > call (unlocked version of) serial8250_do_set_termios()
> >
> > Serge, what do you think?
>
> What an old thread you've digged out.)
Indeed :-)
> Well, AFAIR I didn't create a common baud-rate/clock-update method
> because the baud-rate change was just a two stages action:
> 1. calculate divisor+quot couple based on the new clock,
> 2. update the divisor+quot (+ update the timeout).
> The first stage didn't need to have the IRQsafe lock being held and
> the runtime-PM being enabled, meanwhile the later one needed those.
> So unless the nested locking or try-lock-based pattern is implemented
> each stage required dedicated function introduced, which would have
> been an overkill for that. But even if I got to implement the
> try-lock-based solution with a single function containing both stages
> I still couldn't avoid having the serial8250_get_baud_rate() and
> serial8250_get_divisor() methods executed in the atomic context, which
> isn't required for them and which would needlessly pro-long the CPU
> executing with the IRQs disabled. As you well know it's better to
> speed up the atomic context execution as much as possible.
>
> Secondly I didn't know much about the tty/serial subsystem internals
> back then. So I was afraid to break some parts I didn't aware of if
> the baud-rate/ref-clock change code had some implicit dependencies
> from the surrounding code and vice-versa (like the LCR DLAB flag
> state).
>
> Finally frankly it didn't seem like that much worth bothering about.
> Basically AFAICS there were only four methods which invocation I
> would have needed to move to a separate function:
>
> serial8250_get_baud_rate();
> serial8250_get_divisor();
> // spin-lock
> uart_update_timeout(port, termios->c_cflag, baud);
> serial8250_set_divisor(port, baud, quot, frac);
> // spin-unlock
>
> So I decided to take a simplest and safest path, and created a
> dedicated method for the just the ref-clock updates case leaving the
> baud-rate change task implemented in the framework of the standard
> serial8250_do_set_termios() method.
>
>
> Regarding doing vise-versa and calling the serial8250_do_set_termios()
> method from serial8250_update_uartclk() instead. To be honest I didn't
> consider that option. That might work though, but AFAICS the
> serial8250_do_set_termios() function will do much more than it's
> required in case if the ref-clock has changed.
My point here is that the idea behind clock change is most likely to be
followed up by ->set_termios(). Why to do it differently if it's the case?
And note, ->set_termios() can be called as many times as needed, so if nothing
changes in between it's also fine. But this makes intention much clearer.
Do you agree?
--
With Best Regards,
Andy Shevchenko
On Thu, Feb 15, 2024 at 09:39:12PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 15, 2024 at 10:32:18PM +0300, Serge Semin wrote:
> > On Wed, Feb 14, 2024 at 03:45:16PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jul 23, 2020 at 03:33:54AM +0300, Serge Semin wrote:
> > > > 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.
>
> ...
>
> > > > +/*
> > > > + * 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);
> > > > +
> > > > + serial8250_rpm_get(up);
> > > > + 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);
> > > > + serial8250_rpm_put(up);
> > > > +
> > > > +out_lock:
> > > > + mutex_unlock(&port->state->port.mutex);
> > >
> >
> > > While looking for something else I have stumbled over this function.
> > > My Q is, since it has some duplications with
> > > serial8250_do_set_termios(), can we actually call the latter (or
> > > derevative that can be called in both) in the above code instead of
> > > duplicating some lines?
> > >
> > > if (port UART clock has to be updated)
> > > call (unlocked version of) serial8250_do_set_termios()
> > >
> > > Serge, what do you think?
> >
> > What an old thread you've digged out.)
>
> Indeed :-)
>
> > Well, AFAIR I didn't create a common baud-rate/clock-update method
> > because the baud-rate change was just a two stages action:
> > 1. calculate divisor+quot couple based on the new clock,
> > 2. update the divisor+quot (+ update the timeout).
> > The first stage didn't need to have the IRQsafe lock being held and
> > the runtime-PM being enabled, meanwhile the later one needed those.
> > So unless the nested locking or try-lock-based pattern is implemented
> > each stage required dedicated function introduced, which would have
> > been an overkill for that. But even if I got to implement the
> > try-lock-based solution with a single function containing both stages
> > I still couldn't avoid having the serial8250_get_baud_rate() and
> > serial8250_get_divisor() methods executed in the atomic context, which
> > isn't required for them and which would needlessly pro-long the CPU
> > executing with the IRQs disabled. As you well know it's better to
> > speed up the atomic context execution as much as possible.
> >
> > Secondly I didn't know much about the tty/serial subsystem internals
> > back then. So I was afraid to break some parts I didn't aware of if
> > the baud-rate/ref-clock change code had some implicit dependencies
> > from the surrounding code and vice-versa (like the LCR DLAB flag
> > state).
> >
> > Finally frankly it didn't seem like that much worth bothering about.
> > Basically AFAICS there were only four methods which invocation I
> > would have needed to move to a separate function:
> >
> > serial8250_get_baud_rate();
> > serial8250_get_divisor();
> > // spin-lock
> > uart_update_timeout(port, termios->c_cflag, baud);
> > serial8250_set_divisor(port, baud, quot, frac);
> > // spin-unlock
> >
> > So I decided to take a simplest and safest path, and created a
> > dedicated method for the just the ref-clock updates case leaving the
> > baud-rate change task implemented in the framework of the standard
> > serial8250_do_set_termios() method.
> >
> >
> > Regarding doing vise-versa and calling the serial8250_do_set_termios()
> > method from serial8250_update_uartclk() instead. To be honest I didn't
> > consider that option. That might work though, but AFAICS the
> > serial8250_do_set_termios() function will do much more than it's
> > required in case if the ref-clock has changed.
>
> My point here is that the idea behind clock change is most likely to be
> followed up by ->set_termios(). Why to do it differently if it's the case?
Not always. IIUC what you say is just a one path of the code executed
within the chain:
dw8250_set_termios()->dw8250_do_set_termios()->serial8250_do_set_termios()
But another code-path will be taken if the DW UART port
ref-clock is suddenly and asynchronously changed. In that case the
driver is notified by means of the dw8250_clk_notifier_cb() callback,
which doesn't need the entire set_termios() callback execution but
only what is defined in the serial8250_update_uartclk() method:
dw8250_clk_notifier_cb()
+-> worker:: dw8250_clk_work_cb()->serial8250_update_uartclk().
> And note, ->set_termios() can be called as many times as needed, so if nothing
> changes in between it's also fine. But this makes intention much clearer.
> Do you agree?
If what you suggest is to replace the serial8250_update_uartclk() body
with a direct uart_port::set_termios() invocation then I don't find it
being much clearer really. The serial8250_update_uartclk() is
currently specialized on doing one thing: adjusting the divider in
case of the UART-clock change. If instead the entire
serial8250_set_termios() method is called then for a reader it won't
be easy to understand what is really required for a 8250 serial port
to perceive the ref-clock change. But from the maintainability point
of view I guess that it might be safer to just call
serial8250_set_termios() indeed, since among the other things the
later method implies the divider update too. Thus the maintainer won't
need to support the two clock divider update implementations. From
that perspective I agree, directly calling serial8250_set_termios()
might be more suitable despite of it' doing more than required.
-Serge(y)
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Fri, Feb 16, 2024 at 08:19:37PM +0300, Serge Semin wrote:
> On Thu, Feb 15, 2024 at 09:39:12PM +0200, Andy Shevchenko wrote:
..
(thanks for the detailed explanation why you have done it that way)
> If what you suggest is to replace the serial8250_update_uartclk() body
> with a direct uart_port::set_termios() invocation then I don't find it
> being much clearer really. The serial8250_update_uartclk() is
> currently specialized on doing one thing: adjusting the divider in
> case of the UART-clock change. If instead the entire
> serial8250_set_termios() method is called then for a reader it won't
> be easy to understand what is really required for a 8250 serial port
> to perceive the ref-clock change. But from the maintainability point
> of view I guess that it might be safer to just call
> serial8250_set_termios() indeed, since among the other things the
> later method implies the divider update too. Thus the maintainer won't
> need to support the two clock divider update implementations.
> From that perspective I agree, directly calling serial8250_set_termios()
> might be more suitable despite of it' doing more than required.
Would it be possible for you to cook the patch (and test on your HW,
since it seems the only user of that)?
--
With Best Regards,
Andy Shevchenko
On Mon, Feb 19, 2024 at 05:08:54PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 16, 2024 at 08:19:37PM +0300, Serge Semin wrote:
> > On Thu, Feb 15, 2024 at 09:39:12PM +0200, Andy Shevchenko wrote:
>
> ...
>
> (thanks for the detailed explanation why you have done it that way)
>
> > If what you suggest is to replace the serial8250_update_uartclk() body
> > with a direct uart_port::set_termios() invocation then I don't find it
> > being much clearer really. The serial8250_update_uartclk() is
> > currently specialized on doing one thing: adjusting the divider in
> > case of the UART-clock change. If instead the entire
> > serial8250_set_termios() method is called then for a reader it won't
> > be easy to understand what is really required for a 8250 serial port
> > to perceive the ref-clock change. But from the maintainability point
> > of view I guess that it might be safer to just call
> > serial8250_set_termios() indeed, since among the other things the
> > later method implies the divider update too. Thus the maintainer won't
> > need to support the two clock divider update implementations.
>
> > From that perspective I agree, directly calling serial8250_set_termios()
> > might be more suitable despite of it' doing more than required.
>
> Would it be possible for you to cook the patch (and test on your HW,
> since it seems the only user of that)?
Agreed. The patch should have been just landed on your work and
private inboxes.
Link: https://lore.kernel.org/linux-serial/[email protected]
-Serge(y)
>
> --
> With Best Regards,
> Andy Shevchenko
>
>