| Message ID | 20230809210740.18392-1-jorge.lopez2@hp.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | hp-bioscfg: Update steps how order list elements are evaluated | expand |
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 ? Regards, Hans
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) 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. 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 > > Regards, > > Hans > > >
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
On Mon, Aug 21, 2023 at 6:55 AM Hans de Goede <hdegoede@redhat.com> 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@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 ? > > <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.
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++; } kfree(str_value);
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(-)