linux-kernel - Re: [PATCH 4/5] 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] [day] [month] [year] [list]

Message-ID: <4052294e-7b7f-4238-9b47-92727de4d516@roeck-us.net>
Date: Fri, 20 Sep 2024 14:13:58 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Jerome Brunet <jbrunet@...libre.com>, Liam Girdwood
 <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Jean Delvare <jdelvare@...e.com>, Jonathan Corbet <corbet@....net>
Cc: linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
 linux-doc@...r.kernel.org
Subject: Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write
 protected regulators

On 9/20/24 09:47, 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 5d5dc774187b..76cff65f38d5 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -481,6 +481,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)  \
> @@ -495,6 +497,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)
> @@ -510,6 +513,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 82522fc9090a..363def768888 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2714,8 +2714,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) {
> +		case PB_WP_ALL:
> +			data->flags |= PMBUS_OP_PROTECTED;
> +			fallthrough;
> +		case PB_WP_OP:
> +			data->flags |= PMBUS_VOUT_PROTECTED;
> +			fallthrough;
> +		case PB_WP_VOUT:
>   			data->flags |= PMBUS_WRITE_PROTECTED | PMBUS_SKIP_STATUS_CHECK;
> +			break;
> +
> +		default:
> +			/* Ignore manufacturer specific and invalid as well as errors */
> +			break;
> +		}
>   	}
>   
>   	if (data->info->pages)
> @@ -3172,8 +3185,12 @@ static int pmbus_regulator_list_voltage(struct regulator_dev *rdev,
>   {
>   	struct device *dev = rdev_get_dev(rdev);
>   	struct i2c_client *client = to_i2c_client(dev->parent);
> +	struct pmbus_data *data = i2c_get_clientdata(client);
>   	int val, low, high;
>   
> +	if (data->flags & PMBUS_VOUT_PROTECTED)
> +		return 0;
> +
>   	if (selector >= rdev->desc->n_voltages ||
>   	    selector < rdev->desc->linear_min_sel)
>   		return -EINVAL;
> @@ -3208,6 +3225,22 @@ const struct regulator_ops pmbus_regulator_ops = {
>   };
>   EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
>   
> +int pmbus_regulator_init_cb(struct regulator_dev *rdev,
> +			    struct regulator_config *config)
> +{
> +	struct pmbus_data *data = config->driver_data;
> +	struct regulation_constraints *constraints = rdev->constraints;
> +
> +	if (data->flags & PMBUS_OP_PROTECTED)
> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS;
> +
> +	if (data->flags & PMBUS_VOUT_PROTECTED)
> +		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS);
> +

I am a bit at loss trying to understand why the constraints can't be passed
with the regulator init_data when registering the regulator. Care to explain ?

Thanks,
Guenter