linux-kernel - RE: [PATCH V3] hwmon: scmi-hwmon: implement change

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 25 Jan 2024 07:06:25 +0000
From: Peng Fan <peng.fan@....com>
To: Guenter Roeck <linux@...ck-us.net>, "Peng Fan (OSS)"
	<peng.fan@....nxp.com>, "groeck7@...il.com" <groeck7@...il.com>,
	"sudeep.holla@....com" <sudeep.holla@....com>, "cristian.marussi@....com"
	<cristian.marussi@....com>, "jdelvare@...e.com" <jdelvare@...e.com>
CC: "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for thermal
 zones

Hi Guenter,

> Subject: Re: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for
> thermal zones
> 
> On 1/24/24 22:44, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@....com>
> >
> > The thermal sensors maybe disabled before kernel boot, so add
> > change_mode for thermal zones to support configuring the thermal
> > sensor to enabled state. If reading the temperature when the sensor is
> > disabled, there will be error reported.
> >
> > The cost is an extra config_get all to SCMI firmware to get the status
> > of the thermal sensor. No function level impact.
> >
> > Reviewed-by: Cristian Marussi <cristian.marussi@....com>
> > Signed-off-by: Peng Fan <peng.fan@....com>
> > ---
> >
> > V3:
> >   Update commit log to show it only applys to thermal
> >   Add comments in code
> >   Add R-b from Cristian
> >
> 
> You didn't address my question regarding the behavior of hwmon attributes if
> a sensor is disabled.

Would you please share a bit more on what attributes?
You mean the files under /sys/class/hwmon/hwmon0?

If the sensor is disabled, when cat temp[x]_input, it will
report:
root@...95-19x19-lpddr5-evk:/sys/class/hwmon/hwmon0# cat temp3_input
cat: temp3_input: Protocol error

For enabled, it will report value:
root@...95-19x19-lpddr5-evk:/sys/class/hwmon/hwmon0# cat temp1_input
31900

> 
> >   Guenter, I Cced linux@...ck-us.net when sending V1/V2
> >   Let me Cc Guenter Roeck <groeck7@...il.com> in V3, hope you not mind
> >
> This time I received it twice ;-).
> 
> > V2:
> >   Use SCMI_SENS_CFG_IS_ENABLED & clear BIT[31:9] before update
> > config(Thanks Cristian)
> >
> >   drivers/hwmon/scmi-hwmon.c | 39
> ++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 39 insertions(+)
> >
> > diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
> > index 364199b332c0..af2267fea5f0 100644
> > --- a/drivers/hwmon/scmi-hwmon.c
> > +++ b/drivers/hwmon/scmi-hwmon.c
> > @@ -151,7 +151,46 @@ static int scmi_hwmon_thermal_get_temp(struct
> thermal_zone_device *tz,
> >   	return ret;
> >   }
> >
> > +static int scmi_hwmon_thermal_change_mode(struct
> thermal_zone_device *tz,
> > +					  enum thermal_device_mode
> new_mode) {
> > +	int ret;
> > +	u32 config;
> > +	enum thermal_device_mode cur_mode =
> THERMAL_DEVICE_DISABLED;
> > +	struct scmi_thermal_sensor *th_sensor =
> > +thermal_zone_device_priv(tz);
> > +
> > +	ret = sensor_ops->config_get(th_sensor->ph, th_sensor->info->id,
> > +				     &config);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (SCMI_SENS_CFG_IS_ENABLED(config))
> > +		cur_mode = THERMAL_DEVICE_ENABLED;
> > +
> > +	if (cur_mode == new_mode)
> > +		return 0;
> > +
> > +	/*
> > +	 * Per SENSOR_CONFIG_SET sensor_config description:
> > +	 * BIT[31:11] should be set to 0 if the sensor update interval does
> > +	 * not need to be updated, so clear them.
> > +	 * And SENSOR_CONFIG_GET does not return round up/down, so also
> clear
> > +	 * BIT[10:9] round up/down.
> 
> What does "clear" mean ? Is it going to round up ? Round down ? And why
> would it be necessary to clear those bits if SENSOR_CONFIG_GET does not
> return the current setting in the first place ?

This is to follow Cristian's suggestion to clear [31:9], because we only need
to set the sensor to enabled state, no other attributes.
My understanding is with BIT[31:11] set to 0, BIT[10:9] will not take effect.
Cristian may help comment more since he suggested to clear them in V1/V2

You are right, currently config_get will return [10:2] as reserved,
so config_set bit[10:9] no need touch. But config_get bit[10:2]
may update to return the value in future SCMI spec?

Cristian or Sudeep may help comment here.

Thanks,
Peng.

> 
> Thanks,
> Guenter
> 
> > +	 */
> > +	config &= ~(SCMI_SENS_CFG_UPDATE_SECS_MASK |
> > +		    SCMI_SENS_CFG_UPDATE_EXP_MASK |
> > +		    SCMI_SENS_CFG_ROUND_MASK);
> > +	if (new_mode == THERMAL_DEVICE_ENABLED)
> > +		config |= SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> > +	else
> > +		config &= ~SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> > +
> > +	return sensor_ops->config_set(th_sensor->ph, th_sensor->info->id,
> > +				      config);
> > +}
> > +
> >   static const struct thermal_zone_device_ops scmi_hwmon_thermal_ops =
> > {
> > +	.change_mode = scmi_hwmon_thermal_change_mode,
> >   	.get_temp = scmi_hwmon_thermal_get_temp,
> >   };
> >