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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11/24/20 4:30 PM, Bastien Nocera wrote:
> 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.

Ok, do you have a preference either way? I personally think having the
file only appear if its functional is a bit cleaner. So if it does not
matter much for userspace either way then I suggest we go that route.

> 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.
 
Ugh, right this is a bit different from other hotplug cases, where
you just wait for a device to show up before using it, since in
this case you want to use the p-state driver as alternative mechanism
when there is no platform_profile provider.

I'm afraid that the answer here is that the kernel does not really
have anything to help you here. Since the advent of hotplug we had
this issue of determining when the initial hardware probing / driver
loading is done. This is basically an impossible problem since with
things like thunderbolt, etc. probing may be a bit slow and then if
there is a lot of USB devices connected to the thunderbolt dock,
those are slow to probe too, etc. See either we wait 5 minutes just to
be sure, or we cannot really tell when probing is done.

Time has taught us that determining when probing is done is an unsolvable
problem.

We had e.g. udev-settle for this. But that was both slow and not reliable,
so that is deprectated and we don't want to bring it back. In general whenever
someone wants something like udev-settle the answer is to make the
code wanting that more event driven/dynamic. So I'm afraid that that
is the answer here too.

I have the feeling that in most cases the platform_profile driver will have
loaded before the daemon starts. So you could use that as a base assumption
and then do something somewhat ugly like log a message + restart in case you
loose the race. Assuming that that is easy to implement, you could do that
for starters and then if loosing the race becomes a real problem, then
implement something smarter (and more complicated) later.

Sorry, I have no easy answers here.

Regards,

Hans