Re: [PATCH 5/6] gpio: Add new gpio-macsmc driver for Apple Macs

[Hector Martin <marcan@marcan.st>]

On 05/09/2022 19.20, Russell King (Oracle) wrote:
> So, I'm going with my first suggestion for the hex2bin() conversion
> above, and adding a comment thusly:
> 
>         /*
>          * The most significant nibble comes from k[0] and key bits 15..8
>          * The least significant nibble comes from k[1] and key bits 7..0
>          */
>         k[0] = key >> 8;
>         k[1] = key;
> 
> because I needed the comment to prove to myself that I wasn't breaking
> this code. Maybe it's obvious to you, but it isn't obvious to everyone.

Honestly, I don't see what this buys us over the original code. It's
longer, no more readable, makes you think about the order that
characters are stored in the string as an extra indirection step, which
as as you found out with having to write the comment, is easy to get
backwards.

But I will say it is at least *semantically* correct, if awkward.

Let's back up and talk a bit about SMC keys for a second.

First, SMC is a legacy mess and plays around with endianness wrong in
several places - there are values which are in wrong-endian for no
reason, etc. So any discussion over "what would happen on a big-endian
platform" is ultimately speculation. If this ever ends up running on
some ancient PowerPC Mac (did any of those ever ship with an SMC that
followed these semantics?) then we'll have to deal with the endianness
issues then and correct any incorrect assumptions, because right now we
just don't have the information on what Apple's *intent* was when
designing this whole thing, if there was an intent at all.

That said. When I designed this driver, and the way I understand the
hardware, I consider SMC keys to be 32-bit integers containing packed
ASCII characters in natural integer order, that is, 0xAABBCCDD
representing the fourcc ABCD in that order. The SMC backend is designed
with this in mind, and puts things in the right endian in the right
contexts when it comes to the actual interface with the SMC coprocessor
(which is, itself, a mix of shared memory - which is a bag of bytes -
and 64-bit mailbox messages - which are fundamentally integers and
merely represented in little-endian at the hardware level - so I'm sure
how you can see how this gets interesting).

In other words, at the driver level, *SMC keys are not character
strings, nor integers stored in some byte order*. They are integers.
Integers do not have a byte order until they are stored to memory.
Therefore, using functions that operate on strings on SMC keys is wrong,
and requires you to make a trip through endian-land to get it right (as
you found out).

Making the representation of SMC keys in the driver 32-bit integers
makes manipulating them easier and ergonomic in C, and allows for things
like comparisons (look at how the GPIO code uses < to compare SMC keys,
which maps to ASCIIbetical sort the way the keys are naturally encoded),
while basically relegating all the endian issues to the SMC core. For
comparison, if the data structure were a char[4] in reading order, there
would be no ergonomic way to do comparisons without some helper
function/macro. And comparisons are used quite a bit as part of the
self-discovery aspects of SMC (there's that binary search function to
find key indices, which also took like 4 tries to get right... please
don't break it! :).

This is why I added a printk specifier, because V4L/etc already had a
very special-purpose specifier with fancy rules just for them, and I
think a generic FOURCC style format specifier that works in any context
is useful (this isn't the only driver dealing with this kind of
FOURCC-style construct). The printk patch in particular adds 4
variations to the existing v4l specifier that that interpret endianness
differently, so it can be used in any context (in this context, the
specifier is 'h' which means 'host endian' and is the correct specifier
for abstract integers, which are passed by reference in this case and
therefore inherit the host endianness).

- Hector

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel