| Message ID | 20230725125428.3966803-3-Naresh.Solanki@9elements.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/3] pmbus_core: Refactor pmbus_is_enabled function | expand |
On Tue, Jul 25, 2023 at 02:54:27PM +0200, Naresh Solanki wrote: > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > pmbus_regulator_get_error_flags() will also acquire the update_lock, > thus unlock the mutex before trying to lock it again from within > the same thread. > > Fixes a deadlock when trying to read the regulator status. > The idea was that the lock would cover both pmbus_get_status() and pmbus_regulator_get_error_flags() to avoid inconsistencies. This change defeats that purpose. > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > --- > drivers/hwmon/pmbus/pmbus_core.c | 30 ++++++++++-------------------- > 1 file changed, 10 insertions(+), 20 deletions(-) > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > index 30aeb59062a5..42f4250c53ed 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -2949,37 +2949,27 @@ static int pmbus_regulator_get_status(struct regulator_dev *rdev) > > mutex_lock(&data->update_lock); > status = pmbus_get_status(client, page, PMBUS_STATUS_WORD); > - if (status < 0) { > - ret = status; > - goto unlock; > - } > + mutex_unlock(&data->update_lock); > + if (status < 0) > + return status; > > - if (status & PB_STATUS_OFF) { > - ret = REGULATOR_STATUS_OFF; > - goto unlock; > - } > + if (status & PB_STATUS_OFF) > + return REGULATOR_STATUS_OFF; > > /* If regulator is ON & reports power good then return ON */ > - if (!(status & PB_STATUS_POWER_GOOD_N)) { > - ret = REGULATOR_STATUS_ON; > - goto unlock; > - } > + if (!(status & PB_STATUS_POWER_GOOD_N)) > + return REGULATOR_STATUS_ON; > > ret = pmbus_regulator_get_error_flags(rdev, &status); Why not just call _pmbus_get_flags() here instead ? Guenter > if (ret) > - goto unlock; > + return ret; > > if (status & (REGULATOR_ERROR_UNDER_VOLTAGE | REGULATOR_ERROR_OVER_CURRENT | > REGULATOR_ERROR_REGULATION_OUT | REGULATOR_ERROR_FAIL | REGULATOR_ERROR_OVER_TEMP)) { > - ret = REGULATOR_STATUS_ERROR; > - goto unlock; > + return REGULATOR_STATUS_ERROR; > } > > - ret = REGULATOR_STATUS_UNDEFINED; > - > -unlock: > - mutex_unlock(&data->update_lock); > - return ret; > + return REGULATOR_STATUS_UNDEFINED; > } > > static int pmbus_regulator_get_low_margin(struct i2c_client *client, int page)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index 30aeb59062a5..42f4250c53ed 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -2949,37 +2949,27 @@ static int pmbus_regulator_get_status(struct regulator_dev *rdev) mutex_lock(&data->update_lock); status = pmbus_get_status(client, page, PMBUS_STATUS_WORD); - if (status < 0) { - ret = status; - goto unlock; - } + mutex_unlock(&data->update_lock); + if (status < 0) + return status; - if (status & PB_STATUS_OFF) { - ret = REGULATOR_STATUS_OFF; - goto unlock; - } + if (status & PB_STATUS_OFF) + return REGULATOR_STATUS_OFF; /* If regulator is ON & reports power good then return ON */ - if (!(status & PB_STATUS_POWER_GOOD_N)) { - ret = REGULATOR_STATUS_ON; - goto unlock; - } + if (!(status & PB_STATUS_POWER_GOOD_N)) + return REGULATOR_STATUS_ON; ret = pmbus_regulator_get_error_flags(rdev, &status); if (ret) - goto unlock; + return ret; if (status & (REGULATOR_ERROR_UNDER_VOLTAGE | REGULATOR_ERROR_OVER_CURRENT | REGULATOR_ERROR_REGULATION_OUT | REGULATOR_ERROR_FAIL | REGULATOR_ERROR_OVER_TEMP)) { - ret = REGULATOR_STATUS_ERROR; - goto unlock; + return REGULATOR_STATUS_ERROR; } - ret = REGULATOR_STATUS_UNDEFINED; - -unlock: - mutex_unlock(&data->update_lock); - return ret; + return REGULATOR_STATUS_UNDEFINED; } static int pmbus_regulator_get_low_margin(struct i2c_client *client, int page)