linux-kernel - Re: [PATCH v3 2/3] hwmon: (cros

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: <179a21f9-c578-46e9-89dd-1b9e32889015@t-8ch.de>
Date: Wed, 18 Jun 2025 12:40:04 +0200
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Sung-Chi Li <lschyi@...omium.org>
Cc: Benson Leung <bleung@...omium.org>, 
	Guenter Roeck <groeck@...omium.org>, Jean Delvare <jdelvare@...e.com>, 
	Guenter Roeck <linux@...ck-us.net>, Jonathan Corbet <corbet@....net>, 
	chrome-platform@...ts.linux.dev, linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org, 
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 2/3] hwmon: (cros_ec) add PWM control over fans

On 2025-06-18 15:26:38+0800, Sung-Chi Li wrote:
> On Mon, May 12, 2025 at 09:30:39AM +0200, Thomas Weißschuh wrote:
> 
> Sorry for the late reply, I missed mails for this series.
> 
> > On 2025-05-12 15:11:56+0800, Sung-Chi Li via B4 Relay wrote:
> > > From: Sung-Chi Li <lschyi@...omium.org>
> > >  static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8 *temp)
> > >  {
> > >  	unsigned int offset;
> > > @@ -73,7 +117,9 @@ static long cros_ec_hwmon_temp_to_millicelsius(u8 temp)
> > >  static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> > >  			      u32 attr, int channel, long *val)
> > >  {
> > > +	u8 control_method;
> > >  	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> > > +	u8 pwm_value;
> > >  	int ret = -EOPNOTSUPP;
> > >  	u16 speed;
> > >  	u8 temp;
> > 
> > Ordering again.
> > 
> > This should be:
> > 
> > struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> > int ret = -EOPNOTSUPP;
> > u8 control_method;
> > u8 pwm_value;
> > u16 speed;
> > u8 temp;
> > 
> > or:
> > 
> > struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> > u8 control_method, pwm_value, temp;
> > int ret = -EOPNOTSUPP;
> > u16 speed;
> > 
> > <snip>
> > 
> 
> Would you mind to share the sorting logic, so I do not bother you with checking
> these styling issue? Initially, I thought the sorting is based on the variable
> name, but after seeing your example (which I am appreciated), I am not sure how
> the sorting works. Is it sorted along with the variable types (
> "u8 control_method", "u16 speed", etc)? If that is the case, why "u16 speed" is
> in the middle of the u8 group variables?

Welp, I really should have explained this, instead of assuming it to be
obvious, sorry. The sorting is based on the length of the line, AKA
the "reverse christmas tree".

While this is not a strict rule, the driver is already using it, so I'd
like to stick to it. "Bonus points" for preserving preserving
semantically useful ordering within the line-length based one.

> > > +static inline bool is_cros_ec_cmd_fulfilled(struct cros_ec_device *cros_ec,
> > > +					    u16 cmd, u8 version)
> > 
> > "fulfilled" -> "available" or "present"
> > 
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = cros_ec_get_cmd_versions(cros_ec, cmd);
> > > +	return ret >= 0 && (ret & EC_VER_MASK(version));
> > > +}
> > > +
> > > +static bool cros_ec_hwmon_probe_fan_control_supported(struct cros_ec_device *cros_ec)
> > > +{
> > > +	if (!IS_ENABLED(CONFIG_PM))
> > > +		return false;
> > 
> > Why? This should generally work fine without CONFIG_PM.
> > Only the suspend/resume callbacks are unnecessary in that case.
> > 
> 
> I treat fan control should include restoring the fan setting after resume, so
> I think if no CONFIG_PM, the fan control is not complete. I am good with
> removing this check, and if you have any thoughts after this explanation, please
> share with me, otherwise I will remove it in the next series.

As far as I understand, with CONFIG_PM=n the kernel won't be able to do
any suspend/resume. So cros_ec_hwmon won't need to handle the
suspend/resume case. However even then the regular UAPI for fan duty
management is useful for users.


Thomas