Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230521225901.388455-1-contact@artur-rojek.eu> <20230521225901.388455-3-contact@artur-rojek.eu>
In-Reply-To: <20230521225901.388455-3-contact@artur-rojek.eu>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 22 May 2023 13:58:23 +0300
Message-ID: <CAHp75Vfj_2WJdcn4TC2SesUrWFmyhXHujS9P8O3JXdFcAb=WeQ@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] input: joystick: Fix buffer data parsing
To:     Artur Rojek <contact@artur-rojek.eu>
Cc:     Paul Cercueil <paul@crapouillou.net>,
        Jonathan Cameron <jic23@kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Chris Morgan <macromorgan@hotmail.com>,
        linux-mips@vger.kernel.org, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-input@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, May 22, 2023 at 1:59=E2=80=AFAM Artur Rojek <contact@artur-rojek.eu=
> wrote:
>
> Don't try to access buffer data of a channel by its scan index. Instead,
> calculate its offset in the buffer.
>
> This is necessary, as the scan index of a channel does not represent its
> position in a buffer - the buffer will contain data for enabled channels
> only, affecting data offsets and alignment.
>
> While at it, also fix minor style issue in probe.

a minor
the probe

> Reported-by: Chris Morgan <macromorgan@hotmail.com>
> Closes: https://lore.kernel.org/linux-input/20220408212857.9583-1-macroal=
pha82@gmail.com/

What is this tag? Anything documented? Otherwise use BugLink or Link.

...

>         struct adc_joystick_axis *axes;
>         struct iio_channel *chans;
> +       int *offsets;

Why not unsigned? I.o.w. is there any meaning for negative values?

>         int num_chans;
>         bool polled;

...

> +               off =3D joy->offsets[i];

> +

Move this blank line to be before the previous line.

> +               if (off < 0)
> +                       return -EINVAL;

...

>                 case 1:
> -                       val =3D ((const u8 *)data)[idx];
> +                       val =3D *chan_data;

Might be also

   get_unaligned((const u8 *)chan_data);

And with all this (see below) the chan_data actually can be declared
as const void *.

>                         break;
>                 case 2:
> -                       data_u16 =3D (const u16 *)data + idx;
> -
>                         /*
>                          * Data is aligned to the sample size by IIO core=
.
>                          * Call `get_unaligned_xe16` to hide type casting=
.
>                          */
>                         if (endianness =3D=3D IIO_BE)
> -                               val =3D get_unaligned_be16(data_u16);
> +                               val =3D get_unaligned_be16(chan_data);
>                         else if (endianness =3D=3D IIO_LE)
> -                               val =3D get_unaligned_le16(data_u16);
> +                               val =3D get_unaligned_le16(chan_data);
>                         else /* IIO_CPU */
> -                               val =3D *data_u16;
> +                               val =3D *(const u16 *)chan_data;

This probably needs to be

   get_unaligned((const u16 *)chan_data);

for the sake of consistency with the above.

>                         break;
>                 default:
>                         return -EINVAL;

...

> +static int adc_joystick_si_cmp(const void *a, const void *b, const void =
*priv)
> +{
> +       const struct iio_channel *chans =3D priv;
> +
> +       return chans[*(int *)a].channel->scan_index -
> +              chans[*(int *)b].channel->scan_index;

Discarding const?

> +}

...

> +       offsets =3D kmalloc_array(count, sizeof(int), GFP_KERNEL);

sizeof(*offsets)

> +       if (!offsets)
> +               return ERR_PTR(-ENOMEM);

...

> +       si_order =3D kmalloc_array(count, sizeof(int), GFP_KERNEL);

sizeof(*si_order)

> +       if (!si_order) {
> +               kfree(offsets);
> +               return ERR_PTR(-ENOMEM);
> +       }

...

> +       /* Channels in buffer are ordered by scan index. Sort to match th=
at. */

the buffer

> +       sort_r(si_order, count, sizeof(int), adc_joystick_si_cmp, NULL, c=
hans);

sizeof(*si_order) ?

sizeof(int) is a bit odd, the above will tell better without even
knowing the sort_r() parameters what it is about.

...

> +       for (i =3D 0; i < count; ++i) {
> +               idx =3D si_order[i];
> +               ch =3D chans[idx].channel;
> +               si =3D ch->scan_index;
> +
> +               if (si < 0 || !test_bit(si, indio_dev->active_scan_mask))=
 {
> +                       offsets[idx] =3D -1;
> +                       continue;
> +               }
> +
> +               /* Channels sharing scan indices also share the samples. =
*/
> +               if (idx > 0 && si =3D=3D chans[idx - 1].channel->scan_ind=
ex) {
> +                       offsets[idx] =3D offsets[idx - 1];
> +                       continue;
> +               }
> +
> +               offsets[idx] =3D offset;

> +               length =3D ch->scan_type.storagebits / 8;

BITS_PER_BYTE ?

> +               if (ch->scan_type.repeat > 1)
> +                       length *=3D ch->scan_type.repeat;

> +               /* Account for channel alignment. */
> +               if (offset % length)
> +                       offset +=3D length - (offset % length);

Would one of ALIGN() / rounddown / etc work here?

> +               offset +=3D length;
> +       }

...

> +       joy->offsets =3D adc_joystick_get_chan_offsets(joy->chans,
> +                                                    joy->num_chans);
> +       if (IS_ERR(joy->offsets)) {
> +               dev_err(devp, "Unable to allocate channel offsets\n");

> +               return PTR_ERR(joy->offsets);
> +       }
> +
> +       return 0;

return PTR_ERR_OR_ZERO() ?

...

>                 error =3D devm_add_action_or_reset(dev, adc_joystick_clea=
nup,
>                                                  joy->buffer);
> -               if (error)  {
> +               if (error) {
>                         dev_err(dev, "Unable to add action\n");
>                         return error;
>                 }

Unrelated change.

--=20
With Best Regards,
Andy Shevchenko