[PATCH v1 0/4] lib/vsprintf: Rework header inclusions

[PATCH v1 0/4] lib/vsprintf: Rework header inclusions

[Andy Shevchenko]

Some patches that reduce the mess with the header inclusions related to
vsprintf.c module. Each patch has its own description, and has no
dependencies to each other, except the collisions over modifications
of the same places. Hence the series.

Andy Shevchenko (4):
  lib/vsprintf: Declare no_hash_pointers in a local header
  lib/vsprintf: Sort headers alphabetically
  lib/vsprintf: Remove implied inclusions
  lib/vsprintf: Split out sprintf() and friends

 include/linux/kernel.h  | 30 +-----------------------------
 include/linux/sprintf.h | 24 ++++++++++++++++++++++++
 lib/test_printf.c       |  4 ++--
 lib/vsprintf.c          | 38 ++++++++++++++++++++------------------
 lib/vsprintf.h          |  7 +++++++
 mm/kfence/report.c      |  3 +--
 6 files changed, 55 insertions(+), 51 deletions(-)
 create mode 100644 include/linux/sprintf.h
 create mode 100644 lib/vsprintf.h

-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 1/4] lib/vsprintf: Declare no_hash_pointers in a local header

[Andy Shevchenko]

Sparse is not happy to see non-static variable without declaration:
lib/vsprintf.c:61:6: warning: symbol 'no_hash_pointers' was not declared. Should it be static?

Declare respective variable in the local header.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_printf.c  | 4 ++--
 lib/vsprintf.c     | 1 +
 lib/vsprintf.h     | 7 +++++++
 mm/kfence/report.c | 3 +--
 4 files changed, 11 insertions(+), 4 deletions(-)
 create mode 100644 lib/vsprintf.h

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7677ebccf3c3..9e04b5f7244a 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -24,6 +24,8 @@
 
 #include <linux/property.h>
 
+#include "vsprintf.h"
+
 #include "../tools/testing/selftests/kselftest_module.h"
 
 #define BUF_SIZE 256
@@ -41,8 +43,6 @@ KSTM_MODULE_GLOBALS();
 static char *test_buffer __initdata;
 static char *alloced_buffer __initdata;
 
-extern bool no_hash_pointers;
-
 static int __printf(4, 0) __init
 do_test(int bufsize, const char *expect, int elen,
 	const char *fmt, va_list ap)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 40f560959b16..6774cf84e623 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -54,6 +54,7 @@
 
 #include <linux/string_helpers.h>
 #include "kstrtox.h"
+#include "vsprintf.h"
 
 /* Disable pointer hashing if requested */
 bool no_hash_pointers __ro_after_init;
diff --git a/lib/vsprintf.h b/lib/vsprintf.h
new file mode 100644
index 000000000000..ddffde905824
--- /dev/null
+++ b/lib/vsprintf.h
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LIB_VSPRINTF_H
+#define _LIB_VSPRINTF_H
+
+extern bool no_hash_pointers;
+
+#endif
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index 197430a5be4a..fb28c6abd58e 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -19,6 +19,7 @@
 
 #include <asm/kfence.h>
 
+#include "../../lib/vsprintf.h"
 #include "kfence.h"
 
 /* May be overridden by <asm/kfence.h>. */
@@ -26,8 +27,6 @@
 #define ARCH_FUNC_PREFIX ""
 #endif
 
-extern bool no_hash_pointers;
-
 /* Helper function to either print to a seq_file or to console. */
 __printf(2, 3)
 static void seq_con_printf(struct seq_file *seq, const char *fmt, ...)
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 1/4] lib/vsprintf: Declare no_hash_pointers in a local header

[Marco Elver]

On Fri, 4 Aug 2023 at 10:26, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Sparse is not happy to see non-static variable without declaration:
> lib/vsprintf.c:61:6: warning: symbol 'no_hash_pointers' was not declared. Should it be static?
>
> Declare respective variable in the local header.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/test_printf.c  | 4 ++--
>  lib/vsprintf.c     | 1 +
>  lib/vsprintf.h     | 7 +++++++
>  mm/kfence/report.c | 3 +--
>  4 files changed, 11 insertions(+), 4 deletions(-)
>  create mode 100644 lib/vsprintf.h
>
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 7677ebccf3c3..9e04b5f7244a 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -24,6 +24,8 @@
>
>  #include <linux/property.h>
>
> +#include "vsprintf.h"
> +
>  #include "../tools/testing/selftests/kselftest_module.h"
>
>  #define BUF_SIZE 256
> @@ -41,8 +43,6 @@ KSTM_MODULE_GLOBALS();
>  static char *test_buffer __initdata;
>  static char *alloced_buffer __initdata;
>
> -extern bool no_hash_pointers;
> -
>  static int __printf(4, 0) __init
>  do_test(int bufsize, const char *expect, int elen,
>         const char *fmt, va_list ap)
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 40f560959b16..6774cf84e623 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -54,6 +54,7 @@
>
>  #include <linux/string_helpers.h>
>  #include "kstrtox.h"
> +#include "vsprintf.h"
>
>  /* Disable pointer hashing if requested */
>  bool no_hash_pointers __ro_after_init;
> diff --git a/lib/vsprintf.h b/lib/vsprintf.h
> new file mode 100644
> index 000000000000..ddffde905824
> --- /dev/null
> +++ b/lib/vsprintf.h
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LIB_VSPRINTF_H
> +#define _LIB_VSPRINTF_H
> +
> +extern bool no_hash_pointers;
> +
> +#endif

It seems odd to create such a local header and then refer to it from
other subsystems.

What's the downside of just putting this into the new
include/linux/sprintf.h? If someone wants to access this variable,
they will one way or another.

Re: [PATCH v1 1/4] lib/vsprintf: Declare no_hash_pointers in a local header

[Steven Rostedt]

On Fri, 4 Aug 2023 10:38:22 +0200
Marco Elver <elver@google.com> wrote:

> > +++ b/lib/vsprintf.h
> > @@ -0,0 +1,7 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _LIB_VSPRINTF_H
> > +#define _LIB_VSPRINTF_H
> > +
> > +extern bool no_hash_pointers;
> > +
> > +#endif  
> 
> It seems odd to create such a local header and then refer to it from
> other subsystems.
> 
> What's the downside of just putting this into the new
> include/linux/sprintf.h? If someone wants to access this variable,
> they will one way or another.

 +1

-- Steve

Re: [PATCH v1 1/4] lib/vsprintf: Declare no_hash_pointers in a local header

[Steven Rostedt]

On Fri,  4 Aug 2023 11:26:16 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
> index 197430a5be4a..fb28c6abd58e 100644
> --- a/mm/kfence/report.c
> +++ b/mm/kfence/report.c
> @@ -19,6 +19,7 @@
>  
>  #include <asm/kfence.h>
>  
> +#include "../../lib/vsprintf.h"
>  #include "kfence.h"
>  

Ug, I hate "../../" includes. Can we have this in one of the main header
files?

-- Steve

[PATCH v1 2/4] lib/vsprintf: Sort headers alphabetically

[Andy Shevchenko]

Sorting headers alphabetically helps locating duplicates, and
make it easier to figure out where to insert new headers.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 6774cf84e623..63afffab4249 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -17,42 +17,44 @@
  * - scnprintf and vscnprintf
  */
 
-#include <linux/stdarg.h>
 #include <linux/build_bug.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
-#include <linux/errname.h>
-#include <linux/module.h>	/* for KSYM_SYMBOL_LEN */
-#include <linux/types.h>
-#include <linux/string.h>
+#include <linux/compiler.h>
+#include <linux/cred.h>
 #include <linux/ctype.h>
-#include <linux/kernel.h>
+#include <linux/dcache.h>
+#include <linux/errname.h>
+#include <linux/ioport.h>
 #include <linux/kallsyms.h>
+#include <linux/kernel.h>
 #include <linux/math64.h>
-#include <linux/uaccess.h>
-#include <linux/ioport.h>
-#include <linux/dcache.h>
-#include <linux/cred.h>
+#include <linux/module.h>	/* for KSYM_SYMBOL_LEN */
+#include <linux/notifier.h>
+#include <linux/of.h>
+#include <linux/property.h>
 #include <linux/rtc.h>
+#include <linux/siphash.h>
+#include <linux/stdarg.h>
+#include <linux/string.h>
+#include <linux/string_helpers.h>
 #include <linux/time.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
 #include <linux/uuid.h>
-#include <linux/of.h>
-#include <net/addrconf.h>
-#include <linux/siphash.h>
-#include <linux/compiler.h>
-#include <linux/property.h>
-#include <linux/notifier.h>
+
 #ifdef CONFIG_BLOCK
 #include <linux/blkdev.h>
 #endif
 
+#include <net/addrconf.h>
+
 #include "../mm/internal.h"	/* For the trace_print_flags arrays */
 
-#include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/byteorder.h>	/* cpu_to_le16 */
+#include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/unaligned.h>
 
-#include <linux/string_helpers.h>
 #include "kstrtox.h"
 #include "vsprintf.h"
 
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 3/4] lib/vsprintf: Remove implied inclusions

[Andy Shevchenko]

Remove inclusions that are implied and guaranteed to be provided by others:

  compiler.h	by types.h
  string.hi	by string_helpers.h

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 63afffab4249..eb0934d02722 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -20,7 +20,6 @@
 #include <linux/build_bug.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
-#include <linux/compiler.h>
 #include <linux/cred.h>
 #include <linux/ctype.h>
 #include <linux/dcache.h>
@@ -36,7 +35,6 @@
 #include <linux/rtc.h>
 #include <linux/siphash.h>
 #include <linux/stdarg.h>
-#include <linux/string.h>
 #include <linux/string_helpers.h>
 #include <linux/time.h>
 #include <linux/types.h>
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 3/4] lib/vsprintf: Remove implied inclusions

[Rasmus Villemoes]

On 04/08/2023 10.26, Andy Shevchenko wrote:
> Remove inclusions that are implied and guaranteed to be provided by others:
> 
>   compiler.h	by types.h
>   string.hi	by string_helpers.h

What? No. That's not what we want. Each .c and each .h file should
include the headers that declare the stuff they're using. So if
string_helpers.h magically stops referring to anything from string.h,
one should be allowed to stop including string.h from string_helpers.h.

Sure, those two may forever be so intertwined that it never happens, but
one really can't maintain some matrix of "X always includes Y so if you
include X you don't have to include Y" in one's head.

Rasmus

Re: [PATCH v1 3/4] lib/vsprintf: Remove implied inclusions

[Andy Shevchenko]

On Fri, Aug 04, 2023 at 11:36:15AM +0200, Rasmus Villemoes wrote:
> On 04/08/2023 10.26, Andy Shevchenko wrote:
> > Remove inclusions that are implied and guaranteed to be provided by others:
> > 
> >   compiler.h	by types.h
> >   string.hi	by string_helpers.h
> 
> What? No. That's not what we want. Each .c and each .h file should
> include the headers that declare the stuff they're using.

99.99% of kernel if not more doesn't follow this rule pedantically.
We have to have a compromise between what is guaranteed and what is not.

For example, I'm pretty sure the types.h will always include compiler*.h.

> So if string_helpers.h magically stops referring to anything from string.h,
> one should be allowed to stop including string.h from string_helpers.h.

That's how agreements work. We may agree to guarantee such inclusion or
not. The kernel headers as of today is a complete mess (refer to the
Ingo's 2k+ patch series). But still, some order / agreement is good to have.

> Sure, those two may forever be so intertwined that it never happens, but
> one really can't maintain some matrix of "X always includes Y so if you
> include X you don't have to include Y" in one's head.

Somebody should do that at some point, otherwise it becomes even more mess.

If you want your way, iwyu should be part of the kernel build. And be prepared
for dozens of headers to be added to the every single C file in the kernel.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 4/4] lib/vsprintf: Split out sprintf() and friends

[Andy Shevchenko]

kernel.h is being used as a dump for all kinds of stuff for a long time.
sprintf() and friends are used in many drivers without need of the full
kernel.h dependency train with it.

Here is the attempt on cleaning it up by splitting out sprintf() and
friends.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/kernel.h  | 30 +-----------------------------
 include/linux/sprintf.h | 24 ++++++++++++++++++++++++
 lib/vsprintf.c          |  1 +
 3 files changed, 26 insertions(+), 29 deletions(-)
 create mode 100644 include/linux/sprintf.h

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index b9e76f717a7e..cee8fe87e9f4 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -29,6 +29,7 @@
 #include <linux/panic.h>
 #include <linux/printk.h>
 #include <linux/build_bug.h>
+#include <linux/sprintf.h>
 #include <linux/static_call_types.h>
 #include <linux/instruction_pointer.h>
 #include <asm/byteorder.h>
@@ -203,35 +204,6 @@ static inline void might_fault(void) { }
 
 void do_exit(long error_code) __noreturn;
 
-extern int num_to_str(char *buf, int size,
-		      unsigned long long num, unsigned int width);
-
-/* lib/printf utilities */
-
-extern __printf(2, 3) int sprintf(char *buf, const char * fmt, ...);
-extern __printf(2, 0) int vsprintf(char *buf, const char *, va_list);
-extern __printf(3, 4)
-int snprintf(char *buf, size_t size, const char *fmt, ...);
-extern __printf(3, 0)
-int vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
-extern __printf(3, 4)
-int scnprintf(char *buf, size_t size, const char *fmt, ...);
-extern __printf(3, 0)
-int vscnprintf(char *buf, size_t size, const char *fmt, va_list args);
-extern __printf(2, 3) __malloc
-char *kasprintf(gfp_t gfp, const char *fmt, ...);
-extern __printf(2, 0) __malloc
-char *kvasprintf(gfp_t gfp, const char *fmt, va_list args);
-extern __printf(2, 0)
-const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list args);
-
-extern __scanf(2, 3)
-int sscanf(const char *, const char *, ...);
-extern __scanf(2, 0)
-int vsscanf(const char *, const char *, va_list);
-
-extern int no_hash_pointers_enable(char *str);
-
 extern int get_option(char **str, int *pint);
 extern char *get_options(const char *str, int nints, int *ints);
 extern unsigned long long memparse(const char *ptr, char **retptr);
diff --git a/include/linux/sprintf.h b/include/linux/sprintf.h
new file mode 100644
index 000000000000..00d1fdc70a3e
--- /dev/null
+++ b/include/linux/sprintf.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_KERNEL_SPRINTF_H_
+#define _LINUX_KERNEL_SPRINTF_H_
+
+#include <linux/types.h>
+
+int num_to_str(char *buf, int size, unsigned long long num, unsigned int width);
+
+__printf(2, 3) int sprintf(char *buf, const char * fmt, ...);
+__printf(2, 0) int vsprintf(char *buf, const char *, va_list);
+__printf(3, 4) int snprintf(char *buf, size_t size, const char *fmt, ...);
+__printf(3, 0) int vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
+__printf(3, 4) int scnprintf(char *buf, size_t size, const char *fmt, ...);
+__printf(3, 0) int vscnprintf(char *buf, size_t size, const char *fmt, va_list args);
+__printf(2, 3) __malloc char *kasprintf(gfp_t gfp, const char *fmt, ...);
+__printf(2, 0) __malloc char *kvasprintf(gfp_t gfp, const char *fmt, va_list args);
+__printf(2, 0) const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list args);
+
+__scanf(2, 3) int sscanf(const char *, const char *, ...);
+__scanf(2, 0) int vsscanf(const char *, const char *, va_list);
+
+int no_hash_pointers_enable(char *str);
+
+#endif	/* _LINUX_KERNEL_SPRINTF_H */
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index eb0934d02722..e553bc9e18f3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -34,6 +34,7 @@
 #include <linux/property.h>
 #include <linux/rtc.h>
 #include <linux/siphash.h>
+#include <linux/sprintf.h>
 #include <linux/stdarg.h>
 #include <linux/string_helpers.h>
 #include <linux/time.h>
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 4/4] lib/vsprintf: Split out sprintf() and friends

[Rasmus Villemoes]

On 04/08/2023 10.26, Andy Shevchenko wrote:
> kernel.h is being used as a dump for all kinds of stuff for a long time.
> sprintf() and friends are used in many drivers without need of the full
> kernel.h dependency train with it.
> 
> Here is the attempt on cleaning it up by splitting out sprintf() and
> friends.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/kernel.h  | 30 +-----------------------------
>  include/linux/sprintf.h | 24 ++++++++++++++++++++++++
>  lib/vsprintf.c          |  1 +
>  3 files changed, 26 insertions(+), 29 deletions(-)
>  create mode 100644 include/linux/sprintf.h
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index b9e76f717a7e..cee8fe87e9f4 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -29,6 +29,7 @@
>  #include <linux/panic.h>
>  #include <linux/printk.h>
>  #include <linux/build_bug.h>
> +#include <linux/sprintf.h>
>  #include <linux/static_call_types.h>
>  #include <linux/instruction_pointer.h>
>  #include <asm/byteorder.h>
> @@ -203,35 +204,6 @@ static inline void might_fault(void) { }
>  
>  void do_exit(long error_code) __noreturn;
>  
> -extern int num_to_str(char *buf, int size,
> -		      unsigned long long num, unsigned int width);
> -
> -/* lib/printf utilities */
> -
> -extern __printf(2, 3) int sprintf(char *buf, const char * fmt, ...);
> -extern __printf(2, 0) int vsprintf(char *buf, const char *, va_list);
> -extern __printf(3, 4)
> -int snprintf(char *buf, size_t size, const char *fmt, ...);
> -extern __printf(3, 0)
> -int vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
> -extern __printf(3, 4)
> -int scnprintf(char *buf, size_t size, const char *fmt, ...);
> -extern __printf(3, 0)
> -int vscnprintf(char *buf, size_t size, const char *fmt, va_list args);
> -extern __printf(2, 3) __malloc
> -char *kasprintf(gfp_t gfp, const char *fmt, ...);
> -extern __printf(2, 0) __malloc
> -char *kvasprintf(gfp_t gfp, const char *fmt, va_list args);
> -extern __printf(2, 0)
> -const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list args);
> -
> -extern __scanf(2, 3)
> -int sscanf(const char *, const char *, ...);
> -extern __scanf(2, 0)
> -int vsscanf(const char *, const char *, va_list);
> -
> -extern int no_hash_pointers_enable(char *str);
> -
>  extern int get_option(char **str, int *pint);
>  extern char *get_options(const char *str, int nints, int *ints);
>  extern unsigned long long memparse(const char *ptr, char **retptr);
> diff --git a/include/linux/sprintf.h b/include/linux/sprintf.h
> new file mode 100644
> index 000000000000..00d1fdc70a3e
> --- /dev/null
> +++ b/include/linux/sprintf.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_KERNEL_SPRINTF_H_
> +#define _LINUX_KERNEL_SPRINTF_H_
> +
> +#include <linux/types.h>
> +

Shouldn't this at least also include compiler_attributes.h, to make it
self-contained?

As Marco said, please just declare no_hash_pointers in this file as
well. Perhaps with a comment about not accessing it unless one has good
reason, but I suppose that's true in general for all kernel global
variables, so maybe not worth it for this one.

Rasmus

Re: [PATCH v1 4/4] lib/vsprintf: Split out sprintf() and friends

[Andy Shevchenko]

On Fri, Aug 04, 2023 at 11:31:58AM +0200, Rasmus Villemoes wrote:
> On 04/08/2023 10.26, Andy Shevchenko wrote:

...

> > +#include <linux/types.h>
> 
> Shouldn't this at least also include compiler_attributes.h, to make it
> self-contained?

As I replied in the other email, this is guaranteed by types.h.
But if you insist, I can add it.

> As Marco said, please just declare no_hash_pointers in this file as
> well. Perhaps with a comment about not accessing it unless one has good
> reason, but I suppose that's true in general for all kernel global
> variables, so maybe not worth it for this one.

Sure, thank you for the review!

-- 
With Best Regards,
Andy Shevchenko