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

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

Hi Alex,

On 10/17/24 4:56 PM, Alexander Dahl wrote:
> Hello Quentin,
> 
> Am Tue, Oct 15, 2024 at 04:32:14PM +0200 schrieb Quentin Schulz:
>> 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(-)
>>
>> diff --git a/drivers/core/of_access.c b/drivers/core/of_access.c
>> index d05be273e7bbb68c3ad82ef4c1c036ae7f68ae61..77acd76626257b6da95a27d107052ff8800c2b67 100644
>> --- a/drivers/core/of_access.c
>> +++ b/drivers/core/of_access.c
>> @@ -490,17 +490,17 @@ int of_read_u8(const struct device_node *np, const char *propname, u8 *outp)
>>   {
>>   	const u8 *val;
>>   
>> -	dm_warn("%s: %s: ", __func__, propname);
>> +	log_debug("%s: %s: ", __func__, propname);
> 
> Printing __func__ when using log_* functions, is redundant, isn't it?
> You can enabling printing the function name through the logging
> framework, right?
> 

Only if LOGF_FUNC symbol is enabled, if I understood correctly. Not an 
excuse but:

$ git grep -o "log.*__func__" | wc -l
202

So there are a "few" other places doing that. Will let Simon decide on 
that one, no personal opinion.

>>   	if (!np)
>>   		return -EINVAL;
>>   	val = of_find_property_value_of_size(np, propname, sizeof(*outp));
>>   	if (IS_ERR(val)) {
>> -		dm_warn("(not found)\n");
>> +		log_debug("(not found)\n");
>>   		return PTR_ERR(val);
> 
> What about using log_msg_ret() instead in these cases?
> 

log_msg_ret will log with LOGL_ERR and not LOGL_DEBUG if 
LOG_ERROR_RETURN symbol is enabled, otherwise it'll simply not be 
printed. It's a change of behavior/expectation here.

If we go this route we should at least make this a bit more useful by 
adding the propname to the error message since it would be printed with 
log_debug() at the beginning of the function, and the log level wouldn't 
match. Also, this means that enabling debug log level but not enabling 
LOG_ERROR_RETURN would basically print the first log_debug() and nothing 
else in case it fails. A choice to be made but it's a bit more complex 
than the one above.

Cheers,
Quentin