linux-kernel - Re: [PATCH 2/2] serial: msm: Add support for UARTDM cores

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110120090408.GA22101@n2100.arm.linux.org.uk>
Date:	Thu, 20 Jan 2011 09:04:09 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Stepan Moskovchenko <stepanm@...eaurora.org>
Cc:	Jamie Iles <jamie@...ieiles.com>, linux-arm-msm@...r.kernel.org,
	davidb@...eaurora.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, rlove@...gle.com
Subject: Re: [PATCH 2/2] serial: msm: Add support for UARTDM cores

On Wed, Jan 19, 2011 at 06:38:14PM -0800, Stepan Moskovchenko wrote:
> On 1/19/2011 2:24 PM, Russell King - ARM Linux wrote:
>> On Wed, Jan 19, 2011 at 02:08:29PM -0800, Stepan Moskovchenko wrote:
>>>>> +		/* Mask conditions we're ignorning. */
>>>>> +		sr&= port->read_status_mask;
>>>>> +		if (sr&   UART_SR_RX_BREAK)
>>>>> +			flag = TTY_BREAK;
>>>>> +		else if (sr&   UART_SR_PAR_FRAME_ERR)
>>>>> +			flag = TTY_FRAME;
>>>> It doesn't look like the flag is used anywhere after it has been
>>>> assigned.
>>> An artifact of an old driver. Removed.
>> But still required to support proper error signalling.
>>
>
> On second thought, from poking around the kernel some more, I believe  
> the following should be a better approach:
>         if (sr & UART_SR_RX_BREAK) {
>             tty_insert_flip_char(tty, 0, TTY_BREAK);
>         } else if (sr & UART_SR_PAR_FRAME_ERR) {
>             tty_insert_flip_char(tty, 0, TTY_FRAME);
>         }
>
> Russell, what do you think? Shall I make the change in v2?

Why not take a look at what a known good serial driver such as 8250.c
does?  The expected flow is something like:

        lsr = serial_inp(up, UART_LSR);
        do {
		if (!(lsr & UART_LSR_DR))
			break;
                ch = serial_inp(up, UART_RX);
                flag = TTY_NORMAL;
                up->port.icount.rx++;
                if (unlikely(lsr & UART_LSR_BRK_ERROR_BITS)) {

This is where we handle errors.

                        /*
                         * For statistics only
                         */
                        if (lsr & UART_LSR_BI) {

A break error shouldn't flag framing or parity errors

                                lsr &= ~(UART_LSR_FE | UART_LSR_PE);
                                up->port.icount.brk++;
                                /*
                                 * We do the SysRQ and SAK checking
                                 * here because otherwise the break
                                 * may get masked by ignore_status_mask
                                 * or read_status_mask.
                                 */
                                if (uart_handle_break(&up->port))
                                        continue;

You only need that if you want SAK handling (secure attention key) or
magic sysrq - which is useful for debugging.

                        } else if (lsr & UART_LSR_PE)
                                up->port.icount.parity++;
                        else if (lsr & UART_LSR_FE)
                                up->port.icount.frame++;
                        if (lsr & UART_LSR_OE)
                                up->port.icount.overrun++;

These update the per-port statistics.

                        /*
                         * Mask off conditions which should be ignored.
                         */
                        lsr &= up->port.read_status_mask;

This gets rid of conditions that the user has told TTY that they're not
interested in (based on termios flags).  Setting up this mask is specific
to the hardware.

                        if (lsr & UART_LSR_BI) {
                                DEBUG_INTR("handling break....");
                                flag = TTY_BREAK;
                        } else if (lsr & UART_LSR_PE)
                                flag = TTY_PARITY;
                        else if (lsr & UART_LSR_FE)
                                flag = TTY_FRAME;

This sets the error marker for the tty layer.

                }
                if (uart_handle_sysrq_char(&up->port, ch))
                        continue;

Again, if you're handling sysrq, then this passes the character to the
sysrq handlers.

                uart_insert_char(&up->port, lsr, UART_LSR_OE, ch, flag);

This finally does the handling of which characters to ignore, again
based on the users termios settings.

        } while (max_count-- > 0);

Without the handling with these errors, the termios settings for ignoring
parity, parity checking, break handling, marking parity errors, etc.  The
handling of these termios flags depend on this code:

  INPCK, BRKINT, PARMRK, IGNPAR, IGNBRK and CREAD.
--
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/