Re: [PATCH] hp-bioscfg: Update steps how order list elements are evaluated
From: Hans de Goede
Date: Mon Aug 21 2023 - 07:57:00 EST
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 ?
>
> 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