Hi,

On Tue, Aug 14, 2018 at 05:52:49PM -0700, Florian Fainelli wrote:

On 08/14/2018 05:22 PM, Brian Norris wrote:

quoted

 * in any of the above (and in any other case of lack of clarity), one
   can make slightly different choices when, e.g., submitting a device
   tree upstream vs. a downstream tree. While we may try our hardest to
   document and stick to documented bindings, I personally can't
   guarantee that one of these choices will be made differently during
   review, possibly breaking any firmware that made assumptions based on
   those choices. So I might end up with a firmware that satisfies
   documented bindings and works with a downstream device tree, but
   doesn't work with a device tree that gets submitted upstream.

Sure, this is kind of a self inflicted problem but agreed this does exist.

You can say "self-inflicted", but of all the things that need to go
upstream, the DTS files themselves are the least integral. I mean, why
else do we ever pretend to have anything close to an ABI for device
tree bindings?

quoted

quoted

Also, aliases in DT are meant to provide some stability.

How, specifically? I don't see any relevant binding description for
aliases under Documentation/devicetree/bindings/net/.

Indeed they are not, likewise, we should probably update devicetree-spec
to come up with standard names that of_alias_get_id() already consumes.

A quick grep shows we already have divergence: both "eth" and "ethernet"
are in use.

But anyway, would the idea be that you just put 'ethernet{0,1,...}' and
'wifi{0,1,...}' aliases in the /chosen node, then require boot firmware
to insert any {ethernet,wifi}_mac{0,1,...} into the paths represented by
the corresponding aliases? I suppose that would reduce the problems with
(1), but it still doesn't really help with (2).

In some circles, the gold standard of boot firmware is to be as thin as
possible, doing only what's needed to get a kernel up and running, and
this function seems wholly unrelated to the firmware's core
functionality. I mean, the kernel already knows how to parse VPD, so why
can't it learn to find the right field?

quoted

quoted

quoted

 (2) Other than this device-tree shim requirement, system firmware may
     have no reason to understand anything about network devices.

So instead, I'm looking for a way to have a device node describe where
to find its MAC address, rather than having the device node contain the
MAC address directly. Then system firmware doesn't have to manage
anything.

In particular, I add support for the Google Vital Product Data (VPD)
format, used within the Coreboot project. The format is described here:

https://chromium.googlesource.com/chromiumos/platform/vpd/+/master/README.md

TL;DR: VPD consists of a TLV-like table, with key/value pairs of
strings. This is often stored persistently on the boot flash and
presented via in-memory Coreboot tables, for the operating system to
read.

We already have a VPD driver that parses this table and presents it to
user space. This series extends that driver to allow in-kernel lookups
of MAC address entries.

A possible alternative approach is to have the VPD driver become a NVMEM
producer to expose the VPD keys, did you look into that and possibly
found that it was not a good model? The downside to that approach though
is that you might have to have a phandle for the VPD provider in the
Device Tree, but AFAICS this should solve your needs?

I did notice some NVMEM work. The MTD links you point at shouldn't be
relevant, since this table is already present in RAM. But I suppose I
could shoehorn the memory table into being a fake NVMEM...

And then, how would you recommend doing the parameterization I note
here? Is the expectation that I define a new cell for every single type?
Each cell might have a different binary format, so I'd have to describe:
(a) that they contain MAC addresses (so the "reader" knows to translate
    the ASCII strings into equivalent binary representation) and

I see, in your current patch series that knowledge is pushed to both the
VPD producer and the specific object lookup function, so this scales better.

Yeah, one of the advantages is that my API is specialized to exactly one
data type ;) With the nvmem API, the data format isn't really specified,
so you gotta hope that either the NVMEM stores MAC addresses as 6 bytes
of binary data, or else that the NVMEM driver figures out how to do any
translation for you implicitly.

If I understand the NVMEM subsystem correctly, that is.

quoted

(b) which key matches (it's not just "mac_address=xxxxx"; there may be
    many MAC addresses, with keys "ether_mac0", "ether_mac1",
    "wifi_mac0")

The key to lookup is definitively node specific, it is just unfortunate
that there is not a better way to infer which key to lookup for (as
opposed to just having to specify it directly) based on the Device Tree
topology. By that I mean, if you have a "mac-address-lookup" property
associated with Wi-Fi adapter #1 (with numbering starting at 0), then
this automatically means looking up for "wifi_mac1", etc.

Would that really be a virtue, though? Keys can really be anything (in
VPD, or in any other hypothetical MAC address store), and it seems nice
to avoid entangling them with device tree specifics too much. And how
does one figure out what's Device 0 anyway? Based on the FDT layout? I
don't actually know what order 'dtc' puts my nodes in.

quoted

Part (a) especially doesn't really sound like the typical NVMEM, which
seems to pretend it provides raw access to these memory regions.

Additionally, VPD is not stored at a fixed address, nor are any
particular entries within it (it uses TLV), so it seems like there are
plenty of other bits of the nvmem.txt documentation I'd have to violate
to get there, such as the definition of 'reg':

reg:    specifies the offset in byte within the storage device.

And finally, this may be surmountable, but the existing APIs seem very
device tree centric. We use this same format on ACPI systems, and the
current series would theoretically work on both [1]. I'd have to rewrite
the current (OF-only) helpers to get equivalent support...

All fair points, never mind NVMEM, I was just too keen on thinking this
would be finally the way to make the consumers and producers of such
information into a single API, but your proposal appears valid too.

I don't want to throw out any notion of unification from the start, but
I don't immediately see how one would do that reasonably. I'm still open
to education though, and I'm definitely not wedded to my specific
proposal.

Is ChromeOS' directly inspired from the PCI's spec VPD?

I really don't have any idea. From glancing at the PCI spec, its format
isn't very similar, relative to how simple each of them are.

quoted

BTW, it's quite annoying that we have all of these:

fwnode_get_mac_address()
device_get_mac_address()
of_get_mac_address()
of_get_nvmem_mac_address()

and only 2 of those share any code! Brilliant!

Sounds like you just found another area to improve on ;)

Sure ;) I knew about the useless duplication of of_get_mac_address(),
but I wasn't aware of of_get_nvmem_mac_address(). It seems like it might
have come out of a deficiency that I already noticed in
of_get_mac_address(): that it assumes you can return a const pointer.

I'll probably try to remove as many uses of of_get_mac_address() as
possible and settle on fwnode_get_mac_address() /
device_get_mac_address(). Probably would be good to have
fwnode_get_mac_address() also do the of_get_nvmem_mac_address() work and
deprecate of_get_nvmem_mac_address() too.

Regards,
Brian