| 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
| ||
|
Message-ID: <Z6uBJ9AC5XgZTlJG@black.fi.intel.com>
Date: Tue, 11 Feb 2025 18:56:07 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Kurt Borja <kuurtb@...il.com>
Cc: platform-driver-x86@...r.kernel.org,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Armin Wolf <W_Armin@....de>,
Mario Limonciello <mario.limonciello@....com>,
Hans de Goede <hdegoede@...hat.com>, Dell.Client.Kernel@...l.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v10 11/14] platform/x86: Split the alienware-wmi driver
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!
...
> +#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...
> +#include "alienware-wmi.h"
...
> +static struct attribute *hdmi_attrs[] = {
> + &dev_attr_cable.attr,
> + &dev_attr_source.attr,
> + NULL,
No comma in the terminator entry.
> +};
...
> +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.
> + }
> +
> + 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()
> + 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.
...
> +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.
> + 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?
> + }
> +
> + 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()?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists