[PATCH v2 1/3] pmbus_core: Refactor pmbus_is_enabled function

[PATCH v2 1/3] pmbus_core: Refactor pmbus_is_enabled function

[Naresh Solanki]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

Refactor the pmbus_is_enabled() function to return the raw status
without any additional processing as its already done in
_pmbus_is_enabled function.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index fa06325f5a7c..42fb7286805b 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2768,7 +2768,7 @@ static int __maybe_unused pmbus_is_enabled(struct device *dev, u8 page)
 	ret = _pmbus_is_enabled(dev, page);
 	mutex_unlock(&data->update_lock);
 
-	return !!(ret & PB_OPERATION_CONTROL_ON);
+	return ret;
 }
 
 #define to_dev_attr(_dev_attr) \

base-commit: 55612007f16b5d7b1fb83a7b0f5bb686829db7c7
-- 
2.41.0

[PATCH v2 2/3] pmbus_core: Fix NULL pointer dereference

[Naresh Solanki]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

Pass i2c_client to _pmbus_is_enabled to drop the assumption
that a regulator device is passed in.

This will fix the issue of a NULL pointer dereference when called from
_pmbus_get_flags.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
Changes in V2:
- Fix build error.
---
 drivers/hwmon/pmbus/pmbus_core.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 42fb7286805b..30aeb59062a5 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2745,9 +2745,8 @@ static const struct pmbus_status_category __maybe_unused pmbus_status_flag_map[]
 	},
 };
 
-static int _pmbus_is_enabled(struct device *dev, u8 page)
+static int _pmbus_is_enabled(struct i2c_client *client, u8 page)
 {
-	struct i2c_client *client = to_i2c_client(dev->parent);
 	int ret;
 
 	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
@@ -2758,14 +2757,13 @@ static int _pmbus_is_enabled(struct device *dev, u8 page)
 	return !!(ret & PB_OPERATION_CONTROL_ON);
 }
 
-static int __maybe_unused pmbus_is_enabled(struct device *dev, u8 page)
+static int __maybe_unused pmbus_is_enabled(struct i2c_client *client, u8 page)
 {
-	struct i2c_client *client = to_i2c_client(dev->parent);
 	struct pmbus_data *data = i2c_get_clientdata(client);
 	int ret;
 
 	mutex_lock(&data->update_lock);
-	ret = _pmbus_is_enabled(dev, page);
+	ret = _pmbus_is_enabled(client, page);
 	mutex_unlock(&data->update_lock);
 
 	return ret;
@@ -2844,7 +2842,7 @@ static int _pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *flag
 	if (status < 0)
 		return status;
 
-	if (_pmbus_is_enabled(dev, page)) {
+	if (_pmbus_is_enabled(client, page)) {
 		if (status & PB_STATUS_OFF) {
 			*flags |= REGULATOR_ERROR_FAIL;
 			*event |= REGULATOR_EVENT_FAIL;
@@ -2898,7 +2896,10 @@ static int __maybe_unused pmbus_get_flags(struct pmbus_data *data, u8 page, unsi
 #if IS_ENABLED(CONFIG_REGULATOR)
 static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
 {
-	return pmbus_is_enabled(rdev_get_dev(rdev), rdev_get_id(rdev));
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+
+	return pmbus_is_enabled(client, rdev_get_id(rdev));
 }
 
 static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable)
-- 
2.41.0

Re: [PATCH v2 2/3] pmbus_core: Fix NULL pointer dereference

[Guenter Roeck]

On Tue, Jul 25, 2023 at 02:54:26PM +0200, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> Pass i2c_client to _pmbus_is_enabled to drop the assumption
> that a regulator device is passed in.
> 
> This will fix the issue of a NULL pointer dereference when called from
> _pmbus_get_flags.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>

Applied.

Thanks,
Guenter

> ---
> Changes in V2:
> - Fix build error.
> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 42fb7286805b..30aeb59062a5 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2745,9 +2745,8 @@ static const struct pmbus_status_category __maybe_unused pmbus_status_flag_map[]
>  	},
>  };
>  
> -static int _pmbus_is_enabled(struct device *dev, u8 page)
> +static int _pmbus_is_enabled(struct i2c_client *client, u8 page)
>  {
> -	struct i2c_client *client = to_i2c_client(dev->parent);
>  	int ret;
>  
>  	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
> @@ -2758,14 +2757,13 @@ static int _pmbus_is_enabled(struct device *dev, u8 page)
>  	return !!(ret & PB_OPERATION_CONTROL_ON);
>  }
>  
> -static int __maybe_unused pmbus_is_enabled(struct device *dev, u8 page)
> +static int __maybe_unused pmbus_is_enabled(struct i2c_client *client, u8 page)
>  {
> -	struct i2c_client *client = to_i2c_client(dev->parent);
>  	struct pmbus_data *data = i2c_get_clientdata(client);
>  	int ret;
>  
>  	mutex_lock(&data->update_lock);
> -	ret = _pmbus_is_enabled(dev, page);
> +	ret = _pmbus_is_enabled(client, page);
>  	mutex_unlock(&data->update_lock);
>  
>  	return ret;
> @@ -2844,7 +2842,7 @@ static int _pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *flag
>  	if (status < 0)
>  		return status;
>  
> -	if (_pmbus_is_enabled(dev, page)) {
> +	if (_pmbus_is_enabled(client, page)) {
>  		if (status & PB_STATUS_OFF) {
>  			*flags |= REGULATOR_ERROR_FAIL;
>  			*event |= REGULATOR_EVENT_FAIL;
> @@ -2898,7 +2896,10 @@ static int __maybe_unused pmbus_get_flags(struct pmbus_data *data, u8 page, unsi
>  #if IS_ENABLED(CONFIG_REGULATOR)
>  static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
>  {
> -	return pmbus_is_enabled(rdev_get_dev(rdev), rdev_get_id(rdev));
> +	struct device *dev = rdev_get_dev(rdev);
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +
> +	return pmbus_is_enabled(client, rdev_get_id(rdev));
>  }
>  
>  static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable)

[PATCH v2 3/3] pmbus_core: Fix Deadlock

[Naresh Solanki]

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.

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);
 	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)
-- 
2.41.0

Re: [PATCH v2 3/3] pmbus_core: Fix Deadlock

[Guenter Roeck]

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)

Re: [PATCH v2 1/3] pmbus_core: Refactor pmbus_is_enabled function

[Guenter Roeck]

On Tue, Jul 25, 2023 at 02:54:25PM +0200, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> Refactor the pmbus_is_enabled() function to return the raw status
> without any additional processing as its already done in
> _pmbus_is_enabled function.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>

Applied. I rephrased the commit message so don't be surprised that
it looks different.

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> base-commit: 55612007f16b5d7b1fb83a7b0f5bb686829db7c7
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index fa06325f5a7c..42fb7286805b 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2768,7 +2768,7 @@ static int __maybe_unused pmbus_is_enabled(struct device *dev, u8 page)
>  	ret = _pmbus_is_enabled(dev, page);
>  	mutex_unlock(&data->update_lock);
>  
> -	return !!(ret & PB_OPERATION_CONTROL_ON);
> +	return ret;
>  }
>  
>  #define to_dev_attr(_dev_attr) \