linux-kernel - Re: [PATCH v3 2/6] hwmon: (pmbus/core) improve handling of write protected regulators

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: <228997a3-fd6e-4028-a822-1507d43bcf84@roeck-us.net>
Date: Fri, 1 Nov 2024 07:59:54 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Jerome Brunet <jbrunet@...libre.com>
Cc: Jean Delvare <jdelvare@...e.com>, Jonathan Corbet <corbet@....net>,
	Patrick Rudolph <patrick.rudolph@...ements.com>,
	Naresh Solanki <naresh.solanki@...ements.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Delphine CC Chiu <Delphine_CC_Chiu@...ynn.com>,
	linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org, devicetree@...r.kernel.org,
	linux-i2c@...r.kernel.org
Subject: Re: [PATCH v3 2/6] hwmon: (pmbus/core) improve handling of write
 protected regulators

On Thu, Oct 24, 2024 at 08:10:36PM +0200, Jerome Brunet wrote:
> Writing PMBus protected registers does succeed from the smbus perspective,
> even if the write is ignored by the device and a communication fault is
> raised. This fault will silently be caught and cleared by pmbus irq if one
> has been registered.
> 
> This means that the regulator call may return succeed although the
> operation was ignored.
> 
> With this change, the operation which are not supported will be properly
> flagged as such and the regulator framework won't even try to execute them.
> 
> Signed-off-by: Jerome Brunet <jbrunet@...libre.com>
> ---
>  drivers/hwmon/pmbus/pmbus.h      |  4 ++++
>  drivers/hwmon/pmbus/pmbus_core.c | 35 ++++++++++++++++++++++++++++++++++-
>  include/linux/pmbus.h            | 14 ++++++++++++++
>  3 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index d605412a3173b95041524285ad1fde52fb64ce5a..ddb19c9726d62416244f83603b92d81d82e64891 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -487,6 +487,8 @@ struct pmbus_driver_info {
>  /* Regulator ops */
>  
>  extern const struct regulator_ops pmbus_regulator_ops;
> +int pmbus_regulator_init_cb(struct regulator_dev *rdev,
> +			    struct regulator_config *config);
>  
>  /* Macros for filling in array of struct regulator_desc */
>  #define PMBUS_REGULATOR_STEP(_name, _id, _voltages, _step, _min_uV)  \
> @@ -501,6 +503,7 @@ extern const struct regulator_ops pmbus_regulator_ops;
>  		.n_voltages = _voltages,			\
>  		.uV_step = _step,				\
>  		.min_uV = _min_uV,				\
> +		.init_cb = pmbus_regulator_init_cb,		\
>  	}
>  
>  #define PMBUS_REGULATOR(_name, _id)   PMBUS_REGULATOR_STEP(_name, _id, 0, 0, 0)
> @@ -516,6 +519,7 @@ extern const struct regulator_ops pmbus_regulator_ops;
>  		.n_voltages = _voltages,			\
>  		.uV_step = _step,				\
>  		.min_uV = _min_uV,				\
> +		.init_cb = pmbus_regulator_init_cb,		\
>  	}
>  
>  #define PMBUS_REGULATOR_ONE(_name)   PMBUS_REGULATOR_STEP_ONE(_name, 0, 0, 0)
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 085a4dc91d9bad3d2aacdd946b74a094ea9ae458..7bdd8f2ffcabc51500437182f411e9826cd7a55d 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2721,8 +2721,21 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
>  	if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) {
>  		ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT);
>  
> -		if (ret > 0 && (ret & PB_WP_ANY))
> +		switch (ret) {

This changes semantics. The mask for PB_WP_ANY was there explicitly to
avoid situations where vendors set more than those bits for whatever
reason. With this change, write protect status will no longer be
recognized for such devices.

Yes, I know, "the standard says", but standard violations are common
with PMBus devices. That is the whole point of all those callbacks.

Long story short, please do not make changes like this. "ret" still needs
to be masked.

Guenter