linux-kernel - Re: [PATCH v2 3/3] hwmon: (cros_ec) register fans into thermal framework cooling devices

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: <ad81f125-90e1-4033-8037-10be83088b12@t-8ch.de>
Date: Sat, 10 May 2025 09:58:10 +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 v2 3/3] hwmon: (cros_ec) register fans into thermal
 framework cooling devices

On 2025-05-06 10:49:37+0800, Sung-Chi Li wrote:
> On Sat, May 03, 2025 at 09:27:18AM +0200, Thomas Weißschuh wrote:
> > On 2025-05-02 13:34:47+0800, Sung-Chi Li via B4 Relay wrote:
> > > From: Sung-Chi Li <lschyi@...omium.org>

<snip>

> > > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> > > index c5e42e2a03a0c8c68d3f8afbb2bb45b93a58b955..abfcf44fb7505189124e78c651b0eb1e0533b4e8 100644
> > > --- a/drivers/hwmon/cros_ec_hwmon.c
> > > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > > @@ -13,6 +13,7 @@
> > >  #include <linux/platform_device.h>
> > >  #include <linux/platform_data/cros_ec_commands.h>
> > >  #include <linux/platform_data/cros_ec_proto.h>
> > > +#include <linux/thermal.h>
> > 
> > Needs a dependency on CONFIG_THERMAL.
> > 
> 
> I think adding the `if (!IS_ENABLED(CONFIG_THERMAL))` you suggested is
> sufficient, and turning on or off CONFIG_THERMAL both can compile, so I'll only
> add the guarding statement in the `cros_ec_hwmon_register_fan_cooling_devices`.

Agreed.

> > > +
> > > +	if (!priv->fan_control_supported)
> > > +		return;
> > > +
> > > +	for (i = 0; i < EC_FAN_SPEED_ENTRIES; i++) {
> > > +		if (!(priv->usable_fans & BIT(i)))
> > > +			continue;
> > > +
> > > +		cpriv = devm_kzalloc(dev, sizeof(*cpriv), GFP_KERNEL);
> > > +		if (!cpriv)
> > > +			return;
> > > +
> > > +		cpriv->hwmon_priv = priv;
> > > +		cpriv->index = i;
> > > +		devm_thermal_of_cooling_device_register(
> > > +			dev, NULL, devm_kasprintf(dev, GFP_KERNEL, "cros-ec-fan%zu", i), cpriv,
> > 
> > What happens for multiple/chained ECs? If both provide sensors the
> > thermal device names will collide.
> > 
> 
> How about changing the "cros-ec-fan%zu" to "%s-fan%zu", which prefixes the
> `dev_name()`? Here is an example from a device: cros-ec-hwmon.12.auto-fan0.

Sounds good in general.
It should match what a potential future HWMON_C_REGISTER_TCD would do.
Which in turn should mirror HWMON_C_REGISTER_TZ, if that has a specific
naming scheme.

> > Error handling for devm_kasprintf() is missing.
> > 
> 
> Thank you for catching this, I will skip registering that device if the
> devm_kasprintf() fails.

This should also mirror what HWMON_C_REGISTER_TZ is doing on errors.
If I read the code correctly, probing is aborted there.

> 
> > > +			&cros_ec_thermal_cooling_ops);
> > 
> > Error handling for devm_thermal_of_cooling_device_register() is missing.
> > 
> 
> I think we should continue registering other fans, so maybe we add a warning
> here if the registration fails?

Same as above.

<snip>