| 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
| ||
|
Date: Thu, 7 Aug 2014 16:46:15 -0400
From: Murali Karicheri <m-karicheri2@...com>
To: Peter Hurley <peter@...leysoftware.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
<devicetree@...r.kernel.org>, <linux-doc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-serial@...r.kernel.org>,
Rob Herring <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
Randy Dunlap <rdunlap@...radead.org>,
Jiri Slaby <jslaby@...e.cz>,
Santosh Shilimkar <santosh.shilimkar@...com>
Subject: Re: [PATCH v2] serial: uart: add hw flow control support configuration
On 08/07/2014 02:33 PM, Peter Hurley wrote:
> On 08/07/2014 01:34 PM, Murali Karicheri wrote:
>> On 08/07/2014 01:25 PM, Peter Hurley wrote:
>>> On 08/07/2014 01:12 PM, Greg Kroah-Hartman wrote:
>>>> On Thu, Aug 07, 2014 at 12:16:59PM -0400, Murali Karicheri wrote:
>>>>> On 08/07/2014 11:29 AM, Peter Hurley wrote:
>>>>>> On 05/01/2014 03:04 PM, Murali Karicheri wrote:
>>>>>>> 8250 uart driver currently supports only software assisted hw flow
>>>>>>> control. The software assisted hw flow control maintains a hw_stopped
>>>>>>> flag in the tty structure to stop and start transmission and use modem
>>>>>>> status interrupt for the event to drive the handshake signals. This is
>>>>>>> not needed if hw has flow control capabilities. This patch adds a
>>>>>>> DT attribute for enabling hw flow control for a uart port. Also skip
>>>>>>> stop and start if this flag is present in flag field of the port
>>>>>>> structure.
>>>>>>>
>>>>>>> Signed-off-by: Murali Karicheri<m-karicheri2@...com>
>>>>>>>
>>>>>>> CC: Rob Herring<robh+dt@...nel.org>
>>>>>>> CC: Pawel Moll<pawel.moll@....com>
>>>>>>> CC: Mark Rutland<mark.rutland@....com>
>>>>>>> CC: Ian Campbell<ijc+devicetree@...lion.org.uk>
>>>>>>> CC: Kumar Gala<galak@...eaurora.org>
>>>>>>> CC: Randy Dunlap<rdunlap@...radead.org>
>>>>>>> CC: Greg Kroah-Hartman<gregkh@...uxfoundation.org>
>>>>>>> CC: Jiri Slaby<jslaby@...e.cz>
>>>>>>> CC: Santosh Shilimkar<santosh.shilimkar@...com>
>>>>>>> ---
>>>>>>> .../devicetree/bindings/serial/of-serial.txt | 1 +
>>>>>>> drivers/tty/serial/8250/8250_core.c | 6 ++++--
>>>>>>> drivers/tty/serial/of_serial.c | 4 ++++
>>>>>>> drivers/tty/serial/serial_core.c | 12 +++++++++---
>>>>>>> 4 files changed, 18 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>>>>>>> index b68550d..851707a 100644
>>>>>>> --- a/drivers/tty/serial/serial_core.c
>>>>>>> +++ b/drivers/tty/serial/serial_core.c
>>>>>>> @@ -174,8 +174,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>>>>>>> if (tty->termios.c_cflag& CBAUD)
>>>>>>> uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
>>>>>>> }
>>>>>>> -
>>>>>>> - if (tty_port_cts_enabled(port)) {
>>>>>>> + /*
>>>>>>> + * if hw support flow control without software intervention,
>>>>>>> + * then skip the below check
>>>>>>> + */
>>>>>>> + if (tty_port_cts_enabled(port)&&
>>>>>>> + !(uport->flags& UPF_HARD_FLOW)) {
>>>>>>> spin_lock_irq(&uport->lock);
>>>>>>> if (!(uport->ops->get_mctrl(uport)& TIOCM_CTS))
>>>>>>> tty->hw_stopped = 1;
>>>>>>> @@ -2772,7 +2776,9 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
>>>>>>>
>>>>>>> uport->icount.cts++;
>>>>>>>
>>>>>>> - if (tty_port_cts_enabled(port)) {
>>>>>>> + /* skip below code if the hw flow control is supported */
>>>>>>> + if (tty_port_cts_enabled(port)&&
>>>>>>> + !(uport->flags& UPF_HARD_FLOW)) {
>>>>>> Why is a modem status interrupt being generated for DCTS
>>>>>> if autoflow control is enabled?
>>>>>>
>>>>>> This should be:
>>>>>>
>>>>>> WARN_ON_ONCE(uport->flags& UPF_HARD_FLOW);
>>>>>>
>>>>>> to indicate a mis-configured driver/device.
>>>>> This patch is already merged to the upstream branch and if you see any
>>>>> issue, please
>>>>> post a patch for review.
>>>>
>>>> If someone points out a problem in a patch of yours that is accepted
>>>> upstream, it is nice to provide a fix, otherwise I will just revert it
>>>> upstream, as it looks to be incorrect.
>>>>
>>>> So, should I just revert it?
>>>
>>> Greg,
>>>
>>> As I look this patch over further, this should just be reverted.
>>
>> Sorry, I would suggest to fix it rather revert it.
>>
>>>
>>> 1. The patch enables UPF_HARD_FLOW, but provides no throttle() and unthrottle()
>>> methods for 8250, which is guaranteed to blow-up when either uart_throttle() or
>>> uart_unthrottle() is called.
>>>
>>> 2. The patch adds capabilities which already exist, namely UART_CAP_AFE.
>
>> AFAIK, UART_CAP_AFE is a software assisted hw flow control and it was described in my commit log as well where as this patch add support for pure h/w controlled flow control and no software intervention is needed. Do you think uart_throttle() or uart_unthrottle() is applicable
>> in this case?
>
> UART_CAP_AFE is used to indicate 16750-compatible hw flow control, which is
> auto-CTS and auto-RTS flow control as described in the TI datasheet at
> http://www.ti.com/lit/ds/symlink/tl16c750.pdf
Peter,
This patch was added to support hw flow control on boards based on
Keystone SoCs that has UART with h/w flow control capability. Keystone
SoCs UART spec is at http://www.ti.com/lit/ug/sprugp1/sprugp1.pdf
and it uses tl16550c as per the document. The equivalent spec seems to
be http://www.ti.com/lit/ds/symlink/tl16c550c.pdf. Only difference
between tl16c750 and tl16c550 seems to be 16 byte FIFO available instead
of 16 or 64 byte FIFO in 16c750.
And about your original question
>>>>>> Why is a modem status interrupt being generated for DCTS
>>>>>> if autoflow control is enabled?
Are you asking why this patch didn't disable generating CTS interrupt
when h/w flow control is enable? If so,
unsigned int serial8250_modem_status(struct uart_8250_port *up)
{
...
if (status & UART_MSR_DCTS)
uart_handle_cts_change(port, status & UART_MSR_CTS);
}
Probably in the above check if UPF_HARD_FLOW is set, we can avoid
calling uart_handle_cts_change() and undo the current code change in
uart_handle_cts_change() and just add a WARN_ON_ONCE() as you have
suggested.
>
> uart_throttle() and uart_unthrottle() are used indirectly by line disciplines
> for high-level rx flow control, such as when a read buffer fills up because
> there is no userspace reader. The 8250 core doesn't define a throttle/unthrottle
> method because writing MCR to drop RTS is sufficient to disable auto-RTS.
As per spec. hardware has rx threshold levels set to trigger an RTS
level change to tell the remote from sending more bytes. So if h/w flow
control is enabled, then not sure why uart_throttle() is to be doing
anything when h/w flow control is supported? A dummy function required
to satisfy the line discipline?
Also 8250.c support other port types that doesn't have AFE. So shoudl
this be driving RTS line to stop the sender and resume when
uart_unthrottle() is called?
I want to work to fix this rather than revert this change as our
customer is already using this feature. I will send out a patch based on
this discussion.
Thanks and Regards,
Murali
>
> Regards,
> Peter Hurley
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists