Re: [PATCH 11/13] platform/x86: hp-bioscfg: advance elem past consumed array elements in string-attributes

Hi Ilpo,

Thanks a lot for reviewing the series!

I have posted v2 [1], where patches 9-13 from v1 are squashed into a
single patch (now patch 9/9) covering all five attribute parsers, as
you suggested.

Regarding the cleanup and consolidation of the attribute parsing functions
(e.g., hp_populate_integer_elements_from_package() and
hp_populate_string_elements_from_package()): I completely agree that
there is significant code duplication across these attribute-type files.
Once this bugfix series lands, I will take a look at consolidating those
common parsing parts into a shared helper/macro as you suggested.

[1] https://lore.kernel.org/all/[email protected]/

Thanks,
Muhammad


On Wed, Aug 12, 2026 at 3:41 PM Ilpo Järvinen
<[email protected]> wrote:
>
> On Mon, 3 Aug 2026, Muhammad Bilal wrote:
>
> > Same defect as the companion fix to enum-attributes.c in this series:
> > the PREREQUISITES case consumes "size" consecutive ACPI package
> > elements via elem + reqs, but the outer loop only advances "elem" by
> > one per iteration, causing the next iteration to misread a leftover
> > prerequisite entry as the next property and abort the parse with
> > -EIO on the resulting type mismatch.
> >
> > Fix by advancing "elem" by (size - 1) after the loop.
> >
> > Fixes: e6c7b3e15559 ("platform/x86: hp-bioscfg: string-attributes")
> > Cc: [email protected]
> > Signed-off-by: Muhammad Bilal <[email protected]>
> > ---
> >  drivers/platform/x86/hp/hp-bioscfg/string-attributes.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/platform/x86/hp/hp-bioscfg/string-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/string-attributes.c
> > index fe5a9a3a4ef1..5abec8995911 100644
> > --- a/drivers/platform/x86/hp/hp-bioscfg/string-attributes.c
> > +++ b/drivers/platform/x86/hp/hp-bioscfg/string-attributes.c
> > @@ -233,6 +233,8 @@ static int hp_populate_string_elements_from_package(union acpi_object *string_ob
> >                               kfree(str_value);
> >                               str_value = NULL;
> >                       }
> > +                     if (size)
> > +                             elem += size - 1;
> >                       break;
> >
> >               case SECURITY_LEVEL:
> >
>
> Hi,
>
> First of all, thanks for working with this driver.
>
> There's no need to do the same fix in n patches. If the problem is the
> same in multiple files, they can be in the same patch. You just need to
> generalize your shortlog prefix a bit when combining.
>
> I'll probably take this series once that is fixed (and after I do a
> deeper review on them changes) ...
>
> ... BUT,
>
> This entire driver was merged half-baked, with outstanding comments from
> me unaddressed. One of the most important unaddressed comment related to
> the massive use of copy-pasted code which leads to having the same bug in
> multiple places.
>
> Once this series is done, could you try to look into consolidating similar
> parts in these attribute type files so that there would be a common
> function that is called by each of these attribute type files (and if a
> common function is not possible due to types, perhaps macro could be tried
> instead)? I'd be very welcoming for such a cleanup to finally start
> cleaning up the mess this driver is.
>
> If you extract e.g. hp_populate_integer_elements_from_package() and
> hp_populate_string_elements_from_package() and diff those two functions,
> you'll see what I mean. A struct is even called "common" in the struct
> string/integer_data so I expect it could be parsed by a common function.
>
> --
>  i.
>