[v11,05/14] HP BIOSCFG driver - ordered-attributes

Message ID	20230420165454.9517-6-jorge.lopez2@hp.com (mailing list archive)
State	Changes Requested, archived
Headers	show Return-Path: <platform-driver-x86-owner@vger.kernel.org> From: Jorge Lopez <jorgealtxwork@gmail.com> To: hdegoede@redhat.com, platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org, thomas@t-8ch.de Subject: [PATCH v11 05/14] HP BIOSCFG driver - ordered-attributes Date: Thu, 20 Apr 2023 11:54:45 -0500 Message-Id: <20230420165454.9517-6-jorge.lopez2@hp.com> In-Reply-To: <20230420165454.9517-1-jorge.lopez2@hp.com> References: <20230420165454.9517-1-jorge.lopez2@hp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	HP BIOSCFG driver \| expand [v11,00/14] HP BIOSCFG driver [v11,01/14] HP BIOSCFG driver - Documentation [v11,02/14] HP BIOSCFG driver - biosattr-interface [v11,03/14] HP BIOSCFG driver - bioscfg [v11,04/14] HP BIOSCFG driver - int-attributes [v11,05/14] HP BIOSCFG driver - ordered-attributes [v11,06/14] HP BIOSCFG driver - passwdobj-attributes [v11,07/14] HP BIOSCFG driver - string-attributes [v11,08/14] HP BIOSCFG driver - bioscfg-h [v11,09/14] HP BIOSCFG driver - enum-attributes [v11,10/14] HP BIOSCFG driver - passwdattr-interface [v11,11/14] HP BIOSCFG driver - spmobj-attributes [v11,12/14] HP BIOSCFG driver - surestart-attributes [v11,13/14] HP BIOSCFG driver - Makefile [v11,14/14] HP BIOSCFG driver - MAINTAINERS

Jorge Lopez April 20, 2023, 4:54 p.m. UTC

HP BIOS Configuration driver purpose is to provide a driver supporting
the latest sysfs class firmware attributes framework allowing the user
to change BIOS settings and security solutions on HP Inc.’s commercial
notebooks.

Many features of HP Commercial notebooks can be managed using Windows
Management Instrumentation (WMI). WMI is an implementation of Web-Based
Enterprise Management (WBEM) that provides a standards-based interface
for changing and monitoring system settings. HP BIOSCFG driver provides
a native Linux solution and the exposed features facilitates the
migration to Linux environments.

The Linux security features to be provided in hp-bioscfg driver enables
managing the BIOS settings and security solutions via sysfs, a virtual
filesystem that can be used by user-mode applications. The new
documentation cover HP-specific firmware sysfs attributes such Secure
Platform Management and Sure Start. Each section provides security
feature description and identifies sysfs directories and files exposed
by the driver.

Many HP Commercial notebooks include a feature called Secure Platform
Management (SPM), which replaces older password-based BIOS settings
management with public key cryptography. PC secure product management
begins when a target system is provisioned with cryptographic keys
that are used to ensure the integrity of communications between system
management utilities and the BIOS.

HP Commercial notebooks have several BIOS settings that control its
behaviour and capabilities, many of which are related to security.
To prevent unauthorized changes to these settings, the system can
be configured to use a cryptographic signature-based authorization
string that the BIOS will use to verify authorization to modify the
setting.

Linux Security components are under development and not published yet.
The only linux component is the driver (hp bioscfg) at this time.
Other published security components are under Windows.

Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>

---
Based on the latest platform-drivers-x86.git/for-next
---
 .../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

Thomas Weißschuh April 23, 2023, 6:54 a.m. UTC | #1

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
> +++ b/drivers/platform/x86/hp/hp-bioscfg/ordered-attributes.c
> @@ -0,0 +1,563 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Functions corresponding to ordered list type attributes under
> + * BIOS ORDERED LIST GUID for use with hp-bioscfg driver.
> + *
> + *  Copyright (c) 2022 HP Development Company, L.P.
> + */
> +
> +#include "bioscfg.h"
> +
> +GET_INSTANCE_ID(ordered_list);
> +
> +static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +
> +	int instance_id = get_ordered_list_instance_id(kobj);
> +
> +	if (instance_id < 0)
> +		return -EIO;
> +
> +	return sysfs_emit(buf, "%s\n",
> +			 bioscfg_drv.ordered_list_data[instance_id].current_value);
> +}
> +
> +/*
> + * validate_ordered_list_value -
> + * Validate input of current_value against possible values

Does the firmware not also validate this?

If so it may be easier to just let it do so and remove the validations
from the driver.

> + *
> + * @instance_id: The instance on which input is validated
> + * @buf: Input value
> + */
> +static int validate_ordered_list_values(int instance_id, const char *buf)
> +{
> +	int ret = 0;
> +	int found = 0;
> +	char *new_values = NULL;
> +	char *value;
> +	int elem;
> +	int elem_found = 0;
> +
> +	/* Is it a read only attribute */
> +	if (bioscfg_drv.ordered_list_data[instance_id].common.is_readonly)
> +		return -EIO;
> +
> +	new_values = kstrdup(buf, GFP_KERNEL);
> +
> +	/*
> +	 * Changes to ordered list values require checking that new
> +	 * values are found in the list of elements.
> +	 */
> +	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.

> +		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.

> +				found = 1;
> +				break;
> +			}
> +		}
> +
> +
> +		if (!found) {
> +			ret = -EINVAL;
> +			goto out_list_value;
> +		}
> +	}
> +
> +	if (elem_found == bioscfg_drv.ordered_list_data[instance_id].elements_size) {
> +		pr_warn("Number of new values is not equal to number of ordered list elements (%d)\n",
> +			bioscfg_drv.ordered_list_data[instance_id].elements_size);
> +		ret = -EINVAL;
> +		goto out_list_value;
> +	}
> +
> +out_list_value:
> +	kfree(new_values);
> +	return ret;
> +}

This algorithm does not seem to validate that different values are
provided.

So if "possible_values" is "foo,bar,baz" this function would accept
"foo,foo,foo".

> +
> +/*
> + * validate_ordered_input() -
> + * Validate input of current_value against possible values
> + *
> + * @instance_id: The instance on which input is validated
> + * @buf: Input value
> + */
> +static int validate_ordered_list_input(int instance_id, const char *buf)
> +{
> +	int ret = 0;
> +
> +	ret = validate_ordered_list_values(instance_id, buf);
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	/*
> +	 * set pending reboot flag depending on
> +	 * "RequiresPhysicalPresence" value
> +	 */
> +	if (bioscfg_drv.ordered_list_data[instance_id].common.requires_physical_presence)
> +		bioscfg_drv.pending_reboot = true;
> +
> +	return ret;
> +}
> +
> +static void update_ordered_list_value(int instance_id, char *attr_value)
> +{
> +	strscpy(bioscfg_drv.ordered_list_data[instance_id].current_value,
> +		attr_value,
> +		sizeof(bioscfg_drv.ordered_list_data[instance_id].current_value));
> +}
> +
> +ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name_language_code, ordered_list);
> +static struct kobj_attribute ordered_list_display_langcode =
> +	__ATTR_RO(display_name_language_code);
> +
> +ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name, ordered_list);
> +static struct kobj_attribute ordered_list_display_name =
> +	__ATTR_RO(display_name);
> +
> +ATTRIBUTE_PROPERTY_STORE(current_value, ordered_list);
> +static struct kobj_attribute ordered_list_current_val =
> +	__ATTR_RW_MODE(current_value, 0644);
> +
> +
> +ATTRIBUTE_N_COMMON_PROPERTY_SHOW(prerequisites_size, ordered_list);
> +static struct kobj_attribute  ordered_list_prerequisites_size_val =
> +	__ATTR_RO(prerequisites_size);
> +
> +ATTRIBUTE_V_COMMON_PROPERTY_SHOW(prerequisites, ordered_list);
> +static struct kobj_attribute  ordered_list_prerequisites_val =
> +	__ATTR_RO(prerequisites);
> +
> +ATTRIBUTE_N_PROPERTY_SHOW(elements_size, ordered_list);
> +static struct kobj_attribute  ordered_list_elements_size_val =
> +	__ATTR_RO(elements_size);

"size" and "length" attributes are fairly useless to userspace.
They can't be trusted to provide information about another attribute as
the information can be out of date when the other attribute is read.

> +
> +ATTRIBUTE_VALUES_PROPERTY_SHOW(elements, ordered_list);
> +static struct kobj_attribute  ordered_list_elements_val =
> +	__ATTR_RO(elements);
> +
> +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
> +			 char *buf)
> +{
> +	return sysfs_emit(buf, "ordered-list\n");
> +}
> +static struct kobj_attribute ordered_list_type =
> +	__ATTR_RO(type);
> +
> +static struct attribute *ordered_list_attrs[] = {
> +	&ordered_list_display_langcode.attr,
> +	&ordered_list_display_name.attr,
> +	&ordered_list_current_val.attr,
> +	&ordered_list_prerequisites_size_val.attr,
> +	&ordered_list_prerequisites_val.attr,
> +	&ordered_list_elements_val.attr,
> +	&ordered_list_elements_size_val.attr,
> +	&ordered_list_type.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group ordered_list_attr_group = {
> +	.attrs = ordered_list_attrs,
> +};
> +
> +int alloc_ordered_list_data(void)
> +{
> +	int ret = 0;
> +
> +	bioscfg_drv.ordered_list_instances_count =
> +		get_instance_count(HP_WMI_BIOS_ORDERED_LIST_GUID);
> +	bioscfg_drv.ordered_list_data = kcalloc(bioscfg_drv.ordered_list_instances_count,
> +						sizeof(struct ordered_list_data), GFP_KERNEL);
> +	if (!bioscfg_drv.ordered_list_data) {
> +		bioscfg_drv.ordered_list_instances_count = 0;
> +		ret = -ENOMEM;
> +	}
> +	return ret;
> +}
> +
> +/*
> + * populate_ordered_list_package_data() -
> + * Populate all properties of an instance under ordered_list attribute
> + *
> + * @order_obj: ACPI object with ordered_list data
> + * @instance_id: The instance to enumerate
> + * @attr_name_kobj: The parent kernel object
> + */
> +int populate_ordered_list_package_data(union acpi_object *order_obj, int instance_id,
> +				       struct kobject *attr_name_kobj)
> +{
> +	bioscfg_drv.ordered_list_data[instance_id].attr_name_kobj = attr_name_kobj;
> +
> +	populate_ordered_list_elements_from_package(order_obj,
> +						    order_obj->package.count,
> +						    instance_id);
> +	update_attribute_permissions(bioscfg_drv.ordered_list_data[instance_id].common.is_readonly,
> +				     &ordered_list_current_val);
> +	friendly_user_name_update(bioscfg_drv.ordered_list_data[instance_id].common.path,
> +				  attr_name_kobj->name,
> +				  bioscfg_drv.ordered_list_data[instance_id].common.display_name,
> +				  sizeof(bioscfg_drv.ordered_list_data[instance_id].common.display_name));
> +	return sysfs_create_group(attr_name_kobj, &ordered_list_attr_group);
> +}
> +
> +/* Expected Values types associated with each element */
> +static const acpi_object_type expected_order_types[] = {
> +	[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,
> +	[ORD_LIST_SIZE] = ACPI_TYPE_INTEGER,
> +	[ORD_LIST_ELEMENTS] = ACPI_TYPE_STRING
> +};
> +
> +
> +int populate_ordered_list_elements_from_package(union acpi_object *order_obj,
> +						int order_obj_count,
> +						int instance_id)
> +{
> +	char *str_value = NULL;
> +	int value_len;
> +	int ret = 0;
> +	u32 size = 0;
> +	u32 int_value;
> +	int elem = 0;
> +	int reqs;
> +	int eloc;
> +	char *tmpstr = NULL;
> +	char *part_tmp = NULL;
> +	int tmp_len = 0;
> +	char *part = NULL;
> +
> +	if (!order_obj)
> +		return -EINVAL;
> +
> +	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?

> +
> +	for (elem = 1, eloc = 1; elem < order_obj_count; elem++, eloc++) {
> +
> +		/* ONLY look at the first ORDERED_ELEM_CNT elements */
> +		if (eloc == ORDERED_ELEM_CNT)
> +			goto exit_list_package;
> +
> +		switch (order_obj[elem].type) {
> +		case ACPI_TYPE_STRING:
> +
> +			if (elem != PREREQUISITES && elem != ORD_LIST_ELEMENTS) {
> +				ret = convert_hexstr_to_str(order_obj[elem].string.pointer,
> +							    order_obj[elem].string.length,
> +							    &str_value, &value_len);
> +				if (ret)
> +					continue;
> +			}
> +			break;
> +		case ACPI_TYPE_INTEGER:
> +			int_value = (u32)order_obj[elem].integer.value;
> +			break;
> +		default:
> +			pr_warn("Unsupported object type [%d]\n", order_obj[elem].type);
> +			continue;
> +		}
> +
> +		/* Check that both expected and read object type match */
> +		if (expected_order_types[eloc] != order_obj[elem].type) {
> +			pr_err("Error expected type %d for elem  %d, but got type %d instead\n",
> +			       expected_order_types[eloc], elem, order_obj[elem].type);
> +			return -EIO;
> +		}
> +
> +		/* Assign appropriate element value to corresponding field*/
> +		switch (eloc) {
> +		case VALUE:
> +			strscpy(bioscfg_drv.ordered_list_data[instance_id].current_value,
> +				str_value, sizeof(bioscfg_drv.ordered_list_data[instance_id].current_value));
> +			break;
> +		case PATH:
> +			strscpy(bioscfg_drv.ordered_list_data[instance_id].common.path, str_value,
> +				sizeof(bioscfg_drv.ordered_list_data[instance_id].common.path));
> +			break;
> +		case IS_READONLY:
> +			bioscfg_drv.ordered_list_data[instance_id].common.is_readonly = int_value;
> +			break;
> +		case DISPLAY_IN_UI:
> +			bioscfg_drv.ordered_list_data[instance_id].common.display_in_ui = int_value;
> +			break;
> +		case REQUIRES_PHYSICAL_PRESENCE:
> +			bioscfg_drv.ordered_list_data[instance_id].common.requires_physical_presence = int_value;
> +			break;
> +		case SEQUENCE:
> +			bioscfg_drv.ordered_list_data[instance_id].common.sequence = int_value;
> +			break;
> +		case PREREQUISITES_SIZE:
> +			bioscfg_drv.ordered_list_data[instance_id].common.prerequisites_size = int_value;
> +			if (int_value > MAX_PREREQUISITES_SIZE)
> +				pr_warn("Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n");
> +
> +			/*
> +			 * This HACK is needed to keep the expected
> +			 * element list pointing to the right obj[elem].type
> +			 * when the size is zero.  PREREQUISITES
> +			 * object is omitted by BIOS when the size is
> +			 * zero.
> +			 */
> +			if (int_value == 0)
> +				eloc++;
> +			break;
> +		case PREREQUISITES:
> +			size = bioscfg_drv.ordered_list_data[instance_id].common.prerequisites_size;
> +
> +			for (reqs = 0; reqs < size && reqs < MAX_PREREQUISITES_SIZE; reqs++) {
> +				ret = convert_hexstr_to_str(order_obj[elem + reqs].string.pointer,
> +							    order_obj[elem + reqs].string.length,
> +							    &str_value, &value_len);
> +
> +				if (ret)
> +					continue;
> +
> +				strscpy(bioscfg_drv.ordered_list_data[instance_id].common.prerequisites[reqs],
> +					str_value,
> +					sizeof(bioscfg_drv.ordered_list_data[instance_id].common.prerequisites[reqs]));
> +
> +				kfree(str_value);
> +			}
> +			break;
> +
> +		case SECURITY_LEVEL:
> +			bioscfg_drv.ordered_list_data[instance_id].common.security_level = int_value;
> +			break;
> +
> +		case ORD_LIST_SIZE:
> +			bioscfg_drv.ordered_list_data[instance_id].elements_size = int_value;
> +			if (int_value > MAX_ELEMENTS_SIZE)
> +				pr_warn("Ordered List size value exceeded the maximum number of elements supported or data may be malformed\n");
> +			/*
> +			 * This HACK is needed to keep the expected
> +			 * element list pointing to the right obj[elem].type
> +			 * when the size is zero.  ORD_LIST_ELEMENTS
> +			 * object is omitted by BIOS when the size is
> +			 * zero.
> +			 */
> +			if (int_value == 0)
> +				eloc++;
> +			break;
> +		case ORD_LIST_ELEMENTS:
> +			size = bioscfg_drv.ordered_list_data[instance_id].elements_size;
> +
> +			/*
> +			 * Ordered list data is stored in hex and comma separated format
> +			 * Convert the data and split it to show each element
> +			 */
> +			ret = convert_hexstr_to_str(str_value, value_len, &tmpstr, &tmp_len);
> +			if (ret)
> +				goto exit_list_package;
> +
> +			part_tmp = tmpstr;
> +			part = strsep(&part_tmp, ",");
> +			if (!part)
> +				strscpy(bioscfg_drv.ordered_list_data[instance_id].elements[0],
> +					tmpstr,
> +					sizeof(bioscfg_drv.ordered_list_data[instance_id].elements[0]));
> +
> +			for (elem = 1; elem < MAX_ELEMENTS_SIZE && part; elem++) {
> +				strscpy(bioscfg_drv.ordered_list_data[instance_id].elements[elem],
> +					part,
> +					sizeof(bioscfg_drv.ordered_list_data[instance_id].elements[elem]));
> +				part = strsep(&part_tmp, ",");
> +			}
> +
> +			kfree(tmpstr);
> +			break;
> +		default:
> +			pr_warn("Invalid element: %d found in Ordered_List attribute or data may be malformed\n", elem);
> +			break;
> +		}
> +		kfree(tmpstr);
> +		kfree(str_value);
> +	}
> +
> +exit_list_package:
> +	kfree(tmpstr);
> +	kfree(str_value);
> +	return 0;
> +}
> +
> +/*
> + * populate_ordered_list_data() - Populate all properties of an
> + * instance under ordered list attribute
> + *
> + * @buffer_ptr: Buffer pointer
> + * @buffer_size: Buffer size
> + * @instance_id: The instance to enumerate
> + * @attr_name_kobj: The parent kernel object
> + * @enum_property_count: Total properties count under ordered list type
> + */
> +int populate_ordered_list_buffer_data(u8 *buffer_ptr, u32 *buffer_size, int instance_id,
> +				      struct kobject *attr_name_kobj)
> +{
> +
> +	bioscfg_drv.ordered_list_data[instance_id].attr_name_kobj = attr_name_kobj;
> +
> +	/* Populate ordered list elements */
> +	populate_ordered_list_elements_from_buffer(buffer_ptr, buffer_size,
> +						   instance_id);
> +	update_attribute_permissions(bioscfg_drv.ordered_list_data[instance_id].common.is_readonly,
> +				     &ordered_list_current_val);
> +	friendly_user_name_update(bioscfg_drv.ordered_list_data[instance_id].common.path,
> +				  attr_name_kobj->name,
> +				  bioscfg_drv.ordered_list_data[instance_id].common.display_name,
> +				  sizeof(bioscfg_drv.ordered_list_data[instance_id].common.display_name));
> +
> +	return sysfs_create_group(attr_name_kobj, &ordered_list_attr_group);
> +}
> +
> +int populate_ordered_list_elements_from_buffer(u8 *buffer_ptr, u32 *buffer_size,
> +					       int instance_id)
> +{
> +	int ret;
> +	char *dst = NULL;
> +	int elem;
> +	int reqs;
> +	int integer;
> +	int size = 0;
> +	int values;
> +	int dst_size = *buffer_size / sizeof(u16);
> +
> +	dst = kcalloc(dst_size, sizeof(char), GFP_KERNEL);
> +	if (!dst)
> +		return -ENOMEM;
> +
> +	elem = 0;
> +	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));
> +
> +	for (elem = 1; elem < 3; elem++) {
> +
> +		ret = get_string_from_buffer(&buffer_ptr, buffer_size, dst, dst_size);
> +		if (ret < 0)
> +			continue;
> +
> +		switch (elem) {
> +		case VALUE:
> +			strscpy(bioscfg_drv.ordered_list_data[instance_id].current_value,
> +				dst, sizeof(bioscfg_drv.ordered_list_data[instance_id].current_value));
> +			break;
> +		case PATH:
> +			strscpy(bioscfg_drv.ordered_list_data[instance_id].common.path, dst,
> +				sizeof(bioscfg_drv.ordered_list_data[instance_id].common.path));
> +			break;
> +		default:
> +			pr_warn("Invalid element: %d found in Ordered list attribute or data may be malformed\n", elem);
> +			break;
> +		}
> +	}
> +	for (elem = 3; elem < ORDERED_ELEM_CNT; elem++) {
> +
> +		if (elem != PREREQUISITES && elem != ORD_LIST_ELEMENTS) {
> +			ret = get_integer_from_buffer((int **)&buffer_ptr, buffer_size, (int *)&integer);
> +			if (ret < 0)
> +				continue;
> +		}
> +
> +		switch (elem) {
> +
> +		case IS_READONLY:
> +			bioscfg_drv.ordered_list_data[instance_id].common.is_readonly = integer;
> +			break;
> +		case DISPLAY_IN_UI:
> +			bioscfg_drv.ordered_list_data[instance_id].common.display_in_ui = integer;
> +			break;
> +		case REQUIRES_PHYSICAL_PRESENCE:
> +			bioscfg_drv.ordered_list_data[instance_id].common.requires_physical_presence = integer;
> +			break;
> +		case SEQUENCE:
> +			bioscfg_drv.ordered_list_data[instance_id].common.sequence = integer;
> +			break;
> +		case PREREQUISITES_SIZE:
> +			bioscfg_drv.ordered_list_data[instance_id].common.prerequisites_size = integer;
> +			if (integer > MAX_PREREQUISITES_SIZE)
> +				pr_warn("Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n");
> +
> +			// PREREQUISITES:
> +			elem++;
> +			size = bioscfg_drv.ordered_list_data[instance_id].common.prerequisites_size;
> +			for (reqs = 0; reqs < size && reqs < MAX_PREREQUISITES_SIZE; reqs++) {
> +				ret = get_string_from_buffer(&buffer_ptr, buffer_size, dst, dst_size);
> +				if (ret < 0)
> +					continue;
> +
> +				strscpy(bioscfg_drv.ordered_list_data[instance_id].common.prerequisites[reqs],
> +					dst,
> +					sizeof(bioscfg_drv.ordered_list_data[instance_id].common.prerequisites[reqs]));
> +			}
> +			break;
> +		case SECURITY_LEVEL:
> +			bioscfg_drv.ordered_list_data[instance_id].common.security_level = integer;
> +			break;
> +		case ORD_LIST_SIZE:
> +			bioscfg_drv.ordered_list_data[instance_id].elements_size = integer;
> +			if (integer > MAX_ELEMENTS_SIZE)
> +				pr_warn("Ordered List size value exceeded the maximum number of elements supported or data may be malformed\n");
> +
> +			// ORD_LIST_ELEMENTS:
> +			elem++;
> +			size = bioscfg_drv.ordered_list_data[instance_id].elements_size;
> +			for (values = 0; values < size && values < MAX_ELEMENTS_SIZE; values++) {
> +				ret = get_string_from_buffer(&buffer_ptr, buffer_size, dst, dst_size);
> +				if (ret < 0)
> +					continue;
> +
> +				strscpy(bioscfg_drv.ordered_list_data[instance_id].elements[values],
> +					dst,
> +					sizeof(bioscfg_drv.ordered_list_data[instance_id].elements)[values]);
> +			}
> +			break;
> +		default:
> +			pr_warn("Invalid element: %d found in Ordered list attribute or data may be malformed\n", elem);
> +			break;
> +		}
> +	}
> +	kfree(dst);
> +
> +	return 0;
> +}
> +
> +
> +/*
> + * exit_ordered_list_attributes() - Clear all attribute data
> + *
> + * Clears all data allocated for this group of attributes
> + */
> +void exit_ordered_list_attributes(void)
> +{
> +	int instance_id;
> +
> +	for (instance_id = 0; instance_id < bioscfg_drv.ordered_list_instances_count; instance_id++) {
> +		struct kobject *attr_name_kobj = bioscfg_drv.ordered_list_data[instance_id].attr_name_kobj;
> +
> +		if (attr_name_kobj)
> +			sysfs_remove_group(attr_name_kobj,
> +					   &ordered_list_attr_group);
> +	}
> +	bioscfg_drv.ordered_list_instances_count = 0;
> +
> +	kfree(bioscfg_drv.ordered_list_data);
> +	bioscfg_drv.ordered_list_data = NULL;
> +}
> -- 
> 2.34.1
>

Jorge Lopez May 5, 2023, 4:09 p.m. UTC | #2

On Sun, Apr 23, 2023 at 1:55 AM <thomas@t-8ch.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
> > +++ b/drivers/platform/x86/hp/hp-bioscfg/ordered-attributes.c
> > @@ -0,0 +1,563 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Functions corresponding to ordered list type attributes under
> > + * BIOS ORDERED LIST GUID for use with hp-bioscfg driver.
> > + *
> > + *  Copyright (c) 2022 HP Development Company, L.P.
> > + */
> > +
> > +#include "bioscfg.h"
> > +
> > +GET_INSTANCE_ID(ordered_list);
> > +
> > +static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > +{
> > +
> > +     int instance_id = get_ordered_list_instance_id(kobj);
> > +
> > +     if (instance_id < 0)
> > +             return -EIO;
> > +
> > +     return sysfs_emit(buf, "%s\n",
> > +                      bioscfg_drv.ordered_list_data[instance_id].current_value);
> > +}
> > +
> > +/*
> > + * validate_ordered_list_value -
> > + * Validate input of current_value against possible values
>
> Does the firmware not also validate this?
>
> If so it may be easier to just let it do so and remove the validations
> from the driver.

Yes.  the firmware validates the data.
Will remove the validation
>
> > + *
> > + * @instance_id: The instance on which input is validated
> > + * @buf: Input value
> > + */
> > +static int validate_ordered_list_values(int instance_id, const char *buf)
> > +{
> > +     int ret = 0;
> > +     int found = 0;
> > +     char *new_values = NULL;
> > +     char *value;
> > +     int elem;
> > +     int elem_found = 0;
> > +
> > +     /* Is it a read only attribute */
> > +     if (bioscfg_drv.ordered_list_data[instance_id].common.is_readonly)
> > +             return -EIO;
> > +
> > +     new_values = kstrdup(buf, GFP_KERNEL);
> > +
> > +     /*
> > +      * Changes to ordered list values require checking that new
> > +      * values are found in the list of elements.
> > +      */
> > +     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

>
> > +             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.

>
> > +                             found = 1;
> > +                             break;
> > +                     }
> > +             }
> > +
> > +
> > +             if (!found) {
> > +                     ret = -EINVAL;
> > +                     goto out_list_value;
> > +             }
> > +     }
> > +
> > +     if (elem_found == bioscfg_drv.ordered_list_data[instance_id].elements_size) {
> > +             pr_warn("Number of new values is not equal to number of ordered list elements (%d)\n",
> > +                     bioscfg_drv.ordered_list_data[instance_id].elements_size);
> > +             ret = -EINVAL;
> > +             goto out_list_value;
> > +     }
> > +
> > +out_list_value:
> > +     kfree(new_values);
> > +     return ret;
> > +}
>
> This algorithm does not seem to validate that different values are
> provided.
>
> So if "possible_values" is "foo,bar,baz" this function would accept
> "foo,foo,foo".
>

BIOS will reject strings such as "foo,foo,foo" when the current value
is "foo,bar,baz".   It is ok to provide a string which items are
ordered differently.  i.e. "baz,bar,foo"
validate_ordered_list_values() function will be removed as indicated earlier.

> > +
> > +/*
> > + * validate_ordered_input() -
> > + * Validate input of current_value against possible values
> > + *
> > + * @instance_id: The instance on which input is validated
> > + * @buf: Input value
> > + */
> > +static int validate_ordered_list_input(int instance_id, const char *buf)
> > +{
> > +     int ret = 0;
> > +
> > +     ret = validate_ordered_list_values(instance_id, buf);
> > +     if (ret < 0)
> > +             return -EINVAL;
> > +
> > +     /*
> > +      * set pending reboot flag depending on
> > +      * "RequiresPhysicalPresence" value
> > +      */
> > +     if (bioscfg_drv.ordered_list_data[instance_id].common.requires_physical_presence)
> > +             bioscfg_drv.pending_reboot = true;
> > +
> > +     return ret;
> > +}
> > +
> > +static void update_ordered_list_value(int instance_id, char *attr_value)
> > +{
> > +     strscpy(bioscfg_drv.ordered_list_data[instance_id].current_value,
> > +             attr_value,
> > +             sizeof(bioscfg_drv.ordered_list_data[instance_id].current_value));
> > +}
> > +
> > +ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name_language_code, ordered_list);
> > +static struct kobj_attribute ordered_list_display_langcode =
> > +     __ATTR_RO(display_name_language_code);
> > +
> > +ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name, ordered_list);
> > +static struct kobj_attribute ordered_list_display_name =
> > +     __ATTR_RO(display_name);
> > +
> > +ATTRIBUTE_PROPERTY_STORE(current_value, ordered_list);
> > +static struct kobj_attribute ordered_list_current_val =
> > +     __ATTR_RW_MODE(current_value, 0644);
> > +
> > +
> > +ATTRIBUTE_N_COMMON_PROPERTY_SHOW(prerequisites_size, ordered_list);
> > +static struct kobj_attribute  ordered_list_prerequisites_size_val =
> > +     __ATTR_RO(prerequisites_size);
> > +
> > +ATTRIBUTE_V_COMMON_PROPERTY_SHOW(prerequisites, ordered_list);
> > +static struct kobj_attribute  ordered_list_prerequisites_val =
> > +     __ATTR_RO(prerequisites);
> > +
> > +ATTRIBUTE_N_PROPERTY_SHOW(elements_size, ordered_list);
> > +static struct kobj_attribute  ordered_list_elements_size_val =
> > +     __ATTR_RO(elements_size);
>
> "size" and "length" attributes are fairly useless to userspace.
> They can't be trusted to provide information about another attribute as
> the information can be out of date when the other attribute is read.
>

Prerequisites, prerequisites_size and elements_size will be removed

> > +
> > +ATTRIBUTE_VALUES_PROPERTY_SHOW(elements, ordered_list);
> > +static struct kobj_attribute  ordered_list_elements_val =
> > +     __ATTR_RO(elements);
> > +

<snip>

> > +
> > +
> > +int populate_ordered_list_elements_from_package(union acpi_object *order_obj,
> > +                                             int order_obj_count,
> > +                                             int instance_id)
> > +{
> > +     char *str_value = NULL;
> > +     int value_len;
> > +     int ret = 0;
> > +     u32 size = 0;
> > +     u32 int_value;
> > +     int elem = 0;
> > +     int reqs;
> > +     int eloc;
> > +     char *tmpstr = NULL;
> > +     char *part_tmp = NULL;
> > +     int tmp_len = 0;
> > +     char *part = NULL;
> > +
> > +     if (!order_obj)
> > +             return -EINVAL;
> > +
> > +     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.
>

<snip>

Thomas Weißschuh May 5, 2023, 9:11 p.m. UTC | #3

On 2023-05-05 11:09:55-0500, Jorge Lopez wrote:
> On Sun, Apr 23, 2023 at 1:55 AM <thomas@t-8ch.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
> > > +++ b/drivers/platform/x86/hp/hp-bioscfg/ordered-attributes.c
> > > @@ -0,0 +1,563 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Functions corresponding to ordered list type attributes under
> > > + * BIOS ORDERED LIST GUID for use with hp-bioscfg driver.
> > > + *
> > > + *  Copyright (c) 2022 HP Development Company, L.P.
> > > + */
> > > +
> > > +#include "bioscfg.h"
> > > +
> > > +GET_INSTANCE_ID(ordered_list);
> > > +
> > > +static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > > +{
> > > +
> > > +     int instance_id = get_ordered_list_instance_id(kobj);
> > > +
> > > +     if (instance_id < 0)
> > > +             return -EIO;
> > > +
> > > +     return sysfs_emit(buf, "%s\n",
> > > +                      bioscfg_drv.ordered_list_data[instance_id].current_value);
> > > +}
> > > +
> > > +/*
> > > + * validate_ordered_list_value -
> > > + * Validate input of current_value against possible values
> >
> > Does the firmware not also validate this?
> >
> > If so it may be easier to just let it do so and remove the validations
> > from the driver.
> 
> Yes.  the firmware validates the data.
> Will remove the validation
> >
> > > + *
> > > + * @instance_id: The instance on which input is validated
> > > + * @buf: Input value
> > > + */
> > > +static int validate_ordered_list_values(int instance_id, const char *buf)
> > > +{
> > > +     int ret = 0;
> > > +     int found = 0;
> > > +     char *new_values = NULL;
> > > +     char *value;
> > > +     int elem;
> > > +     int elem_found = 0;
> > > +
> > > +     /* Is it a read only attribute */
> > > +     if (bioscfg_drv.ordered_list_data[instance_id].common.is_readonly)
> > > +             return -EIO;
> > > +
> > > +     new_values = kstrdup(buf, GFP_KERNEL);
> > > +
> > > +     /*
> > > +      * Changes to ordered list values require checking that new
> > > +      * values are found in the list of elements.
> > > +      */
> > > +     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.

> > > +             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.
> 
> >
> > > +                             found = 1;
> > > +                             break;
> > > +                     }
> > > +             }
> > > +
> > > +
> > > +             if (!found) {
> > > +                     ret = -EINVAL;
> > > +                     goto out_list_value;
> > > +             }
> > > +     }
> > > +
> > > +     if (elem_found == bioscfg_drv.ordered_list_data[instance_id].elements_size) {
> > > +             pr_warn("Number of new values is not equal to number of ordered list elements (%d)\n",
> > > +                     bioscfg_drv.ordered_list_data[instance_id].elements_size);
> > > +             ret = -EINVAL;
> > > +             goto out_list_value;
> > > +     }
> > > +
> > > +out_list_value:
> > > +     kfree(new_values);
> > > +     return ret;
> > > +}
> >
> > This algorithm does not seem to validate that different values are
> > provided.
> >
> > So if "possible_values" is "foo,bar,baz" this function would accept
> > "foo,foo,foo".
> >
> 
> BIOS will reject strings such as "foo,foo,foo" when the current value
> is "foo,bar,baz".   It is ok to provide a string which items are
> ordered differently.  i.e. "baz,bar,foo"
> validate_ordered_list_values() function will be removed as indicated earlier.
> 
> > > +
> > > +/*
> > > + * validate_ordered_input() -
> > > + * Validate input of current_value against possible values
> > > + *
> > > + * @instance_id: The instance on which input is validated
> > > + * @buf: Input value
> > > + */
> > > +static int validate_ordered_list_input(int instance_id, const char *buf)
> > > +{
> > > +     int ret = 0;
> > > +
> > > +     ret = validate_ordered_list_values(instance_id, buf);
> > > +     if (ret < 0)
> > > +             return -EINVAL;
> > > +
> > > +     /*
> > > +      * set pending reboot flag depending on
> > > +      * "RequiresPhysicalPresence" value
> > > +      */
> > > +     if (bioscfg_drv.ordered_list_data[instance_id].common.requires_physical_presence)
> > > +             bioscfg_drv.pending_reboot = true;
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static void update_ordered_list_value(int instance_id, char *attr_value)
> > > +{
> > > +     strscpy(bioscfg_drv.ordered_list_data[instance_id].current_value,
> > > +             attr_value,
> > > +             sizeof(bioscfg_drv.ordered_list_data[instance_id].current_value));
> > > +}
> > > +
> > > +ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name_language_code, ordered_list);
> > > +static struct kobj_attribute ordered_list_display_langcode =
> > > +     __ATTR_RO(display_name_language_code);
> > > +
> > > +ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name, ordered_list);
> > > +static struct kobj_attribute ordered_list_display_name =
> > > +     __ATTR_RO(display_name);
> > > +
> > > +ATTRIBUTE_PROPERTY_STORE(current_value, ordered_list);
> > > +static struct kobj_attribute ordered_list_current_val =
> > > +     __ATTR_RW_MODE(current_value, 0644);
> > > +
> > > +
> > > +ATTRIBUTE_N_COMMON_PROPERTY_SHOW(prerequisites_size, ordered_list);
> > > +static struct kobj_attribute  ordered_list_prerequisites_size_val =
> > > +     __ATTR_RO(prerequisites_size);
> > > +
> > > +ATTRIBUTE_V_COMMON_PROPERTY_SHOW(prerequisites, ordered_list);
> > > +static struct kobj_attribute  ordered_list_prerequisites_val =
> > > +     __ATTR_RO(prerequisites);
> > > +
> > > +ATTRIBUTE_N_PROPERTY_SHOW(elements_size, ordered_list);
> > > +static struct kobj_attribute  ordered_list_elements_size_val =
> > > +     __ATTR_RO(elements_size);
> >
> > "size" and "length" attributes are fairly useless to userspace.
> > They can't be trusted to provide information about another attribute as
> > the information can be out of date when the other attribute is read.
> >
> 
> Prerequisites, prerequisites_size and elements_size will be removed
> 
> > > +
> > > +ATTRIBUTE_VALUES_PROPERTY_SHOW(elements, ordered_list);
> > > +static struct kobj_attribute  ordered_list_elements_val =
> > > +     __ATTR_RO(elements);
> > > +
> 
> <snip>
> 
> > > +
> > > +
> > > +int populate_ordered_list_elements_from_package(union acpi_object *order_obj,
> > > +                                             int order_obj_count,
> > > +                                             int instance_id)
> > > +{
> > > +     char *str_value = NULL;
> > > +     int value_len;
> > > +     int ret = 0;
> > > +     u32 size = 0;
> > > +     u32 int_value;
> > > +     int elem = 0;
> > > +     int reqs;
> > > +     int eloc;
> > > +     char *tmpstr = NULL;
> > > +     char *part_tmp = NULL;
> > > +     int tmp_len = 0;
> > > +     char *part = NULL;
> > > +
> > > +     if (!order_obj)
> > > +             return -EINVAL;
> > > +
> > > +     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?

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...

Jorge Lopez May 5, 2023, 9:57 p.m. UTC | #4

On Fri, May 5, 2023 at 4:11 PM Thomas Weißschuh <thomas@t-8ch.de> wrote:
>
> On 2023-05-05 11:09:55-0500, Jorge Lopez wrote:
> > On Sun, Apr 23, 2023 at 1:55 AM <thomas@t-8ch.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
> > > > +++ b/drivers/platform/x86/hp/hp-bioscfg/ordered-attributes.c
> > > > @@ -0,0 +1,563 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Functions corresponding to ordered list type attributes under
> > > > + * BIOS ORDERED LIST GUID for use with hp-bioscfg driver.
> > > > + *
> > > > + *  Copyright (c) 2022 HP Development Company, L.P.
> > > > + */
> > > > +
> > > > +#include "bioscfg.h"
> > > > +
> > > > +GET_INSTANCE_ID(ordered_list);
> > > > +
> > > > +static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > > > +{
> > > > +
> > > > +     int instance_id = get_ordered_list_instance_id(kobj);
> > > > +
> > > > +     if (instance_id < 0)
> > > > +             return -EIO;
> > > > +
> > > > +     return sysfs_emit(buf, "%s\n",
> > > > +                      bioscfg_drv.ordered_list_data[instance_id].current_value);
> > > > +}
> > > > +
> > > > +/*
> > > > + * validate_ordered_list_value -
> > > > + * Validate input of current_value against possible values
> > >
> > > Does the firmware not also validate this?
> > >
> > > If so it may be easier to just let it do so and remove the validations
> > > from the driver.
> >
> > Yes.  the firmware validates the data.
> > Will remove the validation
> > >
> > > > + *
> > > > + * @instance_id: The instance on which input is validated
> > > > + * @buf: Input value
> > > > + */
> > > > +static int validate_ordered_list_values(int instance_id, const char *buf)
> > > > +{
> > > > +     int ret = 0;
> > > > +     int found = 0;
> > > > +     char *new_values = NULL;
> > > > +     char *value;
> > > > +     int elem;
> > > > +     int elem_found = 0;
> > > > +
> > > > +     /* Is it a read only attribute */
> > > > +     if (bioscfg_drv.ordered_list_data[instance_id].common.is_readonly)
> > > > +             return -EIO;
> > > > +
> > > > +     new_values = kstrdup(buf, GFP_KERNEL);
> > > > +
> > > > +     /*
> > > > +      * Changes to ordered list values require checking that new
> > > > +      * values are found in the list of elements.
> > > > +      */
> > > > +     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.


> > > > +             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.
> >
> > >
> > > > +                             found = 1;
> > > > +                             break;
> > > > +                     }
> > > > +             }
> > > > +
> > > > +
> > > > +             if (!found) {
> > > > +                     ret = -EINVAL;
> > > > +                     goto out_list_value;
> > > > +             }
> > > > +     }
> > > > +
> > > > +     if (elem_found == bioscfg_drv.ordered_list_data[instance_id].elements_size) {
> > > > +             pr_warn("Number of new values is not equal to number of ordered list elements (%d)\n",
> > > > +                     bioscfg_drv.ordered_list_data[instance_id].elements_size);
> > > > +             ret = -EINVAL;
> > > > +             goto out_list_value;
> > > > +     }
> > > > +
> > > > +out_list_value:
> > > > +     kfree(new_values);
> > > > +     return ret;
> > > > +}
> > >
> > > This algorithm does not seem to validate that different values are
> > > provided.
> > >
> > > So if "possible_values" is "foo,bar,baz" this function would accept
> > > "foo,foo,foo".
> > >
> >
> > BIOS will reject strings such as "foo,foo,foo" when the current value
> > is "foo,bar,baz".   It is ok to provide a string which items are
> > ordered differently.  i.e. "baz,bar,foo"
> > validate_ordered_list_values() function will be removed as indicated earlier.
> >
> > > > +
> > > > +/*
> > > > + * validate_ordered_input() -
> > > > + * Validate input of current_value against possible values
> > > > + *
> > > > + * @instance_id: The instance on which input is validated
> > > > + * @buf: Input value
> > > > + */
> > > > +static int validate_ordered_list_input(int instance_id, const char *buf)
> > > > +{
> > > > +     int ret = 0;
> > > > +
> > > > +     ret = validate_ordered_list_values(instance_id, buf);
> > > > +     if (ret < 0)
> > > > +             return -EINVAL;
> > > > +
> > > > +     /*
> > > > +      * set pending reboot flag depending on
> > > > +      * "RequiresPhysicalPresence" value
> > > > +      */
> > > > +     if (bioscfg_drv.ordered_list_data[instance_id].common.requires_physical_presence)
> > > > +             bioscfg_drv.pending_reboot = true;
> > > > +
> > > > +     return ret;
> > > > +}
> > > > +
> > > > +static void update_ordered_list_value(int instance_id, char *attr_value)
> > > > +{
> > > > +     strscpy(bioscfg_drv.ordered_list_data[instance_id].current_value,
> > > > +             attr_value,
> > > > +             sizeof(bioscfg_drv.ordered_list_data[instance_id].current_value));
> > > > +}
> > > > +
> > > > +ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name_language_code, ordered_list);
> > > > +static struct kobj_attribute ordered_list_display_langcode =
> > > > +     __ATTR_RO(display_name_language_code);
> > > > +
> > > > +ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name, ordered_list);
> > > > +static struct kobj_attribute ordered_list_display_name =
> > > > +     __ATTR_RO(display_name);
> > > > +
> > > > +ATTRIBUTE_PROPERTY_STORE(current_value, ordered_list);
> > > > +static struct kobj_attribute ordered_list_current_val =
> > > > +     __ATTR_RW_MODE(current_value, 0644);
> > > > +
> > > > +
> > > > +ATTRIBUTE_N_COMMON_PROPERTY_SHOW(prerequisites_size, ordered_list);
> > > > +static struct kobj_attribute  ordered_list_prerequisites_size_val =
> > > > +     __ATTR_RO(prerequisites_size);
> > > > +
> > > > +ATTRIBUTE_V_COMMON_PROPERTY_SHOW(prerequisites, ordered_list);
> > > > +static struct kobj_attribute  ordered_list_prerequisites_val =
> > > > +     __ATTR_RO(prerequisites);
> > > > +
> > > > +ATTRIBUTE_N_PROPERTY_SHOW(elements_size, ordered_list);
> > > > +static struct kobj_attribute  ordered_list_elements_size_val =
> > > > +     __ATTR_RO(elements_size);
> > >
> > > "size" and "length" attributes are fairly useless to userspace.
> > > They can't be trusted to provide information about another attribute as
> > > the information can be out of date when the other attribute is read.
> > >
> >
> > Prerequisites, prerequisites_size and elements_size will be removed
> >
> > > > +
> > > > +ATTRIBUTE_VALUES_PROPERTY_SHOW(elements, ordered_list);
> > > > +static struct kobj_attribute  ordered_list_elements_val =
> > > > +     __ATTR_RO(elements);
> > > > +
> >
> > <snip>
> >
> > > > +
> > > > +
> > > > +int populate_ordered_list_elements_from_package(union acpi_object *order_obj,
> > > > +                                             int order_obj_count,
> > > > +                                             int instance_id)
> > > > +{
> > > > +     char *str_value = NULL;
> > > > +     int value_len;
> > > > +     int ret = 0;
> > > > +     u32 size = 0;
> > > > +     u32 int_value;
> > > > +     int elem = 0;
> > > > +     int reqs;
> > > > +     int eloc;
> > > > +     char *tmpstr = NULL;
> > > > +     char *part_tmp = NULL;
> > > > +     int tmp_len = 0;
> > > > +     char *part = NULL;
> > > > +
> > > > +     if (!order_obj)
> > > > +             return -EINVAL;
> > > > +
> > > > +     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.

>
> 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...

Thomas Weißschuh May 6, 2023, 5:51 a.m. UTC | #5

On 2023-05-05 16:57:59-0500, Jorge Lopez wrote:
> On Fri, May 5, 2023 at 4:11 PM Thomas Weißschuh <thomas@t-8ch.de> wrote:
> >
> > On 2023-05-05 11:09:55-0500, Jorge Lopez wrote:
> > > On Sun, Apr 23, 2023 at 1:55 AM <thomas@t-8ch.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
> > > > > +++ b/drivers/platform/x86/hp/hp-bioscfg/ordered-attributes.c
> > > > > @@ -0,0 +1,563 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Functions corresponding to ordered list type attributes under
> > > > > + * BIOS ORDERED LIST GUID for use with hp-bioscfg driver.
> > > > > + *
> > > > > + *  Copyright (c) 2022 HP Development Company, L.P.
> > > > > + */
> > > > > +
> > > > > +#include "bioscfg.h"
> > > > > +
> > > > > +GET_INSTANCE_ID(ordered_list);
> > > > > +
> > > > > +static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > > > > +{
> > > > > +
> > > > > +     int instance_id = get_ordered_list_instance_id(kobj);
> > > > > +
> > > > > +     if (instance_id < 0)
> > > > > +             return -EIO;
> > > > > +
> > > > > +     return sysfs_emit(buf, "%s\n",
> > > > > +                      bioscfg_drv.ordered_list_data[instance_id].current_value);
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * validate_ordered_list_value -
> > > > > + * Validate input of current_value against possible values
> > > >
> > > > Does the firmware not also validate this?
> > > >
> > > > If so it may be easier to just let it do so and remove the validations
> > > > from the driver.
> > >
> > > Yes.  the firmware validates the data.
> > > Will remove the validation
> > > >
> > > > > + *
> > > > > + * @instance_id: The instance on which input is validated
> > > > > + * @buf: Input value
> > > > > + */
> > > > > +static int validate_ordered_list_values(int instance_id, const char *buf)
> > > > > +{
> > > > > +     int ret = 0;
> > > > > +     int found = 0;
> > > > > +     char *new_values = NULL;
> > > > > +     char *value;
> > > > > +     int elem;
> > > > > +     int elem_found = 0;
> > > > > +
> > > > > +     /* Is it a read only attribute */
> > > > > +     if (bioscfg_drv.ordered_list_data[instance_id].common.is_readonly)
> > > > > +             return -EIO;
> > > > > +
> > > > > +     new_values = kstrdup(buf, GFP_KERNEL);
> > > > > +
> > > > > +     /*
> > > > > +      * Changes to ordered list values require checking that new
> > > > > +      * values are found in the list of elements.
> > > > > +      */
> > > > > +     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?

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.

> > > > > +             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.
> > >
> > > >
> > > > > +                             found = 1;
> > > > > +                             break;
> > > > > +                     }
> > > > > +             }
> > > > > +
> > > > > +
> > > > > +             if (!found) {
> > > > > +                     ret = -EINVAL;
> > > > > +                     goto out_list_value;
> > > > > +             }
> > > > > +     }
> > > > > +
> > > > > +     if (elem_found == bioscfg_drv.ordered_list_data[instance_id].elements_size) {
> > > > > +             pr_warn("Number of new values is not equal to number of ordered list elements (%d)\n",
> > > > > +                     bioscfg_drv.ordered_list_data[instance_id].elements_size);
> > > > > +             ret = -EINVAL;
> > > > > +             goto out_list_value;
> > > > > +     }
> > > > > +
> > > > > +out_list_value:
> > > > > +     kfree(new_values);
> > > > > +     return ret;
> > > > > +}
> > > >
> > > > This algorithm does not seem to validate that different values are
> > > > provided.
> > > >
> > > > So if "possible_values" is "foo,bar,baz" this function would accept
> > > > "foo,foo,foo".
> > > >
> > >
> > > BIOS will reject strings such as "foo,foo,foo" when the current value
> > > is "foo,bar,baz".   It is ok to provide a string which items are
> > > ordered differently.  i.e. "baz,bar,foo"
> > > validate_ordered_list_values() function will be removed as indicated earlier.
> > >
> > > > > +
> > > > > +/*
> > > > > + * validate_ordered_input() -
> > > > > + * Validate input of current_value against possible values
> > > > > + *
> > > > > + * @instance_id: The instance on which input is validated
> > > > > + * @buf: Input value
> > > > > + */
> > > > > +static int validate_ordered_list_input(int instance_id, const char *buf)
> > > > > +{
> > > > > +     int ret = 0;
> > > > > +
> > > > > +     ret = validate_ordered_list_values(instance_id, buf);
> > > > > +     if (ret < 0)
> > > > > +             return -EINVAL;
> > > > > +
> > > > > +     /*
> > > > > +      * set pending reboot flag depending on
> > > > > +      * "RequiresPhysicalPresence" value
> > > > > +      */
> > > > > +     if (bioscfg_drv.ordered_list_data[instance_id].common.requires_physical_presence)
> > > > > +             bioscfg_drv.pending_reboot = true;
> > > > > +
> > > > > +     return ret;
> > > > > +}
> > > > > +
> > > > > +static void update_ordered_list_value(int instance_id, char *attr_value)
> > > > > +{
> > > > > +     strscpy(bioscfg_drv.ordered_list_data[instance_id].current_value,
> > > > > +             attr_value,
> > > > > +             sizeof(bioscfg_drv.ordered_list_data[instance_id].current_value));
> > > > > +}
> > > > > +
> > > > > +ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name_language_code, ordered_list);
> > > > > +static struct kobj_attribute ordered_list_display_langcode =
> > > > > +     __ATTR_RO(display_name_language_code);
> > > > > +
> > > > > +ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name, ordered_list);
> > > > > +static struct kobj_attribute ordered_list_display_name =
> > > > > +     __ATTR_RO(display_name);
> > > > > +
> > > > > +ATTRIBUTE_PROPERTY_STORE(current_value, ordered_list);
> > > > > +static struct kobj_attribute ordered_list_current_val =
> > > > > +     __ATTR_RW_MODE(current_value, 0644);
> > > > > +
> > > > > +
> > > > > +ATTRIBUTE_N_COMMON_PROPERTY_SHOW(prerequisites_size, ordered_list);
> > > > > +static struct kobj_attribute  ordered_list_prerequisites_size_val =
> > > > > +     __ATTR_RO(prerequisites_size);
> > > > > +
> > > > > +ATTRIBUTE_V_COMMON_PROPERTY_SHOW(prerequisites, ordered_list);
> > > > > +static struct kobj_attribute  ordered_list_prerequisites_val =
> > > > > +     __ATTR_RO(prerequisites);
> > > > > +
> > > > > +ATTRIBUTE_N_PROPERTY_SHOW(elements_size, ordered_list);
> > > > > +static struct kobj_attribute  ordered_list_elements_size_val =
> > > > > +     __ATTR_RO(elements_size);
> > > >
> > > > "size" and "length" attributes are fairly useless to userspace.
> > > > They can't be trusted to provide information about another attribute as
> > > > the information can be out of date when the other attribute is read.
> > > >
> > >
> > > Prerequisites, prerequisites_size and elements_size will be removed
> > >
> > > > > +
> > > > > +ATTRIBUTE_VALUES_PROPERTY_SHOW(elements, ordered_list);
> > > > > +static struct kobj_attribute  ordered_list_elements_val =
> > > > > +     __ATTR_RO(elements);
> > > > > +
> > >
> > > <snip>
> > >
> > > > > +
> > > > > +
> > > > > +int populate_ordered_list_elements_from_package(union acpi_object *order_obj,
> > > > > +                                             int order_obj_count,
> > > > > +                                             int instance_id)
> > > > > +{
> > > > > +     char *str_value = NULL;
> > > > > +     int value_len;
> > > > > +     int ret = 0;
> > > > > +     u32 size = 0;
> > > > > +     u32 int_value;
> > > > > +     int elem = 0;
> > > > > +     int reqs;
> > > > > +     int eloc;
> > > > > +     char *tmpstr = NULL;
> > > > > +     char *part_tmp = NULL;
> > > > > +     int tmp_len = 0;
> > > > > +     char *part = NULL;
> > > > > +
> > > > > +     if (!order_obj)
> > > > > +             return -EINVAL;
> > > > > +
> > > > > +     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.

> > 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...

Jorge Lopez May 8, 2023, 1:56 p.m. UTC | #6

On Sat, May 6, 2023 at 12:51 AM Thomas Weißschuh <thomas@t-8ch.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@t-8ch.de> wrote:
> > >
> > > On 2023-05-05 11:09:55-0500, Jorge Lopez wrote:
> > > > On Sun, Apr 23, 2023 at 1:55 AM <thomas@t-8ch.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...

Thomas Weißschuh May 8, 2023, 8:50 p.m. UTC | #7

On 2023-05-08 08:56:55-0500, Jorge Lopez wrote:
> On Sat, May 6, 2023 at 12:51 AM Thomas Weißschuh <thomas@t-8ch.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@t-8ch.de> wrote:
> > > >
> > > > On 2023-05-05 11:09:55-0500, Jorge Lopez wrote:
> > > > > On Sun, Apr 23, 2023 at 1:55 AM <thomas@t-8ch.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 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.

> > > > 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...

Jorge Lopez May 8, 2023, 9:25 p.m. UTC | #8

On Mon, May 8, 2023 at 3:50 PM Thomas Weißschuh <thomas@t-8ch.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@t-8ch.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@t-8ch.de> wrote:
> > > > >
> > > > > On 2023-05-05 11:09:55-0500, Jorge Lopez wrote:
> > > > > > On Sun, Apr 23, 2023 at 1:55 AM <thomas@t-8ch.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...

Jorge Lopez May 9, 2023, 6:38 p.m. UTC | #9

On Mon, May 8, 2023 at 3:50 PM Thomas Weißschuh <thomas@t-8ch.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@t-8ch.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@t-8ch.de> wrote:
> > > > >
> > > > > On 2023-05-05 11:09:55-0500, Jorge Lopez wrote:
> > > > > > On Sun, Apr 23, 2023 at 1:55 AM <thomas@t-8ch.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 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.

Added conversion routine (replace commas to semicolon and vice versa).
Documentation was updated to reflect that semicolons are used to
separate values.
Userspace is not expose to the implementation details but just the
consistent and documented 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.

Done!
>
> > > > > 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...

[v11,05/14] HP BIOSCFG driver - ordered-attributes

Commit Message

Comments

Patch