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

["Kurt Borja" <kuurtb@gmail.com>]

On Mon Feb 3, 2025 at 9:09 AM -05, Ilpo Järvinen wrote:
> On Mon, 3 Feb 2025, Kurt Borja wrote:
>
>> 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),
>
> IMHO, just define WMAX_GROUPS in the header and use it here.
>
> Similar to e.g. ARCH_PCI_DEV_GROUPS in drivers/pci/pci-sysfs.c.

Thanks for the example, I was overthinking it. I'll send a v8 with this
approach!

~ Kurt

>
>> 	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.
>>