Re: [PATCH v1 7/7] ACPI: property: Allow counting a single value as an array of 1 element

[Rafael J. Wysocki]

On Thu, Feb 11, 2021 at 4:42 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Wed, Feb 10, 2021 at 04:44:34PM +0100, Rafael J. Wysocki wrote:
> > On Wed, Feb 10, 2021 at 4:42 PM Andy Shevchenko
> > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > On Wed, Feb 10, 2021 at 04:01:16PM +0100, Rafael J. Wysocki wrote:
> > > > On Wed, Feb 10, 2021 at 3:48 PM Andy Shevchenko
> > > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > > > On Wed, Feb 10, 2021 at 02:48:09PM +0100, Rafael J. Wysocki wrote:
> > > > > > On Wednesday, February 10, 2021 2:31:48 PM CET Rafael J. Wysocki wrote:
> > > > > > > On Wednesday, February 10, 2021 1:36:00 PM CET Rafael J. Wysocki wrote:
> > > > > > > > On Wed, Feb 10, 2021 at 12:51 PM Andy Shevchenko
> > > > > > > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> ...
>
> > > > > > > > > -       if (val && nval == 1) {
> > > > > > > > > +       /* Try to read as a single value first */
> > > > > > > > > +       if (!val || nval == 1) {
> > > > > > > > >                 ret = acpi_data_prop_read_single(data, propname, proptype, val);
> > > > > > > >
> > > > > > > > This returns -EINVAL if val is NULL.
> > > > >
> > > > > Nope. That's why it's a patch 7. Patch 6 solves this.
> > > >
> > > > That's my point.  Patch 7 should be the first one in the series.
> > >
> > > Ah, okay. Since you want this let me rebase.
> >
> > Thanks!
>
> I started rebasing and realised that your approach has swapped the error codes,
> i.e. if it's a single-value and it is, e.g., 16-bit wide, the u8 read will
> return 1, while it has to return -EOVERFLOW.

Well, that's a bug in my patch.

I thought that you would reorder the series to put the fix into the
front of it, but I didn't really mean to rebase it on top of my patch.
Sorry for the confusion.

However, not that you have started to do it apparently, let me post
that patch properly with all of the issues addressed.

> If you prefer, I can move two patches to the beginning, so one will be a good
> prerequisite for this fix. And I'm still unsure about stable (Fixes tag is okay
> to me), because the counting never worked from the day 1.

Well, the code has never worked as intended, so why don't we make
"stable" work as intended too?