Re: [PATCH] dm: core: downgrade some dm_warn messages to log_debug()

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Simon,

On 10/27/24 6:16 PM, Simon Glass wrote:
> Hi Quentin,
> 
> On Tue, 15 Oct 2024 at 16:32, Quentin Schulz <foss+uboot@0leil.net> wrote:
>>
>> From: Quentin Schulz <quentin.schulz@cherry.de>
>>
>> People complained that enabling (SPL_)DM_WARN was now totally unusable
>> due to the amount of messages printed on the console.
>>
>> Let's downgrade the log level of some messages that are clearly not on
>> the error path.
>>
>> Note that there's one pr_debug in there, because it is followed by
>> pr_cont so it made sense to reuse the same family of functions.
>>
>> Reported-by: Alexander Dahl <ada@thorsis.com>
>> Fixes: 6afdb1585112 ("dm: core: migrate debug() messages to use dm_warn")
>> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
>> ---
>> Note that I am not entirely sure about the "not found" and "not large
>> enough" changes there.
>>
>> Another note, %#x isn't handled by tinyprintf so it just prints "x"
>> instead of the value.
>>
>> Finally, I don't know how one can enable LOG_DEBUG level without
>> enabling DEBUG which enables assert() so I just tested that by removing
>> the #define DEBUG in include/log.h :)
>> ---
>>   drivers/core/of_access.c | 36 ++++++++++++-------------
>>   drivers/core/of_addr.c   | 26 +++++++++---------
>>   drivers/core/of_extra.c  |  6 ++---
>>   drivers/core/ofnode.c    | 68 ++++++++++++++++++++++++------------------------
>>   4 files changed, 68 insertions(+), 68 deletions(-)
> 
> This is an improvement, so:
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> For -EOVERFLOW I think warning would be better, but in that case you
> need to print a prefix, since otherwise the warning will have nothing
> before it. So it is a little tricky. I can imagine some support in the
> log module to help with this, although I haven't thought about it too
> hard.
> 

Agreed. We could also simply print the propname in the dm_warn() call 
and keep the one at the top of the function to use dm_debug() instead.

> Also, we should really remove __func__ when adding logging, since
> people can enable the option if they want function names to be stored.
> 

But then we'll have a bunch of functions printing one of their 
parameters and "(not found)" for example. A bit odd to me. But on the 
theoretical level, I agree, enabling the appropriate symbol would 
already add that line, it's just that I see limit use of those messages 
if it's not set.

Thanks for the review!

Cheers,
Quentin