Re: [PATCH v2] Introduce support for Systems Management Driver over WMI for Dell Systems

[Hans de Goede]

Hi,

On 9/14/20 7:12 PM, Limonciello, Mario wrote:

<snip>

> > > +static int call_biosattributes_interface(struct wmi_device *wdev, char
> > *in_args, size_t size,
> > > +					int method_id)
> > > +{
> > > +	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> > > +	struct acpi_buffer input;
> > > +	union acpi_object *obj;
> > > +	acpi_status status;
> > > +	int ret = -EIO;
> > > +
> > > +	input.length =  (acpi_size) size;
> > > +	input.pointer = in_args;
> > > +	status = wmidev_evaluate_method(wdev, 0, method_id, &input, &output);
> > > +	if (ACPI_FAILURE(status))
> > > +		return -EIO;
> > > +	obj = (union acpi_object *)output.pointer;
> > > +	if (obj->type == ACPI_TYPE_INTEGER)
> > > +		ret = obj->integer.value;
> > > +
> > > +	kfree(output.pointer);
> > > +	kobject_uevent(&wdev->dev.kobj, KOBJ_CHANGE);
> >
> > Why are you generating an uevent here? I think this needs a comment (or it
> > should be removed).
>
> It's there so that userspace can know that a setting has been changed and doesn't
> need to poll the attribute indicating a reboot is pending.
>
> We can certainly add a comment to this effect.

uevents are somewhat expensive they get processed by udevd, causing it
to evaluate all loaded udev rules and send out events to any userspace
processes listening for udev events.

The generic sysfs ABI allows polling using the poll() system-call
(so event driven) on sysfs attributes. IMHO it would be much better to
use this mechanism. Userspace can then poll() (watching for POLL_PRI)
on the pending_reboot sysfs attribute.

This could then be combined with a little helper for setting the
pending_changes bool, which checks if it is actually transitioning
from false to true and in that case does something like this:

kn = sysfs_get_dirent(..., "pending_reboot");
if (kn) {
	sysfs_notify_dirent(kn);
	sysfs_put(kn);
}

Note something like this really ought to be part of the sysfs ABI
documentation.


> > > +	return map_wmi_error(ret);
> > > +}
> > > +
> > > +/**
> > > + * set_attribute() - Update an attribute value
> > > + * @a_name: The attribute name
> > > + * @a_value: The attribute value
> > > + *
> > > + * Sets an attribute to new value
> > > + **/
> > > +int set_attribute(const char *a_name, const char *a_value)
> > > +{
> > > +	size_t security_area_size, string_area_size, buffer_size;
> > > +	char *attribute_name, *attribute_value;
> > > +	u8 *name_len, *value_len;
> > > +	char *buffer;
> > > +	int ret;
> > > +
> > > +	/* build/calculate buffer */
> > > +	security_area_size = calculate_security_buffer();
> > > +	string_area_size = (strlen(a_name) + strlen(a_value))*2;
> > > +	buffer_size = security_area_size + string_area_size + sizeof(u16) * 2;
> > > +	buffer = kzalloc(buffer_size, GFP_KERNEL);
> > > +	if (!buffer)
> > > +		return -ENOMEM;
> > > +
> > > +	/* build security area */
> > > +	if (strlen(wmi_priv.current_admin_password) > 0)
> > > +		populate_security_buffer(buffer, wmi_priv.current_admin_password);
> > > +
> > > +	/* build variables to set */
> > > +	name_len = buffer + security_area_size;
> > > +	attribute_name = name_len + sizeof(u16);
> > > +	*name_len = utf8s_to_utf16s(a_name, strlen(a_name), UTF16_HOST_ENDIAN,
> > > +				    (wchar_t *) attribute_name, MAX_BUFF) * 2;
> > > +	if (*name_len < 0) {
> > > +		ret = -EINVAL;
> > > +		pr_err("UTF16 conversion failed");
> > > +		goto out_set_attribute;
> > > +	}
> > > +
> > > +	value_len = (u8 *) attribute_name + *name_len;
> > > +	attribute_value = value_len + sizeof(u16);
> > > +	*value_len = utf8s_to_utf16s(a_value, strlen(a_value),
> > UTF16_HOST_ENDIAN,
> > > +				    (wchar_t *) attribute_value, MAX_BUFF) * 2;
> > > +	if (*value_len < 0) {
> > > +		ret = -EINVAL;
> > > +		pr_err("UTF16 conversion failed");
> > > +		goto out_set_attribute;
> > > +	}
> > > +
> > > +	mutex_lock(&call_mutex);
> > > +	if (!wmi_priv.bios_attr_wdev) {
> > > +		ret = -ENODEV;
> > > +		pr_err("no WMI backend bound");
> > > +		goto out_set_attribute;
> > > +	}
> >
> > It is probably better to move this to the top, as I mentioned in my reply
> > to Mario, you could add something like this to the top of each function,
> > even before allocating the buffer and filling it.
>
> The placement was intentional to avoid taking the mutex until data was validated.
> Is it preferable to make it specifically on entry into functions then?

See below.

> >       mutex_lock(&wmi_priv.mutex);
> >
> >       if (!wmi_priv.bios_attr_wdev || !wmi_priv.password_attr_wdev) {
> >           ret = -ENODEV;
> >           goto out_set_attribute;
> >       }
> >
> > Note this requires that you initialize the buf pointer to NULL
> > when declaring it.
> >
> > Having this at the top of each function establishes a pattern where
> > it easy for a reader of the code to:
> >
> > a) See that the check is present when checking that the driver being
> > unbound is checked everywhere
> >
> > b) Ignore the check as "boilerplate" when looking at other parts of
> > the code
>
> I guess this argument makes sense - setting up a pattern is a good way
> to spot the function that stands out when a coding mistake occurs.

Right.

Regards,

Hans