Message-ID: <5c639541-0fba-276f-7b57-3f5a379c5bac@linux.intel.com>
Date: Tue, 16 Jul 2024 14:06:02 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Luke Jones <luke@...nes.dev>
cc: platform-driver-x86@...r.kernel.org, corentin.chary@...il.com, 
    Hans de Goede <hdegoede@...hat.com>, 
    Mario Limonciello <mario.limonciello@....com>, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/5] platform/x86 asus-bioscfg: move existing tunings to
 asus-bioscfg module

On Tue, 16 Jul 2024, Luke Jones wrote:

> On Tue, 16 Jul 2024, at 9:45 PM, Ilpo Järvinen wrote:
> > On Tue, 16 Jul 2024, Luke D. Jones wrote:
> > 
> > > The fw_attributes_class provides a much cleaner interface to all of the
> > > attributes introduced to asus-wmi. This patch moves all of these extra
> > > attributes over to fw_attributes_class, and shifts the bulk of these
> > > definitions to a new kernel module to reduce the clutter of asus-wmi
> > > with the intention of deprecating the asus-wmi attributes in future.
> > > 
> > > The work applies only to WMI methods which don't have a clearly defined
> > > place within the sysfs and as a result ended up lumped together in
> > > /sys/devices/platform/asus-nb-wmi/ with no standard API.
> > > 
> > > Where possible the fw attrs now implement defaults, min, max, scalar,
> > > choices, etc. As en example dgpu_disable becomes:
> > > 
> > > /sys/class/firmware-attributes/asus-bioscfg/attributes/dgpu_disable/
> > > ├── current_value
> > > ├── display_name
> > > ├── possible_values
> > > └── type
> > > 
> > > as do other attributes.
> > > 
> > > Signed-off-by: Luke D. Jones <luke@...nes.dev>
> > > ---
> > >  drivers/platform/x86/Kconfig               |  14 +
> > >  drivers/platform/x86/Makefile              |   1 +
> > >  drivers/platform/x86/asus-bioscfg.c        | 666 +++++++++++++++++++++
> > >  drivers/platform/x86/asus-bioscfg.h        | 243 ++++++++
> > >  drivers/platform/x86/asus-wmi.c            |  18 +-
> > >  include/linux/platform_data/x86/asus-wmi.h |  11 +
> > >  6 files changed, 952 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/platform/x86/asus-bioscfg.c
> > >  create mode 100644 drivers/platform/x86/asus-bioscfg.h
> > > 
> > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > index 665fa9524986..b4a5a5bec7f3 100644
> > > --- a/drivers/platform/x86/Kconfig
> > > +++ b/drivers/platform/x86/Kconfig
> > > @@ -265,6 +265,18 @@ config ASUS_WIRELESS
> > >    If you choose to compile this driver as a module the module will be
> > >    called asus-wireless.
> > >  
> > > +config ASUS_BIOS
> > > + tristate "ASUS BIOS Driver"
> > > + depends on ACPI_WMI
> > > + depends on ASUS_WMI
> > > + select FW_ATTR_CLASS
> > > + help
> > > +   Say Y here if you have a WMI aware Asus laptop and would like to use the
> > > +   firmware_attributes API.
> > > +
> > > +   To compile this driver as a module, choose M here: the module will
> > > +   be called asus-bios.
> > > +
> > >  config ASUS_WMI
> > >  tristate "ASUS WMI Driver"
> > >  depends on ACPI_WMI
> > > @@ -276,6 +288,8 @@ config ASUS_WMI
> > >  depends on HOTPLUG_PCI
> > >  depends on ACPI_VIDEO || ACPI_VIDEO = n
> > >  depends on SERIO_I8042 || SERIO_I8042 = n
> > > + select ASUS_BIOS
> > 
> > Selecting user visible configs is not a good idea. Also, there 
> > seems to be circular dependency now between ASUS_BIOS & ASUS_WMI ?
> 
> Is "selects" the same as "depends"?

It's not the same. Selects ask to enabled another symbol (with caveats) 
and depends only shows this symbol if the other symbol is already enabled.

Select comes with many many caveats and should only be used for the 
config symbols which are truly library type (and not presented to user in 
the first place).

> I did just remove:
> 	select ASUS_WMI_BIOS
> which should not be there at all.
> 
> ASUS_BIOS does need ASUS_WMI. And I'd like ASUS_BIOS to be selected by 
> defualt, is this not the right way to do that? 

Default should not be handled with either depends on / select I think, 
but I'm not Kconfig expert.

There's also default clause but it should be used sparingly as each and 
every developer naturally thinks their thing is so important it must be on 
by default so we know where that thinking ends to. :-)

Distros tend enable about everything anyway so it might not be so 
important in the end what the default is.

> > > + select ASUS_WMI_BIOS
> > >  select INPUT_SPARSEKMAP
> > >  select LEDS_CLASS
> > >  select NEW_LEDS

> > > + struct mutex mutex;
> > > +} asus_bioscfg = {
> > > + .mutex = __MUTEX_INITIALIZER(asus_bioscfg.mutex),
> > 
> > Don't try to initialize it on the same go like this.
> > 
> > You might want static too.
> 
> Ack both
> 
> > 
> > > +};
> > > +
> > > +static struct fw_attrs_group {
> > > + u32 pending_reboot;
> > > +} fw_attrs = {
> > > + .pending_reboot = 0,
> > > +};
> > 
> > Same here.
> 
> It was probably done like this in code I read as a reference. I'll shift 
> to the module init function. 

???

I just meant this split:

struct fw_attrs_group {
	u32 pending_reboot;
};

static struct fw_attrs_group fw_attrs = {
	.pending_reboot = 0,
};

> > > +static bool asus_bios_requires_reboot(struct kobj_attribute *attr)
> > > +{
> > > + return !strcmp(attr->attr.name, "gpu_mux_mode");
> > > + !strcmp(attr->attr.name, "panel_hd_mode");
> > 
> > ???
> > 
> > Semicolon and && confusion here?
> 
> Yeah i know, bad rebase I didn't catch.

For the record, || is correct here, not && but you probably know this 
already.

> > > + if (result)
> > > + return result;
> > > +
> > > + if (value < min || value > max)
> > > + return -EINVAL;
> > > +
> > > + asus_wmi_set_devstate(wmi_dev, value, &result);
> > 
> > Type confusion, u32 * vs int pointer being passed.
> 
> I miss rust...



> > > + if (result) {
> > > + pr_err("Failed to set %s: %d\n", attr->attr.name, result);
> > > + return result;
> > > + }
> > > +
> > > + if (result > 1) {
> > 
> > What's this supposed to mean given you've the type confusion to begin 
> > with and return on the earlier line if result is non-zero?
> > 
> > Did you mean to capture the return value of asus_wmi_set_devstate() and 
> > test that in the first if ()?
> 
> Yep.. this whole bit is a mess. I've fixed the type mess, and added a 
> comment to clarify the "if (result > 1) {" 
> (WMI methods return 0 = fail, 1 = success, anything else is error)

Don't add these comments into everywhere in the code but document 
asus_wmi_set_devstate() instead with kerneldoc.

> > If you make a previously internal function such as asus_wmi_set_devstate() 
> > EXPORTed, you should document it with kerneldoc so the interface is clear.
> 
> I'm not sure how to do this, I'll read up. Also didn't know about it so 
> thanks for the pointer. 

It's this format:

/**
 * funcname - func short desc
 * @param1: foo
 * ...
 *
 * Func long description (IMHO, often optional if not some major API).
 *
 * Returns: important info about return value
 */

You'll find plenty of examples with varying quality with grep but the most 
imporant bits are the return value and parameters, and if there are 
caveats the caller should know, the long desciption might be handy.


> > > + err = -ENODEV;
> > > + pr_warn("Can not switch MUX to dGPU mode when dGPU is disabled: %d\n", err);
> > > + return err;
> > > + }
> > > + }
> > > +
> > > + if (asus_bioscfg.egpu_enable_available) {
> > > + err = asus_wmi_get_devstate_dsts(ASUS_WMI_DEVID_EGPU, &result);
> > > + if (err)
> > > + return err;
> > > + if (result && !optimus) {
> > > + err = -ENODEV;
> > > + pr_warn("Can not switch MUX to dGPU mode when eGPU is enabled: %d\n", err);
> > > + return err;
> > > + }
> > > + }
> > > +
> > > + err = asus_wmi_set_devstate(asus_bioscfg.gpu_mux_dev_id, optimus, &result);
> > > + if (err) {
> > > + pr_err("%s Failed to set GPU MUX mode: %d\nn", __func__, err);
> > 
> > Never use __func__ for messages shown to normal user.
> 
> Must have been a holdover from debug. Also wasn't aware of that rule, thanks.

For pr_debug() I can give some leeway but for anything info/warn/error 
should definitely be user readable and preferrably understandable too :-D.

> > > + return err;
> > > + }
> > > + /* !1 is considered a fail by ASUS */
> > 
> > If the interface is documented with kerneldoc, this is unnecessary 
> > comment. Is 0 also a failure (this differs from >1 checks elsewhere)?
> 
> I've changed the other checks to match. But I'll also try and do a 
> deeper analysis of those particular WMI functions to see if I can find 
> the actual causes for other returns and their significance (0 and 2). 1 
> is most definitely success though. 

Understood, I don't expect you to know everything about these interfaces 
but the difference just caught my eye so I mentioned it in case it's a 
mistake.

> > As general feel of the readiness of this code, I suspect there were many 
> > more problems which I failed to notice :-(.
> 
> I'd put money on it (sorry). I definitely should have cleaned up better 
> than I did so you weren't pointing out silly little things, but I was 
> never expecting to get over the line on the first try and desperately 
> needed some insight for the overall patch series to see if what I was 
> doing was actually going to be acceptable or not. 

It's not problem in itself if you were just after an early review of the 
concept but there are little things like not maintaining consistency in 
variable naming which easily trips one here and there which I really 
recommend you change because then many patterns can be checked with grep / 
find if things are consistent (and it helps the code reader too if the 
variable naming doesn't suddenly became opposite of what it was in the 
previous function).

> As always, thanks so much for your time and review.

-- 
 i.

Powered by blists - more mailing lists