Message-ID: <CAOOmCE8gByOYcoAwC=ZkXwk54jqYyWNkhDnvjgxEnV1YdYo-KQ@mail.gmail.com>
Date:   Mon, 8 May 2023 16:25:11 -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 Mon, May 8, 2023 at 3:50 PM Thomas Weißschuh <thomas@...ch.de> wrote:
>
> On 2023-05-08 08:56:55-0500, Jorge Lopez wrote:
> > 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.
>
> If this conversion is done by the driver why would the users have to
> care?

The driver currently does not do any conversion hence the reason for
my concerns.
I will reinstate the conversion which was removed in an earlier review.
>
> > >
> > > 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.
>
> A device drivers task it to translate from the domain of the kernel and
> its unified APIs to whatever a specific device needs.
>
> If we agree that it is more consistent to use semicolons for class
> firmware-attribute APIs then the driver would make sure that userspace
> always sees semicolons while the ACPI calls always use commas.
>
> This means translating semicolon->comma when writing to and
> comma->semicolon when reading from ACPI.
> Userspace should not be exposed to the implementation details but just
> the consistent and documented kernel API.
>
> > > > > > > > +             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.
>
> Yes please.
>
> Create an attribute in bioscfg.c that can be used in all the other
> attribute_groups.

Will do!
>
> > > > > 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