Message-ID: <20180815014436.GA17200@ban.mtv.corp.google.com>
Date:   Tue, 14 Aug 2018 18:44:36 -0700
From:   Brian Norris <computersforpeace@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Guenter Roeck <linux@...ck-us.net>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Julius Werner <jwerner@...omium.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Brian Norris <briannorris@...omium.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Subject: Re: [RFC PATCH v1 0/3] device property: Support MAC address in VPD

Hi,

On Tue, Aug 14, 2018 at 05:52:49PM -0700, Florian Fainelli wrote:
> On 08/14/2018 05:22 PM, Brian Norris wrote:
> >  * 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?

> >> 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?

> >>>  (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.

> > (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.

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

> > 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

Powered by blists - more mailing lists