linux-kernel - Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support

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: <045d1888-4a23-cf73-ab4c-009e29fde974@cogentembedded.com>
Date:   Wed, 17 May 2017 11:04:04 +0300
From:   Nikita Yushchenko <nikita.yoush@...entembedded.com>
To:     Dong Aisheng <dongas86@...il.com>
Cc:     "A.S. Dong" <aisheng.dong@....com>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        Andy Duan <fugang.duan@....com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "Y.B. Lu" <yangbo.lu@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stefan@...er.ch" <stefan@...er.ch>,
        Mingkai Hu <mingkai.hu@....com>,
        "jslaby@...e.com" <jslaby@...e.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register
 support

Hi

My view of your statement is:
- you currently assume only a few cases for this driver - builtin UART
in vf610, ls1012a and imx7,
- in each of these cases, all lpuart instances share same endian, thus
having that in global var works for these cases,
- having that in global var makes it possible for you to write less
lines of code

My complain is:
- in Linux, we are trying to keep drivers generic,
- in Linux, having less lines of code has never been sufficient to break
basic data structure consistency,
- having driver to keep per-device capability in global var is a clear
case of breaking consistency.


>>> That's for special case, normally we wouldn't do that.
>>
>> For me this "special case" looks like "let's break data structure
>> consistency to reuse several lines of code".
>>
>> With code snippets you show, it looks even worse: you assign same global
>> variable in several places for different uses. 
> 
> If you mean lpuart_is_be, it's not for different uses.
> The purpose is the same to align the correct endian but in two places.

_probe() routine called for device X alters state already in use for
device Y.

> 
>> implicitly assuming that
>> it is for same device. Which can be true in your current system, but not
>> elsewhere (e.g. why not having lpuart programmed into fpga)?
>>
> 
> Sorry, What issues for fpga?

Connect FPGA to IMX7 based system and program LS1012a version of lpuart
core into it.  Have your console on system UART broken at time when
driver gets registered.


> 
>> Alternative solution could be - have separate write path for earlycon.
> 
> It looks to me having the same issue with a separate write patch
> for earlycon as we still need distinguish Little or Big endian
> for Layerscape and IMX.
> 
>> At a glance, it is dozen lines of code.
> 
> Would you please show some sample code?

Do not reuse lpuart32_console_putchar() in earlycon code.

Have two sets of early_setup/early_write/putchar - for BE and
defaut-endian earlycon. And in these putchar's do not use
lpuart_(read|write).


As far as I can see, fsl_lpuart.c already has two drivers in one -
there is separate set of routines for 8bit and 32bit cases.
And those routines that are common, have if blocks that separate cases.
I think these drivers will be cleaner if separated.
However that's completely different story.