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!