Re: [External] Re: [PATCH v6 2/3] ACPI: platform-profile: Add platform profile support

[Maximilian Luz <luzmaximilian@gmail.com>]

On 12/17/20 4:09 PM, Rafael J. Wysocki wrote:
> On Thu, Dec 17, 2020 at 4:02 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>>
>> 2020. december 17., csütörtök 14:36 keltezéssel, Rafael J. Wysocki <rafael@kernel.org> írta:
>>
>>> [...]
>>>>>> My thinking here that I shouldn't make assumptions for future platform
>>>>>> implementations - there may be valid cases in the future where being
>>>>>> able to return an error condition if there was an error would be useful.
>>>>>>
>>>>>> Just trying to keep this somewhat future proof. Returning an error
>>>>>> condition seemed a useful thing to have available.
>>>>>
>>>>> You can still return an error while returning a platform_profile_option value.
>>>>>
>>>>> Just add a special value representing an error to that set.
>>>>>
>>>> I'd like to understand what is better about that approach than having an
>>>> error and a returnable parameter?
>>>>
>>>> I'm probably missing something obvious but if I add an extra
>>>> platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
>>>> error case (and return just an enum platform_profile_option) it seems I
>>>> lose the granularity of being able to return a specific error condition.
>>>> It doesn't feel like an improvement.
>>>
>>> And what's the user expected to do about the different error codes
>>> that can be returned?
>>
>> Even assuming the users of the API cannot or will not handle different errors
>> differently, who would benefit from getting rid of this information which may be
>> an aid in debugging for those who know what they're looking at?
>>
>> And if a new enum value is introduced to signal an error condition, how is that
>> going to be communicated to the users? A magic string like "error" or "-1"?
>> Or under a single errno like -EIO?
> 
> Yes.
> 
>> Personally, I believe neither of these two
>> solutions are better than returning the actual errno which is deemed most
>> appropriate by the platform profile handler. Or am I missing a third way?
> 
> Can we please defer this until we actually have a driver needing to
> return different error values from ->get_profile() (and I find it hard
> to believe that there will be any drivers like that)?

I can maybe give an example. On Microsoft Surface devices, the
performance mode / platform profile is set via a request to the embedded
controller and can be queried the same way. This request can fail (most
notably with ETIMEDOUT and EREMOTEIO). I think at least being able to
show users an error in this case would be helpful.

A driver for that is currently at [1], but I haven't adapted that yet to
the platform profile patchset.

On the other hand, I can also query and store the profile when loading
the driver and then update it when it's changed. That'd require that no
one else changes the profile, but I think we can safely assume that.

[1]: https://github.com/linux-surface/surface-aggregator-module/blob/master/module/src/clients/surface_perfmode.c,

Regards,
Max

> 
> Let's do the simpler thing until we have a real need to do the more
> complicated one.
> 
> Otherwise we're preparing for things that may never happen.
>