Re: [PATCH v7 00/14] platform/x86: alienware-wmi driver rework

[Kurt Borja]

On Mon Feb 3, 2025 at 7:55 AM -05, Ilpo Järvinen wrote:
> On Mon, 3 Feb 2025, Kurt Borja wrote:
>
>> On Mon Feb 3, 2025 at 4:20 AM -05, Ilpo Järvinen wrote:
>> > On Mon, 3 Feb 2025, Kurt Borja wrote:
>> >
>> >> Hi!
>> >>
>> >> I bring some last minute modifications.
>> >> 
>> >> I found commit
>> >> 
>> >> 	8d8fc146dd7a ("nvmem: core: switch to use device_add_groups()")
>> >> 
>> >> which states that it's unnecesary to call device_remove_groups() when
>> >> the device is removed, so I dropped it to simplify things.
>> >
>> > Hi Kurt,
>> 
>> Hi Ilpo,
>> 
>> >
>> >> I also found commit
>> >> 
>> >> 	957961b6dcc8 ("hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups")
>> >> 
>> >> which states that no driver should add sysfs groups while probing the
>> >> device as it races with userspace, so I re-added PROBE_FORCE_SYNCHRONOUS
>> >> to the platform driver, so groups are added only after the device has
>> >> finished probing.
>> >>
>> >> I'm not 100% sure that the second commit message applies here, but it is
>> >> revd-by Greg K-H so I added it just in case.
>> >
>> > Which is why .dev_groups should be used as it is able to avoid those 
>> > races on driver core level.
>> 
>> In previous discussions with Armin we agreed it made more sense to move
>> WMAX-only groups from alienware-wmi-base.c to alienware-wmi-wmax.c when
>> splitting.
>> 
>> I have no problem in moving them back to .dev_groups though.
>> 
>> >
>> > Why you call device_add_groups() at all? Can't you just insert it into 
>> > .dev_groups member in alienware_wmax_wmi_driver?
>> 
>> I'd love to do this as it would simplify things a LOT, but some
>> user-space tools might expect this attributes to be exposed by the
>> "fake" platform device located at
>> 
>> /sys/devices/platform/alienware-wmi
>> 
>> If it were not for this, I would expose every attribute in the WMI
>> device.
>
> Ah, sorry, I didn't pay attention where they were added to. I vaguely 
> recall that discussion.
>
> But still, you could make the groups available through .h and just add 
> them directly into alienfx_groups (with an #ifdef/#else in .h), or is 
> there again something I don't see?

What do you think about something like:

alienware-wmi.h
---------------

#if IS_ENABLED(CONFIG_ALIENWARE_WMI_WMAX)
#define WMAX_ONLY_GROUP(name)		(wmax_##name)

extern const struct attribute_group wmax_hdmi_attribute_group;
...
#else
#define WMAX_ONLY_GROUP(name)		NULL
#endif

alienware-wmi-base.c
--------------------
...
static const struct attribute_group *alienfx_groups[] = {
	&zone_attribute_group,
	WMAX_ONLY_GROUP(hdmi_attribute_group),
	WMAX_ONLY_GROUP(amplifier_attribute_group),
	WMAX_ONLY_GROUP(deepsleep_attribute_group),
	NULL
...

};

>
> Obviously, .is_visible functions need to be extended slightly to filter 
> out by interface but that should be relatively easy too. Also, the group 
> variable names should be properly prefixed when making them cross file 
> boundary like that.