Re: [PROBLEM linux-next]

[Mirsad Todorovac <mtodorovac69@gmail.com>]

Hi, Jonas, Rafał, all,

On 7/7/24 16:09, Mirsad Todorovac wrote:
> 
> 
> On 7/7/24 10:12, Jonas Gorski wrote:
>> On Sun, 7 Jul 2024 at 09:38, Rafał Miłecki <rafal@milecki.pl> wrote:
>>>
>>> Some more descriptive subject would be nice :)
>>>
>>>
>>> On 7.07.2024 02:10, Mirsad Todorovac wrote:
>>>> In file included from ./include/asm-generic/bug.h:22,
>>>>                   from ./arch/x86/include/asm/bug.h:87,
>>>>                   from ./include/linux/bug.h:5,
>>>>                   from ./include/linux/fortify-string.h:6,
>>>>                   from ./include/linux/string.h:374,
>>>>                   from ./arch/x86/include/asm/page_32.h:18,
>>>>                   from ./arch/x86/include/asm/page.h:14,
>>>>                   from ./arch/x86/include/asm/processor.h:20,
>>>>                   from ./arch/x86/include/asm/timex.h:5,
>>>>                   from ./include/linux/timex.h:67,
>>>>                   from ./include/linux/time32.h:13,
>>>>                   from ./include/linux/time.h:60,
>>>>                   from ./include/linux/stat.h:19,
>>>>                   from ./include/linux/module.h:13,
>>>>                   from drivers/mtd/mtdpart.c:10:
>>>> drivers/mtd/mtdpart.c: In function ‘parse_mtd_partitions’:
>>>> drivers/mtd/mtdpart.c:693:34: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
>>>>    693 |                         pr_debug("%s: got parser %s\n", master->name,
>>>>        |                                  ^~~~~~~~~~~~~~~~~~~~~
>>>> ./include/linux/printk.h:376:21: note: in definition of macro ‘pr_fmt’
>>>>    376 | #define pr_fmt(fmt) fmt
>>>>        |                     ^~~
>>>> ./include/linux/dynamic_debug.h:248:9: note: in expansion of macro ‘__dynamic_func_call_cls’
>>>>    248 |         __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
>>>>        |         ^~~~~~~~~~~~~~~~~~~~~~~
>>>> ./include/linux/dynamic_debug.h:250:9: note: in expansion of macro ‘_dynamic_func_call_cls’
>>>>    250 |         _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
>>>>        |         ^~~~~~~~~~~~~~~~~~~~~~
>>>> ./include/linux/dynamic_debug.h:269:9: note: in expansion of macro ‘_dynamic_func_call’
>>>>    269 |         _dynamic_func_call(fmt, __dynamic_pr_debug,             \
>>>>        |         ^~~~~~~~~~~~~~~~~~
>>>> ./include/linux/printk.h:610:9: note: in expansion of macro ‘dynamic_pr_debug’
>>>>    610 |         dynamic_pr_debug(fmt, ##__VA_ARGS__)
>>>>        |         ^~~~~~~~~~~~~~~~
>>>> drivers/mtd/mtdpart.c:693:25: note: in expansion of macro ‘pr_debug’
>>>>    693 |                         pr_debug("%s: got parser %s\n", master->name,
>>>>        |                         ^~~~~~~~
>>>> drivers/mtd/mtdpart.c:693:50: note: format string is defined here
>>>>    693 |                         pr_debug("%s: got parser %s\n", master->name,
>>>>        |                                                  ^~
>>>>
>>>> Offending commit is 5b644aa012f67.
>>>
>>> Actually it goes back to 2015 to the commit 8e2c992b59fc ("mtd: mtdpart: add debug prints to partition parser.").
>>>
>>>
>>>> Proposed non-intrusive fix resolves the warning/error, but I could not test the code.
>>>> (I don't have the physical device.)
>>>>
>>>> -----------------><------------------------------------------
>>>> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
>>>> index 6811a714349d..81665d67ed2d 100644
>>>> --- a/drivers/mtd/mtdpart.c
>>>> +++ b/drivers/mtd/mtdpart.c
>>>> @@ -691,7 +691,7 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>>>>                          if (!parser && !request_module("%s", *types))
>>>>                                  parser = mtd_part_parser_get(*types);
>>>>                          pr_debug("%s: got parser %s\n", master->name,
>>>> -                               parser ? parser->name : NULL);
>>>> +                               parser ? parser->name : "(null"));
>>>>                          if (!parser)
>>>>                                  continue;
>>>>                          ret = mtd_part_do_parse(parser, master, &pparts, data);
>>>>
>>>>
>>>> Hope this helps.
>>>
>>> I'd say it's simple enough to send patch without actual hw testing.
>>
>> Though the kernel's vsprintf will already handle NULL pointers and
>> print "(null)" [1], so I'm not sure if this is an actual improvement.
>>
>> The only way this can be NULL though is if the request_module()
>> failed, so maybe a proper error message would be better here instead
>> of an obscure "got parser (null)". You don't even know which type
>> wasn't available. E.g. pr_debug("%: no parser for type %s
>> available\n", master->name, *types).
> 
> Agreed. Your proposal is much more descriptive.
> 
> Will you do this patch or should I and put a Suggested-by: ?
> 
> However, I would sleep much better if this is tested on actual hw. :-/

Hi Jonas, Rafał,

Is this what you had in mind?

-----><------------------------
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 6811a714349d..dd02690abf3a 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -690,8 +690,12 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
                        parser = mtd_part_parser_get(*types);
                        if (!parser && !request_module("%s", *types))
                                parser = mtd_part_parser_get(*types);
-                       pr_debug("%s: got parser %s\n", master->name,
-                               parser ? parser->name : NULL);
+                       if (!parser)
+                               pr_debug("%s: no parser available for type %s\n",
+                                       master->name, *types);
+                       else
+                               pr_debug("%s: got parser %s\n", master->name,
+                                       parser->name);
                        if (!parser)
                                continue;
                        ret = mtd_part_do_parse(parser, master, &pparts, data);
--

This will both eliminate warning and be more descriptive.

I agree that vsprintf() will print "(null)" for NULL ptr, but GCC 12.3.0 doesn't tolerate such
application and we cannot build w -Werror ...

How does this look to you?

Best regards,
Mirsad Todorovac

> Best regards,
> Mirsad Todorovac
> 
>> [1] https://elixir.bootlin.com/linux/latest/source/lib/vsprintf.c#L696
>>
>> Best Regards,
>> Jonas