linux-kernel - Re: [RFC PATCH 16/21] alienware-wmi: Make running control state part of platdata

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: <d0828df1-4a46-2cd3-f15a-b08e5d011bba@linux.intel.com>
Date: Thu, 5 Dec 2024 13:32:46 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Kurt Borja <kuurtb@...il.com>
cc: Dell.Client.Kernel@...l.com, Hans de Goede <hdegoede@...hat.com>, 
    LKML <linux-kernel@...r.kernel.org>, mario.limonciello@....com, 
    platform-driver-x86@...r.kernel.org, w_armin@....de
Subject: Re: [RFC PATCH 16/21] alienware-wmi: Make running control state part
 of platdata

On Wed, 4 Dec 2024, Kurt Borja wrote:

> Both WMI devices have a different "RUNNING" control state code. Make the
> WMI drivers decide which code to use, and refactor sysfs methods
> accordingly.
> 
> Signed-off-by: Kurt Borja <kuurtb@...il.com>
> ---
>  drivers/platform/x86/dell/alienware-wmi.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> index 25e0139ed78c..fa21a50d66bd 100644
> --- a/drivers/platform/x86/dell/alienware-wmi.c
> +++ b/drivers/platform/x86/dell/alienware-wmi.c
> @@ -431,6 +431,7 @@ struct alienfx_platdata {
>  	bool hdmi_mux;
>  	bool amplifier;
>  	bool deepslp;
> +	u8 running_code;
>  };
>  
>  static u8 interface;
> @@ -576,18 +577,18 @@ static ssize_t lighting_control_state_store(struct device *dev,
>  					    const char *buf, size_t count)
>  {
>  	struct alienfx_priv *priv;
> +	struct alienfx_platdata *pdata;
>  	u8 val;
>  
>  	priv = dev_get_drvdata(dev);
> +	pdata = dev_get_platdata(dev);
>  
>  	if (strcmp(buf, "booting\n") == 0)
>  		val = LEGACY_BOOTING;
>  	else if (strcmp(buf, "suspend\n") == 0)
>  		val = LEGACY_SUSPEND;
> -	else if (interface == LEGACY)
> -		val = LEGACY_RUNNING;
>  	else
> -		val = WMAX_RUNNING;
> +		val = pdata->running_code;
>  
>  	priv->lighting_control_state = val;
>  	pr_debug("alienware-wmi: updated control state to %d\n",
> @@ -1249,6 +1250,7 @@ static int legacy_wmi_probe(struct wmi_device *wdev, const void *context)
>  		.hdmi_mux = quirks->hdmi_mux,
>  		.amplifier = quirks->amplifier,
>  		.deepslp = quirks->deepslp,
> +		.running_code = LEGACY_RUNNING,
>  	};
>  
>  	if (quirks->num_zones > 0)
> @@ -1333,6 +1335,7 @@ static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
>  		.hdmi_mux = quirks->hdmi_mux,
>  		.amplifier = quirks->amplifier,
>  		.deepslp = quirks->deepslp,
> +		.running_code = WMAX_RUNNING,
>  	};
>  
>  	if (quirks->thermal)
> 

I've not taken extensive look at interdependent changes in the series but 
while reviewing patch 20/21 I noticed that alienfx_probe() changed there
from:

-       if (interface == WMAX)
-               priv->lighting_control_state = WMAX_RUNNING;
-       else if (interface == LEGACY)
-               priv->lighting_control_state = LEGACY_RUNNING;

to:

+       priv->lighting_control_state = pdata->running_code;

Somehow, it felt odd and then looking this patch 16, it felt even odder. 
Perhaps there's a good reason my review that didn't go deep enough failed 
to uncover but please check if this is an indication that something is a 
miss in the series.

-- 
 i.