2016-01-16 21:56 GMT+03:00 Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>: > 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 Sure, that's why I asked. I've looked through the timer implementation, and found that I need an additional variable to keep the state. The running timer is indistinguishable from deleted one. Initially, I hoped to check corresponding timer_list variable from timer function, but this would not work. > > 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) >>>>>>>>> >>> >> >> >> > -- 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