linux-kernel - Re: [PATCH v3 1/5] platform/x86: asus-armoury: move existing tunings to asus-armoury module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <33519933-f171-4bbe-adad-dbe51115cc0a@wanadoo.fr>
Date: Sat, 21 Sep 2024 09:13:56 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: "Luke D. Jones" <luke@...nes.dev>, platform-driver-x86@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, hdegoede@...hat.com,
 ilpo.jarvinen@...ux.intel.com, corentin.chary@...il.com
Subject: Re: [PATCH v3 1/5] platform/x86: asus-armoury: move existing tunings
 to asus-armoury module

Le 18/09/2024 à 11:42, Luke D. Jones a écrit :
> 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.
> 
> Signed-off-by: Luke D. Jones <luke@...nes.dev>

Hi,

a few nitpick below, should it help.

> +/* Boolean style enumeration, base macro. Requires adding show/store */
> +#define __ATTR_GROUP_ENUM(_attrname, _fsname, _possible, _dispname)     \
> +	__ATTR_SHOW_FMT(display_name, _attrname, "%s\n", _dispname);    \
> +	__ATTR_SHOW_FMT(possible_values, _attrname, "%s\n", _possible); \
> +	static struct kobj_attribute attr_##_attrname##_type =          \
> +		__ASUS_ATTR_RO_AS(type, enum_type_show);                \
> +	static struct attribute *_attrname##_attrs[] = {                \
> +		&attr_##_attrname##_current_value.attr,                 \
> +		&attr_##_attrname##_display_name.attr,                  \
> +		&attr_##_attrname##_possible_values.attr,               \
> +		&attr_##_attrname##_type.attr, NULL                     \

It would be more readable if ", NULL" was on its own line, IMHO.

> +	};                                                              \
> +	static const struct attribute_group _attrname##_attr_group = {  \
> +		.name = _fsname, .attrs = _attrname##_attrs             \
> +	}
> +
> +#define ATTR_GROUP_BOOL_RO(_attrname, _fsname, _wmi, _dispname) \
> +	__ATTR_CURRENT_INT_RO(_attrname, _wmi);                 \
> +	__ATTR_GROUP_ENUM(_attrname, _fsname, "0;1", _dispname)
> +
> +#define ATTR_GROUP_BOOL_RW(_attrname, _fsname, _wmi, _dispname) \
> +	__ATTR_CURRENT_INT_RW(_attrname, 0, 1, _wmi);           \
> +	__ATTR_GROUP_ENUM(_attrname, _fsname, "0;1", _dispname)
> +
> +/*
> + * Requires <name>_current_value_show(), <name>_current_value_show()
> + */
> +#define ATTR_GROUP_BOOL_CUSTOM(_attrname, _fsname, _dispname)           \
> +	static struct kobj_attribute attr_##_attrname##_current_value = \
> +		__ASUS_ATTR_RW(_attrname, current_value);               \
> +	__ATTR_GROUP_ENUM(_attrname, _fsname, "0;1", _dispname)
> +
> +#define ATTR_GROUP_ENUM_INT_RO(_attrname, _fsname, _wmi, _possible, _dispname) \
> +	__ATTR_CURRENT_INT_RO(_attrname, _wmi);                                \
> +	__ATTR_GROUP_ENUM(_attrname, _fsname, _possible, _dispname)
> +
> +/*
> + * Requires <name>_current_value_show(), <name>_current_value_show()
> + * and <name>_possible_values_show()
> + */
> +#define ATTR_GROUP_ENUM_CUSTOM(_attrname, _fsname, _dispname)             \
> +	__ATTR_SHOW_FMT(display_name, _attrname, "%s\n", _dispname);      \
> +	static struct kobj_attribute attr_##_attrname##_current_value =   \
> +		__ASUS_ATTR_RW(_attrname, current_value);                 \
> +	static struct kobj_attribute attr_##_attrname##_possible_values = \
> +		__ASUS_ATTR_RO(_attrname, possible_values);               \
> +	static struct kobj_attribute attr_##_attrname##_type =            \
> +		__ASUS_ATTR_RO_AS(type, enum_type_show);                  \
> +	static struct attribute *_attrname##_attrs[] = {                  \
> +		&attr_##_attrname##_current_value.attr,                   \
> +		&attr_##_attrname##_display_name.attr,                    \
> +		&attr_##_attrname##_possible_values.attr,                 \
> +		&attr_##_attrname##_type.attr, NULL                       \

It would be more readable if ", NULL" was on its own line, IMHO.

> +	};                                                                \
> +	static const struct attribute_group _attrname##_attr_group = {    \
> +		.name = _fsname, .attrs = _attrname##_attrs               \
> +	}

...

> +static const struct dmi_system_id asus_rog_ally_device[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
> +		},
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_NAME, "RC72L"),
> +		},
> +	},
> +	{ },

I think that an ending comma is not expected after a terminator.

> +};
> +
>   #endif /* !_ASUS_WMI_H_ */

...

CJ