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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]

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?

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.