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.
--
With Best Regards,
Andy Shevchenko
