On 08/12/2020 14:18, Rafael J. Wysocki wrote:
> On Tue, Dec 8, 2020 at 7:54 PM Mark Pearson <markpearson@xxxxxxxxxx> wrote:
>>
>> Hi Rafael,
>>
>> Thanks for the review - a couple of questions (and a bunch of acks) below
>>
>> On 08/12/2020 13:26, Rafael J. Wysocki wrote:
>>> On Wed, Dec 2, 2020 at 6:16 PM Mark Pearson <markpearson@xxxxxxxxxx> wrote:
>
> [cut]
>
>>>> +static const struct platform_profile_handler *cur_profile;
>>>> +static DEFINE_MUTEX(profile_lock);
>>>> +
>>>> +static const char * const profile_names[] = {
>>>> + [platform_profile_low] = "low-power",
>>>> + [platform_profile_cool] = "cool",
>>>> + [platform_profile_quiet] = "quiet",
>>>> + [platform_profile_balanced] = "balanced",
>>>> + [platform_profile_perform] = "performance",
>>>
>>> The enum values in upper case, please.
>> Sorry, I'm a bit confused here - do you mean change to "Low-power" or
>> something else (maybe PLATFORM_PROFILE_LOW?)
>
> platform_profile_low -> PLATFORM_PROFILE_LOW
> platform_profile_cool -> PLATFORM_PROFILE_COOL
>
> etc.
>
Got it - I'll update. Thanks for the clarification
>> Just want to make sure I'm getting it correct. If I change the strings
>> it will impact patch1 in the series which is integrated into your
>> bleeding-edge branch.
>>
>>>
>>>> +};
>>>> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_last);
>>>> +
>>>> +static ssize_t platform_profile_choices_show(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + char *buf)
>>>> +{
>>>> + int len = 0;
>>>> + int err, i;
>>>> +
>>>> + err = mutex_lock_interruptible(&profile_lock);
>>>
>>> Why interruptible?
>>>
>>> And why is the lock needed in the first place?
>>
>> My thinking was that I don't know what happens when I hand over to thhe
>> platform driver who does the get/set, so having a lock to prevent a get
>> whilst a set is in operation seemed like a good idea.
>
> Taking it over get/set probably is (and you need to protect the
> cur_profile pointer from concurrent updates).
>
> And here you need to ensure that the cur_profile object doesn't go
> away while this is running. So that's why.
>
>> It was interruptible as a suggestion in an earlier reivew as the
>> preferred way of doing these things for functions that could be called
>> by user space.
>
> Well, it is not used consistently this way at least. But OK.
That was based on review comments - interruptible used where it could be
accessed from user space and not where it couldn't. Hans made some good
notes about it previously.
>
>> Do you think the lock is a problem?
>
> No, it isn't in principle.
>
>>>
>>>> + if (err)
>>>> + return err;
>>>> +
>>>> + if (!cur_profile) {
>>>> + mutex_unlock(&profile_lock);
>>>> + return -ENODEV;
>>>> + }
>>>> +
>>>> + for_each_set_bit(i, cur_profile->choices, platform_profile_last) {
>>>> + if (len == 0)
>>>> + len += sysfs_emit_at(buf, len, "%s", profile_names[i]);
>>>> + else
>>>> + len += sysfs_emit_at(buf, len, " %s", profile_names[i]);
>>>> + }
>>>> + len += sysfs_emit_at(buf, len, "\n");
>>>> + mutex_unlock(&profile_lock);
>>>> + return len;
>>>> +}
>>>> +
>>>> +static ssize_t platform_profile_show(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + char *buf)
>>>> +{
>>>> + enum platform_profile_option profile = platform_profile_balanced;
>>>> + int err;
>>>> +
>>>> + err = mutex_lock_interruptible(&profile_lock);
>>>> + if (err)
>>>> + return err;
>>>> +
>>>> + if (!cur_profile) {
>>>> + mutex_unlock(&profile_lock);
>>>> + return -ENODEV;
>>>> + }
>>>> +
>>>> + err = cur_profile->profile_get(&profile);
>>>
>>> In which cases this can fail?
>> I'm not sure - but as this is supposed to be vendor agnostic I can't
>> foresee what might be wanted or could happen on various hardware.
>
> It returns the index of the current profile AFAICS, so I don't really
> see a reason for it to fail.
>
> Moreover, the index could be maintained by the common code along with
> the cur_profile pointer, couldn't it?
OK - I see your point.
I think that it's good to check for an error from the driver and to not
display a potentially incorrect value in the case of an error.
I double checked and in the Lenovo case (patch 3) all of this works
exactly as you describe (so my previous comment was incorrect). The
value is cached in thinkpad_acpi and there can't be an error returned.
But it seems wrong to make assumptions on others wanting to do the same
in the future as their implementation might have different constraints.
Let me know if you feel strongly about this and I can update, from my
point of view I lean towards leaving it as it is but it doesn't impact
the Lenovo implementation.
>
<snip>>>>> +
>>>> +enum platform_profile_option {
>>>> + platform_profile_low,
>>>> + platform_profile_cool,
>>>> + platform_profile_quiet,
>>>> + platform_profile_balanced,
>>>> + platform_profile_perform,
>>>> + platform_profile_last, /*must always be last */
>
> So please use upper-case names in this list.
>
Ack
Thanks for the clarifications
Mark
