Re: [PATCH 1/5] platform/x86 asus-bioscfg: move existing tunings to asus-bioscfg module

[Hans de Goede <hdegoede@redhat.com>]

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

Regards,

Hans