linux-kernel - Re: [linux-sunxi] Designware UART bug

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] [day] [month] [year] [list]

Date:   Thu, 4 May 2017 10:35:49 +0200
From:   Olliver Schinagl <oliver@...inagl.nl>
To:     Tim Kryger <tim.kryger@...il.com>
Cc:     Chen-Yu Tsai <wens@...e.org>, Jamie Iles <jamie@...ieiles.com>,
        Tim Kryger <tim.kryger@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dev <dev@...ux-sunxi.org>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>
Subject: Re: [linux-sunxi] Designware UART bug

Hey Tim,

On 04-05-17 05:51, Tim Kryger wrote:
> On Wed, May 3, 2017 at 8:40 AM, Olliver Schinagl <oliver@...inagl.nl> wrote:
>> Hey Tim,
>
>>
>> Ok, so as far as I understand (from the datasheet) the intended way to do
>> this would be to check for the BUSY IRQ & USR[0] IRQ and if it is busy,
>> (re-write) the LCR. We no longer do this because it did not work due to the
>> delayed interrupt.
>>
>> So one question is, why are we not checking the USR[0] reg whilst doing the
>> loop? Is that register not there for exactly that purpose? But I guess
>> brute-forcing it works more reliable I suppose then.
>
> That status bit isn't particularly helpful since even if it reports
> the UART is idle, there is no guarantee it won't become busy before
> the attempted write of the LCR happens.

I was thinking however of adding this to check_lcr:

		if (lcr & DW_UART_USR_BUSY)
			continue;

e.g. if it is busy, there is no point in trying to update the LCR as 
this will fail and will cause an interrupt again (the busy interrupt).

The rest of the forcefull logic then still holds.
>
>> But secondly, when is the UART_IIR_BUSY interrupt handled? And it not being
>> handled, could that be the actual reason things where failing? (Or as I read
>> somewhere a silicon bug?)
>>
>> Right now, we have serial8250_handle_irq() and if that returns 0, we check
>> if we (still) have an unhandled UART_IIR_BUSY interrupt.
>> But the only way for serial8250_handle_irq() to return 0, is if it has set
>> UART_NO_INT.
>>
>> If we do not have an IRQ, i'm pretty sure, UART_IIR_BUSY can't be triggered
>> right? And if it IS the interrupt that caused us to go into the interrupt
>> handler, well, handle_irq does its thing and then returns 1 for finishing
>> it, which in turn causes the UART_IIR_BUSY check to be skipped.
>>
>> So we never clear the UART_IIR_BUSY interrupt if we manage to trigger that.
>> (Please do correct me if I'm wrong.
>
> Note that the LSB is set in each of the following defines.
>
> #define UART_IIR_NO_INT         0x01 /* No interrupts pending */
> #define UART_IIR_BUSY           0x07 /* DesignWare APB Busy Detect */

Right, I am aware,
>
> Thus, when iir is UART_IIR_BUSY, serial8250_handle_irq bails out and
> returns zero such that the interrupt gets cleared by the read of USR
> in dw8250_handle_irq.

the trickery of course is here that we are treating the iir register as 
a bitfield, when really, it is not (anymore).

It just gets very confusing of course. We get an interrupt, we check 
which one it, either no interrupt, or busy.

So In the dw8250_handle_irq, we could first check if the uart is busy 
and then handle the busy case and return.

The reason while I'm looking so into this, is as I mentioned, we 
sometimes get the too much work for interrupt storm, and what I see from 
debugging, is that the busy interrupt is set, but never cleared (or 
constantly set).

Thanks for listening so far, and I will keep digging :)

>
>> I'm changing things in the interrupt handler a bit now to first check for
>> the busy interrupt first and if that is triggered do the dummy return (clear
>> it) and return (since LCR is handled alternativly.
>>
>> Olliver