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

[Ilpo Järvinen]

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.

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

-- 
 i.