| Message ID | 47e0483d-6e3d-43a8-9273-25278a4a74b9@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PROBLEM,linux-next] | expand |
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.
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). [1] https://elixir.bootlin.com/linux/latest/source/lib/vsprintf.c#L696 Best Regards, Jonas
Hi, Rafał, others, On 7/7/24 09:37, Rafał Miłecki wrote: > Some more descriptive subject would be nice :) I could have sworn I put it there, I wasn't intoxicated :-P > 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."). This is also correct. >> 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. Actually, it isn't simple enough to prevent a typo. Here is the v2. Do I have your Reviewed-by: or Acked-by: ? Andy from Intel said that it has to be given explicitly. ------------------------------><------------------------------------- diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 6811a714349d..6f7e250ef710 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); -- Best regards, Mirsad Todorovac
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. :-/ Best regards, Mirsad Todorovac > [1] https://elixir.bootlin.com/linux/latest/source/lib/vsprintf.c#L696 > > Best Regards, > Jonas
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
Hi, 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. :-/ 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); -- Best regards, Mirsad Todorovac > Best regards, > Mirsad Todorovac > >> [1] https://elixir.bootlin.com/linux/latest/source/lib/vsprintf.c#L696 >> >> Best Regards, >> Jonas
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);
Hi, all, This is the result of testing randconfig with KCONFIG_SEED=0xEE7AB52F in next-20240703 vanilla tree on Ubuntu 22.04 LTS. GCC used is GCC (Ubuntu 12.3.0-1ubuntu1~22.04) 12.3.0. The particular error is as follows: 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. Offending code is here: 668 int parse_mtd_partitions(struct mtd_info *master, const char *const *types, 669 struct mtd_part_parser_data *data) 670 { 671 struct mtd_partitions pparts = { }; 672 struct mtd_part_parser *parser; 673 int ret, err = 0; 674 675 if (!types) 676 types = mtd_is_partition(master) ? default_subpartition_types : 677 default_mtd_part_types; 678 679 for ( ; *types; types++) { 680 /* 681 * ofpart is a special type that means OF partitioning info 682 * should be used. It requires a bit different logic so it is 683 * handled in a separated function. 684 */ 685 if (!strcmp(*types, "ofpart")) { 686 ret = mtd_part_of_parse(master, &pparts); 687 } else { 688 pr_debug("%s: parsing partitions %s\n", master->name, 689 *types); 690 parser = mtd_part_parser_get(*types); 691 if (!parser && !request_module("%s", *types)) 692 parser = mtd_part_parser_get(*types); → 693 pr_debug("%s: got parser %s\n", master->name, → 694 parser ? parser->name : NULL); 695 if (!parser) 696 continue; 697 ret = mtd_part_do_parse(parser, master, &pparts, data); 698 if (ret <= 0) 699 mtd_part_parser_put(parser); 700 } 701 /* Found partitions! */ 702 if (ret > 0) { 703 err = add_mtd_partitions(master, pparts.parts, 704 pparts.nr_parts); 705 mtd_part_parser_cleanup(&pparts); 706 return err ? err : pparts.nr_parts; 707 } 708 /* 709 * Stash the first error we see; only report it if no parser 710 * succeeds 711 */ 712 if (ret < 0 && !err) 713 err = ret; 714 } 715 return err; 716 } Proposed non-intrusive fix resolves the warning/error, but I could not test the code. (I don't have the physical device.) -----------------><------------------------------------------ Hope this helps. Best regards, Mirsad Todorovac