Re: [PATCH 1/2] hp-wmi: Modernize hp_wmi_perform_query() to use the modern API

Am 22.07.26 um 00:20 schrieb yahia:

> From: yahia ahmed <[email protected]>
>
> hp_wmi_perform_query() currently uses the deprecated GUID-based WMI
> interface wmi_evaluate_method().
>
> Migrate hp-wmi to the modern bus-based WMI API by registering a
> struct wmi_driver and replacing wmi_evaluate_method() calls with
> wmidev_evaluate_method().

Hi,

using the WMI device as a global variable is not a good idea, especially w=
ithout locking.
I suggest that you instead perform much of the setup done in hp_wmi_init()=
 inside the
WMI driver probe() callback and replace the usage of global variables with=
 a state container:

https://docs.kernel.org/driver-api/driver-model/design-patterns.html

Doing so requires that you first move the WMI event handling into a proper=
 WMI event driver,
see https://docs.kernel.org/wmi/driver-development-guide.html (sections "W=
MI event drivers"
and "Handling multiple WMI devices at once") for details. You can use the =
uniwill-wmi driver
as an inspiration regarding WMI event handling coupled with notifier block=
s.

Thanks,
Armin Wolf

> Suggested-by: Ilpo J=C3=A4rvinen <[email protected]>
> Signed-off-by: yahia ahmed <[email protected]>
> ---
>   drivers/platform/x86/hp/hp-wmi.c | 49 ++++++++++++++++++++++++++++----
>   1 file changed, 44 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/=
hp-wmi.c
> index 8ba286ed8721..5845b169df39 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -14,6 +14,7 @@
>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  =20
>   #include <linux/acpi.h>
> +#include <linux/wmi.h>
>   #include <linux/cleanup.h>
>   #include <linux/compiler_attributes.h>
>   #include <linux/dmi.h>
> @@ -506,6 +507,14 @@ struct hp_wmi_hwmon_priv {
>   	struct delayed_work keep_alive_dwork;
>   };
>  =20
> +struct wmi_device *hp_wmi_wdev;
> +
> +static const struct wmi_device_id hp_wmi_guid_table[] =3D {
> +	{ .guid_string =3D HPWMI_BIOS_GUID },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(wmi, hp_wmi_guid_table);
> +
>   struct victus_s_fan_table_header {
>   	u8 num_fans;
>   	u8 unknown;
> @@ -606,7 +615,7 @@ static int hp_wmi_perform_query(int query, enum hp_w=
mi_command command,
>   	args->datasize =3D insize;
>   	memcpy(args->data, buffer, flex_array_size(args, data, insize));
>  =20
> -	ret =3D wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
> +	ret =3D wmidev_evaluate_method(hp_wmi_wdev, 0, mid, &input, &output);
>   	if (ret)
>   		goto out_free;
>  =20
> @@ -2717,14 +2726,40 @@ static void __init setup_active_thermal_profile_=
params(void)
>   	}
>   }
>  =20
> +static int hp_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> +	hp_wmi_wdev =3D wdev;
> +	return 0;
> +}
> +
> +static void hp_wmi_remove(struct wmi_device *wdev)
> +{
> +	hp_wmi_wdev =3D NULL;
> +}
> +
> +static struct wmi_driver hp_wmi_wdev_driver =3D {
> +	.driver =3D {
> +		.name =3D "hp-wmi-wdev",
> +	},
> +	.id_table =3D hp_wmi_guid_table,
> +	.probe =3D hp_wmi_probe,
> +	.remove =3D hp_wmi_remove,
> +};
> +
> +
>   static int __init hp_wmi_init(void)
>   {
>   	int event_capable =3D wmi_has_guid(HPWMI_EVENT_GUID);
>   	int bios_capable =3D wmi_has_guid(HPWMI_BIOS_GUID);
>   	int err, tmp =3D 0;
> +	err =3D wmi_driver_register(&hp_wmi_wdev_driver);
> +	if (err)
> +		return err;
>  =20
> -	if (!bios_capable && !event_capable)
> -		return -ENODEV;
> +	if (!bios_capable && !event_capable) {
> +		err =3D -ENODEV;
> +		goto err_unregister_wmi;
> +	}
>  =20
>   	if (hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_READ, &tmp,
>   				 sizeof(tmp), sizeof(tmp)) =3D=3D HPWMI_RET_INVALID_PARAMETERS)
> @@ -2733,7 +2768,7 @@ static int __init hp_wmi_init(void)
>   	if (event_capable) {
>   		err =3D hp_wmi_input_setup();
>   		if (err)
> -			return err;
> +			goto err_unregister_wmi;
>   	}
>  =20
>   	if (bios_capable) {
> @@ -2771,7 +2806,8 @@ static int __init hp_wmi_init(void)
>   err_destroy_input:
>   	if (event_capable)
>   		hp_wmi_input_destroy();
> -
> +err_unregister_wmi:
> +	wmi_driver_unregister(&hp_wmi_wmi_driver);
>   	return err;
>   }
>   module_init(hp_wmi_init);
> @@ -2794,5 +2830,8 @@ static void __exit hp_wmi_exit(void)
>   		platform_device_unregister(hp_wmi_platform_dev);
>   		platform_driver_unregister(&hp_wmi_driver);
>   	}
> +
> +	if (hp_wmi_wdev)
> +		wmi_driver_unregister(&hp_wmi_wdev_driver);
>   }
>   module_exit(hp_wmi_exit);