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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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:
> > > On Fri, Sep 2, 2022 at 5:46 PM Russell King (Oracle)
> > > <linux@armlinux.org.uk> wrote:
> > > > On Fri, Sep 02, 2022 at 04:39:16PM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Sep 2, 2022 at 2:33 PM Russell King (Oracle)
> > > > > <linux@armlinux.org.uk> wrote:
> > > > > > On Fri, Sep 02, 2022 at 01:37:14PM +0300, Andy Shevchenko wrote:
> > > > > > > On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle)
> > > > > > > <linux@armlinux.org.uk> wrote:
> > > > > > > > On Thu, Sep 01, 2022 at 09:55:23PM +0300, Andy Shevchenko wrote:
> > > > > > > > > > +static int macsmc_gpio_nr(smc_key key)
> > > > > > > > > > +{
> > > > > > > > > > +       int low = hex_to_bin(key & 0xff);
> > > > > > > > > > +       int high = hex_to_bin((key >> 8) & 0xff);
> > > > > > > > > > +
> > > > > > > > > > +       if (low < 0 || high < 0)
> > > > > > > > > > +               return -1;
> > > > > > > > > > +
> > > > > > > > > > +       return low | (high << 4);
> > > > > > > > > > +}
> > > > > > > > >
> > > > > > > > > NIH hex2bin().
> > > > > > > >
> > > > > > > > Is using hex2bin really better?
> > > > > > >
> > > > > > > Yes.
> > > > > > >
> > > > > > > > static int macsmc_gpio_nr(smc_key key)
> > > > > > > > {
> > > > > > > >         char k[2];
> > > > > > > >         u8 result;
> > > > > > > >         int ret;
> > > > > > > >
> > > > > > > >         k[0] = key;
> > > > > > > >         k[1] = key >> 8;
> > > > > > > >
> > > > > > > >         ret = hex2bin(&result, k, 2);
> > > > > > > >         if (ret < 0)
> > > > > > > >                 return ret;
> > > > > > > >
> > > > > > > >         return result;
> > > > > > > > }
> > > > > > > >
> > > > > > > > This looks to me like it consumes more CPU cycles - because we have to
> > > > > > > > write each "character" to the stack, then call a function, only to then
> > > > > > > > call the hex_to_bin() function. One can't just pass "key" into hex2bin
> > > > > > > > because that will bring with it endian issues.
> > > > > > >
> > > > > > > With one detail missed, why do you need all that if you can use
> > > > > > > byteorder helpers()? What's the stack? Just replace this entire
> > > > > > > function with the respectful calls to hex2bin().
> > > > > >
> > > > > > Sorry, I don't understand what you're suggesting, because it doesn't
> > > > > > make sense to me. The byteorder helpers do not give a char array, which
> > > > > > is what hex2bin() wants, so we end up with something like:
> > > > > >
> > > > > >         __le16 foo = cpu_to_le16(key);
> > > > > >         u8 result;
> > > > > >
> > > > > >         ret = hex2bin(&result, (char *)&foo, 1);
> > > > > >         if (ret < 0)
> > > > > >                 return ret;
> > > > > >
> > > > > >         return result;
> > > > > >
> > > > > > This to me looks like yucky code, It still results in "foo" having to
> > > > > > be on the stack, because the out-of-line hex2bin() requires a pointer
> > > > > > to be passed as the second argument.
> > > > > >
> > > > > > Maybe you could provide an example of what you're thinking of, because
> > > > > > I'm at a loss to understand what you're thinking this should look like.
> > > > >
> > > > > So, let's look into the real callers to see, oh wait, it's a single caller!
> > > > > Why can't you simply do
> > > > >
> > > > >          ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1);
> > > > >          if (ret < 0)
> > > > >                  return ret;
> > > > >
> > > > > in-place there?
> > > >
> > > > This is not legal C.
> > >
> > > I acknowledged this, sorry.
> > >
> > > > Please can we back up this discussion, and start
> > > > over with legal C suggestions. Thanks.
> > >
> > > Suggestion was given as well, let's create a helper used by apple
> > > stuff and later on we will consider the separate submission for the
> > > (new) specifier. Would it work for you?
> >
> > This sub-thread isn't about the %p4ch specifier. It's about a
> > reasonable implementation of macsmc_gpio_nr().
> >
> > Extracting from the context above, the original code was:
> >
> > static int macsmc_gpio_nr(smc_key key)
> > {
> >        int low = hex_to_bin(key & 0xff);
> >        int high = hex_to_bin((key >> 8) & 0xff);
> >
> >        if (low < 0 || high < 0)
> >                return -1;
> >
> >        return low | (high << 4);
> > }
> >
> > I suggested:
> >
> > static int macsmc_gpio_nr(smc_key key)
> > {
> >          char k[2];
> >          u8 result;
> >          int ret;
> >
> >          k[0] = key;
> >          k[1] = key >> 8;
> >
> >          ret = hex2bin(&result, k, 2);
> >          if (ret < 0)
> >                  return ret;
> >
> >          return result;
> > }
> >
> > You didn't like that, so I then suggested:
> >
> > 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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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