2016-08-26 15:49 GMT+03:00 Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>:
> There are calls to serial8250_rpm_{get|put}() in __do_stop_tx_rs485() that are
> certainly placed in a wrong location. I dunno how it had been tested with
> runtime PM enabled because it is obvious "sleep in atomic context" error.
>
> Besides that serial8250_rpm_get() is called immediately after an IO just
> happened. It implies that the device is already powered on, see implementation
> of serial8250_em485_rts_after_send() and serial8250_clear_fifos() for the
> details.
>
> There is no bug have been seen due to, as I can guess, use of auto suspend mode
> when scheduled transaction to suspend is invoked quite lately than it's needed
> for a few writes to the port. It might be possible to trigger a warning if
> stop_tx_timer fires when device is suspended.
>
> Refactor the code to use runtime PM only in case of timer function.
>
> Cc: "Matwey V. Kornilov" <matwey@xxxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> drivers/tty/serial/8250/8250_port.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 9bad1c5..bc7cd1a 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1414,12 +1414,8 @@ static void __do_stop_tx_rs485(struct uart_8250_port *p)
> if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> serial8250_clear_fifos(p);
>
> - serial8250_rpm_get(p);
> -
> p->ier |= UART_IER_RLSI | UART_IER_RDI;
> serial_port_out(&p->port, UART_IER, p->ier);
> -
> - serial8250_rpm_put(p);
This has been introduced by 0c66940d584d1aac92f6a78460dc0ba2efd3b7ba
Don't you mind add corresponding Fixes tag?
Fixes: 0c66940d584d ("tty/serial/8250: fix RS485 half-duplex RX")
And I would like also
Cc: Yegor Yefremov <yegorslists@xxxxxxxxxxxxxx>
> }
> }
>
> @@ -1429,6 +1425,7 @@ static void serial8250_em485_handle_stop_tx(unsigned long arg)
> struct uart_8250_em485 *em485 = p->em485;
> unsigned long flags;
>
> + serial8250_rpm_get(p);
> spin_lock_irqsave(&p->port.lock, flags);
> if (em485 &&
> em485->active_timer == &em485->stop_tx_timer) {
> @@ -1436,6 +1433,7 @@ static void serial8250_em485_handle_stop_tx(unsigned long arg)
> em485->active_timer = NULL;
> }
> spin_unlock_irqrestore(&p->port.lock, flags);
> + serial8250_rpm_put(p);
> }
>
> static void __stop_tx_rs485(struct uart_8250_port *p)
> @@ -1475,7 +1473,7 @@ static inline void __stop_tx(struct uart_8250_port *p)
> unsigned char lsr = serial_in(p, UART_LSR);
> /*
> * To provide required timeing and allow FIFO transfer,
> - * __stop_tx_rs485 must be called only when both FIFO and
> + * __stop_tx_rs485() must be called only when both FIFO and
> * shift register are empty. It is for device driver to enable
> * interrupt on TEMT.
> */
> @@ -1484,9 +1482,10 @@ static inline void __stop_tx(struct uart_8250_port *p)
>
> del_timer(&em485->start_tx_timer);
> em485->active_timer = NULL;
> +
> + __stop_tx_rs485(p);
> }
> __do_stop_tx(p);
> - __stop_tx_rs485(p);
> }
>
> static void serial8250_stop_tx(struct uart_port *port)
> --
> 2.8.1
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
