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] [thread-next>] [day] [month] [year] [list]
Message-ID: <cd12cd7b-dec1-2a1c-8769-39fc3d6cf8ae@schinagl.nl>
Date:   Wed, 3 May 2017 13:26:37 +0200
From:   Olliver Schinagl <oliver@...inagl.nl>
To:     Chen-Yu Tsai <wens@...e.org>
Cc:     jamie@...ieiles.com, 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 Chen-Yu

On 03-05-17 12:40, Chen-Yu Tsai wrote:
> On Wed, May 3, 2017 at 6:17 PM, Olliver Schinagl <oliver@...inagl.nl> wrote:
>> Hey Jamie,
>>
>> Several years ago you wrote the glue-code [0] for the DW 8250 IP. Over the
>> years various 'fixes' have been applied to resolve certain 'weird' problems
>> that Tim tried to fix with [1].
>>
>> After going over the datasheets and code with a comb several times now, I
>> think I may have found one (of a few others) reasons and would like both
>> your and Tim's thoughts here.
>>
>> The current (and original) code [2] uses the register offset 0x1f for the
>> UART_USR register.
>>
>> I searched far and wide, various datasheet of physical uarts (8250 - 16950)
>> and some IP cores and none seem to have the USR (and specifically the USR[0]
>> bit) register, so it seems to be specific to the DW_apb_uart. However
>> looking at the only databook available to me [3] of the UART IP, I cannot
>> seem to find anything at register offset 0x1f.
>>
>> The platform I'm using uses the Allwinner A20 SoC, which also features the
>> DW uart IP and also here, there is nothing at offset 0x1f.
>>
>> The intended register IS however actually at 0x7c.
>>
>> My question is thus twofold.
>>
>> Why was 0x1f used? Is this specific to a certain (version) UART or is this a
>> long uncaught typo.
>
> The original 8250 datasheets have registers at byte offsets. Note that the
> registers are only 8 bits wide.
Yes, I did account for that difference :) Or rather, it should be the 
first register after the scratchpad, but there is nothing after the 
scratchpad on the 8250's.
>
> On Allwinner, and many other platforms, the registers are still 8 bits
> wide, but are 32bit-aligned, likely for aligned bus access. 0x7c = 0x1f << 2.
> The 8250 core accessor functions are supposed to handle this for you.
Actually, they are 32 bit registers, but only the lowest 8 are used, to 
remain code-compatible with true 8 bitters. The end result, is the same 
of course.

As for the shift, I see now!

readb(p->membase + (offset << p->regshift));

And indeed, the regular defines are all indeed 8 bit offsets. Oversight 
on my part, and I changed the comment to make this slightly more clear, 
which goes into my greater uart series.

Thanks Chen-Yu for pointing this oversight of mine out!

>
> ChenYu
>
>> Tim, assuming it is a typo, could this the cause which made you implement
>> [1]? From what I understand, you keep writing the LCR until it takes.
So with ChenYu's explanation, the USR register holds a valid status, but 
it was and is never checked and thus the below part is still a valid 
question?

>>
>> Initially, the UART_IIR_BUSY check looked like this:
>>         if (serial8250_handle_irq(p, iir)) {
>>                 return 1;
>>         } else if ((iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
>>                 /* Clear the USR and write the LCR again. */
>>                 (void)p->serial_in(p, d->usr_reg);
>>                 p->serial_out(p, UART_LCR, d->last_lcr);
>>
>>                 return 1;
>>         }
>>
>> what I'm missing here is, that if UART_IIR_BUSY is set, we have to:
>> * check the d->usr_reg (UART_USR) bit 0
>> * wait until it becomes cleared (do not allow new data to be pushed out,
>> optionally force the data to be pushed out after a timeout)
>> * write LCR register (and check if it took (and no longer loop over the LCR
>> to see if the values stuck, in theory)).
>> * if we never get un-busy, something is wrong?
>>
>> All of this btw is currently moot anyway, as the only way to get into the
>> (else) if, is if serial8250_handle_irq returns false. And from what I can
>> see, this is only if iir == UART_IIR_NO_IRQ, in which case we never ever
>> clear the USR and thus never ever clear the UART_IIR_BUSY flag.
especially this point is important I suppose, hence the need for the 
workaround [1].

Olliver
>>
>> Olliver
>>
>>
>> [0]
>> https://github.com/torvalds/linux/commit/6b1a98d1c4851235d9b6764b3f7b7db7909fc760
>>
>> [1]
>> https://github.com/torvalds/linux/commit/c49436b657d0a56a6ad90d14a7c3041add7cf64d
>>
>> [2]
>> https://github.com/torvalds/linux/commit/6b1a98d1c4851235d9b6764b3f7b7db7909fc760#diff-d6e619fc4c51febf7880632fde5d0208R63
>>
>> [3] http://linux-sunxi.org/images/d/d2/Dw_apb_uart_db.pdf
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "linux-sunxi" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to linux-sunxi+unsubscribe@...glegroups.com.
>> For more options, visit https://groups.google.com/d/optout.