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 for Android: free password hash cracker in your pocket

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

Date: Tue, 14 May 2024 02:53:10 +0000
From: Peng Fan <peng.fan@....com>
To: Cristian Marussi <cristian.marussi@....com>, Guenter Roeck
	<linux@...ck-us.net>
CC: "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

> Subject: Re: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for
> thermal zones
> 
> On Thu, Jan 25, 2024 at 08:16:45AM -0800, Guenter Roeck wrote:
> > On 1/25/24 08:09, Cristian Marussi wrote:
> >
> > > Agreed, but it seems that indeed here the attempt is to make sure
> > > that an accidentally disabled sensor is turned on.
> > >
> >
> > From the patch:
> >
> > +static int scmi_hwmon_thermal_change_mode(struct
> thermal_zone_device *tz,
> > +					  enum thermal_device_mode
> new_mode) {
> > ...
> > +	if (new_mode == THERMAL_DEVICE_ENABLED)
> > +		config |= SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> > +	else
> > +		config &= ~SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> >
> > This clearly contradicts your statement. It leaves the sensor in full
> > control by the thermal subsystem. If the thermal subsystem decides to
> > turn it off, it is turned off.
> 
> Yes, indeed, and this is wrong as you explained; what I meant is that it seems
> to me now after this discussion, and from the commit message, that the
> reason (and the aim of this patch) is different from how it achieves it (as a
> side effect)
> 
> "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."
> 
> So when I said:
> 
> > > Agreed, but it seems that indeed here the attempt is to make sure
> > > that an accidentally disabled sensor is turned on.
> 
> and
> 
> >> 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)
> 
> I implied to drop this patch and instead make sure the visible-and-always-
> enabled sensor is default-enabled by the SCMI server running in the firmware,
> given that there won't be any need to ever disable it, from the hwmon
> interface NOR from the thermal subsystem.
> 
> Sorry if I have not been clear (but I maybe still well-wrong anyway :D)

Sorry to bring back this old topic.

The tempsensor is disabled at boot, I will check with FW owner to enable it
by default. But the tempsensor will consume some power, if leaving
it always enabled.

Do we need to export a HWMON_T_ENABLE to temperature sensor if
leaving thermal framework only reading temp?

Thanks,
Peng.
> 
> Thanks,
> Cristian
>