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.
