Hi Thomas,
Please see my comments below.
On Sun, Apr 2, 2023 at 11:28 AM Thomas Weißschuh <thomas@xxxxxxxx> 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!
