On Sat Jan 18, 2025 at 6:48 PM -05, Armin Wolf wrote:
> Am 17.01.25 um 09:13 schrieb Kurt Borja:
>
> > Add WMI drivers for LEGACY and WMAX devices.
> >
> > This involves moving platform driver and device registration to a helper
> > function, which is now called from the driver's preferred WMI device
> > driver probe. However this is only done if !quirks->thermal because
> > newer WMAX interface doesn't support any of the features exposed by this
> > device.
> >
> > Only one driver is registered on module initialization to prevent
> > registering duplicate platform driver and device.
> >
> > Additionally, create_thermal_profile() now takes wmi_device * instead of
> > platform_device *.
> >
> > Signed-off-by: Kurt Borja <kuurtb@xxxxxxxxx>
> > ---
> > drivers/platform/x86/dell/alienware-wmi.c | 200 +++++++++++++++++-----
> > 1 file changed, 160 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> > index 5779b025761b..e1a5a867f144 100644
> > --- a/drivers/platform/x86/dell/alienware-wmi.c
> > +++ b/drivers/platform/x86/dell/alienware-wmi.c
> > @@ -15,6 +15,7 @@
> > #include <linux/platform_profile.h>
> > #include <linux/dmi.h>
> > #include <linux/leds.h>
> > +#include <linux/wmi.h>
> >
> > #define LEGACY_CONTROL_GUID "A90597CE-A997-11DA-B012-B622A1EF5492"
> > #define LEGACY_POWER_CONTROL_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
> > @@ -39,8 +40,6 @@
> > MODULE_AUTHOR("Mario Limonciello <mario.limonciello@xxxxxxxxxxx>");
> > MODULE_DESCRIPTION("Alienware special feature control");
> > MODULE_LICENSE("GPL");
> > -MODULE_ALIAS("wmi:" LEGACY_CONTROL_GUID);
> > -MODULE_ALIAS("wmi:" WMAX_CONTROL_GUID);
> >
> > static bool force_platform_profile;
> > module_param_unsafe(force_platform_profile, bool, 0);
> > @@ -412,7 +411,10 @@ struct alienfx_priv {
> > u8 lighting_control_state;
> > };
> >
> > -static struct platform_device *platform_device;
> > +struct alienfx_platdata {
> > + struct wmi_device *wdev;
> > +};
> > +
> > static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
> >
> > static u8 interface;
> > @@ -1127,11 +1129,11 @@ static const struct platform_profile_ops awcc_platform_profile_ops = {
> > .profile_set = thermal_profile_set,
> > };
> >
> > -static int create_thermal_profile(struct platform_device *platform_device)
> > +static int create_thermal_profile(struct wmi_device *wdev)
> > {
> > struct device *ppdev;
> >
> > - ppdev = devm_platform_profile_register(&platform_device->dev, "alienware-wmi",
> > + ppdev = devm_platform_profile_register(&wdev->dev, "alienware-wmi",
> > NULL, &awcc_platform_profile_ops);
> >
> > return PTR_ERR_OR_ZERO(ppdev);
> > @@ -1166,6 +1168,10 @@ static int alienfx_probe(struct platform_device *pdev)
> >
> > static const struct attribute_group *alienfx_groups[] = {
> > &zone_attribute_group,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group *wmax_alienfx_groups[] = {
> > &hdmi_attribute_group,
> > &lifier_attribute_group,
> > &deepsleep_attribute_group,
> > @@ -1180,19 +1186,150 @@ static struct platform_driver platform_driver = {
> > .probe = alienfx_probe,
> > };
> >
> > -static int __init alienware_wmi_init(void)
> > +static int alienware_alienfx_setup(struct alienfx_platdata *pdata)
> > {
> > + struct platform_device *pdev;
> > int ret;
> >
> > - if (wmi_has_guid(LEGACY_CONTROL_GUID))
> > - interface = LEGACY;
> > - else if (wmi_has_guid(WMAX_CONTROL_GUID))
> > - interface = WMAX;
> > - else {
> > - pr_warn("alienware-wmi: No known WMI GUID found\n");
> > - return -ENODEV;
> > + ret = platform_driver_register(&platform_driver);
> > + if (ret < 0)
> > + return ret;
>
> Can you register this platform driver during module initialization? Otherwise we might encounter
> a kernel crash should a WMI driver probe multiple devices and thus calling this method more than once.
Makes sense, I will register it on module init.
>
> Please note however that platform_device_register_data() can be called multiple times (it simply returns
> an error should the "alienware-wmi" device already exist).
That's good to know. Then it should be ok to set .no_singleton = true
after the above change.
~ Kurt
>
> > +
> > + pdev = platform_device_register_data(NULL, "alienware-wmi",
> > + PLATFORM_DEVID_NONE, pdata,
> > + sizeof(*pdata));
> > +
> > + if (IS_ERR(pdev)) {
> > + platform_driver_unregister(&platform_driver);
> > + return PTR_ERR(pdev);
> > }
> >
> > + dev_set_drvdata(&pdata->wdev->dev, pdev);
> > +
> > + return 0;
> > +}
> > +
> > +static void alienware_alienfx_exit(struct wmi_device *wdev)
> > +{
> > + struct platform_device *pdev = dev_get_drvdata(&wdev->dev);
> > +
> > + platform_device_unregister(pdev);
> > + platform_driver_unregister(&platform_driver);
> > +}
> > +
> > +/*
> > + * Legacy WMI driver
> > + */
> > +static int legacy_wmi_probe(struct wmi_device *wdev, const void *context)
> > +{
> > + struct alienfx_platdata pdata = {
> > + .wdev = wdev,
> > + };
> > +
> > + return alienware_alienfx_setup(&pdata);
> > +}
> > +
> > +static void legacy_wmi_remove(struct wmi_device *wdev)
> > +{
> > + alienware_alienfx_exit(wdev);
> > +}
> > +
> > +static struct wmi_device_id alienware_legacy_device_id_table[] = {
>
> Please make this device id table const.
>
> > + { LEGACY_CONTROL_GUID, NULL },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(wmi, alienware_legacy_device_id_table);
> > +
> > +static struct wmi_driver alienware_legacy_wmi_driver = {
> > + .driver = {
> > + .name = "alienware-wmi-alienfx",
> > + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> > + },
> > + .id_table = alienware_legacy_device_id_table,
> > + .probe = legacy_wmi_probe,
> > + .remove = legacy_wmi_remove,
> > +};
> > +
> > +static int __init alienware_legacy_wmi_init(void)
> > +{
> > + return wmi_driver_register(&alienware_legacy_wmi_driver);
> > +}
> > +
> > +static void __exit alienware_legacy_wmi_exit(void)
> > +{
> > + wmi_driver_unregister(&alienware_legacy_wmi_driver);
> > +}
> > +
> > +/*
> > + * WMAX WMI driver
> > + */
> > +static int wmax_wmi_probe(struct wmi_device *wdev, const void *context)
> > +{
> > + struct alienfx_platdata pdata = {
> > + .wdev = wdev,
> > + };
> > + struct platform_device *pdev;
> > + int ret;
> > +
> > + if (quirks->thermal) {
> > + return create_thermal_profile(wdev);
> > + } else {
> > + ret = alienware_alienfx_setup(&pdata);
> > + if (ret < 0)
> > + return ret;
> > +
> > + pdev = dev_get_drvdata(&wdev->dev);
> > +
> > + ret = device_add_groups(&pdev->dev, wmax_alienfx_groups);
> > + if (ret < 0)
> > + alienware_alienfx_exit(wdev);
> > +
> > + return ret;
> > + }
> > +}
> > +
> > +static void wmax_wmi_remove(struct wmi_device *wdev)
> > +{
> > + struct platform_device *pdev;
> > +
> > + if (!quirks->thermal) {
> > + pdev = dev_get_drvdata(&wdev->dev);
> > +
> > + device_remove_groups(&pdev->dev, wmax_alienfx_groups);
> > + alienware_alienfx_exit(wdev);
> > + }
> > +}
> > +
> > +static struct wmi_device_id alienware_wmax_device_id_table[] = {
>
> Please make this device id table const.
Thanks! I missed these two.
~ Kurt
>
> Other than that the code looks promising.
>
> Thanks,
> Armin Wolf
>
> > + { WMAX_CONTROL_GUID, NULL },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(wmi, alienware_wmax_device_id_table);
> > +
> > +static struct wmi_driver alienware_wmax_wmi_driver = {
> > + .driver = {
> > + .name = "alienware-wmi-wmax",
> > + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> > + },
> > + .id_table = alienware_wmax_device_id_table,
> > + .probe = wmax_wmi_probe,
> > + .remove = wmax_wmi_remove,
> > +};
> > +
> > +static int __init alienware_wmax_wmi_init(void)
> > +{
> > + return wmi_driver_register(&alienware_wmax_wmi_driver);
> > +}
> > +
> > +static void __exit alienware_wmax_wmi_exit(void)
> > +{
> > + wmi_driver_unregister(&alienware_wmax_wmi_driver);
> > +}
> > +
> > +static int __init alienware_wmi_init(void)
> > +{
> > + int ret;
> > +
> > dmi_check_system(alienware_quirks);
> > if (quirks == NULL)
> > quirks = &quirk_unknown;
> > @@ -1207,33 +1344,14 @@ static int __init alienware_wmi_init(void)
> > pr_warn("force_gmode requires platform profile support\n");
> > }
> >
> > - ret = platform_driver_register(&platform_driver);
> > - if (ret)
> > - goto fail_platform_driver;
> > - platform_device = platform_device_alloc("alienware-wmi", PLATFORM_DEVID_NONE);
> > - if (!platform_device) {
> > - ret = -ENOMEM;
> > - goto fail_platform_device1;
> > - }
> > - ret = platform_device_add(platform_device);
> > - if (ret)
> > - goto fail_platform_device2;
> > -
> > - if (quirks->thermal) {
> > - ret = create_thermal_profile(platform_device);
> > - if (ret)
> > - goto fail_prep_thermal_profile;
> > + if (wmi_has_guid(WMAX_CONTROL_GUID)) {
> > + interface = WMAX;
> > + ret = alienware_wmax_wmi_init();
> > + } else {
> > + interface = LEGACY;
> > + ret = alienware_legacy_wmi_init();
> > }
> >
> > - return 0;
> > -
> > -fail_prep_thermal_profile:
> > - platform_device_del(platform_device);
> > -fail_platform_device2:
> > - platform_device_put(platform_device);
> > -fail_platform_device1:
> > - platform_driver_unregister(&platform_driver);
> > -fail_platform_driver:
> > return ret;
> > }
> >
> > @@ -1241,8 +1359,10 @@ module_init(alienware_wmi_init);
> >
> > static void __exit alienware_wmi_exit(void)
> > {
> > - platform_device_unregister(platform_device);
> > - platform_driver_unregister(&platform_driver);
> > + if (interface == WMAX)
> > + alienware_wmax_wmi_exit();
> > + else
> > + alienware_legacy_wmi_exit();
> > }
> >
> > module_exit(alienware_wmi_exit);
