| Message ID | 20190910084707.18380-12-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Device property improvements, add %pfw format specifier | expand |
On Tue, 2019-09-10 at 11:47 +0300, Sakari Ailus wrote: > Add support for %pfw conversion specifier (with "f" and "P" modifiers) to > support printing full path of the node, including its name ("f") and only > the node's name ("P") in the printk family of functions. The two flags > have equivalent functionality to existing %pOF with the same two modifiers > ("f" and "P") on OF based systems. The ability to do the same on ACPI > based systems is added by this patch. checkpatch comment below: A. > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst [] > @@ -418,6 +418,30 @@ Examples:: > > Passed by reference. > > +Fwnode handles > +-------------- > + > +:: > + > + %pfw[fP] > + > +For printing information on fwnode handles. The default is to print the full > +node name, including the path. The modifiers are functionally equivalent to > +%pOF above. > + > + - f - full name of the node, including the path > + - P - the name of the node including an address (if there is one) > + > +Examples (ACPI):: > + > + %pfwf \_SB.PCI0.CIO2.port@1.endpoint@0 - Full node name > + %pfwP endpoint@0 - Node name > + > +Examples (OF):: > + > + %pfwf /ocp@68000000/i2c@48072000/camera@10/port/endpoint - Full name > + %pfwP endpoint - Node name [] > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > @@ -5988,14 +5988,17 @@ sub process { > for (my $count = $linenr; $count <= $lc; $count++) { > my $specifier; > my $extension; > + my $qualifier; > my $bad_specifier = ""; > my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0)); > $fmt =~ s/%%//g; > > - while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) { > + while ($fmt =~ /(\%[\*\d\.]*p(\w)(\w?))/g) { This is not generic enough to allow all possible qualifiers to be captured. This should be something like: while ($fmt =~ /(\%[\*\d\.]*p(\w)(\w*))/g) { > $specifier = $1; > $extension = $2; > - if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxt]/) { > + $qualifier = $3; > + if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxtf]/ || > + ($extension eq "f" && $qualifier ne "w")) { And this should likely test (defined $qualifier && $extension eq 'f' && $qualifier =~ /^w/)) > $bad_specifier = $specifier; > last; > } And here the 'f' isn't really a bad $specifier, it does not have the correct 'w' $extension. The valid extensions may change in the future and this code isn't particularly manageable, so I think the $specifier/$extension pair should likely have some additional testing after validating the $specifier.
Hi Joe, On Tue, Sep 10, 2019 at 03:21:26AM -0700, Joe Perches wrote: > > @@ -5988,14 +5988,17 @@ sub process { > > for (my $count = $linenr; $count <= $lc; $count++) { > > my $specifier; > > my $extension; > > + my $qualifier; > > my $bad_specifier = ""; > > my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0)); > > $fmt =~ s/%%//g; > > > > - while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) { > > + while ($fmt =~ /(\%[\*\d\.]*p(\w)(\w?))/g) { > > This is not generic enough to allow all possible qualifiers > to be captured. > > This should be something like: > > while ($fmt =~ /(\%[\*\d\.]*p(\w)(\w*))/g) { > > > $specifier = $1; > > $extension = $2; > > - if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxt]/) { > > + $qualifier = $3; > > + if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxtf]/ || > > + ($extension eq "f" && $qualifier ne "w")) { > > > And this should likely test > (defined $qualifier && > $extension eq 'f' && $qualifier =~ /^w/)) > > > $bad_specifier = $specifier; > > last; > > } > > And here the 'f' isn't really a bad $specifier, > it does not have the correct 'w' $extension. > > The valid extensions may change in the future and > this code isn't particularly manageable, so I > think the $specifier/$extension pair should likely > have some additional testing after validating > the $specifier. There are certainly possibilities of having qualifiers that aren't supported but extensions are also allowed to be followed by any other characters, so this is a bit grey area. I'll make the qualifier one or more letters for v7 as you suggested.
diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index 922a29eb70e6c..1ab3841686dc5 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -418,6 +418,30 @@ Examples:: Passed by reference. +Fwnode handles +-------------- + +:: + + %pfw[fP] + +For printing information on fwnode handles. The default is to print the full +node name, including the path. The modifiers are functionally equivalent to +%pOF above. + + - f - full name of the node, including the path + - P - the name of the node including an address (if there is one) + +Examples (ACPI):: + + %pfwf \_SB.PCI0.CIO2.port@1.endpoint@0 - Full node name + %pfwP endpoint@0 - Node name + +Examples (OF):: + + %pfwf /ocp@68000000/i2c@48072000/camera@10/port/endpoint - Full name + %pfwP endpoint - Node name + Time and date (struct rtc_time) ------------------------------- diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 9c5b398d4a512..51c81921598d8 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1982,6 +1982,36 @@ char *device_node_string(char *buf, char *end, struct device_node *dn, return widen_string(buf, buf - buf_start, end, spec); } +static noinline_for_stack +char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode, + struct printf_spec spec, const char *fmt) +{ + struct printf_spec str_spec = spec; + char *buf_start = buf; + + str_spec.field_width = -1; + + if (*fmt != 'w') + return error_string(buf, end, "(%pf?)", spec); + + if (check_pointer(&buf, end, fwnode, spec)) + return buf; + + fmt++; + + switch (*fmt) { + case 'P': /* name */ + buf = string(buf, end, fwnode_get_name(fwnode), str_spec); + break; + case 'f': /* full_name */ + default: + buf = fwnode_full_name_string(fwnode, buf, end); + break; + } + + return widen_string(buf, buf - buf_start, end, spec); +} + /* * Show a '%p' thing. A kernel extension is that the '%p' is followed * by an extra set of alphanumeric characters that are extended format @@ -2084,6 +2114,10 @@ char *device_node_string(char *buf, char *end, struct device_node *dn, * F device node flags * c major compatible string * C full compatible string + * - 'fw[fP]' For a firmware node (struct fwnode_handle) pointer + * Without an option prints the full name of the node + * f full name + * P node name, including a possible unit address * - 'x' For printing the address. Equivalent to "%lx". * * ** When making changes please also update: @@ -2161,6 +2195,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, return flags_string(buf, end, ptr, spec, fmt); case 'O': return device_node_string(buf, end, ptr, spec, fmt + 1); + case 'f': + return fwnode_string(buf, end, ptr, spec, fmt + 1); case 'x': return pointer_string(buf, end, ptr, spec); } diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index a60c241112cd4..93470283ea243 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5988,14 +5988,17 @@ sub process { for (my $count = $linenr; $count <= $lc; $count++) { my $specifier; my $extension; + my $qualifier; my $bad_specifier = ""; my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0)); $fmt =~ s/%%//g; - while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) { + while ($fmt =~ /(\%[\*\d\.]*p(\w)(\w?))/g) { $specifier = $1; $extension = $2; - if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxt]/) { + $qualifier = $3; + if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxtf]/ || + ($extension eq "f" && $qualifier ne "w")) { $bad_specifier = $specifier; last; }
Add support for %pfw conversion specifier (with "f" and "P" modifiers) to support printing full path of the node, including its name ("f") and only the node's name ("P") in the printk family of functions. The two flags have equivalent functionality to existing %pOF with the same two modifiers ("f" and "P") on OF based systems. The ability to do the same on ACPI based systems is added by this patch. On ACPI based systems the resulting strings look like \_SB.PCI0.CIO2.port@1.endpoint@0 where the nodes are separated by a dot (".") and the first three are ACPI device nodes and the latter two ACPI data nodes. 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 | 24 +++++++++++++++ lib/vsprintf.c | 36 +++++++++++++++++++++++ scripts/checkpatch.pl | 7 +++-- 3 files changed, 65 insertions(+), 2 deletions(-)