Move IER handling out of rs485_start_tx() callback and into a new
wrapper serial8250_rs485_start_tx(). Replace all callback call sites
with wrapper, except for the console write() callback, where it is
inappropriate to modify IER.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
drivers/tty/serial/8250/8250.h | 2 ++
drivers/tty/serial/8250/8250_port.c | 27 +++++++++++++++++++++------
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index e5310c65cf52..6e90223ba1d6 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -236,6 +236,8 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p);
void serial8250_em485_destroy(struct uart_8250_port *p);
extern struct serial_rs485 serial8250_em485_supported;
+void serial8250_rs485_start_tx(struct uart_8250_port *up);
+
/* MCR <-> TIOCM conversion */
static inline int serial8250_TIOCM_to_MCR(int tiocm)
{
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 2786918aea98..ba8d9cefc451 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1370,7 +1370,6 @@ static void serial8250_stop_rx(struct uart_port *port)
serial8250_rpm_get(up);
up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
- up->port.read_status_mask &= ~UART_LSR_DR;
serial_port_out(port, UART_IER, up->ier);
serial8250_rpm_put(up);
@@ -1543,16 +1542,20 @@ static inline void __start_tx(struct uart_port *port)
*
* 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.
- * (Some chips use inverse semantics.) Further assumes that reception is
- * 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.)
+ * (Some chips use inverse semantics.)
+ * It does not disable RX interrupts. Use the wrapper function
+ * serial8250_rs485_start_tx() if that is also needed.
*/
void serial8250_em485_start_tx(struct uart_8250_port *up)
{
unsigned char mcr = serial8250_in_MCR(up);
+ /*
+ * Some chips set the UART_LSR_DR bit even when UART_IER_RDI is
+ * disabled, so explicitly mask it.
+ */
if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
- serial8250_stop_rx(&up->port);
+ up->port.read_status_mask &= ~UART_LSR_DR;
if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
mcr |= UART_MCR_RTS;
@@ -1562,6 +1565,18 @@ void serial8250_em485_start_tx(struct uart_8250_port *up)
}
EXPORT_SYMBOL_GPL(serial8250_em485_start_tx);
+/**
+ * serial8250_rs485_start_tx() - stop rs485 reception, enable transmission
+ * @up: uart 8250 port
+ */
+void serial8250_rs485_start_tx(struct uart_8250_port *up)
+{
+ if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
+ serial8250_stop_rx(&up->port);
+
+ up->rs485_start_tx(up);
+}
+
/* Returns false, if start_tx_timer was setup to defer TX start */
static bool start_tx_rs485(struct uart_port *port)
{
@@ -1585,7 +1600,7 @@ static bool start_tx_rs485(struct uart_port *port)
if (em485->tx_stopped) {
em485->tx_stopped = false;
- up->rs485_start_tx(up);
+ serial8250_rs485_start_tx(up);
if (up->port.rs485.delay_rts_before_send > 0) {
em485->active_timer = &em485->start_tx_timer;
--
2.39.2On Fri 2024-09-13 16:11:35, John Ogness wrote:
> Move IER handling out of rs485_start_tx() callback and into a new
> wrapper serial8250_rs485_start_tx(). Replace all callback call sites
> with wrapper, except for the console write() callback, where it is
> inappropriate to modify IER.
Sigh, I am trying to review this patch but I am not familiar with the
code. Feel free to ignore me when the questions are completely off.
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1370,7 +1370,6 @@ static void serial8250_stop_rx(struct uart_port *port)
> serial8250_rpm_get(up);
>
> up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
> - up->port.read_status_mask &= ~UART_LSR_DR;
> serial_port_out(port, UART_IER, up->ier);
>
> serial8250_rpm_put(up);
> @@ -1543,16 +1542,20 @@ static inline void __start_tx(struct uart_port *port)
> *
> * 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.
> - * (Some chips use inverse semantics.) Further assumes that reception is
> - * 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.)
> + * (Some chips use inverse semantics.)
> + * It does not disable RX interrupts. Use the wrapper function
> + * serial8250_rs485_start_tx() if that is also needed.
> */
> void serial8250_em485_start_tx(struct uart_8250_port *up)
> {
> unsigned char mcr = serial8250_in_MCR(up);
>
> + /*
> + * Some chips set the UART_LSR_DR bit even when UART_IER_RDI is
> + * disabled, so explicitly mask it.
> + */
> if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> - serial8250_stop_rx(&up->port);
> + up->port.read_status_mask &= ~UART_LSR_DR;
This change is related to disabling UART_IER_RDI but we do not longer
disable it in this code path.
Why do we need to do it here, please?
Why is it needed only in the em485-specific path, please?
I tried to understand the code and am in doubts:
On one hand, the comment talks about UART_LSR_DR and UART_IER_RDI
so seems to be relater.
But the "Some chips set..." comment has been added by the commit
058bc104f7ca5c83d81 ("serial: 8250: Generalize rs485 software emulation").
And I do not see any explanation why it was added in this code path
even though UART_LSR_DR and UART_IER_RDI were manipulated in
serial8250_stop_rx() which can be called also in other code
paths via uport->ops->stop_rx().
Also the comment suggests that this fixes a bug in some chips but
the line has been added into 1.1.60 back in 2007.
--- a/drivers/char/ChangeLog
+++ b/drivers/char/ChangeLog
@@ -1,3 +1,28 @@
+Sat Oct 29 18:17:34 1994 Theodore Y. Ts'o (tytso@rt-11)
+
+ * serial.c (rs_ioctl, get_lsr_info): Added patch suggested by Arne
+ Riiber so that user mode programs can tell when the
+ transmitter shift register is empty.
+
+Thu Oct 27 23:14:29 1994 Theodore Y. Ts'o (tytso@rt-11)
+
+ * tty_ioctl.c (wait_until_sent): Added debugging printk statements
+ (under the #ifdef TTY_DEBUG_WAIT_UNTL_SENT)
+
+ * serial.c (rs_interrupt, rs_interrupt_single, receive_chars,
+ change_speed, rs_close): rs_close now disables receiver
+ interrupts when closing the serial port. This allows the
+ serial port to close quickly when Linux and a modem (or a
+ mouse) are engaged in an echo war; when closing the serial
+ port, we now first stop listening to incoming characters,
+ and *then* wait for the transmit buffer to drain.
+
+ In order to make this change, the info->read_status_mask
+ is now used to control what bits of the line status
+ register are looked at in the interrupt routine in all
+ cases; previously it was only used in receive_chars to
+ select a few of the status bits.
+
--- a/drivers/char/serial.c
+++ b/drivers/char/serial.c
[...]
@@ -1780,6 +1830,15 @@ static void rs_close(struct tty_struct *tty, struct file * filp)
info->normal_termios = *tty->termios;
if (info->flags & ASYNC_CALLOUT_ACTIVE)
info->callout_termios = *tty->termios;
+ /*
+ * At this point we stop accepting input. To do this, we
+ * disable the receive line status interrupts, and tell the
+ * interrut driver to stop checking the data ready bit in the
+ * line status register.
+ */
+ info->IER &= ~UART_IER_RLSI;
+ serial_out(info, UART_IER, info->IER);
+ info->read_status_mask &= ~UART_LSR_DR;
if (info->flags & ASYNC_INITIALIZED) {
wait_until_sent(tty, 3000); /* 30 seconds timeout */
/*
=> It looks like it was not a fix for a "buggy chips". It looks
like it was part of the design.
>
> if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
> mcr |= UART_MCR_RTS;
> @@ -1562,6 +1565,18 @@ void serial8250_em485_start_tx(struct uart_8250_port *up)
> }
> EXPORT_SYMBOL_GPL(serial8250_em485_start_tx);
>
> +/**
> + * serial8250_rs485_start_tx() - stop rs485 reception, enable transmission
> + * @up: uart 8250 port
> + */
> +void serial8250_rs485_start_tx(struct uart_8250_port *up)
> +{
> + if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> + serial8250_stop_rx(&up->port);
> +
> + up->rs485_start_tx(up);
> +}
> +
> /* Returns false, if start_tx_timer was setup to defer TX start */
> static bool start_tx_rs485(struct uart_port *port)
> {
> @@ -1585,7 +1600,7 @@ static bool start_tx_rs485(struct uart_port *port)
> if (em485->tx_stopped) {
> em485->tx_stopped = false;
>
> - up->rs485_start_tx(up);
> + serial8250_rs485_start_tx(up);
If I get this correctly then this keeps the existing behavior when
up->rs485_start_tx == serial8250_em485_start_tx
Is this always the case, please?
The callback has been added by the commit 058bc104f7ca5c83d81
("serial: 8250: Generalize rs485 software emulation") because
8250_bcm2835aux.c driver needed to do something else.
Can start_tx_rs485() be called for the 8250_bcm2835aux.c driver?
Will it still work as expected?
>
> if (up->port.rs485.delay_rts_before_send > 0) {
> em485->active_timer = &em485->start_tx_timer;
Best Regards,
Petr
On 2024-09-17, Petr Mladek <pmladek@suse.com> wrote:
> Sigh, I am trying to review this patch but I am not familiar with the
> code. Feel free to ignore me when the questions are completely off.
I appreciate you researching where the code came from. I made my changes
based on what I see the code doing now.
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> void serial8250_em485_start_tx(struct uart_8250_port *up)
>> {
>> unsigned char mcr = serial8250_in_MCR(up);
>>
>> + /*
>> + * Some chips set the UART_LSR_DR bit even when UART_IER_RDI is
>> + * disabled, so explicitly mask it.
>> + */
>> if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
>> - serial8250_stop_rx(&up->port);
>> + up->port.read_status_mask &= ~UART_LSR_DR;
>
> This change is related to disabling UART_IER_RDI but we do not longer
> disable it in this code path.
Correct. It will be disabled in the new wrapper
serial8250_em485_start_tx(). For the console write() callback, RDI is
already being disabled (IER is cleared). It will not use the wrapper.
> Why do we need to do it here, please?
Because the console write() callback also needs to clear LSR_DR. That
part of the callback needs to stay.
> Why is it needed only in the em485-specific path, please?
Only RS485 deals with controlling TX/RX directions.
> On one hand, the comment talks about UART_LSR_DR and UART_IER_RDI
> so seems to be relater.
I do not know if the LSR_DR modify is strictly necessary. I am just
preserving the existing behavior (and related comment). The disabling of
IER_RDI will still happen (via wrapper or explicitly as in the console
write() callback).
>> static bool start_tx_rs485(struct uart_port *port)
>> {
>> @@ -1585,7 +1600,7 @@ static bool start_tx_rs485(struct uart_port *port)
>> if (em485->tx_stopped) {
>> em485->tx_stopped = false;
>>
>> - up->rs485_start_tx(up);
>> + serial8250_rs485_start_tx(up);
>
> If I get this correctly then this keeps the existing behavior when
>
> up->rs485_start_tx == serial8250_em485_start_tx
Correct.
> Is this always the case, please?
Yes.
> Can start_tx_rs485() be called for the 8250_bcm2835aux.c driver?
Yes.
> Will it still work as expected?
Yes, but it does perform an extra read. And since someone added a
comment just to mention that, I assume it was important for some use
case.
John
On Wed 2024-09-18 17:10:53, John Ogness wrote:
> On 2024-09-17, Petr Mladek <pmladek@suse.com> wrote:
> > Sigh, I am trying to review this patch but I am not familiar with the
> > code. Feel free to ignore me when the questions are completely off.
>
> I appreciate you researching where the code came from. I made my changes
> based on what I see the code doing now.
>
> >> --- a/drivers/tty/serial/8250/8250_port.c
> >> +++ b/drivers/tty/serial/8250/8250_port.c
> >> void serial8250_em485_start_tx(struct uart_8250_port *up)
> >> {
> >> unsigned char mcr = serial8250_in_MCR(up);
> >>
> >> + /*
> >> + * Some chips set the UART_LSR_DR bit even when UART_IER_RDI is
> >> + * disabled, so explicitly mask it.
> >> + */
> >> if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> >> - serial8250_stop_rx(&up->port);
> >> + up->port.read_status_mask &= ~UART_LSR_DR;
> >
> > This change is related to disabling UART_IER_RDI but we do not longer
> > disable it in this code path.
>
> Correct. It will be disabled in the new wrapper
> serial8250_em485_start_tx(). For the console write() callback, RDI is
> already being disabled (IER is cleared). It will not use the wrapper.
>
> > Why do we need to do it here, please?
>
> Because the console write() callback also needs to clear LSR_DR. That
> part of the callback needs to stay.
>
> > Why is it needed only in the em485-specific path, please?
>
> Only RS485 deals with controlling TX/RX directions.
>
> > On one hand, the comment talks about UART_LSR_DR and UART_IER_RDI
> > so seems to be relater.
>
> I do not know if the LSR_DR modify is strictly necessary. I am just
> preserving the existing behavior (and related comment). The disabling of
> IER_RDI will still happen (via wrapper or explicitly as in the console
> write() callback).
>
> >> static bool start_tx_rs485(struct uart_port *port)
> >> {
> >> @@ -1585,7 +1600,7 @@ static bool start_tx_rs485(struct uart_port *port)
> >> if (em485->tx_stopped) {
> >> em485->tx_stopped = false;
> >>
> >> - up->rs485_start_tx(up);
> >> + serial8250_rs485_start_tx(up);
> >
> > If I get this correctly then this keeps the existing behavior when
> >
> > up->rs485_start_tx == serial8250_em485_start_tx
>
> Correct.
>
> > Is this always the case, please?
>
> Yes.
>
> > Can start_tx_rs485() be called for the 8250_bcm2835aux.c driver?
>
> Yes.
IMHO, the answer "Yes" to both last questions can't be valid.
The 8250_bcm2835aux driver does:
static int bcm2835aux_serial_probe(struct platform_device *pdev)
{
[...]
up.rs485_start_tx = bcm2835aux_rs485_start_tx;
[...]
}
As a result, the 1st "Yes" was not correct:
up->rs485_start_tx != serial8250_em485_start_tx
and this patch would change the behavior for the 8250_bcm2835aux driver.
Before, start_tx_rs485() called directly:
up->rs485_start_tx(up);
Newly, it would call:
void serial8250_rs485_start_tx(struct uart_8250_port *up)
{
if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
serial8250_stop_rx(&up->port);
up->rs485_start_tx(up);
}
It means that it could call serial8250_stop_rx() even when it was not
called by the original code.
And SER_RS485_RX_DURING_TX seems to be checked even in
drivers/tty/serial/8250/8250_bcm2835aux.c. So, it looks like it
might be (un)set even for this driver.
Or is this code path prevented in start_tx_rs485()? I mean that
em485->tx_stopped could never be true for the 8250_bcm2835aux
driver?
But I see
static int bcm2835aux_serial_probe(struct platform_device *pdev)
{
[...]
up.port.rs485_config = serial8250_em485_config;
up.port.rs485_supported = serial8250_em485_supported;
[...]
}
=> It looks like even bcm2835aux driver could have the em485 thing.
But it obviously wanted to something special in
up->rs485_start_tx().
It looks to me that the change might either cause regression.
Or it would deserve a comment unless the validity is obvious for people
familiar with the code.
Best Regards,
Petr
© 2016 - 2024 Red Hat, Inc.