Re: [PATCH] mc13892: sanity check num_regulators parsed vs. registered
On Mon, Jan 21, 2013 at 12:25:45PM -0600, Matt Sealey wrote: > Imagine a situation where a device tree has a few regulators in an > appropriate node: Applied, thanks. Always use subject lines appropriate for the subsystem. signature.asc Description: Digital signature
Re: [PATCH] mc13892: sanity check num_regulators parsed vs. registered
On Mon, Jan 21, 2013 at 12:25:45PM -0600, Matt Sealey wrote: Imagine a situation where a device tree has a few regulators in an appropriate node: Applied, thanks. Always use subject lines appropriate for the subsystem. signature.asc Description: Digital signature
[PATCH] mc13892: sanity check num_regulators parsed vs. registered
Imagine a situation where a device tree has a few regulators in an
appropriate node:
regulators {
sw1 {
..
};
vvideo {
..
};
:
vfake {
..
};
vtypo {
..
};
};
In the above example, the node name "vfake" is an attempt to match a
regulator name inside the driver which just so happens to not exist. The
node name "vtypo" represents an accidental typographical error in a
regulator name which may have been introduced to a device tree.
In these cases, the number of regulators the mc13892 driver thinks it has
does not match the number of regulators it parsed and registered. Since
it will go over this array based on this number, it will actually
re-register regulator "0" (which happens to be SW1) over and over
again until it reaches the number, resulting in messages on the kernel
log such as these:
SW1: at 1100 mV
VVIDEO: at 2775mV
:
SW1: at 1100 mV
SW1: at 1100 mV
.. up to that number of "mismatched" regulators. Nobody using DT can/will
consume these regulators, so it should not be possible for it to cause any
real regulator problems or driver breakages, but it is an easy thing to
miss in a kernel log and is an immediate indication of a problem with the
device tree authoring.
This patch effectively sanity checks the number of counted children of
the regulators node vs. the number that actually matched driver names,
and sets the appropriate num_regulators value. It also gives a little
warning for device tree authors that they MAY have screwed something up,
such that this patch does not hide the device tree authoring problem.
Signed-off-by: Matt Sealey
Tested-by: Steev Klimaszewski
Cc: Shawn Guo
Cc: Fabio Estevam
---
drivers/regulator/mc13892-regulator.c | 39 ++--
drivers/regulator/mc13xxx-regulator-core.c | 10 +--
drivers/regulator/mc13xxx.h|4 +--
3 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/drivers/regulator/mc13892-regulator.c
b/drivers/regulator/mc13892-regulator.c
index e4f2197..7c86040 100644
--- a/drivers/regulator/mc13892-regulator.c
+++ b/drivers/regulator/mc13892-regulator.c
@@ -535,15 +535,18 @@ static int mc13892_regulator_probe(struct platform_device
*pdev)
struct mc13xxx_regulator_init_data *mc13xxx_data;
struct regulator_config config = { };
int i, ret;
- int num_regulators = 0;
+ int num_regulators = 0, num_parsed;
u32 val;
num_regulators = mc13xxx_get_num_regulators_dt(pdev);
+
if (num_regulators <= 0 && pdata)
num_regulators = pdata->num_regulators;
if (num_regulators <= 0)
return -EINVAL;
+ num_parsed = num_regulators;
+
priv = devm_kzalloc(>dev, sizeof(*priv) +
num_regulators * sizeof(priv->regulators[0]),
GFP_KERNEL);
@@ -586,7 +589,39 @@ static int mc13892_regulator_probe(struct platform_device
*pdev)
= mc13892_vcam_get_mode;
mc13xxx_data = mc13xxx_parse_regulators_dt(pdev, mc13892_regulators,
- ARRAY_SIZE(mc13892_regulators));
+ ARRAY_SIZE(mc13892_regulators),
+ _parsed);
+
+ /*
+* Perform a little sanity check on the regulator tree - if we found
+* a number of regulators from mc13xxx_get_num_regulators_dt and
+* then parsed a smaller number in mc13xxx_parse_regulators_dt then
+* there is a regulator defined in the regulators node which has
+* not matched any usable regulator in the driver. In this case,
+* there is one missing and what will happen is the first regulator
+* will get registered again.
+*
+* Fix this by basically making our number of registerable regulators
+* equal to the number of regulators we parsed. We are allocating
+* too much memory for priv, but this is unavoidable at this point.
+*
+* As an example of how this can happen, try making a typo in your
+* regulators node (vviohi {} instead of viohi {}) so that the name
+* does not match..
+*
+* The check will basically pass for platform data (non-DT) because
+* mc13xxx_parse_regulators_dt for !CONFIG_OF will not touch num_parsed.
+*
+*/
+ if (num_parsed != num_regulators) {
+ dev_warn(>dev,
+ "parsed %d != regulators %d - check your device tree!\n",
+ num_parsed, num_regulators);
+
+ num_regulators = num_parsed;
+ priv->num_regulators = num_regulators;
+ }
+
for (i = 0; i < num_regulators; i++) {
struct regulator_init_data *init_data;
struct regulator_desc *desc;
diff --git a/drivers/regulator/mc13xxx-regulator-core.c
[PATCH] mc13892: sanity check num_regulators parsed vs. registered
Imagine a situation where a device tree has a few regulators in an
appropriate node:
regulators {
sw1 {
..
};
vvideo {
..
};
:
vfake {
..
};
vtypo {
..
};
};
In the above example, the node name vfake is an attempt to match a
regulator name inside the driver which just so happens to not exist. The
node name vtypo represents an accidental typographical error in a
regulator name which may have been introduced to a device tree.
In these cases, the number of regulators the mc13892 driver thinks it has
does not match the number of regulators it parsed and registered. Since
it will go over this array based on this number, it will actually
re-register regulator 0 (which happens to be SW1) over and over
again until it reaches the number, resulting in messages on the kernel
log such as these:
SW1: at 1100 mV
VVIDEO: at 2775mV
:
SW1: at 1100 mV
SW1: at 1100 mV
.. up to that number of mismatched regulators. Nobody using DT can/will
consume these regulators, so it should not be possible for it to cause any
real regulator problems or driver breakages, but it is an easy thing to
miss in a kernel log and is an immediate indication of a problem with the
device tree authoring.
This patch effectively sanity checks the number of counted children of
the regulators node vs. the number that actually matched driver names,
and sets the appropriate num_regulators value. It also gives a little
warning for device tree authors that they MAY have screwed something up,
such that this patch does not hide the device tree authoring problem.
Signed-off-by: Matt Sealey m...@genesi-usa.com
Tested-by: Steev Klimaszewski st...@genesi-usa.com
Cc: Shawn Guo shawn@linaro.org
Cc: Fabio Estevam fabio.este...@freescale.com
---
drivers/regulator/mc13892-regulator.c | 39 ++--
drivers/regulator/mc13xxx-regulator-core.c | 10 +--
drivers/regulator/mc13xxx.h|4 +--
3 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/drivers/regulator/mc13892-regulator.c
b/drivers/regulator/mc13892-regulator.c
index e4f2197..7c86040 100644
--- a/drivers/regulator/mc13892-regulator.c
+++ b/drivers/regulator/mc13892-regulator.c
@@ -535,15 +535,18 @@ static int mc13892_regulator_probe(struct platform_device
*pdev)
struct mc13xxx_regulator_init_data *mc13xxx_data;
struct regulator_config config = { };
int i, ret;
- int num_regulators = 0;
+ int num_regulators = 0, num_parsed;
u32 val;
num_regulators = mc13xxx_get_num_regulators_dt(pdev);
+
if (num_regulators = 0 pdata)
num_regulators = pdata-num_regulators;
if (num_regulators = 0)
return -EINVAL;
+ num_parsed = num_regulators;
+
priv = devm_kzalloc(pdev-dev, sizeof(*priv) +
num_regulators * sizeof(priv-regulators[0]),
GFP_KERNEL);
@@ -586,7 +589,39 @@ static int mc13892_regulator_probe(struct platform_device
*pdev)
= mc13892_vcam_get_mode;
mc13xxx_data = mc13xxx_parse_regulators_dt(pdev, mc13892_regulators,
- ARRAY_SIZE(mc13892_regulators));
+ ARRAY_SIZE(mc13892_regulators),
+ num_parsed);
+
+ /*
+* Perform a little sanity check on the regulator tree - if we found
+* a number of regulators from mc13xxx_get_num_regulators_dt and
+* then parsed a smaller number in mc13xxx_parse_regulators_dt then
+* there is a regulator defined in the regulators node which has
+* not matched any usable regulator in the driver. In this case,
+* there is one missing and what will happen is the first regulator
+* will get registered again.
+*
+* Fix this by basically making our number of registerable regulators
+* equal to the number of regulators we parsed. We are allocating
+* too much memory for priv, but this is unavoidable at this point.
+*
+* As an example of how this can happen, try making a typo in your
+* regulators node (vviohi {} instead of viohi {}) so that the name
+* does not match..
+*
+* The check will basically pass for platform data (non-DT) because
+* mc13xxx_parse_regulators_dt for !CONFIG_OF will not touch num_parsed.
+*
+*/
+ if (num_parsed != num_regulators) {
+ dev_warn(pdev-dev,
+ parsed %d != regulators %d - check your device tree!\n,
+ num_parsed, num_regulators);
+
+ num_regulators = num_parsed;
+ priv-num_regulators = num_regulators;
+ }
+
for (i = 0; i num_regulators; i++) {
struct regulator_init_data *init_data;
struct
