| 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
| ||
|
Message-ID: <0fb5b5e4-ac23-01c9-a4c7-356db9ab0763@9elements.com>
Date: Fri, 18 Nov 2022 20:32:26 +0530
From: Naresh Solanki <naresh.solanki@...ements.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: linux-hwmon@...r.kernel.org, Jean Delvare <jdelvare@...e.com>,
Patrick Rudolph <patrick.rudolph@...ements.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] hwmon: pm_bus: core: Implement regulator get_status
Hi Guenter,
On 18-11-2022 05:40 pm, Guenter Roeck wrote:
> On Fri, Nov 18, 2022 at 09:46:36AM +0100, Naresh Solanki wrote:
>> From: Patrick Rudolph <patrick.rudolph@...ements.com>
>>
>> Add get_status for pmbus_regulator_ops.
>>
>> Signed-off-by: Patrick Rudolph <patrick.rudolph@...ements.com>
>> Signed-off-by: Naresh Solanki <Naresh.Solanki@...ements.com>
>> ---
>
> Please provide change logs.
Sure
>
>> drivers/hwmon/pmbus/pmbus_core.c | 72 ++++++++++++++++++++++++++++++++
>> 1 file changed, 72 insertions(+)
>>
>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>> index 7ec04934747e..5dde345c7679 100644
>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>> @@ -2851,6 +2851,77 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
>> return 0;
>> }
>>
>> +static int pmbus_regulator_get_status(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);
>> + u8 page = rdev_get_id(rdev);
>> + int status, status2;
>> +
>> + mutex_lock(&data->update_lock);
>> + status = pmbus_get_status(client, page, PMBUS_STATUS_WORD);
>> + mutex_unlock(&data->update_lock);
>
> I do not see the point of this lock here and elsewhere in this function.
> If you want to ensure that the status is consistent, you would need to
> hold the lock over the entire function, not repeatedly acquire and release
> it. Even then there would be no guarantee that the status is consistent
> because it can change anytime on the chip side.
Will hold the lock till end of function. Agree that chip side, status
may change any moment.
With this I got a question i.e., what should regulator status be
reported if regulator had previously encountered some fault but
currently while reading the status found that pgood bit indicate output
power is good.
Should we continue to still report historical fault or instead report
current regulator output status based on pgood ?
IMHO, it should report regulator status as ON if pgood bit is
clear(i.e., output is good) & leave previously encountered error
reporting to get_error_flag function.
I need your Suggestion here.
>
>> + if (status < 0)
>> + return status;
>> +
>> + if (status & (PB_STATUS_VIN_UV | PB_STATUS_IOUT_OC | PB_STATUS_VOUT_OV |
>> + PB_STATUS_UNKNOWN))
>
> Please align continuation lines with the matching '('.
>
Sure.
>> + return REGULATOR_STATUS_ERROR;
>> +
>> + if (status & (PB_STATUS_OFF | PB_STATUS_POWER_GOOD_N))
>> + return REGULATOR_STATUS_OFF;
>> +
>> + if (status & PB_STATUS_VOUT &&
>> + data->info->func[page] & PMBUS_HAVE_STATUS_VOUT) {
>> + mutex_lock(&data->update_lock);
>> + status2 = _pmbus_read_byte_data(client, page,
>> + PMBUS_STATUS_VOUT);
>> + mutex_unlock(&data->update_lock);
>> + if (status2 < 0)
>> + return status2;
>> + if (status2 & (PB_VOLTAGE_OV_FAULT | PB_VOLTAGE_UV_FAULT))
>> + return REGULATOR_STATUS_ERROR;
>> + }
>> + if (status & PB_STATUS_IOUT_POUT &&
>> + data->info->func[page] & PMBUS_HAVE_STATUS_IOUT) {
>> + mutex_lock(&data->update_lock);
>> + status2 = _pmbus_read_byte_data(client, page,
>> + PMBUS_STATUS_IOUT);
>
> Ok to avoid continuation lines as long as the result has less then 100
> columns.
>
Sure
>> + mutex_unlock(&data->update_lock);
>> + if (status2 < 0)
>> + return status2;
>> + if (status2 & (PB_POUT_OP_FAULT | PB_IOUT_UC_FAULT |
>> + PB_IOUT_OC_LV_FAULT | PB_IOUT_OC_FAULT))
>> + return REGULATOR_STATUS_ERROR;
>> + }
>> + if (status & PB_STATUS_INPUT &&
>> + data->info->func[page] & PMBUS_HAVE_STATUS_INPUT) {
>> + mutex_lock(&data->update_lock);
>> + status2 = _pmbus_read_byte_data(client, page,
>> + PMBUS_STATUS_INPUT);
>> + mutex_unlock(&data->update_lock);
>> + if (status2 < 0)
>> + return status2;
>> + if (status2 & (PB_IIN_OC_FAULT | PB_VOLTAGE_OV_FAULT |
>> + PB_VOLTAGE_UV_FAULT))
>> + return REGULATOR_STATUS_ERROR;
>> + }
>> + if (status & PB_STATUS_TEMPERATURE &&
>> + data->info->func[page] & PMBUS_HAVE_STATUS_TEMP) {
>> + mutex_lock(&data->update_lock);
>> + status2 = _pmbus_read_byte_data(client, page,
>> + PMBUS_STATUS_TEMPERATURE);
>> + mutex_unlock(&data->update_lock);
>> + if (status2 < 0)
>> + return status2;
>> + if (status2 & (PB_TEMP_UT_FAULT | PB_TEMP_OT_FAULT))
>> + return REGULATOR_STATUS_ERROR;
>> + }
>> +
>> + return REGULATOR_STATUS_ON;
>> +}
>> +
>> static int pmbus_regulator_get_low_margin(struct i2c_client *client, int page)
>> {
>> struct pmbus_data *data = i2c_get_clientdata(client);
>> @@ -2991,6 +3062,7 @@ const struct regulator_ops pmbus_regulator_ops = {
>> .disable = pmbus_regulator_disable,
>> .is_enabled = pmbus_regulator_is_enabled,
>> .get_error_flags = pmbus_regulator_get_error_flags,
>> + .get_status = pmbus_regulator_get_status,
>> .get_voltage = pmbus_regulator_get_voltage,
>> .set_voltage = pmbus_regulator_set_voltage,
>> .list_voltage = pmbus_regulator_list_voltage,
>>
>> base-commit: 27fea302952d8c90cafbdbee96bafeca03544401
>> --
>> 2.37.3
>>
Powered by blists - more mailing lists