[RFC PATCH] drivers: regulator: add error checks for exported APIs

[RFC PATCH] drivers: regulator: add error checks for exported APIs

[Kishon Vijay Abraham I]

Added error checks for exported APIs in regulator framework so that
incorrect use of these APIs does not result in a crash.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
compile tested omap2plus_defconfig

 drivers/regulator/core.c |  119 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 104 insertions(+), 15 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f092588..5e88bb2 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1506,12 +1506,17 @@ static int _regulator_enable(struct regulator_dev *rdev)
  */
 int regulator_enable(struct regulator *regulator)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int ret = 0;
 
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
 	if (regulator->always_on)
 		return 0;
 
+	rdev = regulator->rdev;
+
 	if (rdev->supply) {
 		ret = regulator_enable(rdev->supply);
 		if (ret != 0)
@@ -1603,12 +1608,17 @@ static int _regulator_disable(struct regulator_dev *rdev)
  */
 int regulator_disable(struct regulator *regulator)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int ret = 0;
 
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
 	if (regulator->always_on)
 		return 0;
 
+	rdev = regulator->rdev;
+
 	mutex_lock(&rdev->mutex);
 	ret = _regulator_disable(rdev);
 	mutex_unlock(&rdev->mutex);
@@ -1652,9 +1662,13 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
  */
 int regulator_force_disable(struct regulator *regulator)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int ret;
 
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
+	rdev = regulator->rdev;
 	mutex_lock(&rdev->mutex);
 	regulator->uA_load = 0;
 	ret = _regulator_force_disable(regulator->rdev);
@@ -1714,12 +1728,16 @@ static void regulator_disable_work(struct work_struct *work)
  */
 int regulator_disable_deferred(struct regulator *regulator, int ms)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int ret;
 
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
 	if (regulator->always_on)
 		return 0;
 
+	rdev = regulator->rdev;
 	mutex_lock(&rdev->mutex);
 	rdev->deferred_disables++;
 	mutex_unlock(&rdev->mutex);
@@ -1817,6 +1835,9 @@ int regulator_is_enabled(struct regulator *regulator)
 {
 	int ret;
 
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
 	if (regulator->always_on)
 		return 1;
 
@@ -1838,7 +1859,12 @@ EXPORT_SYMBOL_GPL(regulator_is_enabled);
  */
 int regulator_count_voltages(struct regulator *regulator)
 {
-	struct regulator_dev	*rdev = regulator->rdev;
+	struct regulator_dev	*rdev;
+
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
+	rdev = regulator->rdev;
 
 	return rdev->desc->n_voltages ? : -EINVAL;
 }
@@ -1901,10 +1927,16 @@ EXPORT_SYMBOL_GPL(regulator_list_voltage_table);
  */
 int regulator_list_voltage(struct regulator *regulator, unsigned selector)
 {
-	struct regulator_dev	*rdev = regulator->rdev;
-	struct regulator_ops	*ops = rdev->desc->ops;
+	struct regulator_dev	*rdev;
+	struct regulator_ops	*ops;
 	int			ret;
 
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
+	rdev = regulator->rdev;
+	ops = rdev->desc->ops;
+
 	if (!ops->list_voltage || selector >= rdev->desc->n_voltages)
 		return -EINVAL;
 
@@ -1935,9 +1967,14 @@ EXPORT_SYMBOL_GPL(regulator_list_voltage);
 int regulator_is_supported_voltage(struct regulator *regulator,
 				   int min_uV, int max_uV)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int i, voltages, ret;
 
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
+	rdev = regulator->rdev;
+
 	/* If we can't change voltage check the current voltage */
 	if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)) {
 		ret = regulator_get_voltage(regulator);
@@ -2207,9 +2244,14 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
  */
 int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int ret = 0;
 
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
+	rdev = regulator->rdev;
+
 	mutex_lock(&rdev->mutex);
 
 	/* If we're setting the same range as last time the change
@@ -2258,13 +2300,19 @@ EXPORT_SYMBOL_GPL(regulator_set_voltage);
 int regulator_set_voltage_time(struct regulator *regulator,
 			       int old_uV, int new_uV)
 {
-	struct regulator_dev	*rdev = regulator->rdev;
-	struct regulator_ops	*ops = rdev->desc->ops;
+	struct regulator_dev	*rdev;
+	struct regulator_ops	*ops;
 	int old_sel = -1;
 	int new_sel = -1;
 	int voltage;
 	int i;
 
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
+	rdev = regulator->rdev;
+	ops = rdev->desc->ops;
+
 	/* Currently requires operations to do this */
 	if (!ops->list_voltage || !ops->set_voltage_time_sel
 	    || !rdev->desc->n_voltages)
@@ -2340,9 +2388,14 @@ EXPORT_SYMBOL_GPL(regulator_set_voltage_time_sel);
  */
 int regulator_sync_voltage(struct regulator *regulator)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int ret, min_uV, max_uV;
 
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
+	rdev = regulator->rdev;
+
 	mutex_lock(&rdev->mutex);
 
 	if (!rdev->desc->ops->set_voltage &&
@@ -2410,6 +2463,9 @@ int regulator_get_voltage(struct regulator *regulator)
 {
 	int ret;
 
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
 	mutex_lock(&regulator->rdev->mutex);
 
 	ret = _regulator_get_voltage(regulator->rdev);
@@ -2439,9 +2495,14 @@ EXPORT_SYMBOL_GPL(regulator_get_voltage);
 int regulator_set_current_limit(struct regulator *regulator,
 			       int min_uA, int max_uA)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int ret;
 
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
+	rdev = regulator->rdev;
+
 	mutex_lock(&rdev->mutex);
 
 	/* sanity check */
@@ -2491,6 +2552,9 @@ out:
  */
 int regulator_get_current_limit(struct regulator *regulator)
 {
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
 	return _regulator_get_current_limit(regulator->rdev);
 }
 EXPORT_SYMBOL_GPL(regulator_get_current_limit);
@@ -2508,10 +2572,15 @@ EXPORT_SYMBOL_GPL(regulator_get_current_limit);
  */
 int regulator_set_mode(struct regulator *regulator, unsigned int mode)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int ret;
 	int regulator_curr_mode;
 
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
+	rdev = regulator->rdev;
+
 	mutex_lock(&rdev->mutex);
 
 	/* sanity check */
@@ -2567,6 +2636,9 @@ out:
  */
 unsigned int regulator_get_mode(struct regulator *regulator)
 {
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
 	return _regulator_get_mode(regulator->rdev);
 }
 EXPORT_SYMBOL_GPL(regulator_get_mode);
@@ -2599,11 +2671,16 @@ EXPORT_SYMBOL_GPL(regulator_get_mode);
  */
 int regulator_set_optimum_mode(struct regulator *regulator, int uA_load)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	struct regulator *consumer;
 	int ret, output_uV, input_uV = 0, total_uA_load = 0;
 	unsigned int mode;
 
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
+	rdev = regulator->rdev;
+
 	if (rdev->supply)
 		input_uV = regulator_get_voltage(rdev->supply);
 
@@ -2683,6 +2760,9 @@ EXPORT_SYMBOL_GPL(regulator_set_optimum_mode);
 int regulator_register_notifier(struct regulator *regulator,
 			      struct notifier_block *nb)
 {
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
 	return blocking_notifier_chain_register(&regulator->rdev->notifier,
 						nb);
 }
@@ -2698,6 +2778,9 @@ EXPORT_SYMBOL_GPL(regulator_register_notifier);
 int regulator_unregister_notifier(struct regulator *regulator,
 				struct notifier_block *nb)
 {
+	if (IS_ERR_OR_NULL(regulator))
+		return -ENODEV;
+
 	return blocking_notifier_chain_unregister(&regulator->rdev->notifier,
 						  nb);
 }
@@ -3477,6 +3560,9 @@ EXPORT_SYMBOL_GPL(rdev_get_drvdata);
  */
 void *regulator_get_drvdata(struct regulator *regulator)
 {
+	if (IS_ERR_OR_NULL(regulator))
+		return NULL;
+
 	return regulator->rdev->reg_data;
 }
 EXPORT_SYMBOL_GPL(regulator_get_drvdata);
@@ -3488,6 +3574,9 @@ EXPORT_SYMBOL_GPL(regulator_get_drvdata);
  */
 void regulator_set_drvdata(struct regulator *regulator, void *data)
 {
+	if (IS_ERR_OR_NULL(regulator))
+		return;
+
 	regulator->rdev->reg_data = data;
 }
 EXPORT_SYMBOL_GPL(regulator_set_drvdata);
-- 
1.7.9.5

Re: [RFC PATCH] drivers: regulator: add error checks for exported APIs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 923 bytes --]

On Thu, Jul 26, 2012 at 04:00:03PM +0530, Kishon Vijay Abraham I wrote:

> Added error checks for exported APIs in regulator framework so that
> incorrect use of these APIs does not result in a crash.

The commit (especially the subject) should be more descriptive, it's not
that we're missing error checks it's that you're handling garbage passed
in from the user a bit more gracefully.

I'm not sure how useful this is as-is -  if you're not checking your
errors you're not checking your errors so it's probably just pushing the
robustness failures around, especially since...

> +	if (IS_ERR_OR_NULL(regulator))
> +		return -ENODEV;

...this is too quiet, this should at the very least be generating a loud
warning so people can see there's a problem (probably with WARN_ON so we
can see what called it).  Can you respin with something noisier please?

Also NULL is a valid regulator (though we don't use it right now).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH] drivers: regulator: add error checks for exported APIs

[ABRAHAM, KISHON VIJAY]

Hi,

On Thu, Jul 26, 2012 at 4:40 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Jul 26, 2012 at 04:00:03PM +0530, Kishon Vijay Abraham I wrote:
>
>> Added error checks for exported APIs in regulator framework so that
>> incorrect use of these APIs does not result in a crash.
>
> The commit (especially the subject) should be more descriptive, it's not
> that we're missing error checks it's that you're handling garbage passed
> in from the user a bit more gracefully.
>
> I'm not sure how useful this is as-is -  if you're not checking your
> errors you're not checking your errors so it's probably just pushing the
> robustness failures around, especially since...
>
>> +     if (IS_ERR_OR_NULL(regulator))
>> +             return -ENODEV;
>
> ...this is too quiet, this should at the very least be generating a loud
> warning so people can see there's a problem (probably with WARN_ON so we
> can see what called it).  Can you respin with something noisier please?

Sure. I'll respin a new version.
>
> Also NULL is a valid regulator (though we don't use it right now).
I see a lot of dereferencing *regulator*. How is it supposed to work
when *regulator* is NULL?

Thanks
Kishon

Re: [RFC PATCH] drivers: regulator: add error checks for exported APIs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 398 bytes --]

On Thu, Jul 26, 2012 at 04:49:21PM +0530, ABRAHAM, KISHON VIJAY wrote:
> On Thu, Jul 26, 2012 at 4:40 PM, Mark Brown

> > Also NULL is a valid regulator (though we don't use it right now).

> I see a lot of dereferencing *regulator*. How is it supposed to work
> when *regulator* is NULL?

Like I say we don't use this now but the spec in the API is that
anything except an error pointer is valid.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH] drivers: regulator: add error checks for exported APIs

[ABRAHAM, KISHON VIJAY]

On Thu, Jul 26, 2012 at 4:52 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Jul 26, 2012 at 04:49:21PM +0530, ABRAHAM, KISHON VIJAY wrote:
>> On Thu, Jul 26, 2012 at 4:40 PM, Mark Brown
>
>> > Also NULL is a valid regulator (though we don't use it right now).
>
>> I see a lot of dereferencing *regulator*. How is it supposed to work
>> when *regulator* is NULL?
>
> Like I say we don't use this now but the spec in the API is that
> anything except an error pointer is valid.

Ok.

Thanks
Kishon