| Message ID | 20190902135732.23455-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Device property improvements, add %pfw format specifier | expand |
On Mon, Sep 02, 2019 at 04:57:32PM +0300, Sakari Ailus wrote: > Add a test for the %pfw printk modifier using software nodes. > +static void __init fwnode_pointer(void) > +{ > + const struct software_node softnodes[] = { > + { .name = "first", }, > + { .name = "second", .parent = &softnodes[0], }, > + { .name = "third", .parent = &softnodes[1], }, > + { NULL /* Guardian */ }, Comma is still here :-) > + }; > + test(full_name_second, "%pfw", > + software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 3])); > + test(full_name, "%pfw", > + software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2])); > + test(full_name, "%pfwf", > + software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2])); > + test(second_name, "%pfwP", > + software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 3])); > + test(third_name, "%pfwP", > + software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2])); I have another thought about these. The test cases will fail in either of adding, inserting or removing items in softnodes array. So, using the above "protective" scheme doesn't bring any value except making readability worse.
On Mon 2019-09-02 16:57:30, Sakari Ailus wrote: > Factor out static kobject_string() function that simply calls > device_node_string(), and thus remove references to kobjects (as these are > struct device_node). > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > lib/vsprintf.c | 16 ++++------------ > 1 file changed, 4 insertions(+), 12 deletions(-) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index a04a2167101ef..4ad9332d54ba6 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -1905,6 +1905,9 @@ char *device_node_string(char *buf, char *end, struct device_node *dn, > struct printf_spec str_spec = spec; > str_spec.field_width = -1; > > + if (fmt[0] != 'F') > + return error_string(buf, end, "(%pO?)", spec); > + > if (!IS_ENABLED(CONFIG_OF)) > return error_string(buf, end, "(%pOF?)", spec); > > @@ -1978,17 +1981,6 @@ char *device_node_string(char *buf, char *end, struct device_node *dn, > return widen_string(buf, buf - buf_start, end, spec); > } > > -static char *kobject_string(char *buf, char *end, void *ptr, > - struct printf_spec spec, const char *fmt) > -{ > - switch (fmt[1]) { > - case 'F': > - return device_node_string(buf, end, ptr, spec, fmt + 1); > - } > - > - return error_string(buf, end, "(%pO?)", 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 > @@ -2167,7 +2159,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, > case 'G': > return flags_string(buf, end, ptr, spec, fmt); > case 'O': > - return kobject_string(buf, end, ptr, spec, fmt); > + return device_node_string(buf, end, ptr, spec, fmt + 1); I know that this come from from kobject_string(). But please, modify it to follow the style used by other %p modifiers. I mean to pass "fmt" as is and then use: if (fmt[1] != 'F') With the above change: Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr
On Tue 2019-09-03 10:52:33, Petr Mladek wrote: > On Mon 2019-09-02 16:57:30, Sakari Ailus wrote: > > Factor out static kobject_string() function that simply calls > > device_node_string(), and thus remove references to kobjects (as these are > > struct device_node). > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > lib/vsprintf.c | 16 ++++------------ > > 1 file changed, 4 insertions(+), 12 deletions(-) > > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > > index a04a2167101ef..4ad9332d54ba6 100644 > > --- a/lib/vsprintf.c > > +++ b/lib/vsprintf.c > > @@ -1905,6 +1905,9 @@ char *device_node_string(char *buf, char *end, struct device_node *dn, > > struct printf_spec str_spec = spec; > > str_spec.field_width = -1; > > > > + if (fmt[0] != 'F') > > + return error_string(buf, end, "(%pO?)", spec); > > + > > if (!IS_ENABLED(CONFIG_OF)) > > return error_string(buf, end, "(%pOF?)", spec); > > > > @@ -1978,17 +1981,6 @@ char *device_node_string(char *buf, char *end, struct device_node *dn, > > return widen_string(buf, buf - buf_start, end, spec); > > } > > > > -static char *kobject_string(char *buf, char *end, void *ptr, > > - struct printf_spec spec, const char *fmt) > > -{ > > - switch (fmt[1]) { > > - case 'F': > > - return device_node_string(buf, end, ptr, spec, fmt + 1); > > - } > > - > > - return error_string(buf, end, "(%pO?)", 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 > > @@ -2167,7 +2159,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, > > case 'G': > > return flags_string(buf, end, ptr, spec, fmt); > > case 'O': > > - return kobject_string(buf, end, ptr, spec, fmt); > > + return device_node_string(buf, end, ptr, spec, fmt + 1); > > I know that this come from from kobject_string(). But please, modify > it to follow the style used by other %p modifiers. I mean to pass > "fmt" as is and then use: > > if (fmt[1] != 'F') Ah, I see that it would need more changes in device_node_string(). OK, let's leave the patch as is. I am sorry for the noise. Best Regards, Petr
On Mon, 2019-09-02 at 16:57 +0300, Sakari Ailus wrote: > Hi all, > > This set adds functionality into the device property API (counting a > node's parents as well as obtaining its name) in order to support printing > fwnode names using a new conversion specifier "%pfw". The names that are > produced are equivalent to its OF counterpart "%pOF" on OF systems for the > two supported modifiers ("f" and "P"). > > Printing a node's name is something that's been available on OF for a long > time and if something is converted to device property API (such as the > V4L2 fwnode framework) it always got removed of a nice feature that was > sometimes essential in debugging. With this set, that no longer is the > case. Doesn't this still have dependencies on removing all existing %p[fF] uses? In Linus' tree: tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt:or events have "%pF" or "%pS" parameter in its format string. It is common to tools/lib/traceevent/event-parse.c: if (asprintf(&format, "%%pf: (NO FORMAT FOUND at %llx)\n", addr) < 0) tools/lib/traceevent/event-parse.c: if (asprintf(&format, "%s: %s", "%pf", printk->printk) < 0) And these in -next: drivers/scsi/lpfc/lpfc_hbadisc.c: "3185 FIND node filter %pf DID " drivers/scsi/lpfc/lpfc_hbadisc.c: "3186 FIND node filter %pf NOT FOUND.\n", filter); drivers/scsi/lpfc/lpfc_sli.c: "(%d):0307 Mailbox cmd x%x (x%x/x%x) Cmpl %pf "
On Tue, Sep 03, 2019 at 11:28:16AM +0200, Petr Mladek wrote: > On Tue 2019-09-03 10:52:33, Petr Mladek wrote: > > On Mon 2019-09-02 16:57:30, Sakari Ailus wrote: > > > Factor out static kobject_string() function that simply calls > > > device_node_string(), and thus remove references to kobjects (as these are > > > struct device_node). > > > - return kobject_string(buf, end, ptr, spec, fmt); > > > + return device_node_string(buf, end, ptr, spec, fmt + 1); > > > > I know that this come from from kobject_string(). But please, modify > > it to follow the style used by other %p modifiers. I mean to pass > > "fmt" as is and then use: > > > > if (fmt[1] != 'F') > > Ah, I see that it would need more changes in device_node_string(). > OK, let's leave the patch as is. I am sorry for the noise. I came to the same conclusions, though can we consider to drop this patch?
Hi Joe, On Tue, Sep 03, 2019 at 02:38:48AM -0700, Joe Perches wrote: > On Mon, 2019-09-02 at 16:57 +0300, Sakari Ailus wrote: > > Hi all, > > > > This set adds functionality into the device property API (counting a > > node's parents as well as obtaining its name) in order to support printing > > fwnode names using a new conversion specifier "%pfw". The names that are > > produced are equivalent to its OF counterpart "%pOF" on OF systems for the > > two supported modifiers ("f" and "P"). > > > > Printing a node's name is something that's been available on OF for a long > > time and if something is converted to device property API (such as the > > V4L2 fwnode framework) it always got removed of a nice feature that was > > sometimes essential in debugging. With this set, that no longer is the > > case. > > Doesn't this still have dependencies on removing all > existing %p[fF] uses? > > In Linus' tree: > > tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt:or events have "%pF" or "%pS" parameter in its format string. It is common to > tools/lib/traceevent/event-parse.c: if (asprintf(&format, "%%pf: (NO FORMAT FOUND at %llx)\n", addr) < 0) > tools/lib/traceevent/event-parse.c: if (asprintf(&format, "%s: %s", "%pf", printk->printk) < 0) > > And these in -next: > > drivers/scsi/lpfc/lpfc_hbadisc.c: "3185 FIND node filter %pf DID " > drivers/scsi/lpfc/lpfc_hbadisc.c: "3186 FIND node filter %pf NOT FOUND.\n", filter); > drivers/scsi/lpfc/lpfc_sli.c: "(%d):0307 Mailbox cmd x%x (x%x/x%x) Cmpl %pf " Thanks for bringing these up. I'll submit patches to address both.
Hi Andy, On Mon, Sep 02, 2019 at 07:13:52PM +0300, Andy Shevchenko wrote: > On Mon, Sep 02, 2019 at 04:57:32PM +0300, Sakari Ailus wrote: > > Add a test for the %pfw printk modifier using software nodes. > > > +static void __init fwnode_pointer(void) > > +{ > > + const struct software_node softnodes[] = { > > + { .name = "first", }, > > + { .name = "second", .parent = &softnodes[0], }, > > + { .name = "third", .parent = &softnodes[1], }, > > + { NULL /* Guardian */ }, > > Comma is still here :-) Oops. I ended up removing the comma in a wrong patch which wasn't submitted to the list. Will fix for v6. > > > + }; > > > + test(full_name_second, "%pfw", > > + software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 3])); > > + test(full_name, "%pfw", > > + software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2])); > > + test(full_name, "%pfwf", > > + software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2])); > > + test(second_name, "%pfwP", > > + software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 3])); > > + test(third_name, "%pfwP", > > + software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2])); > > I have another thought about these. The test cases will fail in either of > adding, inserting or removing items in softnodes array. So, using the above > "protective" scheme doesn't bring any value except making readability worse. Agreed, to be addressed in v6.
On Wed, Sep 04, 2019 at 07:10:51PM +0300, Sakari Ailus wrote: > On Mon, Sep 02, 2019 at 07:13:52PM +0300, Andy Shevchenko wrote: > > On Mon, Sep 02, 2019 at 04:57:32PM +0300, Sakari Ailus wrote: > > > Add a test for the %pfw printk modifier using software nodes. > > > > > +static void __init fwnode_pointer(void) > > > +{ > > > + const struct software_node softnodes[] = { > > > + { .name = "first", }, > > > + { .name = "second", .parent = &softnodes[0], }, > > > + { .name = "third", .parent = &softnodes[1], }, > > > + { NULL /* Guardian */ }, > > > > Comma is still here :-) > > Oops. I ended up removing the comma in a wrong patch which wasn't submitted > to the list. Will fix for v6. Also you may remove NULL there since it's default.
On Wed, Sep 04, 2019 at 08:22:22PM +0300, Andy Shevchenko wrote: > On Wed, Sep 04, 2019 at 07:10:51PM +0300, Sakari Ailus wrote: > > On Mon, Sep 02, 2019 at 07:13:52PM +0300, Andy Shevchenko wrote: > > > On Mon, Sep 02, 2019 at 04:57:32PM +0300, Sakari Ailus wrote: > > > > Add a test for the %pfw printk modifier using software nodes. > > > > > > > +static void __init fwnode_pointer(void) > > > > +{ > > > > + const struct software_node softnodes[] = { > > > > + { .name = "first", }, > > > > + { .name = "second", .parent = &softnodes[0], }, > > > > + { .name = "third", .parent = &softnodes[1], }, > > > > + { NULL /* Guardian */ }, > > > > > > Comma is still here :-) > > > > Oops. I ended up removing the comma in a wrong patch which wasn't submitted > > to the list. Will fix for v6. > > Also you may remove NULL there since it's default. Then it'd become GCC specific. Albeit I'm not sure that's any kind of a problem in practice. I guess Clang must cope with that, too? Still, I prefer not to use compiler specific syntax if there's no need to.
On Tue, Sep 03, 2019 at 02:28:00PM +0300, Andy Shevchenko wrote: > On Tue, Sep 03, 2019 at 11:28:16AM +0200, Petr Mladek wrote: > > On Tue 2019-09-03 10:52:33, Petr Mladek wrote: > > > On Mon 2019-09-02 16:57:30, Sakari Ailus wrote: > > > > Factor out static kobject_string() function that simply calls > > > > device_node_string(), and thus remove references to kobjects (as these are > > > > struct device_node). > > > > > - return kobject_string(buf, end, ptr, spec, fmt); > > > > + return device_node_string(buf, end, ptr, spec, fmt + 1); > > > > > > I know that this come from from kobject_string(). But please, modify > > > it to follow the style used by other %p modifiers. I mean to pass > > > "fmt" as is and then use: > > > > > > if (fmt[1] != 'F') > > > > Ah, I see that it would need more changes in device_node_string(). > > OK, let's leave the patch as is. I am sorry for the noise. > > I came to the same conclusions, though can we consider to drop this patch? It's a cleanup. I'd prefer to keep the patch. Albeit fmt++; in device_node_string() would do the trick of avoiding fmt + 1 in the caller. That said, I'd prefer to keep the original patch as-is.
Hi all, This set adds functionality into the device property API (counting a node's parents as well as obtaining its name) in order to support printing fwnode names using a new conversion specifier "%pfw". The names that are produced are equivalent to its OF counterpart "%pOF" on OF systems for the two supported modifiers ("f" and "P"). Printing a node's name is something that's been available on OF for a long time and if something is converted to device property API (such as the V4L2 fwnode framework) it always got removed of a nice feature that was sometimes essential in debugging. With this set, that no longer is the case. since v4: - Improved documentation for fwnode_get_nth_parent(). - Removed comma from the guardian entry in fwnode_pointer() testcase. since v3: - Remove underscores in argument name of fwnode_count_parents(). - Re-introduce "%pO?" error string. - Unwrap a call to string() in fwnode_string(). - Removed a useless Depends-on: on a patch that was merged long ago. - Unwrap a Fixes: line. - Added a patch to move fwnode_get_parent() up to make the review of the following patch easier. since v2: - Better comments in acpi_fwnode_get_name_prefix(). - Added swnode implementation. - Fixed swnode refcounting in get_parent() ("swnode: Get reference to parent swnode in get_parent op") - Make argument to to_software_node() const (a new patch) - Factored out confusingly named kobject_string() that had a single caller. - Cleaner fwnode_count_parents() implementation (as discussed in review). - Made fwnode_count_parents() argument const. - Added tests (last patch in the set). since v1: - Add patch to remove support for %pf and %pF (depends on another patch removing all use of %pf and %pF) (now 4th patch) - Fix kerneldoc argument documentation for fwnode_get_name (2nd patch) - Align kerneldoc style with the rest of drivers/base/property.c (no extra newline after function name) - Make checkpatch.pl complain about "%pf" not followed by "w" (6th patch) - WARN_ONCE() on use of invalid conversion specifiers ("%pf" not followed by "w") Sakari Ailus (11): software node: Get reference to parent swnode in get_parent op software node: Make argument to to_software_node const device property: Move fwnode_get_parent() up device property: Add functions for accessing node's parents device property: Add fwnode_get_name for returning the name of a node device property: Add a function to obtain a node's prefix lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps lib/vsprintf: Make use of fwnode API to obtain node names and separators lib/vsprintf: OF nodes are first and foremost, struct device_nodes lib/vsprintf: Add %pfw conversion specifier for printing fwnode names lib/test_printf: Add tests for %pfw printk modifier Documentation/core-api/printk-formats.rst | 34 ++++++--- drivers/acpi/property.c | 48 +++++++++++++ drivers/base/property.c | 83 +++++++++++++++++++-- drivers/base/swnode.c | 55 +++++++++++++- drivers/of/property.c | 16 +++++ include/linux/fwnode.h | 4 ++ include/linux/property.h | 8 ++- lib/test_printf.c | 37 ++++++++++ lib/vsprintf.c | 88 ++++++++++++++--------- scripts/checkpatch.pl | 4 +- 10 files changed, 319 insertions(+), 58 deletions(-)