linux-kernel - Re: [PATCH v7 5/6] spi: cs42l43: Add SPI controller support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHp75Vda3nZn8KwqSRvKQL9oeH_PjTnDDPuQytJvTgb2_HDvBQ@mail.gmail.com>
Date: Wed, 31 Jan 2024 22:17:15 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Charles Keepax <ckeepax@...nsource.cirrus.com>
Cc: broonie@...nel.org, lee@...nel.org, robh+dt@...nel.org, 
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, 
	linus.walleij@...aro.org, vkoul@...nel.org, lgirdwood@...il.com, 
	yung-chuan.liao@...ux.intel.com, sanyog.r.kale@...el.com, 
	pierre-louis.bossart@...ux.intel.com, alsa-devel@...a-project.org, 
	patches@...nsource.cirrus.com, devicetree@...r.kernel.org, 
	linux-gpio@...r.kernel.org, linux-spi@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 5/6] spi: cs42l43: Add SPI controller support

On Wed, Jan 31, 2024 at 5:41 PM Charles Keepax
<ckeepax@...nsource.cirrus.com> wrote:
> On Fri, Jan 19, 2024 at 11:49:17AM +0000, Charles Keepax wrote:
> > On Thu, Jan 18, 2024 at 07:06:13PM +0200, andy.shevchenko@...il.com wrote:
> > > Fri, Aug 04, 2023 at 11:46:01AM +0100, Charles Keepax kirjoitti:
> > > > +         while (buf < block) {
> > > > +                 const u8 *word = min(buf + sizeof(u32), block);
> > > > +                 int pad = (buf + sizeof(u32)) - word;
> > > > +
> > > > +                 while (buf < word) {
> > > > +                         val >>= BITS_PER_BYTE;
> > > > +                         val |= FIELD_PREP(GENMASK(31, 24), *buf);
> > > > +
> > > > +                         buf++;
> > > > +                 }
> > >
> > > Is this a reinvented way of get_unaligned_*() APIs?
> > >
> > > > +                 val >>= pad * BITS_PER_BYTE;
> > > > +
> > > > +                 regmap_write(regmap, CS42L43_TX_DATA, val);
> > > > +         }
> > >
> > > ...
> > >
> > > > +                 while (buf < word) {
> > > > +                         *buf = FIELD_GET(GENMASK(7, 0), val);
> > > > +
> > > > +                         val >>= BITS_PER_BYTE;
> > > > +                         buf++;
> > > > +                 }
> > >
> > > put_unaligned_*() ?
> > >
> >
> > Alas as it has been a while I have forgetten the exact context
> > here and this one will take a little more time. I will try to
> > find some spare time to work out if that would actual do the same
> > thing, I have a vague feeling there was something here.
> >
>
> Ok found some time to refresh my memory on this.
>
> The main issue here was as this is processing raw data for the
> SPI we shouldn't assume the data is a multiple of 4-bytes. You
> could write the code such that you used put_unaligned_le32 for
> most of the data and then special case the remainder, which would
> probably be slightly faster. But for the remainder you either end
> with the code here anyway or a special case for each of the cases
> 8,16,24 bits. So the code ends up looking much messier.
> Personally I am inclined to leave this unless performance on the
> SPI becomes a major issue.

Yes, the problem with the code is that it is a NIH existing API or patterns.
We have already in the IIO subsystem a pattern where there is a switch case
and put/get unaligned APIs per case. Perhaps this is what needs to be
factored out
for everybody.

https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/ad4130.c#L472

(some shorter variants)
https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/ltc2497.c#L66
https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/ad4130.c#L472

Here is the abstraction for cameras, perhaps that's what ASoC might need.
drivers/media/v4l2-core/v4l2-cci.c.

> There is perhaps an argument for a comment in the code to explain
> this given it took me time to remember what was going on.

That's for sure.


-- 
With Best Regards,
Andy Shevchenko