Re: [PATCH v6 1/4] Introduction of HP-BIOSCFG driver

From: Jorge Lopez <jorgealtxwork@gmail.com>
To: "Thomas Weißschuh" <thomas@t-8ch.de>
Cc: hdegoede@redhat.com, platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v6 1/4] Introduction of HP-BIOSCFG driver
Date: Wed, 12 Apr 2023 14:37:45 -0500	[thread overview]
Message-ID: <CAOOmCE-=cprrpzEz5EOs00K7B=bp1rnrnZY7Ee0a245piioiJQ@mail.gmail.com> (raw)
In-Reply-To: <dbf97220-03d6-4815-8f14-55ee477b8afb@t-8ch.de>

Hi Thomas,

Please see my comments below.

On Sun, Apr 2, 2023 at 11:28 AM Thomas Weißschuh <thomas@t-8ch.de> wrote:
>
> Hi Jorge,
>
> below a few stylistic comments.
> These are very general and do not only affect the commented locations
> but the whole driver.
>
> That said these are not critical.
>
> First focus on removing dead code and nailing down the userspace API.
> Then it depends on your motivation.
>
> As said before I would focus on reducing the driver to the bare minimum
> that makes it usable, get it merged / clean it up and then re-add pieces
> bit-by-bit.

The driver functionality is the proposed basic functionality.  There
are plans to provide additional support for Sure Recover (Security
component) which is planned to be added in future patches.

>
> I'll probably go over all the files again when I am more familiar with
> the driver.
>
> > +             // append UTF_PREFIX to part and then convert it to unicode
> > +             strprefix = kasprintf(GFP_KERNEL, "%s%s", UTF_PREFIX,
> > +                                   authentication);
> > +             if (!strprefix)
> > +                     goto out_populate_security_buffer;
> > +
> > +             auth = ascii_to_utf16_unicode(auth, strprefix);
> > +     }
> > +out_populate_security_buffer:
>
> There is no need to have the name of the function in the label.
>
> Just "out" would be enough.
>
> > +
> > +     kfree(strprefix);
> > +     strprefix = NULL;
>
> No need to clear stack variables.

I will clear stack variables across all files.
>
> > +}
> > +
> > +ssize_t update_spm_state(void)
> > +{
> > +     int ret;
> > +     struct secureplatform_provisioning_data *data = NULL;
> > +
> > +     data = kmalloc(sizeof(struct secureplatform_provisioning_data),
> > +                    GFP_KERNEL);
>
> Use "sizeof(*data)". It's shorter and more robust.

Done!

> > +/*
> > + * statusbin - Reports SPM status in binary format
> > + *
> > + * @kobj:  Pointer to a kernel object of things that show up as
> > + *      directory in the sysfs filesystem.
> > + * @attr:  Pointer to list of attributes for the operation
> > + * @buf:   Pointer to buffer
>
> The parameters are the same for every attribute_show() function.
> No need to document them.
>
> Also if you document something use proper kerneldoc format:
> https://docs.kernel.org/doc-guide/kernel-doc.html

I will remove any unnecessary documentation.

>

> > +     ret = sysfs_emit(buf, "%s\n",
> > +                      spm_mechanism_types[bioscfg_drv.spm_data.mechanism]);
> > +     return ret;
>
> No need for the temporary variable:

It was an oversight.  Done!

>
> > diff --git a/drivers/platform/x86/hp/hp-bioscfg/string-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/string-attributes.c
> > new file mode 100644
> > index 000000000000..79ec007fbcee
> > --- /dev/null
> > +++ b/drivers/platform/x86/hp/hp-bioscfg/string-attributes.c
> > @@ -0,0 +1,459 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Functions corresponding to string type attributes under
> > + * HP_WMI_BIOS_STRING_GUID for use with hp-bioscfg driver.
> > + *
> > + * Copyright (c) 2022 HP Development Company, L.P.
> > + */
> > +
> > +#include "bioscfg.h"
> > +
> > +#define WMI_STRING_TYPE "HPBIOS_BIOSString"
> > +
> > +get_instance_id(string);
>
> This is weird to read. It looks like a function declaration.
> maybe use DEFINE_GET_INSTANCE_ID(string).
>

get_instance_id part of a group of functions defined in bioscfg.h.
The sample was taken from another driver which declared it in
lowercase.   I will change all functions names declared as a macro to
uppercase and update the names across all files.  The main purpose for
those functions was to avoid duplicating the same functions across all
files.

> > +
> > +static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > +{
> > +     ssize_t ret;
> > +     int instance_id = get_string_instance_id(kobj);
> > +
> > +     if (instance_id < 0)
> > +             return -EIO;
> > +
> > +     ret = sysfs_emit(buf, "%s\n",
> > +                      bioscfg_drv.string_data[instance_id].current_value);
> > +
> > +     return ret;
> > +}
> > +
> > +/*
> > + * validate_string_input() -
> > + * Validate input of current_value against min and max lengths
> > + *
> > + * @instance_id: The instance on which input is validated
> > + * @buf: Input value
> > + */
> > +static int validate_string_input(int instance_id, const char *buf)
>
> Instead of passing around integer ids, that all the callees are using to
> look up some global data, it would be nicer to pass a pointer to the
> concrete instance struct to work on.
>

validate_string_input is part of the defined function
ATTRIBUTE_PROPERTY_STORE in bioscfg.h (line 457).

> This makes the code simpler and removes reference to global state all
> over the place.
>
Changing the values from int to pointer will add unnecessary overhead
since the instance ID is searched only once earlier in the process.

> > +{
> > +     int in_len = strlen(buf);
> > +
> > +     /* BIOS treats it as a read only attribute */
> > +     if (bioscfg_drv.string_data[instance_id].is_readonly)
> > +             return -EIO;
> > +
> > +     if ((in_len < bioscfg_drv.string_data[instance_id].min_length) ||
> > +         (in_len > bioscfg_drv.string_data[instance_id].max_length))
> > +             return -EINVAL;
>
> -ERANGE?
>

Done!

> > +
> > +     /*
> > +      * set pending reboot flag depending on
> > +      * "RequiresPhysicalPresence" value
> > +      */
> > +     if (bioscfg_drv.string_data[instance_id].requires_physical_presence)
> > +             bioscfg_drv.pending_reboot = TRUE;
>
> Just use "true" or "false" instead of "TRUE" and "FALSE".
>

Done!

> > +}
> > +
> > +/* Expected Values types associated with each element */
> > +static acpi_object_type expected_string_types[] = {
>
> Seems this can be const.

Done!
>
> > +     [NAME] = ACPI_TYPE_STRING,
> > +     [VALUE] = ACPI_TYPE_STRING,
> > +     [PATH] = ACPI_TYPE_STRING,
> > +     [IS_READONLY] = ACPI_TYPE_INTEGER,
> > +     [DISPLAY_IN_UI] = ACPI_TYPE_INTEGER,
> > +     [REQUIRES_PHYSICAL_PRESENCE] = ACPI_TYPE_INTEGER,
> > +     [SEQUENCE] = ACPI_TYPE_INTEGER,
> > +     [PREREQUISITES_SIZE] = ACPI_TYPE_INTEGER,
> > +     [PREREQUISITES] = ACPI_TYPE_STRING,
> > +     [SECURITY_LEVEL] = ACPI_TYPE_INTEGER,
> > +     [STR_MIN_LENGTH] = ACPI_TYPE_INTEGER,
> > +     [STR_MAX_LENGTH] = ACPI_TYPE_INTEGER
>
> *Do* add a trailing comma after a non end-of-list marker.
>
Done!

> > +void exit_string_attributes(void)
> > +{
> > +     int instance_id;
> > +
> > +     for (instance_id = 0; instance_id < bioscfg_drv.string_instances_count; instance_id++) {
>
> You can declare loop variables inside the loop. This saves a bunch of
> horizontal space.
>
> > +             if (bioscfg_drv.string_data[instance_id].attr_name_kobj)
> > +                     sysfs_remove_group(bioscfg_drv.string_data[instance_id].attr_name_kobj,
> > +                                        &string_attr_group);
> > +     }
> > +     bioscfg_drv.string_instances_count = 0;
> > +
> > +     kfree(bioscfg_drv.string_data);
> > +     bioscfg_drv.string_data = NULL;
> > +}

Done!  I will keep that in mind when I review the remaining files.

> > diff --git a/drivers/platform/x86/hp/hp-bioscfg/surestart-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/surestart-attributes.c
> > +static struct attribute *sure_start_attrs[] = {
> > +     &sure_start_display_name.attr,
> > +     &sure_start_display_langcode.attr,
> > +     &sure_start_audit_log_entry_count.attr,
> > +     &sure_start_audit_log_entries.attr,
> > +     &sure_start_type.attr,
> > +     NULL,
>
> No trailing comma after end-of-array marker.

Done!