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] [day] [month] [year] [list]
Message-ID:
 <IA1PR20MB49535B0E8AB827FD5672580BBBB12@IA1PR20MB4953.namprd20.prod.outlook.com>
Date: Thu, 1 Aug 2024 06:48:34 +0800
From: Inochi Amaoto <inochiama@...look.com>
To: Guenter Roeck <linux@...ck-us.net>, 
	Inochi Amaoto <inochiama@...look.com>
Cc: Chen Wang <unicorn_wang@...look.com>, Jean Delvare <jdelvare@...e.com>, 
	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, 
	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 Wed, Jul 31, 2024 at 08:02:36AM GMT, Guenter Roeck wrote:
> On Wed, Jul 31, 2024 at 03:17:57PM +0800, Inochi Amaoto wrote:
> > On Wed, Jul 31, 2024 at 02:13:20PM GMT, Chen Wang wrote:
> > > 
> > > 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.
> > > 
> > > [......]
> > > 
> > 
> > Thanks for explanation, I just get the name from the driver of SG2042.
> > This is out of my knowledge.
> > 
> > > > +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？
> > 
> > Yes, it can be merged. but I think using this nested function 
> > is more clear. And gcc can auto inline this function so we
> > got no performance penalty.
> > 
> 
> FWIW, I think that is pointless. Te only difference is unused
> parameters.
> 

OK, I will merge these function.

> > > > +
> > > > +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;
> 
> No lower limit ? -1000000 is ok ?
> 

My fault, it should always > 0.

> > > > +
> > > > +	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;
> 
> Do not overwrite error codes.
> 
> > > > +		}
> > > > +
> > > > +		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;
> 
> Do not overwrite error codes.
> 
> > > > +		}
> > > > +
> > > > +		hyst_temp = temp;
> > > > +		reg = REG_REPOWER_TEMP;
> > > > +		break;
> > > > +	default:
> > > > +		mutex_unlock(&mcu->mutex);
> > > > +		return -EOPNOTSUPP;
> 
> This is inconsistent.
> 

Thanks, I will handle the return properly.

> > > > +	}
> > > > +
> > > 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.
> > 
> > Yeah, this is good idea.
> > 
> > > > +	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;
> > > > +}
> > > > +
> > > [......]