Re: [PATCH v11 04/14] HP BIOSCFG driver - int-attributes

[Jorge Lopez]

On Tue, May 2, 2023 at 4:30 PM Thomas Weißschuh <thomas@xxxxxxxx> wrote:
>
> Hi Jorge,
>
> thanks for incorporating my feedback, I'm curious for the next revision!
>
> The review comments are very terse but that is only to bring across
> their points better. Your effort is appreciated.
>
> On 2023-05-02 15:56:22-0500, Jorge Lopez wrote:
>
> <snip>
>
> > > On 2023-04-20 11:54:44-0500, Jorge Lopez wrote:
> > > > ---
> > > > Based on the latest platform-drivers-x86.git/for-next
> > > > ---
> > > >  .../x86/hp/hp-bioscfg/int-attributes.c        | 474 ++++++++++++++++++
> > > >  1 file changed, 474 insertions(+)
> > > >  create mode 100644 drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> > > >
> > > > diff --git a/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> > > > new file mode 100644
> > > > index 000000000000..d8ee39dac3f9
> > > > --- /dev/null
> > > > +++ b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
>
> <snip>
>
> > > > +int populate_integer_elements_from_package(union acpi_object *integer_obj,
> > > > +                                        int integer_obj_count,
> > > > +                                        int instance_id)
> > > > +{
> > > > +     char *str_value = NULL;
> > > > +     int value_len;
> > > > +     int ret = 0;
> > > > +     u32 size = 0;
> > > > +     u32 int_value;
> > > > +     int elem = 0;
> > > > +     int reqs;
> > > > +     int eloc;
> > > > +
> > > > +     if (!integer_obj)
> > > > +             return -EINVAL;
> > > > +
> > > > +     strscpy(bioscfg_drv.integer_data[instance_id].common.display_name_language_code,
> > > > +             LANG_CODE_STR,
> > > > +             sizeof(bioscfg_drv.integer_data[instance_id].common.display_name_language_code));
> > > > +
> > > > +     for (elem = 1, eloc = 1; elem < integer_obj_count; elem++, eloc++) {
> > > > +
> > > > +             /* ONLY look at the first INTEGER_ELEM_CNT elements */
> > >
> > > Why?
> > The information provided in element 0 from the package is ignored as
> > directed by the BIOS team.
> > Similar action is taken when reading the information from ACPI Buffer
> > (populate_integer_elements_from_buffer())
>
> This should be mentioned somewhere.
>
> But my question was more why to we only look at INTEGER_ELEM_CNT?
> It is clear to me now, but this is very convulted. See below.

I am adding the following information to each attribute file for clarification.

"The total number of elements (INT_ELEM_CNT) read include only data
relevant to this driver and its functionality. BIOS defines the order
in which each element is read.  Element 0 data is not relevant to this
driver hence it is ignored.  For clarity, The switch statement list
all element names (DISPLAY_IN_UI) which defines the order in which is
read and the name matches the variable where the data is stored".

>
> <snip>
>
> > >
> > > > +
> > > > +int populate_integer_elements_from_buffer(u8 *buffer_ptr, u32 *buffer_size,
> > > > +                                       int instance_id)
> > > > +{
> > > > +     char *dst = NULL;
> > > > +     int elem;
> > > > +     int reqs;
> > > > +     int integer;
> > > > +     int size = 0;
> > > > +     int ret;
> > > > +     int dst_size = *buffer_size / sizeof(u16);
> > > > +
> > > > +     dst = kcalloc(dst_size, sizeof(char), GFP_KERNEL);
> > > > +     if (!dst)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     elem = 0;
> > > > +     strscpy(bioscfg_drv.integer_data[instance_id].common.display_name_language_code,
> > > > +             LANG_CODE_STR,
> > > > +             sizeof(bioscfg_drv.integer_data[instance_id].common.display_name_language_code));
> > > > +
> > > > +     for (elem = 1; elem < 3; elem++) {
> > > > +
> > > > +             ret = get_string_from_buffer(&buffer_ptr, buffer_size, dst, dst_size);
> > > > +             if (ret < 0)
> > > > +                     continue;
> > > > +
> > > > +             switch (elem) {
> > > > +             case VALUE:
> > > > +                     ret = kstrtoint(dst, 10, &integer);
> > > > +                     if (ret)
> > > > +                             continue;
> > > > +
> > > > +                     bioscfg_drv.integer_data[instance_id].current_value = integer;
> > > > +                     break;
> > > > +             case PATH:
> > > > +                     strscpy(bioscfg_drv.integer_data[instance_id].common.path, dst,
> > > > +                             sizeof(bioscfg_drv.integer_data[instance_id].common.path));
> > > > +                     break;
> > > > +             default:
> > > > +                     pr_warn("Invalid element: %d found in Integer attribute or data may be malformed\n", elem);
> > > > +                     break;
> > > > +             }
> > > > +     }
> > > > +
> > > > +     for (elem = 3; elem < INTEGER_ELEM_CNT; elem++) {
> > >
> > > This loop pattern seems weird to me.
> > > It is not obvious that the values are read in the order of the switch()
> > > branches from the buffer.
> > >
> >
> > The order in which the data is read from the buffer is set by BIOS.
>
> This I understand.
>
> > The switch statement was used to enforce the reading order of the
> > elements and provide additional clarity
>
> This is not clear from the code alone. One also needs to know the
> concrete values of the enums.
>
> > > Something more obvious would be:
> > >
> > > instance.common.is_readonly = read_int_from_buf(&buffer_ptr);
> > > instance.common.display_in_ui = read_int_from_buf(&buffer_ptr);
> > > instance.common.requires_physical_presence = read_int_from_buf(&buffer_ptr);
>
> The proposed pattern above, just regular function calls, are also
> executed in the correct order, the order in which they are written.
>
The code will be easier to follow and will not require checking of the
results  because failing conditions are ignored.
This will be a good option for functions such
populate_integer_elements_from_buffer().  Buffer elements.
Refactoring  package function,
populate_integer_elements_from_package(), will introduce additional
complexity and obfuscation.


> For a reader it is clear that the order is important and part of the
> ABI of the BIOS.
>
> > > This would make it clear that these are fields read in order from the
> > > buffer. Without having to also look at the numeric values of the
> > > defines.
> > >
> > > Furthermore it would make the code shorter and errorhandling would be
> > > clearer and the API similar to the netlink APIs.
> > >
> > > Or maybe with error reporting:
> > >
> > > ret = read_int_from_buf(&buffer_ptr, &instance.common.is_readonly);
> > > if (ret)
> > >     ...
> >
> > Instance.common.is_readonly is only evaluated when the user attempt to
> > update an attribute current value
>
> is_readonly was only an example on how to more nicely read the data from
> the buffer. It applies to all values of all attribute types.
>
> > > ret = read_int_from_buf(&buffer_ptr, &instance.common.display_in_ui);
> > > if (ret)
> > >     ...
> >
> > Instance.common.display_in_ui has no specific use at this time.
> >
> > The code was made shorter and easier to understand by replacing the
> > long statements with
> >
> > struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> > ...
> > integer_data->common.is_readonly = integer;
> >
> > Same approach was taken for all attribute files.
>
> Thanks!
>
> Please do try to use the "plain functioncall" pattern as outlined above.
> I think it can make the code much shorter and idiomatic.

Understood!