linux-kernel - Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VdjAxAvmYVW4qgV2i91L31=Ctx4nx_eAe9+pqPFEArD3w@mail.gmail.com>
Date:   Mon, 22 May 2023 14:05:05 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Paul Cercueil <paul@...pouillou.net>
Cc:     Artur Rojek <contact@...ur-rojek.eu>,
        Jonathan Cameron <jic23@...nel.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Chris Morgan <macromorgan@...mail.com>,
        linux-mips@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-input@...r.kernel.org
Subject: Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer

On Mon, May 22, 2023 at 1:23 PM Paul Cercueil <paul@...pouillou.net> wrote:
> Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit :
> > On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko
> > <andy.shevchenko@...il.com> wrote:
> > > On Mon, May 22, 2023 at 1:59 AM Artur Rojek
> > > <contact@...ur-rojek.eu> wrote:

...

> > > > +       u16 tdat[6];
> > > > +       u32 val;
> > > > +
> > > > +       memset(tdat, 0, ARRAY_SIZE(tdat));
> > >
> > > Yeah, as LKP tells us this should be sizeof() instead of
> > > ARRAY_SIZE().
> > >
> > > > +       for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) {
> > > > +               if (mask & 0x3) {
> > >
> > > (for the consistency it has to be GENMASK(), but see below)
> > >
> > > First of all, strictly speaking we should use the full mask without
> > > limiting it to the 0 element in the array (I'm talking about
> > > active_scan_mask).
> > >
> > > That said, we may actually use bit operations here in a better way,
> > > i.e.
> > >
> > >   unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] -
> > > 1);
> > >
> > >   j = 0;
> > >   for_each_set_bit(i, active_scan_mask, ...) {
> > >     val = readl(...);
> > >     /* Two channels per sample. Demux active. */
> > >     tdat[j++] = val >> (16 * (i % 2));
> >
> > Alternatively
> >
> >      /* Two channels per sample. Demux active. */
> >      if (i % 2)
> >        tdat[j++] = upper_16_bits(val);
> >      else
> >        tdat[j++] = lower_16_bits(val);
> >
> > which may be better to read.
>
> It's not if/else though. You would check (i % 2) for the upper 16 bits,
> and (i % 1) for the lower 16 bits. Both can be valid at the same time.

Are you sure? Have you looked into the proposed code carefully?

What probably can be done differently is the read part, that can be
called once. But looking at it I'm not sure how it's supposed to work
at all, since the address is always the same. How does the code and
hardware are in sync with the channels?

> > >   }
> > >
> > > > +                       val = readl(adc->base +
> > > > JZ_ADC_REG_ADTCH);
> > > > +                       /* Two channels per sample. Demux active.
> > > > */
> > > > +                       if (mask & BIT(0))
> > > > +                               tdat[i++] = val & 0xffff;
> > > > +                       if (mask & BIT(1))
> > > > +                               tdat[i++] = val >> 16;
> > > > +               }
> > > >         }

-- 
With Best Regards,
Andy Shevchenko