Message-ID: <CAOOmCE-FNUUh+tZLPdJW+y5m0JKufWXiJ8M-7WFEiveNi4rRAg@mail.gmail.com>
Date:   Mon, 8 May 2023 08:56:55 -0500
From:   Jorge Lopez <jorgealtxwork@...il.com>
To:     Thomas Weißschuh <thomas@...ch.de>
Cc:     hdegoede@...hat.com, platform-driver-x86@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v11 05/14] HP BIOSCFG driver - ordered-attributes

On Sat, May 6, 2023 at 12:51 AM Thomas Weißschuh <thomas@...ch.de> wrote:
>
> On 2023-05-05 16:57:59-0500, Jorge Lopez wrote:
> > On Fri, May 5, 2023 at 4:11 PM Thomas Weißschuh <thomas@...ch.de> wrote:
> > >
> > > On 2023-05-05 11:09:55-0500, Jorge Lopez wrote:
> > > > On Sun, Apr 23, 2023 at 1:55 AM <thomas@...ch.de> wrote:
> > > > >
> > > > > On 2023-04-20 11:54:45-0500, Jorge Lopez wrote:
> > > > > >  .../x86/hp/hp-bioscfg/ordered-attributes.c    | 563 ++++++++++++++++++
> > > > > >  1 file changed, 563 insertions(+)
> > > > > >  create mode 100644 drivers/platform/x86/hp/hp-bioscfg/ordered-attributes.c
> > > > > >
> > > > > > diff --git a/drivers/platform/x86/hp/hp-bioscfg/ordered-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/ordered-attributes.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..5e5d540f728d
> > > > > > --- /dev/null

<snip>

> > > > > > +     elem_found = 0;
> > > > > > +     while (elem_found < bioscfg_drv.ordered_list_data[instance_id].elements_size) {
> > > > > > +
> > > > > > +             value = strsep(&new_values, ",");
> > > > >
> > > > > The docs say the separator is semicolon.
> > > >
> > > > BIOS reports the current value using ',' as separator instead of ";".
> > > >
> > > > ./hp-bioscfg/attributes/UEFI Boot Order/current_value
> > > > HDD:M.2:3,HDD:USB:1(Disabled),HDD:M.2:4,HDD:M.2:1,HDD:M.2:2,NETWORK
> > > > IPV4:EMBEDDED:1,NETWORK IPV6:EMBEDDED:1,NETWORK
> > > > IPV4:EXPANSION:1,NETWORK IPV6:EXPANSION:1
> > > >
> > > > To avoid having to convert from "," to ";" and vice versa, I will
> > > > update the documentation to reflect the use of  "'," commas as the
> > > > separator
> > >
> > > The enum data format uses ";". Therefore it makes sense to also stick to
> > > ";".
> > > The implementation detail of the BIOS using ',' should not matter to
> > > users.
> >
> > The use of ',' does matter because BIOS expects the updated string to
> > use commas as separators
> > current_value is reported by BIOS using commas.  Any changes to the
> > order of items in that string needs to use commas.
> >
> > The difference with enum is the fact the user needs to  enter only one
> > value out of those possible values available and no separators are
> > needed.
> > For ordered attributes...
> >
> > when the current value  is "foo,bar,baz".   the user provides a string
> > which items are ordered differently.  i.e. "baz,bar,foo"
> > if the new string is using semicolon instead of comma for the
> > separator, BIOS will reject the data.
>
> Of course the BIOS expects the format with comma.
>
> But the users of this API should not have to care about implementation
> details like that.
> They want a consistent API. As the ordered-list type is fairly general
> it may be promoted to be a general attribute type later.
>
> If this happens the API can't be changed as that would break users of
> hp-bioscfg. So either there would be two APIs for the ordered-list, one
> for hp-bioscfg and one for all other drivers, or different attribute
> types would use different kinds of separators.
>
> Was there an issue with the previous replacement logic?

No issue. I was anticipating a potential problem/confusion and created
a solution.  Customers will start using this driver when it becomes
part of the upstream code.
Users primarily will use Powershell scripts to interact with the
driver.  The use of powershell will require the user to have the
knowledge and convert ';' semicolon to ',' commas prior to submitting
the request to the driver.  AFIK, the boot order is one of those
attributes that is not configured often by the user.  Just my opinion.
>
> The whole point of device drivers is to translate general kernel APIs
> into whatever a specific device needs, switching around ',' and ';'
> seems not so bad.

So, are you saying, it is ok for the driver to convert  ';' semicolon
to ',' commas prior to submitting a request  to change any attribute
of type 'ordered-list' or when reading current value ?
If that is correct, then we can change the documentation back to use
';' semicolons.
Please confirm.
>
> > > > > > +             if (value != NULL) {
> > > > > > +                     if (!*value)
> > > > > > +                             continue;
> > > > > > +                     elem_found++;
> > > > > > +             }
> > > > > > +
> > > > > > +             found = 0;
> > > > > > +             for (elem = 0; elem < bioscfg_drv.ordered_list_data[instance_id].elements_size; elem++) {
> > > > > > +                     if (!strcasecmp(bioscfg_drv.ordered_list_data[instance_id].elements[elem], value)) {
> > > > >
> > > > > It's surprising that this is case-insensitive.
> > > >
> > > > As validated in earlier reviews,  BIOS rejects strings that do not
> > > > match the internal values.
> > > >

<snip>

> > > > > > +
> > > > > > +     strscpy(bioscfg_drv.ordered_list_data[instance_id].common.display_name_language_code,
> > > > > > +             LANG_CODE_STR,
> > > > > > +             sizeof(bioscfg_drv.ordered_list_data[instance_id].common.display_name_language_code));
> > > > >
> > > > > This seems to be the same for every type. Can it not be moved into
> > > > > common code?
> > > >
> > > > Each instance requires to report 'display_name_language_code' hence it
> > > > cannot be moved to a common code.
> > >
> > > Can it every be different from LANG_CODE_STR?
> >
> > Yes.   The string currently is LANG_CODE_STR (en_US.UTF-8)  but it
> > will change as the BIOS provides additional translations at a later
> > time.
> > This is a future enhancement for the driver.
>
> Is this planned for the near future?
> And/Or already implemented in the BIOS?
>
> If there are no concrete plans to implement this soon, in my opinion,
> it would be better to only introduce this code when it does something
> useful.

There are no concrete plans yet for the driver.  BIOS provides
translations for strings (F10) UI but the attributes are reported in
English regardless of the language configured.
firmware-attributes requires 'display_name_language_code' to be
reported.  At this time, the driver can provide a common helper to
assign the LANG_CODE_STR to the corresponding attribute.

>
> > > If not instead of having one kobj_attribute diplay_langcode for each
> > > string/enum/integer/etc you could have one kobj_attribute defined in
> > > bioscfg.c and then added to each string_attrs, enum_attrs...
> > >
> > > The _show() callback for this attribute would just return the constant
> > > string.
> > >
> > > This removes the need for many attribute definition, members in the data
> > > struct, initialization of these member...

Powered by blists - more mailing lists