Hi,
On 8/25/21 10:31 PM, Guenter Roeck wrote:
> On Wed, Aug 25, 2021 at 06:58:52PM +0200, Enver Balalic wrote:
>> Hi,
>>
>> before I go out and send out a V4 of this, I wanted to check
>> if you agree with the changes I plan on making
>>
> [ ... ]
>
>>>>>> static int thermal_profile_get(void)
>>>>>> {
>>>>>> return hp_wmi_read_int(HPWMI_THERMAL_PROFILE_QUERY);
>>>>>> @@ -946,19 +1092,34 @@ static int thermal_profile_setup(void)
>>>>>> int err, tp;
>>>>>>
>>>>>> tp = thermal_profile_get();
>>>>>> - if (tp < 0)
>>>>>> - return tp;
>>>>>> + if (tp >= 0) {
>>>>>> + /*
>>>>>> + * call thermal profile write command to ensure that the firmware correctly
>>>>>> + * sets the OEM variables for the DPTF
>>>>>> + */
>>>>>> + err = thermal_profile_set(tp);
>>>>>> + if (err)
>>>>>> + return err;
>>>>>>
>>>>>> - /*
>>>>>> - * call thermal profile write command to ensure that the firmware correctly
>>>>>> - * sets the OEM variables for the DPTF
>>>>>> - */
>>>>>> - err = thermal_profile_set(tp);
>>>>>> - if (err)
>>>>>> - return err;
>>>>>> + platform_profile_handler.profile_get = platform_profile_get;
>>>>>> + platform_profile_handler.profile_set = platform_profile_set;
>>>>>> + }
>>>>>
>>>>> I don't really understand the above logic change. Why is
>>>>> the error from thermal_profile_get() now ignored ?
>>>>>
>>>>>>
>>>>>> - platform_profile_handler.profile_get = platform_profile_get,
>>>>>> - platform_profile_handler.profile_set = platform_profile_set,
>>>>>> + tp = omen_thermal_profile_get();
>>>>>> + if (tp >= 0) {
>>>>>> + /*
>>>>>> + * call thermal profile write command to ensure that the firmware correctly
>>>>>> + * sets the OEM variables
>>>>>> + */
>>>>>> + err = omen_thermal_profile_set(tp);
>>>>>> + if (err < 0)
>>>>>> + return err;
>>>>>> +
>>>>>> + platform_profile_handler.profile_get = platform_profile_omen_get;
>>>>>> + platform_profile_handler.profile_set = platform_profile_omen_set;
>>>>>
>>>>> It looks like omen_thermal_profile_get() has priority over
>>>>> thermal_profile_get(). If so, it might make more sense to execute it first
>>>>> and only call thermal_profile_get() if omen_thermal_profile_get() returned
>>>>> an error. If ignoring the result from thermal_profile_get() is on purpose,
>>>>> it might make sense to drop that code entirely.
>>>>>
>>>>> I am not entirely sure I understand what this code is supposed to be doing,
>>>>> though. Some comments might be useful.
>>>> Looking at it again, as it stands this is wrong, the omen code should only
>>>> run if the regular thermal_profile_get() returns an error, and not how it
>>>> is now.
>>>>
>>>> Background to this is that the thermal_profile_get() code doesn't work on
>>>> the Omen, so the omen specific path is needed, but only in the case that
>>>> the regular, non-omen code fails.
>>>>
>>>> As for ignoring the errors, I guess that in the case that both the regular
>>>> thermal_profile_get, and omen_thermal_profile_get fail, this function
>>>> should just return -EOPNOTSUPP instead of returning the error code of the
>>>> last function that ran (omen one) like it does now ?
>>>
>>> I can't really say since I am not that involved in the driver.
>>> All I noticed is that the code is odd and difficult to understand.
>>> There should be a better means to determine if the system is an
>>> "Omen" than trial and error, possibly from its DMI data or maybe
>>> from its WMI GUIDs.
>> I took a look at how the Windows Omen Command Center program detects what machine
>> is an Omen, and I found they match the DMI Board Name against a list of Omen
>> board names. I should do the same in this case.
>
> I would think so, but that is really a decision to be made by the driver
> maintainer.
If the Windows driver uses DMI matching to only use the Omen WMI API
on certain devices, then yes please do the same in the Linux code.
>>>>>> + } else {
>>>>>> + return tp;
>>>>>> + }
>>>>>
>>>>> if (tp < 0)
>>>>> return tp;
>>>>>
>>>>> followed by non-error code would be more common.
>>>>>
>>>>>>
>>>>>> set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
>>>>>> set_bit(PLATFORM_PROFILE_BALANCED, platform_profile_handler.choices);
>>>>>> @@ -973,6 +1134,8 @@ static int thermal_profile_setup(void)
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> +static int hp_wmi_hwmon_init(void);
>>>>>> +
>>>>>> static int __init hp_wmi_bios_setup(struct platform_device *device)
>>>>>> {
>>>>>> /* clear detected rfkill devices */
>>>>>> @@ -984,6 +1147,8 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>>>>>> if (hp_wmi_rfkill_setup(device))
>>>>>> hp_wmi_rfkill2_setup(device);
>>>>>>
>>>>>> + hp_wmi_hwmon_init();
>>>>>> +
>>>>> This doesn't really make sense. If it is critical, it should abort here.
>>>>> If it isn't, the function should not return an error only for it to be
>>>>> ignored.
>>>>>
>>>>> Also, if hwmon functionality isn't critical, the driver should not depend
>>>>> on HWMON since it performs perfectly fine without it.
>>>> Here if it's running on an omen and HWMON isn't there, there is no reporting
>>>> of fan speeds and the max/auto toggle won't work. So I don't know if that is
>>>> considered `critical`. I would guess not ?
>>>
>>> The point I am trying to make is
>>>
>>> 1) If the return value from hp_wmi_hwmon_init() is ignored,
>>> hp_wmi_hwmon_init() should not return a value.
>>>
>>> 2) If the return value from hp_wmi_hwmon_init() is ignored, the hwmon
>>> functionality is not critical, and the driver should not depend on HWMON.
>>>
>>> "critical", in the sense I meant, means critical to system operation.
>>> The meaning depends on the driver author, of course. I can not really say
>>> if the driver should depend on HWMON or not. All I can say is that it is
>>> inconsistent to make the driver depend on HWMON and then to ignore that
>>> hwmon device instantiation failed.
>> I took a look at how other vendor's WMI drivers handle this, and a couple of
>> them depend on HWMON (asus, gigabyte), while the thinkpad and eeepc ones
>> select HWMON instead of depending on it. Here I think I should just handle
>> this error properly, and leave the HWMON dependency in this driver ?
>
> Ok with me but, again, the maintainer should have an opinion about this.
I would prefer just selecting HWMON in Kconfig and keeping the code clean from
any special handling which may be necessary when HWMON is unset.
Regards,
Hans
