| Message ID | 20230731203141.30044-1-jorge.lopez2@hp.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hp-bioscfg: Overall fixes and code cleanup | expand |
These are fine. We still need to do something like this. Also we could just get rid of value_len completely. Nothing uses it. 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 cffc1c9ba3e77..6ba0e49e787ec 100644 --- a/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c +++ b/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c @@ -264,7 +264,7 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord * 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); + ret = hp_convert_hexstr_to_str(str_value, size, &tmpstr, &tmp_len); if (ret) goto exit_list;
I will submit a new patch replacing 'value_len' for 'size' in line 267 as indicated. 'value_len' is utilized earlier in the code so we cannot remove it completely from the function. On Tue, Aug 1, 2023 at 8:35 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > These are fine. We still need to do something like this. Also we could > just get rid of value_len completely. Nothing uses it. > > 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 cffc1c9ba3e77..6ba0e49e787ec 100644 > --- a/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c > +++ b/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c > @@ -264,7 +264,7 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord > * 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); > + ret = hp_convert_hexstr_to_str(str_value, size, &tmpstr, &tmp_len); > if (ret) > goto exit_list; >
On Tue, Aug 01, 2023 at 09:52:05AM -0500, Jorge Lopez wrote: > I will submit a new patch replacing 'value_len' for 'size' in line 267 > as indicated. > 'value_len' is utilized earlier in the code so we cannot remove it > completely from the function. > After replacing size then it looks like this. $ grep value_len drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c int value_len = 0; &str_value, &value_len); &str_value, &value_len); It's a write only variable. regards, dan carpenter
Ok. Thanks for the clarification. I will remove 'value_len' and replace all its references with 'size'. On Tue, Aug 1, 2023 at 10:04 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > On Tue, Aug 01, 2023 at 09:52:05AM -0500, Jorge Lopez wrote: > > I will submit a new patch replacing 'value_len' for 'size' in line 267 > > as indicated. > > 'value_len' is utilized earlier in the code so we cannot remove it > > completely from the function. > > > > After replacing size then it looks like this. > > $ grep value_len drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c > int value_len = 0; > &str_value, &value_len); > &str_value, &value_len); > > It's a write only variable. > > regards, > dan carpenter >
On Tue, Aug 01, 2023 at 10:10:05AM -0500, Jorge Lopez wrote: > Ok. Thanks for the clarification. I will remove 'value_len' and > replace all its references with 'size'. Ugh... No, that's worse than the original. Just leave value_len as is in that case. :P regards, dan carpenter
Hi, On 7/31/23 22:31, Jorge Lopez wrote: > Submit individual patches to address memory leaks and uninitialized > variable errors. > Addressed several review comments making the source code more readable. > Removed duplicate use of variable in inner loop. > > Changes were tested with a HP EliteBook x360 1030 G3 > > Jorge Lopez (8): > hp-bioscfg: Fix memory leaks in attribute packages > hp-bioscfg: Fix uninitialized variable errors > hp-bioscfg: Replace the word HACK from source code > hp-bioscfg: Change how prerequisites size is evaluated > hp-bioscfg: Change how order list size is evaluated > hp-bioscfg: Change how enum possible values size is evaluated > hp-bioscfg: Change how password encoding size is evaluated > hp-bioscfg: Remove duplicate use of variable in inner loop > > .../x86/hp/hp-bioscfg/enum-attributes.c | 24 ++++++++---- > .../x86/hp/hp-bioscfg/int-attributes.c | 15 +++++-- > .../x86/hp/hp-bioscfg/order-list-attributes.c | 39 ++++++++++++------- > .../x86/hp/hp-bioscfg/passwdobj-attributes.c | 27 +++++++++---- > .../x86/hp/hp-bioscfg/string-attributes.c | 13 +++++-- > 5 files changed, 82 insertions(+), 36 deletions(-) Thank you for your patch-series, I've applied the series to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans