| Message ID | 20190322152930.16642-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Device property improvements, add %pfw format specifier | expand |
On Fri, Mar 22, 2019 at 05:29:30PM +0200, 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. Do we encourage people to use it instead of %pOF cases where it is suitable? > 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. Do we support swnode here? > +static noinline_for_stack > +char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode, > + struct printf_spec spec, const char *fmt) > +{ > + const char * const modifiers = "fP"; > + struct printf_spec str_spec = spec; > + char *buf_start = buf; > + bool pass; > + > + str_spec.field_width = -1; > + > + if ((unsigned long)fwnode < PAGE_SIZE) > + return string(buf, end, "(null)", spec); Just put there a NULL pointer, we would not like to maintain duplicated strings over the kernel. I remember Petr has a patch series related to address space check, though I don't remember the status of affairs. > + > + /* simple case without anything any more format specifiers */ > + fmt++; > + if (fmt[0] == '\0' || strcspn(fmt, modifiers) > 0) > + fmt = "f"; > + > + for (pass = false; strspn(fmt, modifiers); fmt++, pass = true) { I don't see test cases. What would we get out of %pfwfffPPPfff? Hint: I'm expecting above to be equivalent to %pfwf > + if (pass) { > + if (buf < end) > + *buf = ':'; > + buf++; > + } > + > + switch (*fmt) { > + case 'f': /* full_name */ > + buf = fwnode_gen_full_name(fwnode, buf, end); > + break; > + case 'P': /* name */ > + buf = string(buf, end, fwnode_get_name(fwnode), > + str_spec); > + break; > + default: > + break; > + } > + } > + > + return widen_string(buf, buf - buf_start, end, spec); > +}
Hi Andy, Thanks for the comments. On Fri, Mar 22, 2019 at 07:21:14PM +0200, Andy Shevchenko wrote: > On Fri, Mar 22, 2019 at 05:29:30PM +0200, 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. > > Do we encourage people to use it instead of %pOF cases where it is suitable? For code that is used on both OF and ACPI based systems, I think so. But if you have something that is only used on OF, %pOF is better --- it has more functionality that seems quite OF specific. In general I think the ability to print a node's full name is way more important on OF. On ACPI you don't need it so often --- which is probably the reason it hasn't been supported. > > > 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. > > Do we support swnode here? Good question. The swnodes have no hierarchy at the moment (they're only created for a struct device as a parent) and they do not have human-readable names. So I'd say it's not relevant right now. Should these two change, support for swnode could (and should) be added later on. > > > +static noinline_for_stack > > +char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode, > > + struct printf_spec spec, const char *fmt) > > +{ > > + const char * const modifiers = "fP"; > > + struct printf_spec str_spec = spec; > > + char *buf_start = buf; > > + bool pass; > > + > > + str_spec.field_width = -1; > > + > > > + if ((unsigned long)fwnode < PAGE_SIZE) > > + return string(buf, end, "(null)", spec); > > Just put there a NULL pointer, we would not like to maintain duplicated strings > over the kernel. > > I remember Petr has a patch series related to address space check, though I > don't remember the status of affairs. This bit has been actually adopted from the OF counterpart. If there are improvements in this area, then I'd just change both at the same time. > > > + > > + /* simple case without anything any more format specifiers */ > > + fmt++; > > + if (fmt[0] == '\0' || strcspn(fmt, modifiers) > 0) > > + fmt = "f"; > > + > > + for (pass = false; strspn(fmt, modifiers); fmt++, pass = true) { > > I don't see test cases. > > What would we get out of %pfwfffPPPfff? > > Hint: I'm expecting above to be equivalent to %pfwf I guess it's a matter of expectations. :-) Again this works the same way than the OF counterpart. Right now there's little to print (just the name and the full name), but if support is added for more, then this mechanism is fully relevant again. The alternative would be to remove that now and add it back if it's needed again. I have a slight preference towards keeping it extensible (i.e. as it's now). > > > + if (pass) { > > + if (buf < end) > > + *buf = ':'; > > + buf++; > > + } > > + > > + switch (*fmt) { > > + case 'f': /* full_name */ > > + buf = fwnode_gen_full_name(fwnode, buf, end); > > + break; > > + case 'P': /* name */ > > + buf = string(buf, end, fwnode_get_name(fwnode), > > + str_spec); > > + break; > > + default: > > + break; > > + } > > + } > > + > > + return widen_string(buf, buf - buf_start, end, spec); > > +} >
On Sun, Mar 24, 2019 at 08:17:46PM +0200, Sakari Ailus wrote: > On Fri, Mar 22, 2019 at 07:21:14PM +0200, Andy Shevchenko wrote: > > On Fri, Mar 22, 2019 at 05:29:30PM +0200, 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. > > > > Do we encourage people to use it instead of %pOF cases where it is suitable? > > For code that is used on both OF and ACPI based systems, I think so. But if > you have something that is only used on OF, %pOF is better --- it has more > functionality that seems quite OF specific. In general I think the ability > to print a node's full name is way more important on OF. On ACPI you don't > need it so often --- which is probably the reason it hasn't been supported. But if code is going to support ACPI and DT and at the same time use %pOF extensions that are not covered by %pfw it would be inconsistent somehow. > > > 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. > > > > Do we support swnode here? > > Good question. The swnodes have no hierarchy at the moment (they're only > created for a struct device as a parent) and they do not have human-readable > names. So I'd say it's not relevant right now. Should these two change, > support for swnode could (and should) be added later on. Heikki, what do you think about this? > > > + if ((unsigned long)fwnode < PAGE_SIZE) > > > + return string(buf, end, "(null)", spec); > > > > Just put there a NULL pointer, we would not like to maintain duplicated strings > > over the kernel. > > > > I remember Petr has a patch series related to address space check, though I > > don't remember the status of affairs. > > This bit has been actually adopted from the OF counterpart. If there are > improvements in this area, then I'd just change both at the same time. The patch series by Petr I mentioned takes care about OF case. But it doesn't have covered yours by obvious reasons. > > > + for (pass = false; strspn(fmt, modifiers); fmt++, pass = true) { > > > > I don't see test cases. > > > > What would we get out of %pfwfffPPPfff? > > > > Hint: I'm expecting above to be equivalent to %pfwf > > I guess it's a matter of expectations. :-) Common sense and basic expectations from all of %p extensions. > Again this works the same way > than the OF counterpart. OF lacks of testing apparently. > Right now there's little to print (just the name > and the full name), but if support is added for more, then this mechanism is > fully relevant again. > > The alternative would be to remove that now and add it back if it's needed > again. I have a slight preference towards keeping it extensible (i.e. as > it's now). See how other helpers do parse this.
Hi Andy, On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote: > On Sun, Mar 24, 2019 at 08:17:46PM +0200, Sakari Ailus wrote: > > On Fri, Mar 22, 2019 at 07:21:14PM +0200, Andy Shevchenko wrote: > > > On Fri, Mar 22, 2019 at 05:29:30PM +0200, 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. > > > > > > Do we encourage people to use it instead of %pOF cases where it is suitable? > > > > For code that is used on both OF and ACPI based systems, I think so. But if > > you have something that is only used on OF, %pOF is better --- it has more > > functionality that seems quite OF specific. In general I think the ability > > to print a node's full name is way more important on OF. On ACPI you don't > > need it so often --- which is probably the reason it hasn't been supported. > > But if code is going to support ACPI and DT and at the same time use %pOF > extensions that are not covered by %pfw it would be inconsistent somehow. What you mostly need are the full name and the name of a given node. I wasn't sure whether adding more would have been relevant, and at least it is likely to have few if any users, so I didn't add that yet. Do not that it can be implemented later on if it's needed --- that's also why the modifiers are aligned with %pOF. > > > > > 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. > > > > > > Do we support swnode here? > > > > Good question. The swnodes have no hierarchy at the moment (they're only > > created for a struct device as a parent) and they do not have human-readable > > names. So I'd say it's not relevant right now. Should these two change, > > support for swnode could (and should) be added later on. > > Heikki, what do you think about this? > > > > > + if ((unsigned long)fwnode < PAGE_SIZE) > > > > + return string(buf, end, "(null)", spec); > > > > > > Just put there a NULL pointer, we would not like to maintain duplicated strings > > > over the kernel. > > > > > > I remember Petr has a patch series related to address space check, though I > > > don't remember the status of affairs. > > > > This bit has been actually adopted from the OF counterpart. If there are > > improvements in this area, then I'd just change both at the same time. > > The patch series by Petr I mentioned takes care about OF case. But it doesn't > have covered yours by obvious reasons. Do you happen to have a pointer to it? > > > > > + for (pass = false; strspn(fmt, modifiers); fmt++, pass = true) { > > > > > > I don't see test cases. > > > > > > What would we get out of %pfwfffPPPfff? > > > > > > Hint: I'm expecting above to be equivalent to %pfwf > > > > I guess it's a matter of expectations. :-) > > Common sense and basic expectations from all of %p extensions. > > > Again this works the same way > > than the OF counterpart. > > OF lacks of testing apparently. > > > Right now there's little to print (just the name > > and the full name), but if support is added for more, then this mechanism is > > fully relevant again. > > > > The alternative would be to remove that now and add it back if it's needed > > again. I have a slight preference towards keeping it extensible (i.e. as > > it's now). > > See how other helpers do parse this. The behaviour on others is different indeed, you're generally printing a single item at a time. The question rather is, whether we want to be compatible with %pOF going forward or not. I'd prefer that, so using the fwnode API would be easier.
On Tue, Mar 26, 2019 at 03:39:47PM +0200, Sakari Ailus wrote: > On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote: > > On Sun, Mar 24, 2019 at 08:17:46PM +0200, Sakari Ailus wrote: > > The patch series by Petr I mentioned takes care about OF case. But it doesn't > > have covered yours by obvious reasons. > > Do you happen to have a pointer to it? Petr, can you share what is the state of affairs with that series? > The behaviour on others is different indeed, you're generally printing a > single item at a time. The question rather is, whether we want to be > compatible with %pOF going forward or not. I'd prefer that, so using the > fwnode API would be easier. I would prefer to mimic %pOF and actually to deprecate it in favour of %pfw. But it's just mine opinion. I'm skeptical about getting support on it.
Hi, On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote: > > > > 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. > > > > > > Do we support swnode here? > > > > Good question. The swnodes have no hierarchy at the moment (they're only > > created for a struct device as a parent) and they do not have human-readable > > names. So I'd say it's not relevant right now. Should these two change, > > support for swnode could (and should) be added later on. > > Heikki, what do you think about this? Well, the swnodes do have hierarchy. That was kind of the whole point of introducing them. They now can also be named using "name" property. See commit 344798206f171c5abea7ab1f9762fa526d7f539d. thanks,
Hi Andy, On Tue, Mar 26, 2019 at 03:55:57PM +0200, Andy Shevchenko wrote: > On Tue, Mar 26, 2019 at 03:39:47PM +0200, Sakari Ailus wrote: > > On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote: > > > On Sun, Mar 24, 2019 at 08:17:46PM +0200, Sakari Ailus wrote: > > > > The patch series by Petr I mentioned takes care about OF case. But it doesn't > > > have covered yours by obvious reasons. > > > > Do you happen to have a pointer to it? > > Petr, can you share what is the state of affairs with that series? > > > The behaviour on others is different indeed, you're generally printing a > > single item at a time. The question rather is, whether we want to be > > compatible with %pOF going forward or not. I'd prefer that, so using the > > fwnode API would be easier. > > I would prefer to mimic %pOF and actually to deprecate it in favour of %pfw. > But it's just mine opinion. I'm skeptical about getting support on it. IMHO code that only deals with OF specifically is better to continue to use %pOF. You'd have of_fwnode_handle() in places where you just had the name of the node previously. What could be done though is to unify the implementations; that's something which the set does a little of already. Cc Rob, too.
Moi, On Tue, Mar 26, 2019 at 04:06:33PM +0200, Heikki Krogerus wrote: > Hi, > > On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote: > > > > > 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. > > > > > > > > Do we support swnode here? > > > > > > Good question. The swnodes have no hierarchy at the moment (they're only > > > created for a struct device as a parent) and they do not have human-readable > > > names. So I'd say it's not relevant right now. Should these two change, > > > support for swnode could (and should) be added later on. > > > > Heikki, what do you think about this? > > Well, the swnodes do have hierarchy. That was kind of the whole point > of introducing them. They now can also be named using "name" property. > See commit 344798206f171c5abea7ab1f9762fa526d7f539d. Right; I saw the function after initially replying to Andy but I missed where the node name came from. :-) Now I know... I can add support for swnode, too, if you like.
On Tue, Mar 26, 2019 at 04:12:43PM +0200, Sakari Ailus wrote: > On Tue, Mar 26, 2019 at 04:06:33PM +0200, Heikki Krogerus wrote: > > On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote: > > > > > Do we support swnode here? > > > > > > > > Good question. The swnodes have no hierarchy at the moment (they're only > > > > created for a struct device as a parent) and they do not have human-readable > > > > names. So I'd say it's not relevant right now. Should these two change, > > > > support for swnode could (and should) be added later on. > > > > > > Heikki, what do you think about this? > > > > Well, the swnodes do have hierarchy. That was kind of the whole point > > of introducing them. They now can also be named using "name" property. > > See commit 344798206f171c5abea7ab1f9762fa526d7f539d. > > Right; I saw the function after initially replying to Andy but I missed > where the node name came from. :-) Now I know... > > I can add support for swnode, too, if you like. Definitely!
On Tue, Mar 26, 2019 at 04:12:43PM +0200, Sakari Ailus wrote: > Moi, > > On Tue, Mar 26, 2019 at 04:06:33PM +0200, Heikki Krogerus wrote: > > Hi, > > > > On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote: > > > > > > 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. > > > > > > > > > > Do we support swnode here? > > > > > > > > Good question. The swnodes have no hierarchy at the moment (they're only > > > > created for a struct device as a parent) and they do not have human-readable > > > > names. So I'd say it's not relevant right now. Should these two change, > > > > support for swnode could (and should) be added later on. > > > > > > Heikki, what do you think about this? > > > > Well, the swnodes do have hierarchy. That was kind of the whole point > > of introducing them. They now can also be named using "name" property. > > See commit 344798206f171c5abea7ab1f9762fa526d7f539d. > > Right; I saw the function after initially replying to Andy but I missed > where the node name came from. :-) Now I know... > > I can add support for swnode, too, if you like. It's up to you. thanks,
On Fri 2019-03-22 17:29:30, 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. > > 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. > > Depends-on: ("vsprintf: Remove support for %pF and %pf in favour of %pS and %ps") Reusing obsolete modifiers is dangerous from many reasons: + people might miss the change of the meaning + backporting mistakes + 3rd party modules It might be acceptable if the long term gain is bigger than a short time difficulties. But it would be better to it a safe way when possible. Fortunately, we could keep the backward compatibility for "%pf" and handle only "%pfw*" with the fwnode api. Best Regards, Petr
On Tue 2019-03-26 15:55:57, Andy Shevchenko wrote: > On Tue, Mar 26, 2019 at 03:39:47PM +0200, Sakari Ailus wrote: > > On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote: > > > On Sun, Mar 24, 2019 at 08:17:46PM +0200, Sakari Ailus wrote: > > > > The patch series by Petr I mentioned takes care about OF case. But it doesn't > > > have covered yours by obvious reasons. > > > > Do you happen to have a pointer to it? > > Petr, can you share what is the state of affairs with that series? I might send a new version this week but I do not promise it. It is regularly pushed aside by more urgent work. But we could work on both changes independently. Both can be updated easily depending on which one gets accepted earlier. Best Regards, Petr
On Tue 2019-03-26 16:30:21, Andy Shevchenko wrote: > On Tue, Mar 26, 2019 at 04:12:43PM +0200, Sakari Ailus wrote: > > On Tue, Mar 26, 2019 at 04:06:33PM +0200, Heikki Krogerus wrote: > > > On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote: > > > > > > > Do we support swnode here? > > > > > > > > > > Good question. The swnodes have no hierarchy at the moment (they're only > > > > > created for a struct device as a parent) and they do not have human-readable > > > > > names. So I'd say it's not relevant right now. Should these two change, > > > > > support for swnode could (and should) be added later on. > > > > > > > > Heikki, what do you think about this? > > > > > > Well, the swnodes do have hierarchy. That was kind of the whole point > > > of introducing them. They now can also be named using "name" property. > > > See commit 344798206f171c5abea7ab1f9762fa526d7f539d. > > > > Right; I saw the function after initially replying to Andy but I missed > > where the node name came from. :-) Now I know... > > > > I can add support for swnode, too, if you like. > > Definitely! It might really make sense to obsolete %pOF and handle all three (OF, ACPI, Software) nodes using the same %pfw modifiers. If I get it correctly, we could distinguish them by fwnode->ops, see is_of_node(), is_acpi_static_node(), is_software_node(). Best Regards, Petr
On Fri 2019-03-22 17:29:29, Sakari Ailus wrote: > Instead of implementing our own means of discovering parent nodes, node > names or counting how many parents a node has, use the newly added > functions in the fwnode API to obtain that information. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > lib/vsprintf.c | 37 +++++++++++++++---------------------- > 1 file changed, 15 insertions(+), 22 deletions(-) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 5f60b8d41277..91f2a3e4892e 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -37,6 +37,7 @@ > #include <net/addrconf.h> > #include <linux/siphash.h> > #include <linux/compiler.h> > +#include <linux/property.h> > #ifdef CONFIG_BLOCK > #include <linux/blkdev.h> > #endif > @@ -1720,32 +1721,23 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) > return format_flags(buf, end, flags, names); > } > > -static const char *device_node_name_for_depth(const struct device_node *np, int depth) > -{ > - for ( ; np && depth; depth--) > - np = np->parent; > - > - return kbasename(np->full_name); > -} > - > static noinline_for_stack > -char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end) > +char *fwnode_gen_full_name(struct fwnode_handle *fwnode, char *buf, char *end) I first read this as fwnode_get_full_name(), see "get" vs. "gen". I guess that it was because there is used also fwnode_get_name(). Please, rename it to fwnode_full_name_string() or so ;-) Best Regards, Petr
Hi Petr, Thank you for reviewing these patches. On Wed, Mar 27, 2019 at 01:53:56PM +0100, Petr Mladek wrote: > On Fri 2019-03-22 17:29:29, Sakari Ailus wrote: > > Instead of implementing our own means of discovering parent nodes, node > > names or counting how many parents a node has, use the newly added > > functions in the fwnode API to obtain that information. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > lib/vsprintf.c | 37 +++++++++++++++---------------------- > > 1 file changed, 15 insertions(+), 22 deletions(-) > > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > > index 5f60b8d41277..91f2a3e4892e 100644 > > --- a/lib/vsprintf.c > > +++ b/lib/vsprintf.c > > @@ -37,6 +37,7 @@ > > #include <net/addrconf.h> > > #include <linux/siphash.h> > > #include <linux/compiler.h> > > +#include <linux/property.h> > > #ifdef CONFIG_BLOCK > > #include <linux/blkdev.h> > > #endif > > @@ -1720,32 +1721,23 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) > > return format_flags(buf, end, flags, names); > > } > > > > -static const char *device_node_name_for_depth(const struct device_node *np, int depth) > > -{ > > - for ( ; np && depth; depth--) > > - np = np->parent; > > - > > - return kbasename(np->full_name); > > -} > > - > > static noinline_for_stack > > -char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end) > > +char *fwnode_gen_full_name(struct fwnode_handle *fwnode, char *buf, char *end) > > I first read this as fwnode_get_full_name(), see "get" vs. "gen". > I guess that it was because there is used also fwnode_get_name(). > > Please, rename it to fwnode_full_name_string() or so ;-) Works for me.
Hi Petr, On Tue, Mar 26, 2019 at 04:13:07PM +0100, Petr Mladek wrote: > On Fri 2019-03-22 17:29:30, 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. > > > > 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. > > > > Depends-on: ("vsprintf: Remove support for %pF and %pf in favour of %pS and %ps") > > Reusing obsolete modifiers is dangerous from many reasons: > > + people might miss the change of the meaning > + backporting mistakes > + 3rd party modules > > It might be acceptable if the long term gain is bigger > than a short time difficulties. But it would be better > to it a safe way when possible. > > Fortunately, we could keep the backward compatibility > for "%pf" and handle only "%pfw*" with the fwnode api. The v2 of this patch produces a warning (using WARN_ONCE()) for "%pf" not immediately followed by "w". "%pfw" was not a valid conversion specifier before this set, so we're actually not re-using the exactly same conversion specifiers.
On Wed 2019-03-27 16:10:48, Sakari Ailus wrote: > Hi Petr, > > On Tue, Mar 26, 2019 at 04:13:07PM +0100, Petr Mladek wrote: > > On Fri 2019-03-22 17:29:30, 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. > > > > > > 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. > > > > > > Depends-on: ("vsprintf: Remove support for %pF and %pf in favour of %pS and %ps") > > > > Reusing obsolete modifiers is dangerous from many reasons: > > > > + people might miss the change of the meaning > > + backporting mistakes > > + 3rd party modules > > > > It might be acceptable if the long term gain is bigger > > than a short time difficulties. But it would be better > > to it a safe way when possible. > > > > Fortunately, we could keep the backward compatibility > > for "%pf" and handle only "%pfw*" with the fwnode api. > > The v2 of this patch produces a warning (using WARN_ONCE()) for "%pf" not > immediately followed by "w". "%pfw" was not a valid conversion specifier > before this set, so we're actually not re-using the exactly same conversion > specifiers. I see. I would keep the two patchsets separate. I mean that this patchset should expect that the original handling of %pf is still there. The way how to remove or obsolete "%pf" should be handled in the patchset removing all %pf users or it can be done in a completely separate patch. The conflict can get handled when merging the two patchsets. Best Regards, Petr
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. This set depends on a cleanup set fully releasing the now-deprecated %pf for other use: <URL:https://lore.kernel.org/lkml/20190322135350.2btpno7vspvewxvk@paasikivi.fi.intel.com/T/#t> I was a bit hesitant on how much of this should be covered by the fwnode ops and how much would be just better to reside in lib/vsprintf.c, but opted for putting it to the fwnode interface. This keeps it clean and still rather lean on the fwnode ops side while making it possible to add other firmware types (although that is unlikely I guess). Sakari Ailus (5): 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: Make use of fwnode API to obtain node names and separators lib/vsprintf: Add %pfw conversion specifier for printing fwnode names Documentation/core-api/printk-formats.rst | 24 +++++++++ drivers/acpi/property.c | 46 ++++++++++++++++ drivers/base/property.c | 86 ++++++++++++++++++++++++++--- drivers/of/property.c | 16 ++++++ include/linux/fwnode.h | 4 ++ include/linux/property.h | 5 ++ lib/vsprintf.c | 89 +++++++++++++++++++++++-------- scripts/checkpatch.pl | 2 +- 8 files changed, 242 insertions(+), 30 deletions(-)