| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJs94EZwneXs1S5EpaqV8dcYNw1rjSiC0XqLjS5cLu1t=F2DJQ@mail.gmail.com>
Date: Sat, 16 Jan 2016 11:12:33 +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-16 1:17 GMT+03:00 Peter Hurley <peter@...leysoftware.com>:
> 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.
>
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?
>
>>>>>>> + 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)
>>>>>>>
>
--
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