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

[Hans de Goede <hdegoede@redhat.com>]

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@redhat.com> 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@hp.com>
>>>
>>> ---
>>> 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 ?
> 
> The lines cannot be replaced with a single line for several reasons,
> 1. elements_size is initially set to 1 and it is only incremented when
> a COMMA_SEP is found.  (See part variable)

Right, but we are not incrementing elements_size here, but overriding it,
so the start value does not matter.

olist_elem keeps count of how many times strscpy() has been called
and thus of how many elements there are in
the ordered_list_data->elements_size, so:

                         ordered_list_data->elements_size = olist_elem;

will give us the correct size with much simpler code.

> 2. Limit the number of element_size to  MAX_ELEMENTS_SIZE.  The user
> requires entering all items in the new order when a change is needed.
> For instance, updating boot order.

olist_elem itself is also already limited to MAX_ELEMENTS_SIZE.

> 3. Limiting  elements_size and not just olist_elem to to
> MAX_ELEMENTS_SIZE removes the possibility of  array overflow
> (ordered_list_data->elements[..]).   olist_elem value is 0 (zero)
> based while elements_size is 1 based

As already mentioned olist_elem itself is already limited to MAX_ELEMENTS_SIZE,
so doing:

ordered_list_data->elements_size = olist_elem;

Automatically limits ordered_list_data->elements_size too.

###

I see you also left the "if (!part)" above the loop.

That is not necessary because after the first strsep() call part
will always be non NULL.

For a string without any delimiters, so only 1 element strsep()
will only return NULL on the second strsep() call.

strsep() always returns *part_tmp, which before the first
call is non NULL, so the first call always returns non NULL.

###

And there still is an unused assignment of size directly
after "case ORD_LIST_ELEMENTS" :

                        size = ordered_list_data->elements_size;

###

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.

###

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.

Regards,

Hans