Re: [RFC PATCH 1/3] platform/x86: Add Uniwill WMI driver

[Hans de Goede <hansg@kernel.org>]

Hi,

On 17-Jun-25 11:10 PM, Armin Wolf wrote:
> Am 16.06.25 um 11:21 schrieb Hans de Goede:
> 
>> On 15-Jun-25 19:59, Armin Wolf wrote:
>>> Add a new driver for handling WMI events on Uniwill laptops.
>>> The driver sadly cannot use the WMI GUID for autoloading since Uniwill
>>> just copied it from the Windows driver example.
>>>
>>> The driver is reverse-engineered based on the following information:
>>> - https://github.com/pobrn/qc71_laptop
>>> - https://github.com/tuxedocomputers/tuxedo-drivers
>>> - various OEM software
>>>
>>> Reported-by: cyear <chumuzero@gmail.com>
>>> Closes: https://github.com/lm-sensors/lm-sensors/issues/508
>>> Closes: https://github.com/Wer-Wolf/uniwill-laptop/issues/3
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> ---

...

>>> +static void uniwill_wmi_notify(struct wmi_device *wdev, union acpi_object *obj)
>>> +{
>>> +    struct uniwill_wmi_data *data = dev_get_drvdata(&wdev->dev);
>>> +    u32 value;
>>> +    int ret;
>>> +
>>> +    if (obj->type != ACPI_TYPE_INTEGER)
>>> +        return;
>>> +
>>> +    value = obj->integer.value;
>>> +
>>> +    dev_dbg(&wdev->dev, "Received WMI event %u\n", value);
>>> +
>>> +    ret = blocking_notifier_call_chain(&uniwill_wmi_chain_head, value, NULL);
>>> +    if (ret == NOTIFY_BAD)
>>> +        return;
>>> +
>>> +    mutex_lock(&data->input_lock);
>>> +    sparse_keymap_report_event(data->input_device, value, 1, true);
>>> +    mutex_unlock(&data->input_lock);
>>> +}
>>> +
>>> +static int uniwill_wmi_probe(struct wmi_device *wdev, const void *context)
>>> +{
>>> +    struct uniwill_wmi_data *data;
>>> +    int ret;
>> Hmm, if someone manually modprobe-s this because they think it will
>> help it will still load despite the generic GUID thing.
>>
>> I think this should the same DMI allow-list as the -laptop driver?
>>
>> Or maybe just export the driver as a symbol and have -laptop do
>> the actual registering of the driver ? Then if e.g. a force module
>> option gets added to the -laptop driver that will not need to
>> be duplicated here ...
>>
>> Regards,
>>
>> Hans
> 
> I do not think that a allow-list is necessary here, because the driver
> cannot cause any hardware damage. The driver only listens to firmware
> events, it does not access the underlying hardware in any way.
> 
> If someone really does load this driver manually on an unsupported device
> then the worst things that will happen are random hotkey presses. In most
> cases nothing will happen.

That is true. Still it feels a bit wrong to have this potentially
bind to a completely different WMI device which happens to share
the GUID.

Anyways I'll it up to you if you want to do something about this.

Regards,

Hans