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