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: <f096009a-9bcd-e575-2c81-49d08f99fc44@cogentembedded.com>
Date:   Wed, 17 May 2017 08:50:39 +0300
From:   Nikita Yushchenko <nikita.yoush@...entembedded.com>
To:     Dong Aisheng <dongas86@...il.com>
Cc:     Dong Aisheng <aisheng.dong@....com>, linux-serial@...r.kernel.org,
        fugang.duan@....com, gregkh@...uxfoundation.org, yangbo.lu@....com,
        linux-kernel@...r.kernel.org, stefan@...er.ch, Mingkai.Hu@....com,
        jslaby@...e.com, linux-arm-kernel@...ts.infradead.org
Subject: Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register
 support

>>>>>  static u32 lpuart32_read(void __iomem *addr)
>>>>>  {
>>>>> -	return ioread32be(addr);
>>>>> +	return lpuart_is_be ? ioread32be(addr) : readl(addr);
>>>>>  }
>>>>>  
>>>>>  static void lpuart32_write(u32 val, void __iomem *addr)
>>>>>  {
>>>>> -	iowrite32be(val, addr);
>>>>> +	if (lpuart_is_be)
>>>>> +		iowrite32be(val, addr);
>>>>> +	else
>>>>> +		writel(val, addr);
>>>>>  }
>>>>
>>>> What if this is ever executed on big endian system?
>>>>
>>>
>>> Sorry, not catching the point...
>>>
>>> What issues will meet?
>>
>> Isn't writel() in host endian?
> 
> On big endian systems, it is supposed to run iowrite32be.

Your code states, "force BE if lpuart_is_be, don't care otherwise".
This semantics looks questionable for code reviewer.
If driver handles endian, should't it be explicit in both cases?
And if indeed driver means handling BE explicitly, but don't caring
otherwise, maybe variable name should suggest that (i.e. "force_be")?

Although driver maintainer could think differently. I won't insist on this.

Nikita