| 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: <20c01115-2178-4a92-b600-31f5d3281a35@tuxedocomputers.com>
Date: Tue, 16 Dec 2025 16:14:47 +0100
From: Werner Sembach <wse@...edocomputers.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: W_Armin@....de, Hans de Goede <hansg@...nel.org>,
platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] platform/x86: uniwill-laptop: Introduce device
descriptor system
Am 16.12.25 um 14:40 schrieb Ilpo Järvinen:
> On Thu, 4 Dec 2025, Werner Sembach wrote:
>
>> From: Armin Wolf <W_Armin@....de>
>>
>> Future additions to the driver will depend on device-specific
>> initialization steps. Extend the DMI-based feature detection system
>> to include device descriptors. Each descriptor contains a bitmap of
>> supported features and a set of callback for performing
>> device-specific initialization.
>>
>> Signed-off-by: Armin Wolf <W_Armin@....de>
>> Co-developed-by: Werner Sembach <wse@...edocomputers.com>
>> Signed-off-by: Werner Sembach <wse@...edocomputers.com>
>> ---
>> drivers/platform/x86/uniwill/uniwill-acpi.c | 168 +++++++++++++++++---
>> 1 file changed, 142 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
>> index bd7e63dd51810..01192c32608e5 100644
>> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
>> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
>> @@ -322,6 +322,7 @@ struct uniwill_data {
>> struct device *dev;
>> acpi_handle handle;
>> struct regmap *regmap;
>> + unsigned int features;
>> struct acpi_battery_hook hook;
>> unsigned int last_charge_ctrl;
>> struct mutex battery_lock; /* Protects the list of currently registered batteries */
>> @@ -341,12 +342,21 @@ struct uniwill_battery_entry {
>> struct power_supply *battery;
>> };
>>
>> +struct uniwill_device_descriptor {
>> + unsigned int features;
>> + /* Executed during driver probing */
>> + int (*probe)(struct uniwill_data *data);
>> +};
>> +
>> static bool force;
>> module_param_unsafe(force, bool, 0);
>> MODULE_PARM_DESC(force, "Force loading without checking for supported devices\n");
>>
>> -/* Feature bitmask since the associated registers are not reliable */
>> -static unsigned int supported_features;
>> +/*
>> + * Contains device specific data like the feature bitmap since
>> + * the associated registers are not always reliable.
>> + */
>> +static struct uniwill_device_descriptor device_descriptor __ro_after_init;
>>
>> static const char * const uniwill_temp_labels[] = {
>> "CPU",
>> @@ -411,6 +421,13 @@ static const struct key_entry uniwill_keymap[] = {
>> { KE_END }
>> };
>>
>> +static inline bool uniwill_device_supports(struct uniwill_data *data,
>> + unsigned int features_mask,
>> + unsigned int features)
>> +{
>> + return (data->features & features_mask) == features;
>> +}
>> +
>> static int uniwill_ec_reg_write(void *context, unsigned int reg, unsigned int val)
>> {
>> union acpi_object params[2] = {
>> @@ -799,24 +816,31 @@ static struct attribute *uniwill_attrs[] = {
>>
>> static umode_t uniwill_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
>> {
>> + struct device *dev = kobj_to_dev(kobj);
>> + struct uniwill_data *data = dev_get_drvdata(dev);
>> +
>> if (attr == &dev_attr_fn_lock_toggle_enable.attr) {
>> - if (supported_features & UNIWILL_FEATURE_FN_LOCK_TOGGLE)
>> + if (uniwill_device_supports(data, UNIWILL_FEATURE_FN_LOCK_TOGGLE,
>> + UNIWILL_FEATURE_FN_LOCK_TOGGLE))
>> return attr->mode;
>> }
>>
>> if (attr == &dev_attr_super_key_toggle_enable.attr) {
>> - if (supported_features & UNIWILL_FEATURE_SUPER_KEY_TOGGLE)
>> + if (uniwill_device_supports(data, UNIWILL_FEATURE_SUPER_KEY_TOGGLE,
>> + UNIWILL_FEATURE_SUPER_KEY_TOGGLE))
>> return attr->mode;
>> }
>>
>> if (attr == &dev_attr_touchpad_toggle_enable.attr) {
>> - if (supported_features & UNIWILL_FEATURE_TOUCHPAD_TOGGLE)
>> + if (uniwill_device_supports(data, UNIWILL_FEATURE_TOUCHPAD_TOGGLE,
>> + UNIWILL_FEATURE_TOUCHPAD_TOGGLE))
>> return attr->mode;
>> }
>>
>> if (attr == &dev_attr_rainbow_animation.attr ||
>> attr == &dev_attr_breathing_in_suspend.attr) {
>> - if (supported_features & UNIWILL_FEATURE_LIGHTBAR)
>> + if (uniwill_device_supports(data, UNIWILL_FEATURE_LIGHTBAR,
>> + UNIWILL_FEATURE_LIGHTBAR))
>> return attr->mode;
>> }
>>
>> @@ -944,7 +968,8 @@ static int uniwill_hwmon_init(struct uniwill_data *data)
>> {
>> struct device *hdev;
>>
>> - if (!(supported_features & UNIWILL_FEATURE_HWMON))
>> + if (!uniwill_device_supports(data, UNIWILL_FEATURE_HWMON,
>> + UNIWILL_FEATURE_HWMON))
>> return 0;
>>
>> hdev = devm_hwmon_device_register_with_info(data->dev, "uniwill", data,
>> @@ -1019,7 +1044,8 @@ static int uniwill_led_init(struct uniwill_data *data)
>> unsigned int value;
>> int ret;
>>
>> - if (!(supported_features & UNIWILL_FEATURE_LIGHTBAR))
>> + if (!uniwill_device_supports(data, UNIWILL_FEATURE_LIGHTBAR,
>> + UNIWILL_FEATURE_LIGHTBAR))
>> return 0;
>>
>> ret = devm_mutex_init(data->dev, &data->led_lock);
>> @@ -1232,7 +1258,8 @@ static int uniwill_battery_init(struct uniwill_data *data)
>> {
>> int ret;
>>
>> - if (!(supported_features & UNIWILL_FEATURE_BATTERY))
>> + if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY,
>> + UNIWILL_FEATURE_BATTERY))
>> return 0;
>>
>> ret = devm_mutex_init(data->dev, &data->battery_lock);
>> @@ -1361,6 +1388,19 @@ static int uniwill_probe(struct platform_device *pdev)
>> if (ret < 0)
>> return ret;
>>
>> + data->features = device_descriptor.features;
>> +
>> + /*
>> + * Some devices might need to perform some device-specific initialization steps
>> + * before the supported features are initialized. Because of this we have to call
>> + * this callback just after the EC itself was initialized.
>> + */
>> + if (device_descriptor.probe) {
>> + ret = device_descriptor.probe(data);
>> + if (ret < 0)
>> + return ret;
>> + }
>> +
>> ret = uniwill_battery_init(data);
>> if (ret < 0)
>> return ret;
>> @@ -1385,7 +1425,8 @@ static void uniwill_shutdown(struct platform_device *pdev)
>>
>> static int uniwill_suspend_keyboard(struct uniwill_data *data)
>> {
>> - if (!(supported_features & UNIWILL_FEATURE_SUPER_KEY_TOGGLE))
>> + if (!uniwill_device_supports(data, UNIWILL_FEATURE_SUPER_KEY_TOGGLE,
>> + UNIWILL_FEATURE_SUPER_KEY_TOGGLE))
>> return 0;
>>
>> /*
>> @@ -1397,7 +1438,8 @@ static int uniwill_suspend_keyboard(struct uniwill_data *data)
>>
>> static int uniwill_suspend_battery(struct uniwill_data *data)
>> {
>> - if (!(supported_features & UNIWILL_FEATURE_BATTERY))
>> + if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY,
>> + UNIWILL_FEATURE_BATTERY))
>> return 0;
>>
>> /*
>> @@ -1432,7 +1474,8 @@ static int uniwill_resume_keyboard(struct uniwill_data *data)
>> unsigned int value;
>> int ret;
>>
>> - if (!(supported_features & UNIWILL_FEATURE_SUPER_KEY_TOGGLE))
>> + if (!uniwill_device_supports(data, UNIWILL_FEATURE_SUPER_KEY_TOGGLE,
>> + UNIWILL_FEATURE_SUPER_KEY_TOGGLE))
>> return 0;
>>
>> ret = regmap_read(data->regmap, EC_ADDR_SWITCH_STATUS, &value);
>> @@ -1448,7 +1491,8 @@ static int uniwill_resume_keyboard(struct uniwill_data *data)
>>
>> static int uniwill_resume_battery(struct uniwill_data *data)
>> {
>> - if (!(supported_features & UNIWILL_FEATURE_BATTERY))
>> + if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY,
>> + UNIWILL_FEATURE_BATTERY))
>> return 0;
>>
>> return regmap_update_bits(data->regmap, EC_ADDR_CHARGE_CTRL, CHARGE_CTRL_MASK,
>> @@ -1496,6 +1540,25 @@ static struct platform_driver uniwill_driver = {
>> .shutdown = uniwill_shutdown,
>> };
>>
>> +static struct uniwill_device_descriptor lapac71h_descriptor __initdata = {
>> + .features = UNIWILL_FEATURE_FN_LOCK_TOGGLE |
>> + UNIWILL_FEATURE_SUPER_KEY_TOGGLE |
>> + UNIWILL_FEATURE_TOUCHPAD_TOGGLE |
>> + UNIWILL_FEATURE_BATTERY |
>> + UNIWILL_FEATURE_HWMON
>> +};
>> +
>> +static struct uniwill_device_descriptor lapkc71f_descriptor __initdata = {
>> + .features = UNIWILL_FEATURE_FN_LOCK_TOGGLE |
>> + UNIWILL_FEATURE_SUPER_KEY_TOGGLE |
>> + UNIWILL_FEATURE_TOUCHPAD_TOGGLE |
>> + UNIWILL_FEATURE_LIGHTBAR |
>> + UNIWILL_FEATURE_BATTERY |
>> + UNIWILL_FEATURE_HWMON
>> +};
>> +
>> +static struct uniwill_device_descriptor empty_descriptor __initdata = {};
>> +
>> static const struct dmi_system_id uniwill_dmi_table[] __initconst = {
>> {
>> .ident = "XMG FUSION 15",
>> @@ -1503,6 +1566,7 @@ static const struct dmi_system_id uniwill_dmi_table[] __initconst = {
>> DMI_MATCH(DMI_SYS_VENDOR, "SchenkerTechnologiesGmbH"),
>> DMI_EXACT_MATCH(DMI_BOARD_NAME, "LAPQC71A"),
>> },
>> + .driver_data = &empty_descriptor,
> Hi,
>
> Is there some advantage of having an "empty descriptor" over just NULL
> checking its presence in the code?
One less "if"
In the long run (with more features implemented and tested) there probably wont
be any device using the empty descriptor, then it can be removed again.
>
>> static int __init uniwill_init(void)
>> {
>> + const struct uniwill_device_descriptor *descriptor;
>> const struct dmi_system_id *id;
>> int ret;
>>
>> @@ -1880,10 +1984,22 @@ static int __init uniwill_init(void)
>> return -ENODEV;
>>
>> /* Assume that the device supports all features */
>> - supported_features = UINT_MAX;
>> + device_descriptor.features = UINT_MAX;
>> pr_warn("Loading on a potentially unsupported device\n");
>> } else {
>> - supported_features = (uintptr_t)id->driver_data;
>> + /*
>> + * Some devices might support additional features depending on
>> + * the BIOS version/date, so we call this callback to let them
>> + * modify their device descriptor accordingly.
>> + */
>> + if (id->callback) {
>> + ret = id->callback(id);
>> + if (ret < 0)
>> + return ret;
>> + }
>> +
>> + descriptor = id->driver_data;
>> + device_descriptor = *descriptor;
>> }
>>
>> ret = platform_driver_register(&uniwill_driver);
>>
Powered by blists - more mailing lists