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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Mon, Sep 5, 2022 at 1:20 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
> On Fri, Sep 02, 2022 at 06:43:36PM +0300, Andy Shevchenko wrote:
> > On Fri, Sep 2, 2022 at 6:34 PM Russell King (Oracle)
> > <linux@armlinux.org.uk> wrote:
> > > On Fri, Sep 02, 2022 at 05:53:25PM +0300, Andy Shevchenko wrote:

...

> > > static int macsmc_gpio_nr(smc_key key)
> > > {
> > >          __le16 foo = cpu_to_le16(key);
> > >          u8 result;
> > >          int ret;
> > >
> > >          ret = hex2bin(&result, (char *)&foo, 1);
> > >          if (ret < 0)
> > >                  return ret;
> > >
> > >          return result;
> > > }
> > >
> > > which you also didn't like,
> >
> > ...based on the wrong suggestion below. That said, the above is fine to me.
>
> To be honest, using the endian conversion macro there doesn't feel
> right and is more prone to programming errors. I can't tell just by
> looking at it that either cpu_to_le16() or cpu_to_le32() would be the
> right thing here - and if it's not obvious then it's a bug waiting to
> happen.
>
> As if to prove the point, the above suggestions turn out to *all* be
> buggy.
>
> The initial suggestion gets the k[0] and k[1] assignment round the
> wrong way. The second, le16() is definitely not the right conversion.
> If we start using the endian conversion macros, then this is going to
> screw up if someone runs a BE kernel against the SMC (since the
> _SMC_KEY() macro will still be doing its conversion.)
>
> This seems utterly counter-productive, and I've spent quite a long
> time trying to work out what would be correct.
>
> At this point, I'm not sure that changing what has already been
> established in the Asahi Linux tree for something entirely different
> in mainline is going to be practical - it's a recipe for repeated
> mistakes converting keys from the Asahi kernel to the mainline
> kernel.
>
> It's not _just_ the GPIO driver. There are multiple other drivers
> that will be impacted by changing the scheme here.
>
> Any change to the scheme for these SMC keys  needs to happen in the
> Asahi kernel tree by the Asahi Linux maintainers, not by someone
> pushing the code upstream - doing so would be a recipe for repeated
> trainwrecks.
>
> 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.

And how is it different to the key being __be16 and all operations
against it be correct with the endianness helpers? Adding redundant
comments when the bitwise type exists seems just like being afraid of
the unknown. Ah, I see that in one of your long letters the proposal
somehow switched from (implicit) be16 to (explicit) le16... Still to
me it's not enough justification for the comment, but since it has no
effect on the code generation, add it if you think it would be better.

-- 
With Best Regards,
Andy Shevchenko