linux-kernel - Re: [PATCH v10 04/14] platform/x86: alienware-wmi: Refactor LED control methods

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <D7PSZTY8T63W.16GP7U21D7GEZ@gmail.com>
Date: Tue, 11 Feb 2025 12:47:36 -0500
From: "Kurt Borja" <kuurtb@...il.com>
To: "Andy Shevchenko" <andriy.shevchenko@...el.com>
Cc: <platform-driver-x86@...r.kernel.org>, Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>, "Armin Wolf" <W_Armin@....de>, "Mario
 Limonciello" <mario.limonciello@....com>, "Hans de Goede"
 <hdegoede@...hat.com>, <Dell.Client.Kernel@...l.com>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v10 04/14] platform/x86: alienware-wmi: Refactor LED
 control methods

On Tue Feb 11, 2025 at 11:33 AM -05, Andy Shevchenko wrote:
> On Fri, Feb 07, 2025 at 10:46:00AM -0500, Kurt Borja wrote:
>> Both WMI devices handled by this module specify a distinct interface for
>> LED control. Previously this module handled this by dynamically adapting
>> arguments passed to wmi_evaluate_method() based on the `interface`
>> global variable.
>> 
>> To avoid the use of global variables, and enable the migration to
>> non-deprecated WMI methods, let the WMI drivers define upd_led and
>> upd_brightness operations, which completely replace
>> alienware_update_led() and wmax_brightness().
>> 
>> Also define alienware_wmi_command(), which serves as a wrapper for
>> wmidev_evaluate_method(). This new method is very similar to
>> alienware_wmax_command() but is WMI device agnostic.
>
> ...
>
>> +static int alienware_wmi_command(struct wmi_device *wdev, u32 method_id,
>> +				 void *in_args, size_t in_size, u32 *out_data)
>> +{
>> +	struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL};
>> +	struct acpi_buffer in = {in_size, in_args};
>> +	acpi_status ret;
>> +
>> +	ret = wmidev_evaluate_method(wdev, 0, method_id, &in, out_data ? &out : NULL);
>> +	if (ACPI_FAILURE(ret))
>> +		return -EIO;
>> +
>> +	union acpi_object *obj __free(kfree) = out.pointer;
>
> Actually we have ACPI_FREE(), but it's not big deal (as of today) to use kfree().
>
>> +	if (out_data) {
>> +		if (obj && obj->type == ACPI_TYPE_INTEGER)
>> +			*out_data = (u32)obj->integer.value;
>
> Unneeded casting.

Ack.

>
>> +		else
>> +			return -ENOMSG;
>> +	}
>> +
>> +	return 0;
>> +}
>
> ...
>
>> +	if (legacy_args.state != LEGACY_RUNNING) {
>
> With inverted conditional and duplicated line it will all look better.

Ack.

>
>> +		legacy_args.state = priv->lighting_control_state;
>> +
>> +		input.length = sizeof(legacy_args);
>> +		input.pointer = &legacy_args;
>> +
>> +		status = wmi_evaluate_method(LEGACY_POWER_CONTROL_GUID, 0,
>> +					     location + 1, &input, NULL);
>> +		if (ACPI_FAILURE(status))
>> +			return -EIO;
>> +
>> +		return 0;
>> +	}
>> +
>> +	return alienware_wmi_command(wdev, location + 1, &legacy_args,
>> +				     sizeof(legacy_args), NULL);
>
> ...
>
>> +static int wmax_wmi_update_brightness(struct alienfx_priv *priv,
>> +				      struct wmi_device *wdev, u8 brightness)
>> +{
>> +	struct wmax_brightness_args in_args = {
>> +		.led_mask = 0xFF,
>
> GENMASK()?

Ack.

>
>> +		.percentage = brightness,
>> +	};
>> +
>> +	return alienware_wmi_command(wdev, WMAX_METHOD_BRIGHTNESS, &in_args,
>> +				     sizeof(in_args), NULL);
>> +}