Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write protected regulators

From: Jerome Brunet
Date: Sat Sep 21 2024 - 07:32:25 EST

On Fri 20 Sep 2024 at 14:13, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:

> 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@xxxxxxxxxxxx>
>> ---
>> 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 ?

Sure it something I found while working the problem out.

Simply put:
* you should be able to, in theory.
* currently it would not work
* when fixed I think it would still be more complex to do so.

ATM, if you pass init_data, it will be ignored on DT platforms in
favor of the internal DT parsing of the regulator framework. The DT
parsing sets REGULATOR_CHANGE_STATUS as long as the always-on prop is
not set ... including for write protected regulator obviously.

This is something that might get fix with this change [1]. Even with that
fixed, passing init_data systematically would be convenient only if you
plan on skipping DT provided constraints (there are lot of those), or
redo the parsing in PMBus.

Also a callback can be attached to regulator using the pmbus_ops, and
only those. PMBus core just collect regulators provided by the drivers
in pmbus_regulator_register(), there could other type of regulators in
the provided table (something the tps25990 could use). It is difficult
to set/modify the init_data (or the ops) in pmbus_regulator_register()
because of that.

Using a callback allows to changes almost nothing to what is currently
done, so it is safe and address the problem regardless of the
platform type. I think the solution is fairly simple for both regulator
and pmbus. It could be viewed as just as extending an existing
callback. I chose to replace it make things more clear.

Changes [1] and this patchset are independent because of how I implement
the solution and [1] would still be relevant if this patchset was
rejected. It is the reason why I sent it separately.

[1]: https://lore.kernel.org/r/20240920-regulator-ignored-data-v1-1-7ea4abfe1b0a@xxxxxxxxxxxx

>
> Thanks,
> Guenter

--
Jerome