Re: [PATCH v10 11/14] platform/x86: Split the alienware-wmi driver

["Kurt Borja" <kuurtb@gmail.com>]

On Tue Feb 11, 2025 at 11:56 AM -05, Andy Shevchenko wrote:
> On Fri, Feb 07, 2025 at 10:46:07AM -0500, Kurt Borja wrote:
>> Split alienware-wmi WMI drivers into different files. This is done
>> seamlessly by copying and pasting, however some blocks are reordered.
>
> ...
>
>>  obj-$(CONFIG_ALIENWARE_WMI)		+= alienware-wmi.o
>>  alienware-wmi-objs			:= alienware-wmi-base.o
>> +alienware-wmi-y				+= alienware-wmi-legacy.o
>> +alienware-wmi-y				+= alienware-wmi-wmax.o
>
> Oh my... it's even inconsistent!

Again, this is an already used pattern:

	https://elixir.bootlin.com/linux/v6.14-rc2/source/drivers/platform/x86/dell/Makefile#L14

I add configuration entries later. Is the order of the changes wrong? or
is it the entire approach? Do other modules here need a fix?

>
> ...
>
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/wmi.h>
>
> Too little headers are being included. At very quick glance a dozen
> or so is missing...

Ack.

>
>> +#include "alienware-wmi.h"
>
> ...
>
>> +static struct attribute *hdmi_attrs[] = {
>> +	&dev_attr_cable.attr,
>> +	&dev_attr_source.attr,
>> +	NULL,
>
> No comma in the terminator entry.

Ack.

>
>> +};
>
> ...
>
>> +static struct attribute *amplifier_attrs[] = {
>> +	&dev_attr_status.attr,
>> +	NULL,
>> +};
>
> Ditto.
>
> ...
>
>> +{
>> +	struct alienfx_platdata *pdata = dev_get_platdata(dev);
>> +	struct wmax_basic_args in_args = {
>> +		.arg = 0,
>> +	};
>> +	u32 out_data;
>> +	int ret;
>> +
>> +	ret = alienware_wmi_command(pdata->wdev, WMAX_METHOD_DEEP_SLEEP_STATUS,
>> +				    &in_args, sizeof(in_args), &out_data);
>> +	if (!ret) {
>> +		if (out_data == 0)
>> +			return sysfs_emit(buf, "[disabled] s5 s5_s4\n");
>> +		else if (out_data == 1)
>> +			return sysfs_emit(buf, "disabled [s5] s5_s4\n");
>> +		else if (out_data == 2)
>> +			return sysfs_emit(buf, "disabled s5 [s5_s4]\n");
>
> The whole code inherited same issues like redundant 'else'. Please, refactor.

This is not my code, so a separate patch would be needed.

>
>> +	}
>> +
>> +	pr_err("alienware-wmi: unknown deep sleep status: %d\n", ret);
>> +	return sysfs_emit(buf, "disabled s5 s5_s4 [unknown]\n");
>> +}
>
> ...
>
>> +static ssize_t deepsleep_store(struct device *dev, struct device_attribute *attr,
>> +			       const char *buf, size_t count)
>> +{
>> +	struct alienfx_platdata *pdata = dev_get_platdata(dev);
>> +	struct wmax_basic_args args;
>> +	int ret;
>> +
>> +	if (strcmp(buf, "disabled\n") == 0)
>> +		args.arg = 0;
>> +	else if (strcmp(buf, "s5\n") == 0)
>> +		args.arg = 1;
>> +	else
>> +		args.arg = 2;
>
> sysfs_match_string()

Same as above.

>
>> +	pr_debug("alienware-wmi: setting deep sleep to %d : %s", args.arg, buf);
>> +
>> +	ret = alienware_wmi_command(pdata->wdev, WMAX_METHOD_DEEP_SLEEP_CONTROL,
>> +				    &args, sizeof(args), NULL);
>> +	if (!ret)
>> +		pr_err("alienware-wmi: deep sleep control failed: results: %u\n", ret);
>> +
>> +	return count;
>> +}
>
>> +
>
> Redundant blank line.
>
>> +static DEVICE_ATTR_RW(deepsleep);
>
> ...
>
>> +static struct attribute *deepsleep_attrs[] = {
>> +	&dev_attr_deepsleep.attr,
>> +	NULL,
>
> No comma.
>
>> +};
>
> ...
>
>> +	if ((code & WMAX_THERMAL_TABLE_MASK) == WMAX_THERMAL_TABLE_USTT &&
>> +	    (code & WMAX_THERMAL_MODE_MASK) <= THERMAL_MODE_USTT_LOW_POWER)
>> +		return true;
>> +
>> +	return false;
>
> 	return ...
>
> but if you wish, this one is okay.

This was done for readibility. Also this would require a different
patch.

>
> ...
>
>> +static int thermal_profile_probe(void *drvdata, unsigned long *choices)
>> +{
>> +	enum platform_profile_option profile;
>> +	struct awcc_priv *priv = drvdata;
>> +	enum wmax_thermal_mode mode;
>> +	u8 sys_desc[4];
>> +	u32 first_mode;
>> +	u32 out_data;
>> +	int ret;
>> +
>> +	ret = wmax_thermal_information(priv->wdev, WMAX_OPERATION_SYS_DESCRIPTION,
>> +				       0, (u32 *) &sys_desc);
>
> How do you guarantee an alignment? Yes, it might be good for the specific
> hardware, but in general this is broken code.

This is a good question. I'm not really sure how to fix this tho. Is it
fine to just pass a __packed struct? Also this would require another
patch.

>
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	first_mode = sys_desc[0] + sys_desc[1];
>
>> +	for (u32 i = 0; i < sys_desc[3]; i++) {
>
> Why u32? unsigned int looks more natural here.
>
>> +		ret = wmax_thermal_information(priv->wdev, WMAX_OPERATION_LIST_IDS,
>> +					       i + first_mode, &out_data);
>> +
>> +		if (ret == -EIO)
>> +			return ret;
>> +
>> +		if (ret == -EBADRQC)
>> +			break;
>> +
>> +		if (!is_wmax_thermal_code(out_data))
>> +			continue;
>> +
>> +		mode = out_data & WMAX_THERMAL_MODE_MASK;
>> +		profile = wmax_mode_to_platform_profile[mode];
>> +		priv->supported_thermal_profiles[profile] = out_data;
>
>> +		set_bit(profile, choices);
>
> Do you need it to be atomic?

I don't think so. `choices` belongs to this thread only.

>
>> +	}
>> +
>> +	if (bitmap_empty(choices, PLATFORM_PROFILE_LAST))
>> +		return -ENODEV;
>> +
>> +	if (awcc->gmode) {
>> +		priv->supported_thermal_profiles[PLATFORM_PROFILE_PERFORMANCE] =
>> +			WMAX_THERMAL_MODE_GMODE;
>> +
>> +		set_bit(PLATFORM_PROFILE_PERFORMANCE, choices);
>> +	}
>> +
>> +	return 0;
>> +}
>
> ...
>
>> +static const struct wmi_device_id alienware_wmax_device_id_table[] = {
>> +	{ WMAX_CONTROL_GUID, NULL },
>> +	{ },
>
> No comma.
>
>> +};
>
> ...
>
>> +int __init alienware_wmax_wmi_init(void)
>> +{
>> +	const struct dmi_system_id *id;
>> +
>> +	id = dmi_first_match(awcc_dmi_table);
>> +	if (id)
>> +		awcc = id->driver_data;
>> +
>> +	if (force_platform_profile) {
>> +		if (!awcc)
>> +			awcc = &empty_quirks;
>> +
>> +		awcc->pprof = true;
>> +	}
>> +
>> +	if (force_gmode) {
>> +		if (awcc)
>> +			awcc->gmode = true;
>> +		else
>> +			pr_warn("force_gmode requires platform profile support\n");
>> +	}
>> +
>> +	return wmi_driver_register(&alienware_wmax_wmi_driver);
>> +}
>> +
>> +void __exit alienware_wmax_wmi_exit(void)
>> +{
>> +	wmi_driver_unregister(&alienware_wmax_wmi_driver);
>> +}
>
> Why not moving these boilerplate to ->probe() and use module_wmi_driver()?

This 3 files are a single module and it has two WMI drivers so this
can't be used.

-- 
 ~ Kurt