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

[Guenter Roeck <linux@roeck-us.net>]

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.

> > 
> > > > 
> > > > > +	} 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.

> > 
> > > > 
> > > > >  	thermal_profile_setup();
> > > > >  
> > > > >  	return 0;
> > > > > @@ -1068,6 +1233,139 @@ static struct platform_driver hp_wmi_driver = {
> > > > >  	.remove = __exit_p(hp_wmi_bios_remove),
> > > > >  };
> > > > >  
> > > > > +static umode_t hp_wmi_hwmon_is_visible(const void *data,
> > > > > +					enum hwmon_sensor_types type,
> > > > > +					u32 attr, int channel)
> > > > > +{
> > > > > +	switch (type) {
> > > > > +	case hwmon_temp:
> > > > > +		if (hp_wmi_read_int(HPWMI_HDDTEMP_QUERY) >= 0)
> > > > > +			return 0444;
> > > > > +		else
> > > > 
> > > > else after return is unnecessary (static analyzers will complain).
> > > > 
> > > > > +			return 0;
> > > > > +	case hwmon_pwm:
> > > > > +		return 0644;
> > > > > +	case hwmon_fan:
> > > > > +		if (hp_wmi_get_fan_speed(channel) >= 0)
> > > > > +			return 0444;
> > > > > +		else
> > > > 
> > > > Same as above.
> > > > 
> > > > > +			return 0;
> > > > > +	default:
> > > > > +		return 0;
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +static int hp_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> > > > > +			u32 attr, int channel, long *val)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	switch (type) {
> > > > > +	case hwmon_temp:
> > > > > +		ret = hp_wmi_read_int(HPWMI_HDDTEMP_QUERY);
> > > > > +
> > > > > +		if (ret < 0)
> > > > > +			return ret;
> > > > > +		*val = ret;
> > > > 
> > > > hddtemp is not documented, so the reported units are unknown.
> > > > Is this in milli-degrees C ? If yes, a comment might be useful.
> > > > If not, please adjust.
> > > The previous version of this patch added hwmon to this driver,
> > > before this value was exposed via a non-standard hddtemp param.
> > > That param didn't have a specified unit, so that's an unknown to
> > > me. It isn't documented anywhere in the driver.
> > > The old hddtemp param just printed out the integer value.
> > > Should this be removed then since it's an unknown ?
> > 
> > Question is what that integer value reflects. Presumably you have the
> > system, so you should be able to see the value. From there it should be
> > possible to determine the scale and if it is reported in degrees C or
> > in Kelvin.
> Actually, this functionality doesn't work on my system so I
> can't figure out what the unit it is that way. So maybe it would be best 
> to leave it out of HWMON and just let it be a non-standard sysfs attribute
> like it was before this patch ?

Agreed. Sorry, I assumed that this is working everywhere.

Thanks,
Guenter