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

[Gary R Hook]

On 03/28/2017 09:59 AM, Arnd Bergmann wrote:
> On Tue, Mar 28, 2017 at 4:08 PM, Gary R Hook <ghook@xxxxxxx> 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