Re: [PATCH] hp-bioscfg: Update steps how order list elements are evaluated

[Jorge Lopez]

On Mon, Aug 21, 2023 at 6:55 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi Jorge,
>
> On 8/14/23 15:44, Jorge Lopez wrote:
> > Hi Hans,
> >
> > On Mon, Aug 14, 2023 at 3:41 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>
> >> Hi Jorge,
> >>
> >> On 8/9/23 23:07, Jorge Lopez wrote:
> >>> Update steps how order list elements data and elements size are
> >>> evaluated
> >>>
> >>> Signed-off-by: Jorge Lopez <jorge.lopez2@xxxxxx>
> >>>
> >>> ---
> >>> Based on the latest platform-drivers-x86.git/for-next
> >>> ---
> >>>  .../x86/hp/hp-bioscfg/order-list-attributes.c    | 16 ++++++++++++++--
> >>>  1 file changed, 14 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
> >>> index b19644ed12e0..d2b61ab950d4 100644
> >>> --- a/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
> >>> +++ b/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
> >>> @@ -152,7 +152,7 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord
> >>>
> >>>               switch (order_obj[elem].type) {
> >>>               case ACPI_TYPE_STRING:
> >>> -                     if (elem != PREREQUISITES && elem != ORD_LIST_ELEMENTS) {
> >>> +                     if (elem != PREREQUISITES) {
> >>>                               ret = hp_convert_hexstr_to_str(order_obj[elem].string.pointer,
> >>>                                                              order_obj[elem].string.length,
> >>>                                                              &str_value, &value_len);
> >>> @@ -266,6 +266,15 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord
> >>>                       if (ret)
> >>>                               goto exit_list;
> >>>
> >>> +                     /*
> >>> +                      * It is expected for the element size value
> >>> +                      * to be 1 and not to represent the actual
> >>> +                      * number of elements stored in comma
> >>> +                      * separated format. element size value is
> >>> +                      * recalculated to report the correct number
> >>> +                      * of data elements found.
> >>> +                      */
> >>> +
> >>>                       part_tmp = tmpstr;
> >>>                       part = strsep(&part_tmp, COMMA_SEP);
> >>>                       if (!part)
> >>> @@ -273,11 +282,14 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord
> >>>                                       tmpstr,
> >>>                                       sizeof(ordered_list_data->elements[0]));
> >>>
> >>> -                     for (olist_elem = 1; olist_elem < MAX_ELEMENTS_SIZE && part; olist_elem++) {
> >>> +                     for (olist_elem = 0; olist_elem < MAX_ELEMENTS_SIZE && part; olist_elem++) {
> >>>                               strscpy(ordered_list_data->elements[olist_elem],
> >>>                                       part,
> >>>                                       sizeof(ordered_list_data->elements[olist_elem]));
> >>> +
> >>>                               part = strsep(&part_tmp, COMMA_SEP);
> >>> +                             if (part && ordered_list_data->elements_size < MAX_ELEMENTS_SIZE)
> >>> +                                     ordered_list_data->elements_size++;
> >>>                       }
> >>
> >> I believe that you can replace the:
> >>
> >>                                 if (part && ordered_list_data->elements_size < MAX_ELEMENTS_SIZE)
> >>                                         ordered_list_data->elements_size++;
> >>                         }
> >>
> >> Lines with simply (after the loop has finished) doing:
> >>
> >>                         }
> >>                         ordered_list_data->elements_size = olist_elem'
> >>
> >> Or am I missing something ?
> >
<snip>

 ###
>
> Also you seem to have based this patch on top of a weird base
> commit. This patch assumes both strsep() calls use COMMA_SEP
> as separator. But the latest code in:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
>
> Still uses the wrong SEMICOLON_SEP for the second strsep() call.
>
> Please make sure to base your next version on top of the latest
> review-hans commit.
>

local work branch was reset and now it is in sync with review-hans branch

> ###
>
> TL;DR: for your next version the "case ORD_LIST_ELEMENTS"
> should end up looking like this:
>
>                 case ORD_LIST_ELEMENTS:
>                         /*
>                          * Ordered list data is stored in hex and comma separated format
>                          * Convert the data and split it to show each element
>                          */
>                         ret = hp_convert_hexstr_to_str(str_value, value_len, &tmpstr, &tmp_len);
>                         if (ret)
>                                 goto exit_list;
>
>                         part_tmp = tmpstr;
>                         part = strsep(&part_tmp, COMMA_SEP);
>
>                         for (olist_elem = 0; olist_elem < MAX_ELEMENTS_SIZE && part; olist_elem++) {
>                                 strscpy(ordered_list_data->elements[olist_elem],
>                                         part,
>                                         sizeof(ordered_list_data->elements[olist_elem]));
>                                 part = strsep(&part_tmp, COMMA_SEP);
>                         }
>                         ordered_list_data->elements_size = olist_elem;
>
>                         kfree(str_value);
>                         str_value = NULL;
>                         break;
>
> Unless I'm missing something and you believe that this will not work.

Concur with the proposed solution.  It achieves the same objective
using fewer checks.
New patch will follow

Regards,

Jorge.