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: <foiw2np7nj2qefgdw7t43zuz5kujdgnsllutjqjdoq7i5tkrdy@zfu2pk43crzx>
Date: Thu, 5 Dec 2024 10:10:32 -0300
From: Kurt Borja <kuurtb@...il.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.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 Thu, Dec 05, 2024 at 01:32:46PM +0200, Ilpo Järvinen wrote:
> 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;

This was a workaround. I forgot to add this change in this patch. Also I'll
move this earlier in the series.

> 
> Somehow, it felt odd and then looking this patch 16, it felt even odder. 

The reason behind this is that I don't want AlienFX related methods
depending on global variables like `interface` so I can split the file
cleanly.

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

Thank you so much for commenting on the series!

Also, what do you think about the general approach? I was a bit unsure
about the whole "platdata" stuff, as not many drivers use it in this
subsystem.

>