Date:   Tue, 26 Mar 2019 15:13:53 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     Petr Mladek <pmladek@...e.com>, linux-kernel@...r.kernel.org,
        rafael@...nel.org, linux-acpi@...r.kernel.org,
        devicetree@...r.kernel.org,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Subject: Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for
 printing fwnode names

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@...ndpoint@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

Powered by blists - more mailing lists