Message-Id: <f401fd68-c5e6-4c97-ab82-400141c42cd7@app.fastmail.com>
Date: Tue, 16 Jul 2024 22:48:18 +1200
From: "Luke Jones" <luke@...nes.dev>
To: "Hans de Goede" <hdegoede@...hat.com>, platform-driver-x86@...r.kernel.org
Cc: corentin.chary@...il.com,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 "Mario Limonciello" <mario.limonciello@....com>, 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, at 9:50 PM, Hans de Goede wrote:
> Hi Luke,
> 
> On 7/16/24 7:16 AM, 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>
> 
> Interesting (also see my reply to the cover-letter).
> 
> Note this is not a full review, just a few small remarks
> with things which I noticed while taking a quick look.
> 
> <snip>
> 
> > +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");
> > +}
> 
> The second statement here is never reached, I believe you want
> a || between the 2 strcmp() calls:
> 
> 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");
> }
> 
> <snip>
> 
> > +/* Simple attribute creation */
> 
> I think it would be good to do the following for registering
> these "simple" attributes ... continued below.
> 
> > +ATTR_GROUP_ENUM_INT_RW(thermal_policy, "thermal_policy", ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY,
> > + 0, 3, "0;1;2", "Set the thermal profile: 0=normal, 1=performance, 2=quiet");
> > +ATTR_GROUP_PPT_RW(ppt_pl2_sppt, "ppt_pl2_sppt", ASUS_WMI_DEVID_PPT_PL2_SPPT,
> > + cpu_default, 5, cpu_max, 1, "Set the CPU fast package limit");
> > +ATTR_GROUP_PPT_RW(ppt_apu_sppt, "ppt_apu_sppt", ASUS_WMI_DEVID_PPT_APU_SPPT,
> > + platform_default, 5, platform_max, 1, "Set the CPU slow package limit");
> > +ATTR_GROUP_PPT_RW(ppt_platform_sppt, "ppt_platform_sppt", ASUS_WMI_DEVID_PPT_PLAT_SPPT,
> > + platform_default, 5, platform_max, 1, "Set the CPU slow package limit");
> > +ATTR_GROUP_PPT_RW(ppt_fppt, "ppt_fppt", ASUS_WMI_DEVID_PPT_FPPT,
> > + cpu_default, 5, cpu_max, 1, "Set the CPU slow package limit");
> > +
> > +ATTR_GROUP_PPT_RW(nv_dynamic_boost, "nv_dynamic_boost", ASUS_WMI_DEVID_NV_DYN_BOOST,
> > + nv_boost_default, 5, nv_boost_max, 1, "Set the Nvidia dynamic boost limit");
> > +ATTR_GROUP_PPT_RW(nv_temp_target, "nv_temp_target", ASUS_WMI_DEVID_NV_THERM_TARGET,
> > + nv_temp_default, 75, nv_temp_max, 1, "Set the Nvidia max thermal limit");
> > +
> > +ATTR_GROUP_ENUM_INT_RO(charge_mode, "charge_mode", ASUS_WMI_DEVID_CHARGE_MODE,
> > + "0;1;2", "Show the current mode of charging");
> > +ATTR_GROUP_BOOL_RW(boot_sound, "boot_sound", ASUS_WMI_DEVID_BOOT_SOUND,
> > + "Set the boot POST sound");
> > +ATTR_GROUP_BOOL_RW(mcu_powersave, "mcu_powersave", ASUS_WMI_DEVID_MCU_POWERSAVE,
> > + "Set MCU powersaving mode");
> > +ATTR_GROUP_BOOL_RW(panel_od, "panel_overdrive", ASUS_WMI_DEVID_PANEL_OD,
> > + "Set the panel refresh overdrive");
> > +ATTR_GROUP_BOOL_RW(panel_hd_mode, "panel_hd_mode", ASUS_WMI_DEVID_PANEL_HD,
> > + "Set the panel HD mode to UHD<0> or FHD<1>");
> > +ATTR_GROUP_BOOL_RO(egpu_connected, "egpu_connected", ASUS_WMI_DEVID_EGPU_CONNECTED,
> > + "Show the eGPU connection status");
> 
> Create an array of simple attribute groups for this
> entire set of simple attributes:
> 
> struct asus_attr_group {
> const struct attribute_group *attr_group;
> u32 wmi_devid;
> };
> 
> static const struct asus_attr_group simple_attribute_groups[] = {
> { &thermal_policy_attr_group, ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY },
> { &ppt_pl2_sppt_attr_group, ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY },
> ...
> { &egpu_connected_attr_group, ASUS_WMI_DEVID_EGPU_CONNECTED },
> };
> 
> And then in asus_fw_attr_add() .. continued below:
> 
> > +static int asus_fw_attr_add(void)
> > +{
> > + int ret;
> > +
> > + ret = fw_attributes_class_get(&fw_attr_class);
> > + if (ret)
> > + goto fail_class_created;
> > + else
> > + asus_bioscfg.fw_attr_dev = device_create(fw_attr_class, NULL,
> > + MKDEV(0, 0), NULL, "%s", DRIVER_NAME);
> > +
> > + if (IS_ERR(asus_bioscfg.fw_attr_dev)) {
> > + ret = PTR_ERR(asus_bioscfg.fw_attr_dev);
> > + goto fail_class_created;
> > + }
> > +
> > + asus_bioscfg.fw_attr_kset = kset_create_and_add("attributes", NULL,
> > + &asus_bioscfg.fw_attr_dev->kobj);
> > + if (!asus_bioscfg.fw_attr_dev) {
> > + ret = -ENOMEM;
> > + pr_debug("Failed to create and add attributes\n");
> > + goto err_destroy_classdev;
> > + }
> > +
> > + /* Add any firmware_attributes required */
> > + ret = sysfs_create_file(&asus_bioscfg.fw_attr_kset->kobj, &pending_reboot.attr);
> > + if (ret) {
> > + pr_warn("Failed to create sysfs level attributes\n");
> > + goto fail_class_created;
> > + }
> > +
> > + // TODO: logging
> > + asus_bioscfg.mini_led_dev_id = 0;
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_MINI_LED_MODE)) {
> > + asus_bioscfg.mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE;
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &mini_led_mode_attr_group);
> > + } else if (asus_wmi_is_present(ASUS_WMI_DEVID_MINI_LED_MODE2)) {
> > + asus_bioscfg.mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE2;
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &mini_led_mode_attr_group);
> > + }
> > +
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_GPU_MUX)) {
> > + asus_bioscfg.gpu_mux_dev_id = ASUS_WMI_DEVID_GPU_MUX;
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &gpu_mux_mode_attr_group);
> > + } else if (asus_wmi_is_present(ASUS_WMI_DEVID_GPU_MUX_VIVO)) {
> > + asus_bioscfg.gpu_mux_dev_id = ASUS_WMI_DEVID_GPU_MUX_VIVO;
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &gpu_mux_mode_attr_group);
> > + }
> > +
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_DGPU)) {
> > + asus_bioscfg.dgpu_disable_available = true;
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &dgpu_disable_attr_group);
> > + }
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_EGPU)) {
> > + asus_bioscfg.egpu_enable_available = true;
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &egpu_enable_attr_group);
> > + }
> 
> Replace the block starting here and ending ...
> 
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_EGPU_CONNECTED))
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &egpu_connected_attr_group);
> > +
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY))
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &thermal_policy_attr_group);
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_PPT_PL1_SPL))
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &ppt_pl1_spl_attr_group);
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_PPT_PL2_SPPT))
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &ppt_pl2_sppt_attr_group);
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_PPT_APU_SPPT))
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &ppt_apu_sppt_attr_group);
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_PPT_PLAT_SPPT))
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &ppt_platform_sppt_attr_group);
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_PPT_FPPT))
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &ppt_fppt_attr_group);
> > +
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_NV_DYN_BOOST))
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &nv_dynamic_boost_attr_group);
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_NV_THERM_TARGET))
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &nv_temp_target_attr_group);
> > +
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_CHARGE_MODE))
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &charge_mode_attr_group);
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_BOOT_SOUND))
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &boot_sound_attr_group);
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_MCU_POWERSAVE))
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &mcu_powersave_attr_group);
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_PANEL_OD))
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &panel_od_attr_group);
> > + if (asus_wmi_is_present(ASUS_WMI_DEVID_PANEL_HD))
> > + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &panel_hd_mode_attr_group);
> 
> here, with:
> 
> for (i = 0; i < ARRAY_SIZE(simple_attribute_groups); i++) {
> if (!asus_wmi_is_present(simple_attribute_groups[i].wmi_devid))
> continue;
> 
> sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, simple_attribute_groups[i].attr_group);
> pr_dbg("Created sysfs-group for %s\n", simple_attribute_groups[i].attr_group->name);
> }
> 
> This will make adding new simple attributes a matter of just:
> 
> 1. Declaring the new attr using one of the macros
> 2. Adding it to simple_attribute_groups[]
> 
> And this also takes care of you logging TODO for simple attributes
> without needing to add a ton of pr_debug() calls.

Ah perfect, this was one problem I ws trying to figure out a better way of doing. I'll have a crack at this after addressing all other concerns mentioned so far.

Cheers,
Luke.

> 
> Regards,
> 
> Hans
> 
> 
> 

Powered by blists - more mailing lists