Message-ID: <25BA2FEB-288B-455E-A4B4-11968613EF41@gmail.com>
Date: Tue, 29 Apr 2025 20:23:27 -0700
From: "Derek J. Clark" <derekjohn.clark@...il.com>
To: ALOK TIWARI <alok.a.tiwari@...cle.com>, Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC: Armin Wolf <W_Armin@....de>, Jonathan Corbet <corbet@....net>,
 Mario Limonciello <superm1@...nel.org>, Luke Jones <luke@...nes.dev>,
 Xino Ni <nijs1@...ovo.com>, Zhixin Zhang <zhangzx36@...ovo.com>,
 Mia Shao <shaohz1@...ovo.com>, Mark Pearson <mpearson-lenovo@...ebb.ca>,
 "Pierre-Loup A . Griffais" <pgriffais@...vesoftware.com>,
 "Cody T . -H . Chiu" <codyit@...il.com>, John Martens <johnfanv2@...il.com>,
 platform-driver-x86@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, Mario Limonciello <mario.limonciello@....com>
Subject: Re: [External] : [PATCH v6 3/6] platform/x86: Add Lenovo WMI Events Driver



On April 28, 2025 1:57:30 PM PDT, ALOK TIWARI <alok.a.tiwari@...cle.com> wrote:
>
>
>On 28-04-2025 06:48, Derek J. Clark wrote:
>> --- /dev/null
>> +++ b/drivers/platform/x86/lenovo-wmi-events.c
>> @@ -0,0 +1,196 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Lenovo WMI Events driver. Lenovo WMI interfaces provide various
>> + * hardware triggered events that many drivers need to have propagated.
>> + * This driver provides a uniform entrypoint for these events so that
>> + * any driver that needs to respond to these events can subscribe to a
>> + * notifier chain.
>> + *
>> + * Copyright(C) 2025 Derek J. Clark <derekjohn.clark@...il.com>
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/export.h>
>> +#include <linux/module.h>
>> +#include <linux/notifier.h>
>> +#include <linux/types.h>
>> +#include <linux/wmi.h>
>> +
>> +#include "lenovo-wmi-events.h"
>> +#include "lenovo-wmi-gamezone.h"
>> +
>> +#define THERMAL_MODE_EVENT_GUID "D320289E-8FEA-41E0-86F9-911D83151B5F"
>> +
>> +#define LWMI_EVENT_DEVICE(guid, type)                        \
>> +	.guid_string = (guid), .context = &(enum lwmi_events_type) \
>> +	{                                                          \
>> +		type                                               \
>> +	}
>> +
>> +static BLOCKING_NOTIFIER_HEAD(events_chain_head);
>> +
>> +struct lwmi_events_priv {
>> +	struct wmi_device *wdev;
>> +	enum lwmi_events_type type;
>> +};
>> +
>> +/**
>> + * lwmi_events_register_notifier() - Add a notifier to the notifier chain.
>> + * @nb: The notifier_block struct to register
>> + *
>> + * Call blocking_notifier_chain_register to register the notifier block to the
>> + * lenovo-wmi-events driver blocking notifer chain.
>
>typo notifer -> notifier in all register/unregister
>
>> + *
>> + * Return: 0 on success, %-EEXIST on error.
>> + */
>> +int lwmi_events_register_notifier(struct notifier_block *nb)
>> +{
>> +	return blocking_notifier_chain_register(&events_chain_head, nb);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(lwmi_events_register_notifier, "LENOVO_WMI_EVENTS");
>> +
>> +/**
>> + * lwmi_events_unregister_notifier() - Remove a notifier from the notifier
>> + * chain.
>> + * @nb: The notifier_block struct to register
>
>the @nb parameter is described inconsistently:
>@nb: The notifier_block struct to unregister
>
>> + *
>> + * Call blocking_notifier_chain_unregister to unregister the notifier block
>> + * from the lenovo-wmi-events driver blocking notifer chain.
>> + *
>> + * Return: 0 on success, %-ENOENT on error.
>> + */
>> +int lwmi_events_unregister_notifier(struct notifier_block *nb)
>> +{
>> +	return blocking_notifier_chain_unregister(&events_chain_head, nb);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(lwmi_events_unregister_notifier, "LENOVO_WMI_EVENTS");
>
>can be consider lwmi_events_un/register_notifier as static
>if they are wrapper func?

I do think it's unlikely these would be used externally, but this pattern is consistent with other drivers with this type of function. TBS I'm not tied to one solution if there are objections to it one way or the other.

>> +
>> +/**
>> + * devm_lwmi_events_unregister_notifier() - Remove a notifier from the notifier
>> + * chain.
>> + * @data: Void pointer to the notifier_block struct to register.
>
>* @data: Void pointer to the notifier_block struct to unregister.
>
>> + *
>> + * Call lwmi_events_unregister_notifier to unregister the notifier block from
>> + * the lenovo-wmi-events driver blocking notifer chain.
>> + *
>> + * Return: 0 on success, %-ENOENT on error.
>> + */
>> +static void devm_lwmi_events_unregister_notifier(void *data)
>> +{
>> +	struct notifier_block *nb = data;
>> +
>
>if (nb) Consider checking NULL pointer
>
>> +	lwmi_events_unregister_notifier(nb);
>> +}
>> +
>> +/**
>> + * devm_lwmi_events_register_notifier() - Add a notifier to the notifier chain.
>> + * @dev: The parent device of the notifier_block struct.
>> + * @nb: The notifier_block struct to register
>> + *
>> + * Call lwmi_events_register_notifier to register the notifier block to the
>> + * lenovo-wmi-events driver blocking notifer chain. Then add, as a device
>> + * managed action, unregister_notifier to automatically unregister the
>> + * notifier block upon its parent device removal.
>> + *
>> + * Return: 0 on success, or an error code.
>> + */
>> +int devm_lwmi_events_register_notifier(struct device *dev,
>> +				       struct notifier_block *nb)
>> +{
>> +	int ret;
>> +
>> +	ret = lwmi_events_register_notifier(nb);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return devm_add_action_or_reset(dev, devm_lwmi_events_unregister_notifier, nb);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(devm_lwmi_events_register_notifier, "LENOVO_WMI_EVENTS");
>> +
>> +/**
>> + * lwmi_events_notify() - Call functions for the notifier call chain.
>> + * @wdev: The parent WMI device of the driver.
>> + * @obj: ACPI object passed by the registered WMI Event.
>> + *
>> + * Validate WMI event data and notify all registered drivers of the event and
>> + * its output.
>> + *
>> + * Return: 0 on success, or an error code.
>> + */
>> +static void lwmi_events_notify(struct wmi_device *wdev, union acpi_object *obj)
>> +{
>> +	struct lwmi_events_priv *priv = dev_get_drvdata(&wdev->dev);
>> +	int sel_prof;
>> +	int ret;
>> +
>> +	switch (priv->type) {
>> +	case LWMI_EVENT_THERMAL_MODE:
>> +		if (obj->type != ACPI_TYPE_INTEGER)
>> +			return;
>> +
>> +		sel_prof = obj->integer.value;
>> +
>> +		switch (sel_prof) {
>> +		case LWMI_GZ_THERMAL_MODE_QUIET:
>> +		case LWMI_GZ_THERMAL_MODE_BALANCED:
>> +		case LWMI_GZ_THERMAL_MODE_PERFORMANCE:
>> +		case LWMI_GZ_THERMAL_MODE_EXTREME:
>> +		case LWMI_GZ_THERMAL_MODE_CUSTOM:
>> +			ret = blocking_notifier_call_chain(&events_chain_head,
>> +							   LWMI_EVENT_THERMAL_MODE,
>> +							   &sel_prof);
>> +			if (ret == NOTIFY_BAD)
>> +				dev_err(&wdev->dev,
>> +					"Failed to send notification to call chain for WMI Events\n");
>> +			return;
>> +		default:
>> +			dev_err(&wdev->dev, "Got invalid thermal mode: %x",
>> +				sel_prof);
>> +			return;
>> +		}
>> +		break;
>> +	default:
>> +		return;
>> +	}
>> +}
>> +
>> +static int lwmi_events_probe(struct wmi_device *wdev, const void *context)
>> +{
>> +	struct lwmi_events_priv *priv;
>> +
>> +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	if (!context)
>> +		return -EINVAL;
>> +
>
>might want to check context before using devm_kzalloc()
>to avoid unnecessary memory allocation
>
>> +	priv->wdev = wdev;
>> +	priv->type = *(enum lwmi_events_type *)context;
>> +
>> +	dev_set_drvdata(&wdev->dev, priv);
>> +	return 0;
>> +}
>> +
>> +static const struct wmi_device_id lwmi_events_id_table[] = {
>> +	{ LWMI_EVENT_DEVICE(THERMAL_MODE_EVENT_GUID, LWMI_EVENT_THERMAL_MODE) },
>> +	{}
>> +};
>> +
>> +static struct wmi_driver lwmi_events_driver = {
>> +	.driver = {
>> +		.name = "lenovo_wmi_events",
>> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
>> +	},
>> +	.id_table = lwmi_events_id_table,
>> +	.probe = lwmi_events_probe,
>> +	.notify = lwmi_events_notify,
>> +	.no_singleton = true,
>> +};
>> +
>> +module_wmi_driver(lwmi_events_driver);
>> +
>> +MODULE_DEVICE_TABLE(wmi, lwmi_events_id_table);
>> +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@...il.com>");
>> +MODULE_DESCRIPTION("Lenovo WMI Events Driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/platform/x86/lenovo-wmi-events.h b/drivers/platform/x86/lenovo-wmi-events.h
>> new file mode 100644
>> index 000000000000..e4c5459c2f24
>> --- /dev/null
>> +++ b/drivers/platform/x86/lenovo-wmi-events.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +
>> +/* Copyright(C) 2025 Derek J. Clark <derekjohn.clark@...il.com> */
>> +
>> +#ifndef _LENOVO_WMI_EVENTS_H_
>> +#define _LENOVO_WMI_EVENTS_H_
>> +
>> +struct device;
>> +struct notifier_block;
>> +
>> +enum lwmi_events_type {
>> +	LWMI_EVENT_THERMAL_MODE = 1,
>> +};
>> +
>> +int lwmi_events_register_notifier(struct notifier_block *nb);
>> +int lwmi_events_unregister_notifier(struct notifier_block *nb);
>> +int devm_lwmi_events_register_notifier(struct device *dev,
>> +				       struct notifier_block *nb);
>
>why devm_lwmi_events_unregister_notifier not here ?
>

devm_lwmi_events_unregister_notifier is static as it's automatically added as a removal action during registration. It wouldn't be used directly.

>> +
>> +#endif /* !_LENOVO_WMI_EVENTS_H_ */
>
>
>Thanks,
>Alok

 Ack remaining, thanks.
- Derek

Powered by blists - more mailing lists