[v7,08/13] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps
diff mbox series

Message ID 20190918133419.7969-9-sakari.ailus@linux.intel.com
State Superseded
Headers show
Series
  • Device property improvements, add %pfw format specifier
Related show

Commit Message

Sakari Ailus Sept. 18, 2019, 1:34 p.m. UTC
%pS and %ps are now the preferred conversion specifiers to print function
names. The functionality is equivalent; remove the old, deprecated %pF
and %pf support.

Depends-on: commit 2d44d165e939 ("scsi: lpfc: Convert existing %pf users to %ps")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/core-api/printk-formats.rst | 10 ----------
 lib/vsprintf.c                            |  8 ++------
 scripts/checkpatch.pl                     |  1 -
 3 files changed, 2 insertions(+), 17 deletions(-)

Comments

Rafael J. Wysocki Sept. 19, 2019, 7:58 a.m. UTC | #1
On Wed, Sep 18, 2019 at 3:34 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> %pS and %ps are now the preferred conversion specifiers to print function
> names. The functionality is equivalent; remove the old, deprecated %pF
> and %pf support.
>
> Depends-on: commit 2d44d165e939 ("scsi: lpfc: Convert existing %pf users to %ps")

Where is this commit present?

Not in the mainline as of today.

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  Documentation/core-api/printk-formats.rst | 10 ----------
>  lib/vsprintf.c                            |  8 ++------
>  scripts/checkpatch.pl                     |  1 -
>  3 files changed, 2 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index c6224d039bcbe..922a29eb70e6c 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -86,8 +86,6 @@ Symbols/Function Pointers
>
>         %pS     versatile_init+0x0/0x110
>         %ps     versatile_init
> -       %pF     versatile_init+0x0/0x110
> -       %pf     versatile_init
>         %pSR    versatile_init+0x9/0x110
>                 (with __builtin_extract_return_addr() translation)
>         %pB     prev_fn_of_versatile_init+0x88/0x88
> @@ -97,14 +95,6 @@ The ``S`` and ``s`` specifiers are used for printing a pointer in symbolic
>  format. They result in the symbol name with (S) or without (s)
>  offsets. If KALLSYMS are disabled then the symbol address is printed instead.
>
> -Note, that the ``F`` and ``f`` specifiers are identical to ``S`` (``s``)
> -and thus deprecated. We have ``F`` and ``f`` because on ia64, ppc64 and
> -parisc64 function pointers are indirect and, in fact, are function
> -descriptors, which require additional dereferencing before we can lookup
> -the symbol. As of now, ``S`` and ``s`` perform dereferencing on those
> -platforms (when needed), so ``F`` and ``f`` exist for compatibility
> -reasons only.
> -
>  The ``B`` specifier results in the symbol name with offsets and should be
>  used when printing stack backtraces. The specifier takes into
>  consideration the effect of compiler optimisations which may occur
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index b0967cf17137d..b00b57f9f911f 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -909,7 +909,7 @@ char *symbol_string(char *buf, char *end, void *ptr,
>  #ifdef CONFIG_KALLSYMS
>         if (*fmt == 'B')
>                 sprint_backtrace(sym, value);
> -       else if (*fmt != 'f' && *fmt != 's')
> +       else if (*fmt != 's')
>                 sprint_symbol(sym, value);
>         else
>                 sprint_symbol_no_offset(sym, value);
> @@ -2007,9 +2007,7 @@ static char *kobject_string(char *buf, char *end, void *ptr,
>   *
>   * - 'S' For symbolic direct pointers (or function descriptors) with offset
>   * - 's' For symbolic direct pointers (or function descriptors) without offset
> - * - 'F' Same as 'S'
> - * - 'f' Same as 's'
> - * - '[FfSs]R' as above with __builtin_extract_return_addr() translation
> + * - '[Ss]R' as above with __builtin_extract_return_addr() translation
>   * - 'B' For backtraced symbolic direct pointers with offset
>   * - 'R' For decoded struct resource, e.g., [mem 0x0-0x1f 64bit pref]
>   * - 'r' For raw struct resource, e.g., [mem 0x0-0x1f flags 0x201]
> @@ -2112,8 +2110,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>               struct printf_spec spec)
>  {
>         switch (*fmt) {
> -       case 'F':
> -       case 'f':
>         case 'S':
>         case 's':
>                 ptr = dereference_symbol_descriptor(ptr);
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 93a7edfe0f059..a60c241112cd4 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -6012,7 +6012,6 @@ sub process {
>                                         my $ext_type = "Invalid";
>                                         my $use = "";
>                                         if ($bad_specifier =~ /p[Ff]/) {
> -                                               $ext_type = "Deprecated";
>                                                 $use = " - use %pS instead";
>                                                 $use =~ s/pS/ps/ if ($bad_specifier =~ /pf/);
>                                         }
> --
> 2.20.1
>
Petr Mladek Sept. 24, 2019, 10:38 a.m. UTC | #2
On Wed 2019-09-18 16:34:14, Sakari Ailus wrote:
> %pS and %ps are now the preferred conversion specifiers to print function
> names. The functionality is equivalent; remove the old, deprecated %pF
> and %pf support.
> 
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -909,7 +909,7 @@ char *symbol_string(char *buf, char *end, void *ptr,
>  #ifdef CONFIG_KALLSYMS
>  	if (*fmt == 'B')
>  		sprint_backtrace(sym, value);
> -	else if (*fmt != 'f' && *fmt != 's')
> +	else if (*fmt != 's')
>  		sprint_symbol(sym, value);
>  	else
>  		sprint_symbol_no_offset(sym, value);
> @@ -2007,9 +2007,7 @@ static char *kobject_string(char *buf, char *end, void *ptr,
>   *
>   * - 'S' For symbolic direct pointers (or function descriptors) with offset
>   * - 's' For symbolic direct pointers (or function descriptors) without offset
> - * - 'F' Same as 'S'
> - * - 'f' Same as 's'
> - * - '[FfSs]R' as above with __builtin_extract_return_addr() translation
> + * - '[Ss]R' as above with __builtin_extract_return_addr() translation
>   * - 'B' For backtraced symbolic direct pointers with offset
>   * - 'R' For decoded struct resource, e.g., [mem 0x0-0x1f 64bit pref]
>   * - 'r' For raw struct resource, e.g., [mem 0x0-0x1f flags 0x201]
> @@ -2112,8 +2110,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  	      struct printf_spec spec)
>  {
>  	switch (*fmt) {
> -	case 'F':
> -	case 'f':
>  	case 'S':
>  	case 's':
>  		ptr = dereference_symbol_descriptor(ptr);

IMHO, we should do the same also in vbin_printf().

The compatibility with traceevent tools was discussed in the thread
https://lkml.kernel.org/r/20190910084707.18380-2-sakari.ailus@linux.intel.com

If I understand it correctly the tools should be able to handle stored
'f' and 'F' modifiers because they might be produced by
older kernels. But new kernels should not longer produce them.

Otherwise the patch looks good to me. I am getting used to the fact
that we will remove the obsolete specifiers completely.

Best Regards,
Petr
Sakari Ailus Oct. 2, 2019, 12:06 p.m. UTC | #3
Hi Petr,

Thank you for the review.

On Tue, Sep 24, 2019 at 12:38:29PM +0200, Petr Mladek wrote:
> On Wed 2019-09-18 16:34:14, Sakari Ailus wrote:
> > %pS and %ps are now the preferred conversion specifiers to print function
> > names. The functionality is equivalent; remove the old, deprecated %pF
> > and %pf support.
> > 
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -909,7 +909,7 @@ char *symbol_string(char *buf, char *end, void *ptr,
> >  #ifdef CONFIG_KALLSYMS
> >  	if (*fmt == 'B')
> >  		sprint_backtrace(sym, value);
> > -	else if (*fmt != 'f' && *fmt != 's')
> > +	else if (*fmt != 's')
> >  		sprint_symbol(sym, value);
> >  	else
> >  		sprint_symbol_no_offset(sym, value);
> > @@ -2007,9 +2007,7 @@ static char *kobject_string(char *buf, char *end, void *ptr,
> >   *
> >   * - 'S' For symbolic direct pointers (or function descriptors) with offset
> >   * - 's' For symbolic direct pointers (or function descriptors) without offset
> > - * - 'F' Same as 'S'
> > - * - 'f' Same as 's'
> > - * - '[FfSs]R' as above with __builtin_extract_return_addr() translation
> > + * - '[Ss]R' as above with __builtin_extract_return_addr() translation
> >   * - 'B' For backtraced symbolic direct pointers with offset
> >   * - 'R' For decoded struct resource, e.g., [mem 0x0-0x1f 64bit pref]
> >   * - 'r' For raw struct resource, e.g., [mem 0x0-0x1f flags 0x201]
> > @@ -2112,8 +2110,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> >  	      struct printf_spec spec)
> >  {
> >  	switch (*fmt) {
> > -	case 'F':
> > -	case 'f':
> >  	case 'S':
> >  	case 's':
> >  		ptr = dereference_symbol_descriptor(ptr);
> 
> IMHO, we should do the same also in vbin_printf().
> 
> The compatibility with traceevent tools was discussed in the thread
> https://lkml.kernel.org/r/20190910084707.18380-2-sakari.ailus@linux.intel.com
> 
> If I understand it correctly the tools should be able to handle stored
> 'f' and 'F' modifiers because they might be produced by
> older kernels. But new kernels should not longer produce them.

Agreed.

> 
> Otherwise the patch looks good to me. I am getting used to the fact
> that we will remove the obsolete specifiers completely.

I'll address this in v8 soon.

Patch
diff mbox series

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index c6224d039bcbe..922a29eb70e6c 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -86,8 +86,6 @@  Symbols/Function Pointers
 
 	%pS	versatile_init+0x0/0x110
 	%ps	versatile_init
-	%pF	versatile_init+0x0/0x110
-	%pf	versatile_init
 	%pSR	versatile_init+0x9/0x110
 		(with __builtin_extract_return_addr() translation)
 	%pB	prev_fn_of_versatile_init+0x88/0x88
@@ -97,14 +95,6 @@  The ``S`` and ``s`` specifiers are used for printing a pointer in symbolic
 format. They result in the symbol name with (S) or without (s)
 offsets. If KALLSYMS are disabled then the symbol address is printed instead.
 
-Note, that the ``F`` and ``f`` specifiers are identical to ``S`` (``s``)
-and thus deprecated. We have ``F`` and ``f`` because on ia64, ppc64 and
-parisc64 function pointers are indirect and, in fact, are function
-descriptors, which require additional dereferencing before we can lookup
-the symbol. As of now, ``S`` and ``s`` perform dereferencing on those
-platforms (when needed), so ``F`` and ``f`` exist for compatibility
-reasons only.
-
 The ``B`` specifier results in the symbol name with offsets and should be
 used when printing stack backtraces. The specifier takes into
 consideration the effect of compiler optimisations which may occur
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b0967cf17137d..b00b57f9f911f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -909,7 +909,7 @@  char *symbol_string(char *buf, char *end, void *ptr,
 #ifdef CONFIG_KALLSYMS
 	if (*fmt == 'B')
 		sprint_backtrace(sym, value);
-	else if (*fmt != 'f' && *fmt != 's')
+	else if (*fmt != 's')
 		sprint_symbol(sym, value);
 	else
 		sprint_symbol_no_offset(sym, value);
@@ -2007,9 +2007,7 @@  static char *kobject_string(char *buf, char *end, void *ptr,
  *
  * - 'S' For symbolic direct pointers (or function descriptors) with offset
  * - 's' For symbolic direct pointers (or function descriptors) without offset
- * - 'F' Same as 'S'
- * - 'f' Same as 's'
- * - '[FfSs]R' as above with __builtin_extract_return_addr() translation
+ * - '[Ss]R' as above with __builtin_extract_return_addr() translation
  * - 'B' For backtraced symbolic direct pointers with offset
  * - 'R' For decoded struct resource, e.g., [mem 0x0-0x1f 64bit pref]
  * - 'r' For raw struct resource, e.g., [mem 0x0-0x1f flags 0x201]
@@ -2112,8 +2110,6 @@  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
 	switch (*fmt) {
-	case 'F':
-	case 'f':
 	case 'S':
 	case 's':
 		ptr = dereference_symbol_descriptor(ptr);
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 93a7edfe0f059..a60c241112cd4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6012,7 +6012,6 @@  sub process {
 					my $ext_type = "Invalid";
 					my $use = "";
 					if ($bad_specifier =~ /p[Ff]/) {
-						$ext_type = "Deprecated";
 						$use = " - use %pS instead";
 						$use =~ s/pS/ps/ if ($bad_specifier =~ /pf/);
 					}