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: Mon, 22 Jan 2024 10:59:14 +0000
From: Cristian Marussi <cristian.marussi@....com>
To: "Peng Fan (OSS)" <peng.fan@....nxp.com>
Cc: sudeep.holla@....com, jdelvare@...e.com, linux@...ck-us.net,
	linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
	Peng Fan <peng.fan@....com>
Subject: Re: [PATCH] hwmon: scmi-hwmon: implement change_mode

On Mon, Jan 22, 2024 at 04:04:41PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@....com>
> 
> The sensor maybe disabled before kernel boot, so add change_mode
> to support configuring the sensor to enabled state.
> 

Hi Peng,

minor remarks down below

> Signed-off-by: Peng Fan <peng.fan@....com>
> ---
>  drivers/hwmon/scmi-hwmon.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
> index 364199b332c0..f7bd63d52d15 100644
> --- a/drivers/hwmon/scmi-hwmon.c
> +++ b/drivers/hwmon/scmi-hwmon.c
> @@ -151,7 +151,36 @@ 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 (config & BIT(0))

if SCMI_SENS_CFG_IS_ENABLED(config)

> +		cur_mode = THERMAL_DEVICE_ENABLED;
> +
> +	if (cur_mode == new_mode)
> +		return 0;
> +
> +	if (new_mode == THERMAL_DEVICE_ENABLED)
> +		config |= SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> +	else
> +		config &= ~SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> +

Here you are ORing the enable bit to preserve the value obtained by
config_get (rightly so), BUT unfortunately looking at the spec
CONFIG_SET uses bits [10:9] for setting the rounding mode while
CONFIG_GET does NOT report (bits [10:2] are Reserved) so you are
in fact setting ROUNDING to 00 probably, meaning ROUND_DOWN.

We could have to add (in the future not now) something in CONFIG_GET
to get the round mode (I'll speak with Souvik) BUT in the meantime I
wonder if it is not more safe here to just explicitly set the mode to
10 to signify it is up to the platform to autonomously choose how to
round. (there are SCMI_SENS_CFG_ROUND_ macros and mask in
scmi_protocol.h)

Other than these small things, LGTM.

Reviewed-by: Cristian Marussi <cristian.marussi@....com>

Thanks,
Cristian

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ