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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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