On Tue, Nov 19, 2024 at 10:40:50AM +0200, Ilpo Järvinen wrote:
> On Tue, 19 Nov 2024, Kurt Borja wrote:
>
> > Devices with hdmi_mux, amplifier or deepslp quirks create a sysfs group
> > for each available feature. To accomplish this, helper create/remove
> > functions were called on module init, but they had the following
> > problems:
> >
> > - Create helpers called remove helpers on failure, which in turn tried
> > to remove the sysfs group that failed to be created
> > - If group creation failed mid way, previous successfully created groups
> > were not cleaned up
> > - Module exit only removed hdmi_mux group
> >
> > To improve this, drop all helpers in favor of two helpers that make use
> > of sysfs_create_groups/sysfs_remove_groups to cleanly create/remove
> > groups at module init/exit.
> >
> > Signed-off-by: Kurt Borja <kuurtb@xxxxxxxxx>
> > ---
> >
> > I have a question. Do the created sysfs groups get removed when their
> > kobj reference count goes to 0? I ask because I want to know if this is
> > a bug fix.
> >
> > ---
> > drivers/platform/x86/dell/alienware-wmi.c | 105 ++++++++--------------
> > 1 file changed, 36 insertions(+), 69 deletions(-)
> >
> > diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> > index 44f1f7b57d0a..e9ed2089cba0 100644
> > --- a/drivers/platform/x86/dell/alienware-wmi.c
> > +++ b/drivers/platform/x86/dell/alienware-wmi.c
> > @@ -410,8 +410,10 @@ struct wmax_u32_args {
> > u8 arg3;
> > };
> >
> > +
> > static struct platform_device *platform_device;
> > static struct platform_zone *zone_data;
> > +const struct attribute_group *wmax_groups[4];
> > static struct platform_profile_handler pp_handler;
> > static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
> >
> > @@ -810,22 +812,6 @@ static const struct attribute_group hdmi_attribute_group = {
> > .attrs = hdmi_attrs,
> > };
> >
> > -static void remove_hdmi(struct platform_device *dev)
> > -{
> > - if (quirks->hdmi_mux > 0)
> > - sysfs_remove_group(&dev->dev.kobj, &hdmi_attribute_group);
> > -}
> > -
> > -static int create_hdmi(struct platform_device *dev)
> > -{
> > - int ret;
> > -
> > - ret = sysfs_create_group(&dev->dev.kobj, &hdmi_attribute_group);
> > - if (ret)
> > - remove_hdmi(dev);
> > - return ret;
> > -}
> > -
> > /*
> > * Alienware GFX amplifier support
> > * - Currently supports reading cable status
> > @@ -864,22 +850,6 @@ static const struct attribute_group amplifier_attribute_group = {
> > .attrs = amplifier_attrs,
> > };
> >
> > -static void remove_amplifier(struct platform_device *dev)
> > -{
> > - if (quirks->amplifier > 0)
> > - sysfs_remove_group(&dev->dev.kobj, &lifier_attribute_group);
> > -}
> > -
> > -static int create_amplifier(struct platform_device *dev)
> > -{
> > - int ret;
> > -
> > - ret = sysfs_create_group(&dev->dev.kobj, &lifier_attribute_group);
> > - if (ret)
> > - remove_amplifier(dev);
> > - return ret;
> > -}
> > -
> > /*
> > * Deep Sleep Control support
> > * - Modifies BIOS setting for deep sleep control allowing extra wakeup events
> > @@ -942,22 +912,6 @@ static const struct attribute_group deepsleep_attribute_group = {
> > .attrs = deepsleep_attrs,
> > };
> >
> > -static void remove_deepsleep(struct platform_device *dev)
> > -{
> > - if (quirks->deepslp > 0)
> > - sysfs_remove_group(&dev->dev.kobj, &deepsleep_attribute_group);
> > -}
> > -
> > -static int create_deepsleep(struct platform_device *dev)
> > -{
> > - int ret;
> > -
> > - ret = sysfs_create_group(&dev->dev.kobj, &deepsleep_attribute_group);
> > - if (ret)
> > - remove_deepsleep(dev);
> > - return ret;
> > -}
> > -
> > /*
> > * Thermal Profile control
> > * - Provides thermal profile control through the Platform Profile API
> > @@ -1165,6 +1119,34 @@ static void remove_thermal_profile(void)
> > platform_profile_remove();
> > }
> >
> > +static int __init create_wmax_groups(struct platform_device *pdev)
> > +{
> > + int no_groups = 0;
> > +
> > + if (quirks->hdmi_mux) {
> > + wmax_groups[no_groups] = &hdmi_attribute_group;
> > + no_groups++;
> > + }
> > +
> > + if (quirks->amplifier) {
> > + wmax_groups[no_groups] = &lifier_attribute_group;
> > + no_groups++;
> > + }
> > +
> > + if (quirks->deepslp) {
> > + wmax_groups[no_groups] = &deepsleep_attribute_group;
> > + no_groups++;
> > + }
> > +
> > + return no_groups > 0 ? device_add_groups(&pdev->dev, wmax_groups) : 0;
>
> Couldn't these use .dev_groups and .is_visible?
You're right, I will use this instead!
>
> --
> i.
>
>
> > +}
> > +
> > +static void __exit remove_wmax_groups(struct platform_device *pdev)
> > +{
> > + if (!wmax_groups[0])
> > + device_remove_groups(&pdev->dev, wmax_groups);
> > +}
> > +
> > static int __init alienware_wmi_init(void)
> > {
> > int ret;
> > @@ -1203,23 +1185,9 @@ static int __init alienware_wmi_init(void)
> > goto fail_platform_device1;
> > }
> >
> > - if (quirks->hdmi_mux > 0) {
> > - ret = create_hdmi(platform_device);
> > - if (ret)
> > - goto fail_prep_hdmi;
> > - }
> > -
> > - if (quirks->amplifier > 0) {
> > - ret = create_amplifier(platform_device);
> > - if (ret)
> > - goto fail_prep_amplifier;
> > - }
> > -
> > - if (quirks->deepslp > 0) {
> > - ret = create_deepsleep(platform_device);
> > - if (ret)
> > - goto fail_prep_deepsleep;
> > - }
> > + ret = create_wmax_groups(platform_device);
> > + if (ret)
> > + goto fail_prep_groups;
> >
> > if (quirks->thermal) {
> > ret = create_thermal_profile();
> > @@ -1236,9 +1204,8 @@ static int __init alienware_wmi_init(void)
> > fail_prep_zones:
> > remove_thermal_profile();
> > fail_prep_thermal_profile:
> > -fail_prep_deepsleep:
> > -fail_prep_amplifier:
> > -fail_prep_hdmi:
> > + remove_wmax_groups(platform_device);
> > +fail_prep_groups:
> > platform_device_unregister(platform_device);
> > fail_platform_device1:
> > platform_driver_unregister(&platform_driver);
> > @@ -1251,7 +1218,7 @@ module_init(alienware_wmi_init);
> > static void __exit alienware_wmi_exit(void)
> > {
> > alienware_zone_exit(platform_device);
> > - remove_hdmi(platform_device);
> > + remove_wmax_groups(platform_device);
> > remove_thermal_profile();
> > platform_device_unregister(platform_device);
> > platform_driver_unregister(&platform_driver);
> >
>
