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:

quoted

On Wed, Feb 14, 2024 at 03:45:16PM +0200, Andy Shevchenko wrote:

quoted

On Thu, Jul 23, 2020 at 03:33:54AM +0300, Serge Semin wrote:

quoted

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.

...

quoted

quoted

quoted

+/*
+ * 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);

quoted

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 :-)

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel