| 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: <D7PT98IDXMUV.G2F1LRF8BX7@gmail.com>
Date: Tue, 11 Feb 2025 12:59:53 -0500
From: "Kurt Borja" <kuurtb@...il.com>
To: "Andy Shevchenko" <andriy.shevchenko@...el.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 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
Powered by blists - more mailing lists