Message-ID: <56996FE5.8020604@hurleysoftware.com>
Date:	Fri, 15 Jan 2016 14:17:09 -0800
From:	Peter Hurley <peter@...leysoftware.com>
To:	"Matwey V. Kornilov" <matwey@....msu.ru>
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

On 01/15/2016 01:16 PM, Matwey V. Kornilov wrote:
> 2016-01-15 23:01 GMT+03:00 Matwey V. Kornilov <matwey@....msu.ru>:
>> 2016-01-15 22:45 GMT+03:00 Peter Hurley <peter@...leysoftware.com>:
>>> On 01/15/2016 10:42 AM, Matwey V. Kornilov wrote:
>>>> 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
>>>
>>> I know; that doesn't mean it's ok.
>>>
>>
>> What do you suggest? Run __stop_tx as a tasklet? I am not sure whether
>> it introduces races or not.
> 
> Would it be fine to use workqueues instead of timers? I mean
> schedule_delayed_work and cancel_delayed_work_sync.
> They use same timers with TIMER_IRQSAFE under the hood.
> Or it is better to allocate separate work queue in order to achieve
> better latency than shared system wq can provide?

I think just del_timer() and locking with the port lock should be
sufficient; timer + irq handler is nothing new.


>>>>>> +     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
>>>
>>> Ahh, missed that.
>>>
>>> Maybe it would be better simply to implement the config_rs485()
>>> generically, and just call it from the omap_8250 config_rs485().
>>>
>>> And put a note about the locking in a function comment header
>>>
>>> /**
>>>  *      serial8250_config_em485()       -       rs485 config helper
>>>  *
>>>  *      ....
>>>  */
>>>
>>>
>>>
>>>>>> +     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);
>>>
>>> But __start_tx() is labelled that way to differentiate it from being identified
>>> as the start_tx() handler (which is serial8250_start_tx()). IOW, contributors
>>> unfamiliar with the 8250 driver itself won't become confused when grepping
>>> for start_tx (or at least the idea is to minimize that confusion).
>>>
>>> start_tx_rs485() doesn't need differentiation, so doesn't require the
>>> double __ leader.
>>>
>>> FWIW, this is consistent and typical elsewhere in the kernel.
>>>
>>>
>>>>>> +{
>>>>>> +     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?
>>>
>>> The expected yardstick for inline is some demonstrable speed improvement;
>>> otherwise, size is favored.
>>>
>>> And __stop_tx() is already inlined in 3 places, which really doesn't
>>> need inlining either -- a call/ret is nothing compared to device i/o.
>>>
>>
>> Ok then, probably I am biased with my C++ experience and I am used to
>> think that compiler considers `inline` only as a hint.
>>
>>>
>>> Regards,
>>> Peter Hurley
>>>
>>>>>> +{
>>>>>> +     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)
>>>>>>

Powered by blists - more mailing lists