linux-kernel - Re: [PATCH v8 2/4] drivers: hwmon: sophgo: Add SG2042 external hardware monitor support

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]

Message-ID:
 <MA0P287MB2822D0C770667CFE484EBC95FEB12@MA0P287MB2822.INDP287.PROD.OUTLOOK.COM>
Date: Wed, 31 Jul 2024 14:13:20 +0800
From: Chen Wang <unicorn_wang@...look.com>
To: Inochi Amaoto <inochiama@...look.com>, Jean Delvare <jdelvare@...e.com>,
 Guenter Roeck <linux@...ck-us.net>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>,
 Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
 <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 Guo Ren <guoren@...nel.org>, Chao Wei <chao.wei@...hgo.com>,
 Hal Feng <hal.feng@...rfivetech.com>, Jinyu Tang <tangjinyu@...ylab.org>,
 Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
 chunzhi.lin@...hgo.com, haijiao.liu@...hgo.com
Cc: linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v8 2/4] drivers: hwmon: sophgo: Add SG2042 external
 hardware monitor support


On 2024/7/30 15:50, Inochi Amaoto wrote:
[......]
> +#define REG_CRITICAL_ACTIONS			0x65
The name "REG_CRITICAL_ACTIONS" is ambiguous. I have confirmed with 
sophgo engineers that the complete process is: when the measured 
temperature exceeds the temperature set by REG_CRITICAL_TEMP, the 
processor is powered off and shut down, and then after the temperature 
returns to the temperature set by REG_REPOWER_TEMP, it is decided 
whether to power on again or remain in the shutdown state based on the 
action set by REG_CRITICAL_ACTIONS, whether it is reboot or poweroff.

So based on the above description, I think it would be better to 
call "REG_CRITICAL_ACTIONS" as "REG_REPOWER_ACTIONS". 
"REG_CRITICAL_ACTIONS" gives people the first impression that it is used 
to set actions related to REG_CRITICAL_TEMP.

It is also recommended to add the above description of temperature 
control and action settings in the code. Currently, sophgo does not have 
a clear document description for this part, and adding it will help us 
understand its functions.

Adding sophgo engineers Chunzhi and Haijiao, FYI.

> +#define REG_CRITICAL_TEMP			0x66
> +#define REG_REPOWER_TEMP			0x67
> +
> +#define CRITICAL_ACTION_REBOOT			1
> +#define CRITICAL_ACTION_POWEROFF		2

As I said upon, actions are not related to critical, but is for 
restoring from critical, suggest to give a better name.

[......]

> +static ssize_t critical_action_show(struct device *dev,
[......]
> +static ssize_t critical_action_store(struct device *dev,

[......]

The same reason as upon, "critical_action_xxx" is misleading.

[......]

> +static int sg2042_mcu_read_temp(struct device *dev,
> +				u32 attr, int channel,
> +				long *val)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	int tmp;
> +	u8 reg;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		reg = channel ? REG_BOARD_TEMP : REG_SOC_TEMP;
> +		break;
> +	case hwmon_temp_crit:
> +		reg = REG_CRITICAL_TEMP;
> +		break;
> +	case hwmon_temp_crit_hyst:
> +		reg = REG_REPOWER_TEMP;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	tmp = i2c_smbus_read_byte_data(mcu->client, reg);
> +	if (tmp < 0)
> +		return tmp;
> +	*val = tmp * 1000;
> +
> +	return 0;
> +}
> +
> +static int sg2042_mcu_read(struct device *dev,
> +			   enum hwmon_sensor_types type,
> +			   u32 attr, int channel, long *val)
> +{
> +	return sg2042_mcu_read_temp(dev, attr, channel, val);
> +}
Can we merge sg2042_mcu_read and sg2042_mcu_read_temp？
> +
> +static int sg2042_mcu_write(struct device *dev,
> +			    enum hwmon_sensor_types type,
> +			    u32 attr, int channel, long val)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	int temp = val / 1000;
> +	int hyst_temp, crit_temp;
> +	int ret;
> +	u8 reg;
> +
> +	if (temp > MCU_POWER_MAX)
> +		temp = MCU_POWER_MAX;
> +
> +	mutex_lock(&mcu->mutex);
> +
> +	switch (attr) {
> +	case hwmon_temp_crit:
> +		hyst_temp = i2c_smbus_read_byte_data(mcu->client,
> +						     REG_REPOWER_TEMP);
> +		if (hyst_temp < 0) {
> +			ret = -ENODEV;
> +			goto failed;
> +		}
> +
> +		crit_temp = temp;
> +		reg = REG_CRITICAL_TEMP;
> +		break;
> +	case hwmon_temp_crit_hyst:
> +		crit_temp = i2c_smbus_read_byte_data(mcu->client,
> +						     REG_CRITICAL_TEMP);
> +		if (crit_temp < 0) {
> +			ret = -ENODEV;
> +			goto failed;
> +		}
> +
> +		hyst_temp = temp;
> +		reg = REG_REPOWER_TEMP;
> +		break;
> +	default:
> +		mutex_unlock(&mcu->mutex);
> +		return -EOPNOTSUPP;
> +	}
> +
It is recommended to add some comments to explain why we need to ensure 
that crit_temp is greater than or equal to hyst_temp. This is entirely 
because the current MCU does not limit the input, which may cause user 
to set incorrect crit_temp and hyst_temp.
> +	if (crit_temp < hyst_temp) {
> +		ret = -EINVAL;
> +		goto failed;
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(mcu->client, reg, temp);
> +
> +failed:
> +	mutex_unlock(&mcu->mutex);
> +	return ret;
> +}
> +
[......]