Re: [PATCH v3] platform/x86: hp-wmi: add support for omen laptops

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

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