Re: [PATCH 2/2] crypto: ccp - Mark driver as little-endian only

[Gary R Hook <ghook@amd.com>]

On 03/28/2017 09:59 AM, Arnd Bergmann wrote:
> On Tue, Mar 28, 2017 at 4:08 PM, Gary R Hook <ghook@amd.com> wrote:
>
>>> In fact, the use of bit fields in hardware defined data structures is
>>> not portable to start with, so until all these bit fields get replaced
>>> by something else, the driver cannot work on big-endian machines, and
>>> I'm adding an annotation here to prevent it from being selected.
>>
>>
>> This is a driver that talks to hardware, a device which, AFAIK, has no
>> plan to be implemented in a big endian flavor. I clearly need to be more
>> diligent in building with various checkers enabled. I'd prefer my fix
>> over your suggested refusal to compile, if that's okay.
>
> It's hard to predict the future. If this device ever makes it into an
> ARM based chip, the chances are relatively high that someone
> will eventually run a big-endian kernel on it. As long as it's guaranteed
> to be x86-only, the risk of anyone running into the bug is close to
> zero, but we normally still try to write device drivers in portable C
> code to prevent it from getting copied incorrectly into another driver.

Understood, and I had surmised as such. Totally agree.

>>> The CCPv3 code seems to not suffer from this problem, only v5 uses
>>> bitfields.
>>
>>
>> Yes, I took a different approach when I wrote the code. IMO (arguably)
>> more readable. Same result: words full of hardware-dependent bit patterns.
>>
>> Please help me understand what I could do better.
>
> The rule for portable drivers is that you must not use bitfields in
> structures
> that can be accessed by the hardware. I think you can do this in a more
> readable way by removing the CCP5_CMD_* macros etc completely
> and just accessing the members of the structure as __le32 words.
> The main advantage for readability here is that you can grep for the
> struct members and see where they are used without following the
> macros. If it helps, you can also encapsulate the generation of the
> word inside of an inline function, like:
>

Please see my follow-on patch.

-- 
This is my day job. Follow me at:
IG/Twitter/Facebook: @grhookphoto
IG/Twitter/Facebook: @grhphotographer