Re: [PATCH v6 3/9] platform/x86: asus-armoury: move existing tunings to asus-armoury module

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5506 bytes --]

On Wed, 30 Oct 2024, Luke Jones wrote:
> On Wed, 16 Oct 2024, at 4:54 PM, Ilpo Järvinen wrote:
> > On Mon, 30 Sep 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-armoury/attributes/dgpu_disable/
> >> ├── current_value
> >> ├── display_name
> >> ├── possible_values
> >> └── type
> >> 
> >> as do other attributes.
> >> 
> >> The ppt_* based attributes are removed in this initial patch as the
> >> implementation is somewhat broken due to the WMI methods requiring a
> >> set of limits on the values accepted (which is not provided by WMI).
> >> 
> >> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> >> ---
> >>  drivers/platform/x86/Kconfig               |  12 +
> >>  drivers/platform/x86/Makefile              |   1 +
> >>  drivers/platform/x86/asus-armoury.c        | 593 +++++++++++++++++++++
> >>  drivers/platform/x86/asus-armoury.h        | 146 +++++
> >>  drivers/platform/x86/asus-wmi.c            |   4 -
> >>  include/linux/platform_data/x86/asus-wmi.h |   3 +
> >>  6 files changed, 755 insertions(+), 4 deletions(-)
> >>  create mode 100644 drivers/platform/x86/asus-armoury.c
> >>  create mode 100644 drivers/platform/x86/asus-armoury.h
> >> 
> >> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> >> index 3875abba5a79..80ec8b45022d 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_ARMOURY
> >> +	tristate "ASUS Armoury driver"
> >> +	depends on ASUS_WMI
> >> +	select FW_ATTR_CLASS
> >> +	help
> >> +	  Say Y here if you have a WMI aware Asus machine and would like to use the
> >> +	  firmware_attributes API to control various settings typically exposed in
> >> +	  the ASUS Armoury Crate application available on Windows.
> >> +
> >> +	  To compile this driver as a module, choose M here: the module will
> >> +	  be called asus-armoury.
> >> +
> >>  config ASUS_WMI
> >>  	tristate "ASUS WMI Driver"
> >>  	depends on ACPI_WMI

> >> +static const struct class *fw_attr_class;
> >> +
> >> +struct asus_armoury_priv {
> >> +	struct device *fw_attr_dev;
> >> +	struct kset *fw_attr_kset;
> >> +
> >> +	u32 mini_led_dev_id;
> >> +	u32 gpu_mux_dev_id;
> >> +
> >> +	struct mutex mutex;
> >
> > Please document what this protects.
> 
> I don't fully understand if sysfs writes can be parallel or not, so i 
> was making the assumption that they were and if so, we would want to 
> prevent trying to write many WMI at once. If my understanding is lacking 
> please correct me.

Yes, sysfs write handlers can run parallel so locking necessary.

I believe your mutex is okay solution for concurrency control but you need 
to add a comment on what it protects. It could be also named like 
wmi_mutex if it's for that only.

> >> +static int asus_fw_attr_add(void)
> >> +{
> >> +	int err, i;
> >> +
> >> +	err = fw_attributes_class_get(&fw_attr_class);
> >> +	if (err)
> >> +		return err;
> >> +
> >> +	asus_armoury.fw_attr_dev =
> >> +		device_create(fw_attr_class, NULL, MKDEV(0, 0), NULL, "%s", DRIVER_NAME);
> >
> > Don't split at = but after MKDEV() arg.
> 
> This is another clang-format thing... I'll bite the bullet and manually 
> format for rest of style suggestions in review.

If you want to attempt to keep using it, I suggest you try to look if you 
can make small adjustment to its config file to solve its worst misses.

> >> +static void __exit asus_fw_exit(void)
> >> +{
> >> +	mutex_lock(&asus_armoury.mutex);
> >
> > I'm not sure if this really helps anything. What are you trying to protect 
> > against here with it?
> 
> I'm not even sure anymore. Was supposed to be due to my assumptions 
> about how sysfs writes work.

It doesn't help any. Either you'd deadlock when calling 
sysfs_remove_file() (assuming its waiting for the handlers to finish) or 
the write handler is still executed after unlock which equals not taking 
mutex here at all. I don't see what it could protect with any success.

> One thing in particular is that the dgpu_disable and egpu_enable calls 
> can take 20+ seconds to complete in acpi, and I don't want to make other 
> WMI calls during that time - TBH I'm not sure of best way to handle it.
>
> >> +	sysfs_remove_file(&asus_armoury.fw_attr_kset->kobj, &pending_reboot.attr);
> >> +	kset_unregister(asus_armoury.fw_attr_kset);
> >> +	device_destroy(fw_attr_class, MKDEV(0, 0));
> >> +	fw_attributes_class_put();
> >> +
> >> +	mutex_unlock(&asus_armoury.mutex);
> >> +}


-- 
 i.