Message-ID: <CAJs94Ea3MfLOniYEe39MisTdV=0pZ2CsRR5PBnCbanLU5ng5GQ@mail.gmail.com>
Date:	Fri, 15 Jan 2016 21:42:30 +0300
From:	"Matwey V. Kornilov" <matwey@....msu.ru>
To:	Peter Hurley <peter@...leysoftware.com>
Cc:	Greg KH <gregkh@...uxfoundation.org>, Jiri Slaby <jslaby@...e.com>,
	Andy Shevchenko <andy.shevchenko@...il.com>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	linux-serial@...r.kernel.org
Subject: Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250

2016-01-15 19:14 GMT+03:00 Peter Hurley <peter@...leysoftware.com>:
> On 12/21/2015 10:26 AM, Matwey V. Kornilov wrote:
>> Implementation of software emulation of RS485 direction handling is based
>> on omap_serial driver.
>> Before and after transmission RTS is set to the appropriate value.
>>
>> Note that before calling serial8250_em485_init the caller has to
>> ensure that UART will interrupt when shift register empty. Otherwise,
>> emultaion cannot be used.
>>
>> Both serial8250_em485_init and serial8250_em485_destroy are
>> idempotent functions.
>
> Apologies for the long delay; comments below.
>
>> Signed-off-by: Matwey V. Kornilov <matwey@....msu.ru>
>> ---
>>  drivers/tty/serial/8250/8250.h      |   6 ++
>>  drivers/tty/serial/8250/8250_port.c | 161 +++++++++++++++++++++++++++++++++++-
>>  include/linux/serial_8250.h         |   7 ++
>>  3 files changed, 170 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>> index d54dcd8..0189cb3 100644
>> --- a/drivers/tty/serial/8250/8250.h
>> +++ b/drivers/tty/serial/8250/8250.h
>> @@ -117,6 +117,12 @@ static inline void serial_dl_write(struct uart_8250_port *up, int value)
>>  struct uart_8250_port *serial8250_get_port(int line);
>>  void serial8250_rpm_get(struct uart_8250_port *p);
>>  void serial8250_rpm_put(struct uart_8250_port *p);
>> +int serial8250_em485_init(struct uart_8250_port *p);
>> +void serial8250_em485_destroy(struct uart_8250_port *p);
>> +static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
>> +{
>> +     return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
>
> Under what circumstances is p->em485 != NULL but
> (p->port.rs485.flags & SER_RS485_ENABLED) is true?
>
> ISTM, p->em485 is necessary and sufficient to determine if em485 is enabled.
>
> In which case, this function can be eliminated and callers can be reduced to
>
>         if (p->em485)
>                 ....
>
>> +}
>>
>>  #if defined(__alpha__) && !defined(CONFIG_PCI)
>>  /*
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 8ad0b2d..d67a848 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -37,6 +37,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/uaccess.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/timer.h>
>>
>>  #include <asm/io.h>
>>  #include <asm/irq.h>
>> @@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>>       }
>>  }
>>
>> +static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
>
> Only one call site, so please drop inline.
>
>
>> +{
>> +     unsigned char mcr = serial_in(p, UART_MCR);
>> +
>> +     if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>> +             mcr |= UART_MCR_RTS;
>> +     else
>> +             mcr &= ~UART_MCR_RTS;
>> +     serial_out(p, UART_MCR, mcr);
>> +}
>> +
>> +static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
>
> Doesn't really need to be inline.
>
>
>> +{
>> +     unsigned char mcr = serial_in(p, UART_MCR);
>> +
>> +     if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
>> +             mcr |= UART_MCR_RTS;
>> +     else
>> +             mcr &= ~UART_MCR_RTS;
>> +     serial_out(p, UART_MCR, mcr);
>> +}
>> +
>> +static void serial8250_em485_handle_start_tx(unsigned long arg);
>> +static void serial8250_em485_handle_stop_tx(unsigned long arg);
>> +
>>  void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p)
>>  {
>>       serial8250_clear_fifos(p);
>> @@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
>>  }
>>  EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>
>> +int serial8250_em485_init(struct uart_8250_port *p)
>> +{
>> +     if (p->em485 != NULL)
>> +             return 0;
>> +
>> +     p->em485 = kmalloc(sizeof(struct uart_8250_em485), GFP_KERNEL);
>> +     if (p->em485 == NULL)
>> +             return -ENOMEM;
>> +
>> +     init_timer(&p->em485->stop_tx_timer);
>> +     p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
>> +     p->em485->stop_tx_timer.data = (unsigned long)p;
>> +     p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
>
> Not sure this is going to fly; this would be the only user of TIMER_IRQSAFE
> (which was specifically introduced to workaround workqueue issues and not
> meant for general use).

This is required to call del_timer_sync(&p->em485->start_tx_timer);
from __stop_tx_rs485

>
>
>> +     init_timer(&p->em485->start_tx_timer);
>> +     p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx;
>> +     p->em485->start_tx_timer.data = (unsigned long)p;
>> +     p->em485->start_tx_timer.flags |= TIMER_IRQSAFE;
>> +
>> +     serial8250_em485_rts_after_send(p);
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(serial8250_em485_init);
>
> Newline.
>
>> +void serial8250_em485_destroy(struct uart_8250_port *p)
>> +{
>> +     if (p->em485 == NULL)
>> +             return;
>> +
>> +     del_timer_sync(&p->em485->start_tx_timer);
>> +     del_timer_sync(&p->em485->stop_tx_timer);
>
> What keeps start_tx() from restarting a new timer right here?

Both start_tx and rs485_config (which calls destroy) are wrapped with
port->lock in serial_core.c

>
>> +     kfree(p->em485);
>> +     p->em485 = NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(serial8250_em485_destroy);
>> +
>>  /*
>>   * These two wrappers ensure that enable_runtime_pm_tx() can be called more than
>>   * once and disable_runtime_pm_tx() will still disable RPM because the fifo is
>> @@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
>>       serial8250_rpm_put(up);
>>  }
>>
>> -static inline void __stop_tx(struct uart_8250_port *p)
>> +static __u32 __start_tx_rs485(struct uart_8250_port *p)
>           ^^^^^
> No need to preserve the userspace type here.
>
> The double underline leader in an identifier is typically used to distinguish
> an unlocked version from a locked version. I don't think it's necessary here
> or any of the other newly-introduced functions.

I use double __ for consistency with __start_tx. Now I have:

        if (up->em485)
                __start_tx_rs485(port);
        else
                __start_tx(port);

>
>
>> +{
>> +     if (!serial8250_em485_enabled(p))
>> +             return 0;
>
> Already checked that em485 was enabled in lone caller.
>
>
>> +     if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>> +             serial8250_stop_rx(&p->port);
>> +
>> +     del_timer_sync(&p->em485->stop_tx_timer);
>> +
>> +     if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p, UART_MCR) & UART_MCR_RTS)) {
>
> Line too long. And just one negation is sufficient, ie.
>
>         if (!(....) !=
>             !(....)) {
>

I would like to keep the double negation, in my opinion it is more
clear to the reader and I believe that the compiler is able to
optimize it.

>> +             serial8250_em485_rts_on_send(p);
>> +             return p->port.rs485.delay_rts_before_send;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static inline void __do_stop_tx_rs485(struct uart_8250_port *p)
>
> Does this really need to be inline?
>

Why not?

>> +{
>> +     if (!serial8250_em485_enabled(p))
>> +             return;
>> +
>> +     serial8250_em485_rts_after_send(p);
>> +     /*
>> +     * Empty the RX FIFO, we are not interested in anything
>> +     * received during the half-duplex transmission.
>> +     */
>
> Malformed block comment.
>
>         /*
>          *
>          *
>          */
>
>> +     if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>> +             serial8250_clear_fifos(p);
>> +}
>> +
>> +static void serial8250_em485_handle_stop_tx(unsigned long arg)
>> +{
>> +     struct uart_8250_port *p = (struct uart_8250_port *)arg;
>> +
>> +     __do_stop_tx_rs485(p);
>> +}
>> +
>> +static inline void __stop_tx_rs485(struct uart_8250_port *p)
>
> Single caller so drop inline.
>
>> +{
>> +     if (!serial8250_em485_enabled(p))
>> +             return;
>> +
>> +     del_timer_sync(&p->em485->start_tx_timer);
>> +
>> +     /* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */
>
> Block comment.
>
>> +     if (p->port.rs485.delay_rts_after_send > 0) {
>> +             mod_timer(&p->em485->stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);
>
> Line too long; please re-format.
> This is one problem with overly long identifiers.
>
>> +     } else {
>> +             __do_stop_tx_rs485(p);
>> +     }
>> +}
>> +
>> +static inline void __do_stop_tx(struct uart_8250_port *p)
>>  {
>>       if (p->ier & UART_IER_THRI) {
>>               p->ier &= ~UART_IER_THRI;
>> @@ -1302,6 +1418,21 @@ static inline void __stop_tx(struct uart_8250_port *p)
>>       }
>>  }
>>
>> +static inline void __stop_tx(struct uart_8250_port *p)
>> +{
>> +     if (serial8250_em485_enabled(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 shift register
>> +      * are empty. It is for device driver to enable interrupt on TEMT.
>> +      */
>
> Block indent.
>
> This code path should cancel start timer also.
>
>> +             if (!((lsr & UART_LSR_TEMT) && (lsr & UART_LSR_THRE)))
>
>                 if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>
>> +                     return;
>> +     }
>> +     __do_stop_tx(p);
>> +     __stop_tx_rs485(p);
>> +}
>> +
>>  static void serial8250_stop_tx(struct uart_port *port)
>>  {
>>       struct uart_8250_port *up = up_to_u8250p(port);
>> @@ -1319,12 +1450,10 @@ static void serial8250_stop_tx(struct uart_port *port)
>>       serial8250_rpm_put(up);
>>  }
>>
>> -static void serial8250_start_tx(struct uart_port *port)
>> +static inline void __start_tx(struct uart_port *port)
>>  {
>>       struct uart_8250_port *up = up_to_u8250p(port);
>>
>> -     serial8250_rpm_get_tx(up);
>> -
>>       if (up->dma && !up->dma->tx_dma(up))
>>               return;
>>
>> @@ -1350,6 +1479,30 @@ static void serial8250_start_tx(struct uart_port *port)
>>       }
>>  }
>>
>> +static void serial8250_em485_handle_start_tx(unsigned long arg)
>> +{
>> +     struct uart_8250_port *p = (struct uart_8250_port *)arg;
>> +
>> +     __start_tx(&p->port);
>> +}
>> +
>> +static void serial8250_start_tx(struct uart_port *port)
>> +{
>> +     struct uart_8250_port *up = up_to_u8250p(port);
>> +     __u32 delay;
>
>         int delay;
>
>> +
>> +     serial8250_rpm_get_tx(up);
>> +
>> +     if (up->em485 && timer_pending(&up->em485->start_tx_timer))
>> +             return;
>> +
>> +     if (up->em485 && (delay = __start_tx_rs485(up))) {
>
> No assignment in conditional please.
>
>> +             mod_timer(&up->em485->start_tx_timer, jiffies + delay * HZ / 1000);
>> +     } else {
>> +             __start_tx(port);
>> +     }
>
> Generally, braces aren't used for single statement if..else.
> That probably won't apply here after removing the assignment-in-conditional,
> but I thought it worth mentioning just so you know.
>
> Regards,
> Peter Hurley
>
>> +}
>> +
>>  static void serial8250_throttle(struct uart_port *port)
>>  {
>>       port->throttle(port);
>> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
>> index faa0e03..71516ec 100644
>> --- a/include/linux/serial_8250.h
>> +++ b/include/linux/serial_8250.h
>> @@ -76,6 +76,11 @@ struct uart_8250_ops {
>>       void            (*release_irq)(struct uart_8250_port *);
>>  };
>>
>> +struct uart_8250_em485 {
>> +     struct timer_list       start_tx_timer; /* "rs485 start tx" timer */
>> +     struct timer_list       stop_tx_timer; /* "rs485 stop tx" timer */
>> +};
>> +
>>  /*
>>   * This should be used by drivers which want to register
>>   * their own 8250 ports without registering their own
>> @@ -122,6 +127,8 @@ struct uart_8250_port {
>>       /* 8250 specific callbacks */
>>       int                     (*dl_read)(struct uart_8250_port *);
>>       void                    (*dl_write)(struct uart_8250_port *, int);
>> +
>> +     struct uart_8250_em485 *em485;
>>  };
>>
>>  static inline struct uart_8250_port *up_to_u8250p(struct uart_port *up)
>>
>



-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382

Powered by blists - more mailing lists