[PATCH] regulator: of: Lower the severity of the error with no container

[PATCH] regulator: of: Lower the severity of the error with no container

[Mark Brown]

Description of regulators should generally be optional so if there is no
DT node for the regulators container then we shouldn't print an error
message. Lower the severity of the message to debug level (it might help
someone work out what went wrong) and while we're at it say what we were
looking for.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/of_regulator.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 7a51814abdc5..5a1d4afa4776 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -211,7 +211,8 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 		search = dev->of_node;
 
 	if (!search) {
-		dev_err(dev, "Failed to find regulator container node\n");
+		dev_dbg(dev, "Failed to find regulator container node '%s'\n",
+			desc->regulators_node);
 		return NULL;
 	}
 
-- 
2.1.1

Re: [PATCH] regulator: of: Lower the severity of the error with no container

[Guenter Roeck]

On Wed, Oct 08, 2014 at 11:09:59PM +0100, Mark Brown wrote:
> Description of regulators should generally be optional so if there is no
> DT node for the regulators container then we shouldn't print an error
> message. Lower the severity of the message to debug level (it might help
> someone work out what went wrong) and while we're at it say what we were
> looking for.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Mark Brown <broonie@kernel.org>

Excellent.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Would it also be possible to lower the severity of the "no parameters"
message ?

Thanks,
Guenter

> ---
>  drivers/regulator/of_regulator.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
> index 7a51814abdc5..5a1d4afa4776 100644
> --- a/drivers/regulator/of_regulator.c
> +++ b/drivers/regulator/of_regulator.c
> @@ -211,7 +211,8 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
>  		search = dev->of_node;
>  
>  	if (!search) {
> -		dev_err(dev, "Failed to find regulator container node\n");
> +		dev_dbg(dev, "Failed to find regulator container node '%s'\n",
> +			desc->regulators_node);
>  		return NULL;
>  	}
>  
> -- 
> 2.1.1
> 

Re: [PATCH] regulator: of: Lower the severity of the error with no container

[Mark Brown]

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

On Wed, Oct 08, 2014 at 03:34:03PM -0700, Guenter Roeck wrote:

> Would it also be possible to lower the severity of the "no parameters"
> message ?

Could you be more specific please?

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

Re: [PATCH] regulator: of: Lower the severity of the error with no container

[Guenter Roeck]

On Wed, Oct 08, 2014 at 11:36:01PM +0100, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 03:34:03PM -0700, Guenter Roeck wrote:
> 
> > Would it also be possible to lower the severity of the "no parameters"
> > message ?
> 
> Could you be more specific please?

There is a log message "no parameters" for each regulator. This is printed
unconditionally from print_constraints().

Looking through the code again, looks like this is on purpose. It is just a bit
annoying to get lots of those messages. One of the systems I am dealing with has
17 LTC2978 chips in it, with 8 channels each. That results in 136 times "no
parameters" in the boot log. And that is not even a fully populated system;
if fully populated, there can be more than 60 of those chips. 500+ lines of 
similar log messages is really a bit on the high side.

It might help if there was a way to silence the messages, ie to make
"print_constraints" optional. 

Thanks,
Guenter

Re: [PATCH] regulator: of: Lower the severity of the error with no container

[Mark Brown]

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

On Wed, Oct 08, 2014 at 03:59:12PM -0700, Guenter Roeck wrote:

> There is a log message "no parameters" for each regulator. This is printed
> unconditionally from print_constraints().

> Looking through the code again, looks like this is on purpose. It is just a bit
> annoying to get lots of those messages. One of the systems I am dealing with has
> 17 LTC2978 chips in it, with 8 channels each. That results in 136 times "no
> parameters" in the boot log. And that is not even a fully populated system;
> if fully populated, there can be more than 60 of those chips. 500+ lines of 
> similar log messages is really a bit on the high side.

> It might help if there was a way to silence the messages, ie to make
> "print_constraints" optional. 

Ah, from the constraints rather than from the DT parsing.  I do like
having it there since it's enormously helpful in debugging and that is
a...  specialist number of regulators you have in your system.  We can
definitely at least add a boot argument or something to suppress them,
let me have a think if we want to do that by default.

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

Re: [PATCH] regulator: of: Lower the severity of the error with no container

[Guenter Roeck]

On Thu, Oct 09, 2014 at 12:45:41AM +0100, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 03:59:12PM -0700, Guenter Roeck wrote:
> 
> > There is a log message "no parameters" for each regulator. This is printed
> > unconditionally from print_constraints().
> 
> > Looking through the code again, looks like this is on purpose. It is just a bit
> > annoying to get lots of those messages. One of the systems I am dealing with has
> > 17 LTC2978 chips in it, with 8 channels each. That results in 136 times "no
> > parameters" in the boot log. And that is not even a fully populated system;
> > if fully populated, there can be more than 60 of those chips. 500+ lines of 
> > similar log messages is really a bit on the high side.
> 
> > It might help if there was a way to silence the messages, ie to make
> > "print_constraints" optional. 
> 
> Ah, from the constraints rather than from the DT parsing.  I do like
> having it there since it's enormously helpful in debugging and that is
> a...  specialist number of regulators you have in your system.  We can

Yes, this is a pretty large backbone switch. Kind of amazing how many
sensors are in those systems.

> definitely at least add a boot argument or something to suppress them,
> let me have a think if we want to do that by default.

It is a nuisance, so I might just disable it in our tree if we don't
find some other solution.

Did you notice the problem with debugfs I had mentioned earlier ?
With all those regulators, not all of them being used, I end up with
many having the same name. This causes issues with debugfs, which is
trying to create the same file several times.

Any idea how we could solve this ? The constraints message is annoying,
but this one is a real issue.

Thanks,
Guenter

Re: [PATCH] regulator: of: Lower the severity of the error with no container

[Mark Brown]

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

On Wed, Oct 08, 2014 at 05:05:55PM -0700, Guenter Roeck wrote:
> On Thu, Oct 09, 2014 at 12:45:41AM +0100, Mark Brown wrote:

> > definitely at least add a boot argument or something to suppress them,
> > let me have a think if we want to do that by default.

> It is a nuisance, so I might just disable it in our tree if we don't
> find some other solution.

We'll do something, just a question of what and what the default is.

> Did you notice the problem with debugfs I had mentioned earlier ?
> With all those regulators, not all of them being used, I end up with
> many having the same name. This causes issues with debugfs, which is
> trying to create the same file several times.

> Any idea how we could solve this ? The constraints message is annoying,
> but this one is a real issue.

Shove a dev_name() on the front if we get a collision?  I have to say
I've never cared, the debugfs isn't that important so it doesn't matter
too much if it fails.

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

Re: [PATCH] regulator: of: Lower the severity of the error with no container

[Guenter Roeck]

On Thu, Oct 09, 2014 at 01:12:13AM +0100, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 05:05:55PM -0700, Guenter Roeck wrote:
> > On Thu, Oct 09, 2014 at 12:45:41AM +0100, Mark Brown wrote:
> 
> > > definitely at least add a boot argument or something to suppress them,
> > > let me have a think if we want to do that by default.
> 
> > It is a nuisance, so I might just disable it in our tree if we don't
> > find some other solution.
> 
> We'll do something, just a question of what and what the default is.
> 
Ok. Note that a boot parameter would not work well for our use case,
so it would be great if we can find something else.

> > Did you notice the problem with debugfs I had mentioned earlier ?
> > With all those regulators, not all of them being used, I end up with
> > many having the same name. This causes issues with debugfs, which is
> > trying to create the same file several times.
> 
> > Any idea how we could solve this ? The constraints message is annoying,
> > but this one is a real issue.
> 
> Shove a dev_name() on the front if we get a collision?  I have to say
> I've never cared, the debugfs isn't that important so it doesn't matter
> too much if it fails.

Sure, but, again, I am getting lots and lots of those error messages.
I probably would not care either (and probably not even have noticed)
if not for those messages.

Want me to submit a patch with the dev_name solution ?

Thanks,
Guenter

Re: [PATCH] regulator: of: Lower the severity of the error with no container

[Mark Brown]

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

On Wed, Oct 08, 2014 at 05:25:31PM -0700, Guenter Roeck wrote:
> On Thu, Oct 09, 2014 at 01:12:13AM +0100, Mark Brown wrote:

> > We'll do something, just a question of what and what the default is.

> Ok. Note that a boot parameter would not work well for our use case,
> so it would be great if we can find something else.

Could you explain why please?

> > Shove a dev_name() on the front if we get a collision?  I have to say
> > I've never cared, the debugfs isn't that important so it doesn't matter
> > too much if it fails.

> Sure, but, again, I am getting lots and lots of those error messages.
> I probably would not care either (and probably not even have noticed)
> if not for those messages.

> Want me to submit a patch with the dev_name solution ?

Yes, please.

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

Re: [PATCH] regulator: of: Lower the severity of the error with no container

[Guenter Roeck]

On Thu, Oct 09, 2014 at 04:54:40PM +0100, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 05:25:31PM -0700, Guenter Roeck wrote:
> > On Thu, Oct 09, 2014 at 01:12:13AM +0100, Mark Brown wrote:
> 
> > > We'll do something, just a question of what and what the default is.
> 
> > Ok. Note that a boot parameter would not work well for our use case,
> > so it would be great if we can find something else.
> 
> Could you explain why please?
> 
Some of the system are loaded from u-boot. We can technically change the
environment, but that would not be persistent. Product requirement is that the
default (hard-coded) environment has to be the one that is used. And changing
u-boot in those systems is more difficult than getting an audience with the
Pope - believe me, we went through that. Unless there is a fatal problem,
it simply won't be approved.

On x86 systems, which are booted through grub, we have a similar problem.
The boot menu is secured and for all practical purposes untouchable.

All that makes it much simpler to carry a one-line patch to remove the output
from the log. I may try to do without it and keep the message, but I am quite
sure that someone will complain and we'll have to do it.

> > > Shove a dev_name() on the front if we get a collision?  I have to say
> > > I've never cared, the debugfs isn't that important so it doesn't matter
> > > too much if it fails.
> 
> > Sure, but, again, I am getting lots and lots of those error messages.
> > I probably would not care either (and probably not even have noticed)
> > if not for those messages.
> 
> > Want me to submit a patch with the dev_name solution ?
> 
> Yes, please.

Ok, will do.

Thanks,
Guenter

Re: [PATCH] regulator: of: Lower the severity of the error with no container

[Guenter Roeck]

On Thu, Oct 09, 2014 at 04:54:40PM +0100, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 05:25:31PM -0700, Guenter Roeck wrote:
> 
> > > Shove a dev_name() on the front if we get a collision?  I have to say
> > > I've never cared, the debugfs isn't that important so it doesn't matter
> > > too much if it fails.
> 
> > Sure, but, again, I am getting lots and lots of those error messages.
> > I probably would not care either (and probably not even have noticed)
> > if not for those messages.
> 
> > Want me to submit a patch with the dev_name solution ?
> 
> Yes, please.

Turns out that is already supposed to happen if the function creating
the entry knows about the device. I guess this explains why others don't
see the problem. We'll have to find out what is going on.

Guenter