Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Bastien Nocera <hadess@hadess.net>]

On Sat, 2020-11-21 at 15:27 +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/20/20 8:50 PM, Barnabás Pőcze wrote:
> > Hi
> > 
> > 
> > 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson írta:
> > 
> > > [...]
> > > +int platform_profile_register(struct platform_profile_handler
> > > *pprof)
> > > +{
> > > +       mutex_lock(&profile_lock);
> > > +       /* We can only have one active profile */
> > > +       if (cur_profile) {
> > > +               mutex_unlock(&profile_lock);
> > > +               return -EEXIST;
> > > +       }
> > > +
> > > +       cur_profile = pprof;
> > > +       mutex_unlock(&profile_lock);
> > > +       return sysfs_create_group(acpi_kobj,
> > > &platform_profile_group);
> > > +}
> > > +EXPORT_SYMBOL_GPL(platform_profile_register);
> > > +
> > > +int platform_profile_unregister(void)
> > > +{
> > > +       mutex_lock(&profile_lock);
> > > +       sysfs_remove_group(acpi_kobj, &platform_profile_group);
> > > +       cur_profile = NULL;
> > > +       mutex_unlock(&profile_lock);
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(platform_profile_unregister);
> > > [...]
> > 
> > 
> > I just realized that the sysfs attributes are only created if a
> > profile provider
> > is registered, and it is removed when the provide unregisters
> > itself. I believe
> > it would be easier for system daemons if those attributes existed
> > from module load
> > to module unload since they can just just open the file and watch
> > it using poll,
> > select, etc. If it goes away when the provider unregisters itself,
> > then I believe
> > a more complicated mechanism (like inotify) would need to be
> > implemented in the
> > daemons to be notified when a new provider is registered. Thus my
> > suggestion
> > for the next iteration is to create the sysfs attributes on module
> > load,
> > and delete them on unload.
> > 
> > What do you think?
> 
> Actually I asked Mark to move this to the register / unregister time
> since
> having a non functioning files in sysfs is a bit weird.
> 
> You make a good point about userspace having trouble figuring when
> this will
> show up though (I'm not worried about removal that will normally
> never happen).
> 
> I would expect all modules to be loaded before any interested daemons
> load,
> but that is an assumption and I must admit that that is a bit racy.
> 
> Bastien, what do you think about Barnabás' suggestion to always have
> the
> files present and use poll (POLL_PRI) on it to see when it changes,
> listing
> maybe "none" as available profiles when there is no provider?

Whether the file exists or doesn't, we have ways to monitor it
appearing or disappearing. I can monitor the directory with inotify to
see the file appearing or disappearing, or I can monitor the file with
inotify to see whether it changes. But that doesn't solve the
challenges from user-space.

How do I know whether the computer will have this facility at any
point? Is "none" a placeholder, or a definite answer as to whether the
computer will have support for "platform profile"?

How am I supposed to handle fallbacks, eg. how can I offer support for,
say, changing the "energy performance preference" from the Intel p-
state driver if ACPI platform profile isn't available?

I'm fine with throwing more code at fixing that race, so whatever's
easier for you, it'll be a pain for user-space either way.