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 16:09:08 +0000
From: Cristian Marussi <cristian.marussi@....com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Peng Fan <peng.fan@....com>, "Peng Fan (OSS)" <peng.fan@....nxp.com>,
	"groeck7@...il.com" <groeck7@...il.com>,
	"sudeep.holla@....com" <sudeep.holla@....com>,
	"jdelvare@...e.com" <jdelvare@...e.com>,
	"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

On Thu, Jan 25, 2024 at 06:25:00AM -0800, Guenter Roeck wrote:
> On 1/24/24 23:06, Peng Fan wrote:
> > 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
> > 
> 
> This is wrong. If the sensor is disabled, there should either be no
> sensor attribute (if the condition is permanent), or there should be
> tempX_enable attribute(s) which return the sensor status (and, if
> appropriate, permit it to be enabled). If the condition is not
> permanent, attempts to read the sensor value should return -ENODATA.
> 
If the sensor is permanently disabled it should not have been exposed by
the SCMI server at all, in first place; in such a case it wont appear at
all in hwmon.

In this case seems like the sensor is NOT supposed to be ever disabled
(not even temporarily), it it just that you want to ensure that is
enabled, so I would say @Peng, should not that be done in the fw
SCMI server ? (i.e. to turn on, early on, all those resources that are 
 exposed to the agent and meant to be always on)

Regarding the non-permanent condition, currently if the SCMI reading
failed, for some reason, we return the same error as returned from the
SCMI layer, so I suppose I should post a patch to remap that return
value to -ENODATA.

> Overall, hwmon functionality is orthogonal to thermal subsystem use.
> It is not appropriate for the thermal subsystem to disable any
> temperature sensors in the hwmon subsystem because the user might
> expect to be able to read temperatures from hwmon devices even
> if sensors are not in use by the thermal subsystem.

Agreed, but it seems that indeed here the attempt is to make sure that
an accidentally disabled sensor is turned on.

Thanks,
Cristian