Message-ID: <e7da54e0-177c-46cc-8407-17bdb07010a0@gmx.de>
Date:   Fri, 8 Dec 2023 19:10:46 +0100
From:   Armin Wolf <W_Armin@....de>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc:     Hans de Goede <hdegoede@...hat.com>, corbet@....net,
        Dell.Client.Kernel@...l.com, linux-doc@...r.kernel.org,
        platform-driver-x86@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 5/5] platform/x86: wmi: Add driver development guide

Am 08.12.23 um 13:25 schrieb Ilpo Järvinen:

> On Thu, 7 Dec 2023, Armin Wolf wrote:
>
>> Since 2010, an LWN article covering WMI drivers exists:
>>
>> 	https://lwn.net/Articles/391230/
>>
>> Since the introduction of the modern bus-based interface
>> and other userspace tooling (fwts wmi, bmfdec, ...), this
>> article is outdated and causes people to still submit new
>> WMI drivers using the deprecated GUID-based interface.
>> Fix this by adding a short guid on how to develop WMI drivers
> Too used to typing guid(?), here you want "guide" instead. :-D (I know
> that feeling when my fingers type something else than I think).
>
>> using the modern bus-based interface.
>>
>> Signed-off-by: Armin Wolf <W_Armin@....de>
>> ---
>>   .../wmi/driver-development-guide.rst          | 126 ++++++++++++++++++
>>   Documentation/wmi/index.rst                   |   1 +
>>   2 files changed, 127 insertions(+)
>>   create mode 100644 Documentation/wmi/driver-development-guide.rst
>>
>> diff --git a/Documentation/wmi/driver-development-guide.rst b/Documentation/wmi/driver-development-guide.rst
>> new file mode 100644
>> index 000000000000..a831e2728d25
>> --- /dev/null
>> +++ b/Documentation/wmi/driver-development-guide.rst
>> @@ -0,0 +1,126 @@
>> +.. SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +============================
>> +WMI driver development guide
>> +============================
>> +
>> +The WMI subsystem provides a rich driver api for implementing WMI drivers,
> API
>
>> +documented at Documentation/driver-api/wmi.rst. This document will serve
>> +as an introductory guide for WMI driver writers using this API. It is supposed
>> +t be an successor to the original `LWN article <https://lwn.net/Articles/391230/>`_
> t -> to
>
>> +which deals with WMI drivers using the deprecated GUID-based WMI interface.
>> +
>> +Optaining WMI device information
> Obtaining
>
>> +--------------------------------
>> +
>> +Before developing an WMI driver, information about the WMI device in question
>> +must be optained. The `lswmi <https://pypi.org/project/lswmi>`_ utility can be
> obtained
>
>> +used to display detailed WMI device information using the following command:
>> +
>> +::
>> +
>> +  lswmi -V
>> +
>> +The resulting output will contain information about all WMI devices inside a given
>> +machine, plus some extra information.
>> +
>> +In order to find out more about the interface used to communicate with a WMI device,
>> +the `bmfdec <https://github.com/pali/bmfdec>`_ utilities can be used to decode
>> +the Binary MOF information used to describe WMI devices. The ``wmi-bmof`` driver
> (Managed Object Format)
>
>> +exposes this information to userspace, see Documentation/ABI/stable/sysfs-platform-wmi-bmof.
> This should use a true link to the file.
>
>> +In order to retrieve the decoded Binary MOF information, use the following command (requires root):
>> +
>> +::
>> +
>> +  ./bmf2mof /sys/bus/wmi/devices/05901221-D566-11D1-B2F0-00A0C9062910[-X]/bmof
>> +
>> +Sometimes, looking at the disassembled ACPI tables used to describe the WMI device
>> +helps in understanding how the WMI device is supposed to work. To find out which
>> +ACPI method handles which WMI device, the `fwts <https://github.com/fwts/fwts>`_
>> +program can be used with the following command (requires root):
>> +
>> +::
>> +
>> +  fwts wmi -
>> +
>> +Basic WMI driver structure
>> +--------------------------
>> +
>> +The basic WMI driver is build around the struct wmi_driver, which is then bound
>> +to matching WMI devices using an struct wmi_device_id table. Please note that each
> an struct -> a struct
>
>> +WMI driver should be able to be instantiated multiple times.
>> +
>> +::
>> +
>> +  static const struct wmi_device_id foo_id_table[] = {
>> +         { "936DA01F-9ABD-4D9D-80C7-02AF85C822A8", NULL },
>> +         { }
>> +  };
>> +  MODULE_DEVICE_TABLE(wmi, foo_id_table);
>> +
>> +  static struct wmi_driver foo_driver = {
>> +        .driver = {
>> +                .name = "foo",
>> +                .probe_type = PROBE_PREFER_ASYNCHRONOUS,        /* optional */
>> +                .pm = pm_sleep_ptr(&foo_dev_pm_ops),            /* optional */
>> +        },
>> +        .id_table = foo_id_table,
>> +        .probe = foo_probe,
>> +        .remove = foo_remove,         /* optional, devres is preferred */
>> +        .notify = foo_notify,         /* optional, for event handling */
>> +  };
>> +  module_wmi_driver(foo_driver);
>> +
>> +If your WMI driver is not using any deprecated GUID-based WMI functions and is
>> +able to be instantiated multiple times, please add its GUID to ``allow_duplicates``
>> +inside drivers/platform/x86/wmi.c, so that the WMI subsystem does not block duplicate
>> +GUIDs for it.
> Just voicing wouldn't it be more useful to not burden new stuff with this
> at all and construct the opposite list instead with the GUIDs that have
> a driver that don't support duplicates? It's the existing set of GUIDs we
> have in-tree minus those currently on the list, correct?

You are right about this, i am already thinking about a different approach which
does not rely on such a whitelist.

Basically, the legacy GUID-based functions only act on WMI devices which have an
ID of zero (which means they where found first), so that legacy drivers do not see
WMI devices with a duplicate GUID.
At the same time, WMI drivers would have to set a flag inside their struct wmi_driver
to indicate that they can be safely instantiated multiple times, otherwise they would
only be allowed to bind to WMI devices with an ID of zero (which are unique).

This would replace the whitelist with a flag inside wmi_driver, which can be enabled
by the driver developer without having to touch the WMI driver core at all.

I think i will split out this patch from the next revision of the series, since getting
rid of the whitelist should be a separate series.

Thanks,
Armin Wolf

>> +WMI method drivers
>> +------------------
>> +
>> +WMI drivers can call WMI device methods using wmidev_evaluate_method(), the
>> +structure of the ACPI buffer passed to this function is device-specific and usually
>> +needs some tinkering to get right. Looking at the ACPI tables containing the WMI
>> +device usually helps here. The method id and instance number passed to this function
>> +are also device-specific, looking at the decoded Binary MOF is usually enough to
>> +find the right values.
>> +The maximum instance number can be retrieved during runtime using wmidev_instance_count().
>> +
>> +Take a look at drivers/platform/x86/inspur_platform_profile.c for an example WMI method driver.
>> +
>> +WMI data block drivers
>> +----------------------
>> +
>> +WMI drivers can query WMI device data blocks using wmidev_block_query(), the
>> +structure of the returned ACPI object is again device-specific. Some WMI devices
>> +also allow for setting data blocks using wmidev_block_set().
>> +The maximum instance number can also be retrieved using wmidev_instance_count().
>> +
>> +Take a look at drivers/platform/x86/intel/wmi/sbl-fw-update.c for an example
>> +WMI data block driver.
>> +
>> +WMI event drivers
>> +-----------------
>> +
>> +WMI drivers can receive WMI event notifications by providing the notify() callback
>> +inside the struct wmi_driver. The WMI subsystem will then take care of setting
>> +up the WMI event accordingly. Plase note that the ACPI object passed to this callback
> Plase -> Please
>
>> +is optional and its structure device-specific. It also does not need to be freed,
> structure is device-specific.
>
>> +the WMI subsystem takes care of that.
> I'd state the freeing part more strongly:
>
> Releasing the ACPI object is handled by the WMI subsystem, not the driver.
>
>> +
>> +Take a look at drivers/platform/x86/xiaomi-wmi.c for an example WMI event driver.
>> +
>> +Things to avoid
>> +---------------
>> +
>> +When developing WMI drivers, there are a couple of things which should be avoid
>> +if feasible:
>> +
>> +- usage of the deprecated GUID-based WMI interface
> It would be nice to be more specific because it's far from obvious at this
> point how to differentiate. So perhaps adding something like this would
> help:
>
> (avoid functions with wmi_ prefix that input GUID converting it into
> a wmi_device using wmi_find_device_by_guid()).
>
>> +- bypassing of the WMI subsystem when talking to WMI devices
>> +- WMI drivers which cannot be instantiated multiple times.
>> +
>> +Many older WMI drivers violate one or more points from this list. The reason for
>> +this is that the WMI subsystem evolved significantly over the last two decades,
>> +so there is a lot of legacy cruft inside older WMI drivers.
>

Powered by blists - more mailing lists