Re: [PATCH v11 03/14] HP BIOSCFG driver - bioscfg
From: Thomas Weißschuh
Date: Tue May 02 2023 - 17:14:17 EST
On 2023-05-02 14:52:14-0500, Jorge Lopez wrote:
> On Sat, Apr 22, 2023 at 5:16 PM <thomas@xxxxxxxx> wrote:
> >
> > On 2023-04-20 11:54:43-0500, Jorge Lopez wrote:
> > > ---
> > > drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 961 +++++++++++++++++++
> > > 1 file changed, 961 insertions(+)
> > > create mode 100644 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> > >
> > > diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> > > new file mode 100644
> > > index 000000000000..4b0d4f56e65f
> > > --- /dev/null
> > > +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
<snip>
> > > + int retval = 0;
> > > + u8 *attr_name;
> >
> > const char *
>
> Cannot define attr_name as 'const char *'. attr_name value is set
> within the function
Then use "char *". This is pointing to a NULL-terminated string,
not a number or binary data.
This is also the type used by the functions this value is passed to.
<snip>
> > > + retval = kobject_init_and_add(attr_name_kobj, &attr_name_ktype,
> > > + NULL, "%s", str_value);
> > > +
> > > + if (retval) {
> > > + kobject_put(attr_name_kobj);
> >
> > The kobj was not created, why does it need the kobj_put() ?
> As indicated by kobject_init_and_add ...
>
> * This function combines the call to kobject_init() and kobject_add().
> *
> * If this function returns an error, kobject_put() must be called to
> * properly clean up the memory associated with the object. This is the
> * same type of error handling after a call to kobject_add() and kobject
> * lifetime rules are the same here.
I stand corrected, thanks!
> >
> > > + goto err_attr_init;
> > > + }
> > > +
> > > + /* enumerate all of these attributes */
> > > + switch (attr_type) {
> > > + case HPWMI_STRING_TYPE:
> > > + retval = populate_string_package_data(elements,
> > > + instance_id,
> > > + attr_name_kobj);
> > > + break;
> > > + case HPWMI_INTEGER_TYPE:
> > > + retval = populate_integer_package_data(elements,
> > > + instance_id,
> > > + attr_name_kobj);
> > > + break;
> > > + case HPWMI_ENUMERATION_TYPE:
> > > + retval = populate_enumeration_package_data(elements,
> > > + instance_id,
> > > + attr_name_kobj);
> > > + break;
> > > + case HPWMI_ORDERED_LIST_TYPE:
> > > + retval = populate_ordered_list_package_data(elements,
> > > + instance_id,
> > > + attr_name_kobj);
> > > + break;
> > > + case HPWMI_PASSWORD_TYPE:
> > > + retval = populate_password_package_data(elements,
> > > + instance_id,
> > > + attr_name_kobj);
> > > + break;
> > > + default:
> > > + break;
> >
> > This default does nothing.
> >
> > > + }
> > > +
> > > + kfree(str_value);
> >
> > Why is str_value only freed down here? It has not been used for half a
> > screen of code.
>
> Added early in the development process and failed to clean up here.
> >
> > > + }
> >
> > else
> >
> > > +
> > > + if (obj->type == ACPI_TYPE_BUFFER) {
> > > +
> > > + buffer_size = obj->buffer.length;
> > > + buffer_ptr = obj->buffer.pointer;
> > > +
> > > + retval = get_string_from_buffer(&buffer_ptr, &buffer_size, str, MAX_BUFF);
> > > + if (retval < 0)
> > > + goto err_attr_init;
> > > +
> > > + if (attr_type == HPWMI_PASSWORD_TYPE || attr_type == HPWMI_SECURE_PLATFORM_TYPE)
> > > + tmp_set = bioscfg_drv.authentication_dir_kset;
> > > + else
> > > + tmp_set = bioscfg_drv.main_dir_kset;
> >
> > There is a bunch of common logic duplicated in both the buffer and
> > package branches.
>
> Older BIOS reports the ACPI data as objects of type ACPI_TYPE_PACKAGE
> and the associated data is reported as elements.
> Newer BIOS reports the ACPI data as objects of type ACPI_TYPE_BUFFER.
> (actypes.h union acpi_object)
> The code follows a common logic although the data is acquired
> differently according to the ACPI object type
> >
> > > +
> > > + if (kset_find_obj(tmp_set, str)) {
> > > + pr_warn("Duplicate attribute name found - %s\n", str);
> >
> > Also mention that it is being ignored.
> >
> > > + goto nextobj;
> > > + }
> > > +
> > > + /* build attribute */
> > > + attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL);
> > > + if (!attr_name_kobj) {
> > > + retval = -ENOMEM;
> > > + goto err_attr_init;
> > > + }
> > > +
> > > + attr_name_kobj->kset = tmp_set;
> > > +
> > > + temp_str = str;
> > > + if (attr_type == HPWMI_SECURE_PLATFORM_TYPE)
> > > + temp_str = "SPM";
> > > +
> > > + retval = kobject_init_and_add(attr_name_kobj,
> > > + &attr_name_ktype, NULL, "%s",
> > > + temp_str);
> > > + if (retval) {
> > > + kobject_put(attr_name_kobj);
> > > + goto err_attr_init;
> > > + }
> > > +
> > > + /* enumerate all of these attributes */
> > > + switch (attr_type) {
> > > + case HPWMI_STRING_TYPE:
> > > + retval = populate_string_buffer_data(buffer_ptr,
> > > + &buffer_size,
> > > + instance_id,
> > > + attr_name_kobj);
> > > + break;
> > > + case HPWMI_INTEGER_TYPE:
> > > + retval = populate_integer_buffer_data(buffer_ptr,
> > > + &buffer_size,
> > > + instance_id,
> > > + attr_name_kobj);
> > > + break;
> > > + case HPWMI_ENUMERATION_TYPE:
> > > + retval = populate_enumeration_buffer_data(buffer_ptr,
> > > + &buffer_size,
> > > + instance_id,
> > > + attr_name_kobj);
> > > + break;
> > > + case HPWMI_ORDERED_LIST_TYPE:
> > > + retval = populate_ordered_list_buffer_data(buffer_ptr,
> > > + &buffer_size,
> > > + instance_id,
> > > + attr_name_kobj);
> > > + break;
> > > + case HPWMI_PASSWORD_TYPE:
> > > + retval = populate_password_buffer_data(buffer_ptr,
> > > + &buffer_size,
> > > + instance_id,
> > > + attr_name_kobj);
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > > + }
> >
> > What if it's neither a package nor a buffer?
>
> we return an error if it is neither a package nor a buffer.
>
> if (obj->type != ACPI_TYPE_PACKAGE && obj->type != ACPI_TYPE_BUFFER) {
> pr_err("Error: Expected ACPI-package or buffer type, got:
> %d\n", obj->type);
> retval = -EIO;
> goto err_attr_init;
> }
Indeed, thanks.
This could be pulled together into:
if (package)
...
else if (buffer)
...
else
report error
Note:
The "Error: " prefix is unnecessary. It's already logged as pr_err().
<snip>