Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250

From: Peter Hurley
Date: Sat Jan 16 2016 - 13:57:14 EST

Next message: Dmitry Torokhov: "Re: [PATCH RESEND] driver-core: fix modparam async_probe request"
Previous message: Helge Deller: "Re: [patch 1/4] hrtimer: Handle remaining time proper for TIME_LOW_RES"
In reply to: Matwey V. Kornilov: "Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250"
Next in thread: Matwey V. Kornilov: "Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 01/16/2016 12:12 AM, Matwey V. Kornilov wrote:
> 2016-01-16 1:17 GMT+03:00 Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>:
>> On 01/15/2016 01:16 PM, Matwey V. Kornilov wrote:
>>> 2016-01-15 23:01 GMT+03:00 Matwey V. Kornilov <matwey@xxxxxxxxxx>:
>>>> 2016-01-15 22:45 GMT+03:00 Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>:
>>>>> On 01/15/2016 10:42 AM, Matwey V. Kornilov wrote:
>>>>>> 2016-01-15 19:14 GMT+03:00 Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>:
>>>>>>> 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@xxxxxxxxxx>
>>>>>>>> ---
>>>>>>>> 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.
>>
>
> Do I understand correctly, that internals of
> serial8250_em485_handle_stop_tx and serial8250_em485_handle_start_tx
> should be wrapped with port->lock in order to ensure that they are not
> running during the call going to run del_timer?

Yes.

Of course, you'll need some state mechanism to know in the timer function
that the timer was cancelled. For example, in this situation

CPU 0 CPU 1

start_tx_rs485() [timer fires]
del_timer(stop_tx_timer)
handle_stop_tx()
spin_lock_irqsave(port lock)
*waits*
rts_on_send()
mod_timer(start_tx_timer)
*claims port lock*

* obviously would be bad if *
* do_stop_tx_rs485() ran now *

>>>>>>>> + 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)
>>>>>>>>
>>
>
>
>

Next message: Dmitry Torokhov: "Re: [PATCH RESEND] driver-core: fix modparam async_probe request"
Previous message: Helge Deller: "Re: [patch 1/4] hrtimer: Handle remaining time proper for TIME_LOW_RES"
In reply to: Matwey V. Kornilov: "Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250"
Next in thread: Matwey V. Kornilov: "Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]