For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the
console write callback needs to enable/disable TX. It does this
by calling the rs485_start/stop_tx() callbacks. However, these
callbacks will disable/enable interrupts, which is a problem
for console write, as it must be responsible for
disabling/enabling interrupts.
Add an argument @in_con to the rs485_start/stop_tx() callbacks
to specify if they are being called from console write. If so,
the callbacks will not handle interrupt disabling/enabling.
For all call sites other than console write, there is no
functional change.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
drivers/tty/serial/8250/8250.h | 4 +--
drivers/tty/serial/8250/8250_bcm2835aux.c | 4 +--
drivers/tty/serial/8250/8250_omap.c | 2 +-
drivers/tty/serial/8250/8250_port.c | 34 +++++++++++++++--------
include/linux/serial_8250.h | 4 +--
5 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index e5310c65cf52..0d8717be0df7 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -231,8 +231,8 @@ void serial8250_rpm_put_tx(struct uart_8250_port *p);
int serial8250_em485_config(struct uart_port *port, struct ktermios *termios,
struct serial_rs485 *rs485);
-void serial8250_em485_start_tx(struct uart_8250_port *p);
-void serial8250_em485_stop_tx(struct uart_8250_port *p);
+void serial8250_em485_start_tx(struct uart_8250_port *p, bool in_con);
+void serial8250_em485_stop_tx(struct uart_8250_port *p, bool in_con);
void serial8250_em485_destroy(struct uart_8250_port *p);
extern struct serial_rs485 serial8250_em485_supported;
diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
index fdb53b54e99e..c9a106a86b56 100644
--- a/drivers/tty/serial/8250/8250_bcm2835aux.c
+++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
@@ -46,7 +46,7 @@ struct bcm2835aux_data {
u32 cntl;
};
-static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up)
+static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up, bool in_con)
{
if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
struct bcm2835aux_data *data = dev_get_drvdata(up->port.dev);
@@ -65,7 +65,7 @@ static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up)
serial8250_out_MCR(up, UART_MCR_RTS);
}
-static void bcm2835aux_rs485_stop_tx(struct uart_8250_port *up)
+static void bcm2835aux_rs485_stop_tx(struct uart_8250_port *up, bool in_con)
{
if (up->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
serial8250_out_MCR(up, 0);
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index b3be0fb184a3..fcbed7e98231 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -365,7 +365,7 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
if (up->port.rs485.flags & SER_RS485_ENABLED &&
up->port.rs485_config == serial8250_em485_config)
- serial8250_em485_stop_tx(up);
+ serial8250_em485_stop_tx(up, false);
}
/*
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 09ac521d232a..7c50387194ad 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -558,7 +558,7 @@ static int serial8250_em485_init(struct uart_8250_port *p)
deassert_rts:
if (p->em485->tx_stopped)
- p->rs485_stop_tx(p);
+ p->rs485_stop_tx(p, false);
return 0;
}
@@ -1398,10 +1398,11 @@ static void serial8250_stop_rx(struct uart_port *port)
/**
* serial8250_em485_stop_tx() - generic ->rs485_stop_tx() callback
* @p: uart 8250 port
+ * @in_con: true if called from console write, otherwise false
*
* Generic callback usable by 8250 uart drivers to stop rs485 transmission.
*/
-void serial8250_em485_stop_tx(struct uart_8250_port *p)
+void serial8250_em485_stop_tx(struct uart_8250_port *p, bool in_con)
{
unsigned char mcr = serial8250_in_MCR(p);
@@ -1419,7 +1420,9 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
serial8250_clear_and_reinit_fifos(p);
- __serial8250_start_rx_int(p);
+ /* In console context, caller handles interrupt enabling. */
+ if (!in_con)
+ __serial8250_start_rx_int(p);
}
}
EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
@@ -1434,7 +1437,7 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
serial8250_rpm_get(p);
uart_port_lock_irqsave(&p->port, &flags);
if (em485->active_timer == &em485->stop_tx_timer) {
- p->rs485_stop_tx(p);
+ p->rs485_stop_tx(p, false);
em485->active_timer = NULL;
em485->tx_stopped = true;
}
@@ -1466,7 +1469,7 @@ static void __stop_tx_rs485(struct uart_8250_port *p, u64 stop_delay)
em485->active_timer = &em485->stop_tx_timer;
hrtimer_start(&em485->stop_tx_timer, ns_to_ktime(stop_delay), HRTIMER_MODE_REL);
} else {
- p->rs485_stop_tx(p);
+ p->rs485_stop_tx(p, false);
em485->active_timer = NULL;
em485->tx_stopped = true;
}
@@ -1555,6 +1558,7 @@ static inline void __start_tx(struct uart_port *port)
/**
* serial8250_em485_start_tx() - generic ->rs485_start_tx() callback
* @up: uart 8250 port
+ * @in_con: true if called from console write, otherwise false
*
* Generic callback usable by 8250 uart drivers to start rs485 transmission.
* Assumes that setting the RTS bit in the MCR register means RTS is high.
@@ -1562,12 +1566,20 @@ static inline void __start_tx(struct uart_port *port)
* stoppable by disabling the UART_IER_RDI interrupt. (Some chips set the
* UART_LSR_DR bit even when UART_IER_RDI is disabled, foiling this approach.)
*/
-void serial8250_em485_start_tx(struct uart_8250_port *up)
+void serial8250_em485_start_tx(struct uart_8250_port *up, bool in_con)
{
unsigned char mcr = serial8250_in_MCR(up);
- if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
- serial8250_stop_rx(&up->port);
+ if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
+ /*
+ * In console context, caller handles interrupt disabling. So
+ * only LSR_DR masking is needed.
+ */
+ if (in_con)
+ __serial8250_stop_rx_mask_dr(&up->port);
+ else
+ serial8250_stop_rx(&up->port);
+ }
if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
mcr |= UART_MCR_RTS;
@@ -1600,7 +1612,7 @@ static bool start_tx_rs485(struct uart_port *port)
if (em485->tx_stopped) {
em485->tx_stopped = false;
- up->rs485_start_tx(up);
+ up->rs485_start_tx(up, false);
if (up->port.rs485.delay_rts_before_send > 0) {
em485->active_timer = &em485->start_tx_timer;
@@ -3402,7 +3414,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
if (em485) {
if (em485->tx_stopped)
- up->rs485_start_tx(up);
+ up->rs485_start_tx(up, true);
mdelay(port->rs485.delay_rts_before_send);
}
@@ -3440,7 +3452,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
if (em485) {
mdelay(port->rs485.delay_rts_after_send);
if (em485->tx_stopped)
- up->rs485_stop_tx(up);
+ up->rs485_stop_tx(up, true);
}
serial_port_out(port, UART_IER, ier);
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index e0717c8393d7..c25c026d173d 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -161,8 +161,8 @@ struct uart_8250_port {
void (*dl_write)(struct uart_8250_port *up, u32 value);
struct uart_8250_em485 *em485;
- void (*rs485_start_tx)(struct uart_8250_port *);
- void (*rs485_stop_tx)(struct uart_8250_port *);
+ void (*rs485_start_tx)(struct uart_8250_port *up, bool in_con);
+ void (*rs485_stop_tx)(struct uart_8250_port *up, bool in_con);
/* Serial port overrun backoff */
struct delayed_work overrun_backoff;
--
2.39.5On Fri 2024-10-25 13:03:26, John Ogness wrote: > For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the > console write callback needs to enable/disable TX. It does this > by calling the rs485_start/stop_tx() callbacks. However, these > callbacks will disable/enable interrupts, which is a problem > for console write, as it must be responsible for > disabling/enabling interrupts. It is not clear to me what exactly is the problem. Is the main problem calling pm_runtime*() API because it uses extra locks and can cause deadlocks? Or is it more complicated? IMHO, it would deserve some explanation. > Add an argument @in_con to the rs485_start/stop_tx() callbacks > to specify if they are being called from console write. If so, > the callbacks will not handle interrupt disabling/enabling. > > For all call sites other than console write, there is no > functional change. > > Signed-off-by: John Ogness <john.ogness@linutronix.de> It looks like the code does what the description says. And honestly, I do not have any idea how to improve the naming. I would keep it as is after reading John's answers in the thread. IMHO, one thing which makes things comlicated is that serial8250_em485_start_tx() and serial8250_em485_stop_tx() are not completely reversible operations. Especially, the change done by __serial8250_stop_rx_mask_dr() is not reverted in serial8250_em485_stop_tx(). It makes things look tricky. But I think that it is beyond the scope of this patchset to do anything about it. Just 2 my cents. Best Regaards, Petr
On 2024-11-06, Petr Mladek <pmladek@suse.com> wrote: >> For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the >> console write callback needs to enable/disable TX. It does this >> by calling the rs485_start/stop_tx() callbacks. However, these >> callbacks will disable/enable interrupts, which is a problem >> for console write, as it must be responsible for >> disabling/enabling interrupts. > > It is not clear to me what exactly is the problem. serial8250_em485_stop_tx() blindly sets the RX interrupt bits in IER, because it assumes they were cleared in serial8250_stop_rx(). This is fine for the driver in general, but it is wrong for the console ->write(), which restores those bits on its own later. > Is the main problem calling pm_runtime*() API because it uses extra > locks and can cause deadlocks? Or is it more complicated? pm_runtime*() is a second issue. In the v1 feeback we talked about it. tglx summarized it well here: https://lore.kernel.org/lkml/8734mbdwrf.ffs@tglx/ as well as explaining the need to split off the console-write code from the generic driver code. > IMHO, it would deserve some explanation. This commit message only talks about the first issue, which is enough to justify the patch. I will add that the callbacks are also not appropriate because they call into the PM code, which is not needed by console ->write() and is even unsafe in some contexts. > IMHO, one thing which makes things comlicated is that > serial8250_em485_start_tx() and serial8250_em485_stop_tx() > are not completely reversible operations. Especially, > the change done by __serial8250_stop_rx_mask_dr() is > not reverted in serial8250_em485_stop_tx(). It makes > things look tricky. But I think that it is beyond the scope > of this patchset to do anything about it. I agree that it is strange that the driver does not unmask DR later. I have now run tests and it seems the use of @read_status_mask is partially broken. I did some historical digging on it... For Linux 1.1.60 [0] the @read_status_mask usage was extended to support "stop listening to incoming characters" (text from the changelog [1]). Looking at that version, it is clear why and how it was used. For Linux 2.1.8 [2], the async handling was reworked, basically reverting the change from 1.1.60. However, that revert forgot the piece that clears the UART_LSR_DR bit in serial8250_stop_rx() (back then called rs_close()). And indeed, if you track the @read_status_mask value today, that bit remains cleared until serial8250_do_set_termios() happens to be called. But it didn't matter that the bit was not set again because that bit was not being evaluated at any call sites. For 4.6, RS485 support was added, but with a bug about re-enabling interrupts. When that bug was fixed [3], the fix did not set the UART_LSR_DR bit in @read_status_mask. Still that was not a problem because at that time, that bit still had no users. For 5.7, support was added to avoid reading characters when throttling. This re-introduced a user of the UART_LSR_DR bit in @read_status_mask. And thus now there _is_ a bug that the bit is not set when starting RX in __do_stop_tx_rs485(). Interestingly enough, the OMAP variant of the 8250 _did_ implement setting the bit when unthrottling [5] (also from the same series). So in summary, I will add a patch to my series that fixes [3] (or is it fixing [4]?) by setting the bit in __do_stop_tx_rs485() when re-enabling the RX interrupts. John [0] https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/diff/drivers/char/serial.c?id=ba97e35a1a8b45ff87ed37a58fca3ecf39c1c893 [1] https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/diff/drivers/char/ChangeLog?id=ba97e35a1a8b45ff87ed37a58fca3ecf39c1c893 [2] https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/diff/drivers/char/serial.c?id=0f9cac5b27076f801b29a0867868e1bce7310e00 [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=0c66940d584d1aac92f6a78460dc0ba2efd3b7ba [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=f19c3f6c8109b8bab000afd35580929958e087a9 [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=f4b042a050062b2dec456adfced13d61341939e2
On 25. 10. 24, 12:57, John Ogness wrote:
> For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the
> console write callback needs to enable/disable TX. It does this
> by calling the rs485_start/stop_tx() callbacks. However, these
> callbacks will disable/enable interrupts, which is a problem
> for console write, as it must be responsible for
> disabling/enabling interrupts.
>
> Add an argument @in_con to the rs485_start/stop_tx() callbacks
> to specify if they are being called from console write. If so,
> the callbacks will not handle interrupt disabling/enabling.
>
> For all call sites other than console write, there is no
> functional change.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
...
> @@ -1562,12 +1566,20 @@ static inline void __start_tx(struct uart_port *port)
> * stoppable by disabling the UART_IER_RDI interrupt. (Some chips set the
> * UART_LSR_DR bit even when UART_IER_RDI is disabled, foiling this approach.)
> */
> -void serial8250_em485_start_tx(struct uart_8250_port *up)
> +void serial8250_em485_start_tx(struct uart_8250_port *up, bool in_con)
> {
> unsigned char mcr = serial8250_in_MCR(up);
>
> - if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> - serial8250_stop_rx(&up->port);
> + if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> + /*
> + * In console context, caller handles interrupt disabling. So
> + * only LSR_DR masking is needed.
> + */
> + if (in_con)
> + __serial8250_stop_rx_mask_dr(&up->port);
> + else
> + serial8250_stop_rx(&up->port);
Would it make sense to propagate in_con into serial8250_stop_rx() and do
the logic there? That would effectively eliminate patch 2/6.
thanks,
--
js
suse labs
On 2024-10-30, Jiri Slaby <jirislaby@kernel.org> wrote:
>> -void serial8250_em485_start_tx(struct uart_8250_port *up)
>> +void serial8250_em485_start_tx(struct uart_8250_port *up, bool in_con)
>> {
>> unsigned char mcr = serial8250_in_MCR(up);
>>
>> - if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
>> - serial8250_stop_rx(&up->port);
>> + if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
>> + /*
>> + * In console context, caller handles interrupt disabling. So
>> + * only LSR_DR masking is needed.
>> + */
>> + if (in_con)
>> + __serial8250_stop_rx_mask_dr(&up->port);
>> + else
>> + serial8250_stop_rx(&up->port);
>
> Would it make sense to propagate in_con into serial8250_stop_rx() and do
> the logic there? That would effectively eliminate patch 2/6.
I considered this, however:
1. The whole idea of stopping RX in order to do TX is an RS485
issue. Modifying the general ->stop_rx() callback for this purpose is
kind of out of place.
2. The ->stop_rx() callback is a general uart_ops callback. Changing its
prototype would literally affect all serial drivers. OTOH the
->rs485_start_tx() callback is specific to the 8250 driver. (It seems
each driver has implemented their own method for handling the RS485
hacks.)
So I would prefer to keep the necessary RS485 changes 8250-specific for
now.
John
On Fri, Oct 25, 2024 at 01:03:26PM +0206, John Ogness wrote: > For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the > console write callback needs to enable/disable TX. It does this > by calling the rs485_start/stop_tx() callbacks. However, these It would be nice if you be consistent across the commit messages and also provide the names of the callbacks in full, because I dunno if we have a local stop_tx() or rs485_start() or whatever the above means. If it is "the rs485_start_tx() / rs485_stop_tx() callbacks.", it's much clearer for the reader. > callbacks will disable/enable interrupts, which is a problem toggle? > for console write, as it must be responsible for > disabling/enabling interrupts. toggling ? > Add an argument @in_con to the rs485_start/stop_tx() callbacks As per above. > to specify if they are being called from console write. If so, > the callbacks will not handle interrupt disabling/enabling. toggling ? > For all call sites other than console write, there is no > functional change. So, why not call the parameter better to emphasize that it's about IRQ toggling? bool toggle_irq ? -- With Best Regards, Andy Shevchenko
On 2024-10-25, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >> Add an argument @in_con to the rs485_start/stop_tx() callbacks >> to specify if they are being called from console write. If so, >> the callbacks will not handle interrupt disabling/enabling. > > toggling ? > >> For all call sites other than console write, there is no >> functional change. > > So, why not call the parameter better to emphasize that it's about IRQ > toggling? bool toggle_irq ? Currently there are only 2 users: serial8250_em485_stop_tx() bcm2835aux_rs485_stop_tx() The first one toggles the IER bits, the second one does not. I figured it would make more sense to specify the context rather than what needs to be done and let the 8250-variant decide what it should do. But I have no problems renaming it to toggle_irq. It is an 8250-specific callback with few users. And really the IER bits is the only reason that the argument even needs to exist. John
On Fri, Oct 25, 2024 at 04:31:05PM +0206, John Ogness wrote: > On 2024-10-25, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >> Add an argument @in_con to the rs485_start/stop_tx() callbacks > >> to specify if they are being called from console write. If so, > >> the callbacks will not handle interrupt disabling/enabling. > > > > toggling ? > > > >> For all call sites other than console write, there is no > >> functional change. > > > > So, why not call the parameter better to emphasize that it's about IRQ > > toggling? bool toggle_irq ? > > Currently there are only 2 users: > > serial8250_em485_stop_tx() > bcm2835aux_rs485_stop_tx() > > The first one toggles the IER bits, the second one does not. I figured > it would make more sense to specify the context rather than what needs > to be done and let the 8250-variant decide what it should do. > > But I have no problems renaming it to toggle_irq. It is an 8250-specific > callback with few users. And really the IER bits is the only reason that > the argument even needs to exist. Maybe toggle_ier will be better than? I haven't looked deeply into the implementations, so choose whichever describes better what's behind it. -- With Best Regards, Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.