[RFC PATCH v1 0/3] device property: Support MAC address in VPD

[RFC PATCH v1 0/3] device property: Support MAC address in VPD

[Brian Norris]

Hi all,

Preface: I'm sure there are at least a few minor issues with this
patchset as-is. But I'd appreciate ("RFC") if I can get general feedback
on the approach here; perhaps there are alternatives, or perhaps I've
missed similar proposals in the past. (My problems don't feel all that
unique.)

---

Today, we have generic support for 'mac-address' and 'local-mac-address'
properties in both Device Tree nodes and in generic Device Properties,
such that network device drivers can pick up a hardware address from
there, in cases where the MAC address isn't baked into the network card.
This method of MAC address retrieval presumes that either:
(a) there's a unique device tree (or similar) stored on a given device
    or
(b) some other entity (e.g., boot firmware) will modify device nodes
    runtime to place that MAC address into the appropriate device
    properties.

Option (a) is not feasbile for many systems.

Option (b) can work, but there are some reasons why one might not want
to do that:
 (1) This requires that system firmware understand the device tree
     structure, sometimes to the point of memorizing path names (e.g.,
     /soc/wifi@xxxxxxxx). At least for Device Tree, these path names are
     not necessarily an ABI, and so this introduces unneeded fragility.
 (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.

Thanks,
Brian


Brian Norris (3):
  dt-bindings: net: Add 'mac-address-lookup' property
  device property: Support complex MAC address lookup
  firmware: vpd: add MAC address parser

 .../devicetree/bindings/net/ethernet.txt      | 12 +++
 drivers/base/property.c                       | 83 ++++++++++++++++++-
 drivers/firmware/google/vpd.c                 | 67 +++++++++++++++
 include/linux/property.h                      | 23 +++++
 4 files changed, 183 insertions(+), 2 deletions(-)

-- 
2.18.0.865.gffc8e1a3cd6-goog

[RFC PATCH v1 1/3] dt-bindings: net: Add 'mac-address-lookup' property

[Brian Norris]

Some firmwares present data tables that can be parsed to retrieve
device-specific details, like MAC addresses. While in some cases, one
could teach the firmware to understand the device tree format and insert
a 'mac-address'/'local-mac-address' property into the FDT on its own,
this method can be brittle (e.g., involving memorizing expected FDT
structure), and it's not strictly necessary -- especially when parsers
for such firmware formats are already needed in the OS for other
reasons.

One such format: the Vital Product Data (VPD) [1] used by Coreboot. It
supports a table of key/value pairs, and some systems keep MAC addresses
there in a well-known format. Allow a device tree to specify
  (1) that the MAC address for a given device is stored in the VPD table
      and
  (2) what key should be used to retrieve the MAC address for said
      device (e.g., "ethernet_mac0" or "wifi_mac1").

[1] Ref:
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.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 Documentation/devicetree/bindings/net/ethernet.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
index cfc376bc977a..d3fd1da18bf4 100644
--- a/Documentation/devicetree/bindings/net/ethernet.txt
+++ b/Documentation/devicetree/bindings/net/ethernet.txt
@@ -4,6 +4,18 @@ NOTE: All 'phy*' properties documented below are Ethernet specific. For the
 generic PHY 'phys' property, see
 Documentation/devicetree/bindings/phy/phy-bindings.txt.
 
+- mac-address-lookup: string, indicating a method by which a MAC address may be
+  discovered for this device. Methods may be parameterized by some value, such
+  that the method can determine the device's MAC address using that parameter.
+  For example, a firmware might store MAC addresses in a table, keyed by some
+  predetermined string, and baked in read-only flash. A lookup method "foo"
+  with a parameter "bar" should be written "foo:bar".
+  Supported values for method:
+    "google-vpd" - Google's Vital Product Data (VPD), as used in the Coreboot
+      project. Documentation for VPD can be found at:
+        https://chromium.googlesource.com/chromiumos/platform/vpd/+/master/README.md
+  Example:
+    mac-address-lookup = "google-vpd:ethernet_mac0"
 - local-mac-address: array of 6 bytes, specifies the MAC address that was
   assigned to the network device;
 - mac-address: array of 6 bytes, specifies the MAC address that was last used by
-- 
2.18.0.865.gffc8e1a3cd6-goog

Re: [RFC PATCH v1 1/3] dt-bindings: net: Add 'mac-address-lookup' property

[Rob Herring]

On Tue, Aug 14, 2018 at 03:37:56PM -0700, Brian Norris wrote:
> Some firmwares present data tables that can be parsed to retrieve
> device-specific details, like MAC addresses. While in some cases, one
> could teach the firmware to understand the device tree format and insert
> a 'mac-address'/'local-mac-address' property into the FDT on its own,
> this method can be brittle (e.g., involving memorizing expected FDT
> structure), and it's not strictly necessary -- especially when parsers
> for such firmware formats are already needed in the OS for other
> reasons.

If you have an 'ethernetN' alias then you don't need to know the 
structure. IIRC, that is what u-boot does.

> 
> One such format: the Vital Product Data (VPD) [1] used by Coreboot. It
> supports a table of key/value pairs, and some systems keep MAC addresses
> there in a well-known format. Allow a device tree to specify
>   (1) that the MAC address for a given device is stored in the VPD table
>       and
>   (2) what key should be used to retrieve the MAC address for said
>       device (e.g., "ethernet_mac0" or "wifi_mac1").
> 
> [1] Ref:
> 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.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>  Documentation/devicetree/bindings/net/ethernet.txt | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
> index cfc376bc977a..d3fd1da18bf4 100644
> --- a/Documentation/devicetree/bindings/net/ethernet.txt
> +++ b/Documentation/devicetree/bindings/net/ethernet.txt
> @@ -4,6 +4,18 @@ NOTE: All 'phy*' properties documented below are Ethernet specific. For the
>  generic PHY 'phys' property, see
>  Documentation/devicetree/bindings/phy/phy-bindings.txt.
>  
> +- mac-address-lookup: string, indicating a method by which a MAC address may be
> +  discovered for this device. Methods may be parameterized by some value, such
> +  that the method can determine the device's MAC address using that parameter.
> +  For example, a firmware might store MAC addresses in a table, keyed by some
> +  predetermined string, and baked in read-only flash. A lookup method "foo"
> +  with a parameter "bar" should be written "foo:bar".
> +  Supported values for method:
> +    "google-vpd" - Google's Vital Product Data (VPD), as used in the Coreboot
> +      project. Documentation for VPD can be found at:
> +        https://chromium.googlesource.com/chromiumos/platform/vpd/+/master/README.md
> +  Example:
> +    mac-address-lookup = "google-vpd:ethernet_mac0"

This doesn't strike me as a very DT style way of describing this. I 
would expect something like a phandle to the VPD and an identifier. 

Also, an already common way besides local-mac-address is using nvmem 
binding which wouldn't use this and perhaps could be used here? This 
feels very much Google specific, not common (and maybe that is fine).

Rob

[RFC PATCH v1 2/3] device property: Support complex MAC address lookup

[Brian Norris]

Some firmwares include data tables that can be used to derive a MAC
address for devices on the system, but those firmwares don't directly
stash the MAC address in a device property. Support having other drivers
register lookup functions, where the device property contains
"<format>:<key>" strings; a lookup driver can register support for
handling "<format>", and "<key>" can be used by the format parser to
identify which MAC address is being requested.

This is particularly useful for the Google Vital Product Data (VPD)
format [1], which holds various product-specific key/value pairs, often
stashed in the boot flash.

[1] Ref:
https://chromium.googlesource.com/chromiumos/platform/vpd/+/master/README.md

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/base/property.c  | 83 +++++++++++++++++++++++++++++++++++++++-
 include/linux/property.h | 23 +++++++++++
 2 files changed, 104 insertions(+), 2 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 240ab5230ff6..fae3390fc56c 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -10,6 +10,7 @@
 #include <linux/acpi.h>
 #include <linux/export.h>
 #include <linux/kernel.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_graph.h>
@@ -1264,6 +1265,78 @@ static void *fwnode_get_mac_addr(struct fwnode_handle *fwnode,
 	return NULL;
 }
 
+static LIST_HEAD(mac_addr_providers);
+static DEFINE_MUTEX(mac_addr_providers_mutex);
+
+void device_register_mac_addr_provider(struct device_mac_addr_provider *prov)
+{
+	mutex_lock(&mac_addr_providers_mutex);
+	list_add(&prov->entry, &mac_addr_providers);
+	mutex_unlock(&mac_addr_providers_mutex);
+}
+EXPORT_SYMBOL(device_register_mac_addr_provider);
+
+void device_unregister_mac_addr_provider(struct device_mac_addr_provider *prov)
+{
+	struct device_mac_addr_provider *p;
+
+	mutex_lock(&mac_addr_providers_mutex);
+	list_for_each_entry(p, &mac_addr_providers, entry) {
+		if (p == prov) {
+			list_del(&p->entry);
+			goto out;
+		}
+	}
+
+out:
+	mutex_unlock(&mac_addr_providers_mutex);
+}
+EXPORT_SYMBOL(device_unregister_mac_addr_provider);
+
+static void *fwnode_lookup_mac_addr(struct fwnode_handle *fwnode,
+				    char *addr, int alen)
+{
+	struct device_mac_addr_provider *prov;
+	const char *prop, *sep;
+	u8 mac[ETH_ALEN];
+	int ret;
+
+	ret = fwnode_property_read_string(fwnode, "mac-address-lookup", &prop);
+	if (ret)
+		return NULL;
+
+	sep = strchr(prop, ':');
+	if (!sep)
+		return NULL;
+
+	if (alen != ETH_ALEN)
+		return NULL;
+
+	mutex_lock(&mac_addr_providers_mutex);
+	list_for_each_entry(prov, &mac_addr_providers, entry) {
+		if (strncmp(prov->prefix, prop, strlen(prov->prefix)))
+			continue;
+
+		if (prop + strlen(prov->prefix) != sep)
+			continue;
+
+		ret = prov->lookup(sep + 1, mac);
+		if (ret)
+			continue;
+
+		if (!is_valid_ether_addr(mac))
+			continue;
+
+		ether_addr_copy(addr, mac);
+
+		mutex_unlock(&mac_addr_providers_mutex);
+		return addr;
+	}
+	mutex_unlock(&mac_addr_providers_mutex);
+
+	return NULL;
+}
+
 /**
  * fwnode_get_mac_address - Get the MAC from the firmware node
  * @fwnode:	Pointer to the firmware node
@@ -1274,7 +1347,9 @@ static void *fwnode_get_mac_addr(struct fwnode_handle *fwnode,
  * checked first, because that is supposed to contain to "most recent" MAC
  * address. If that isn't set, then 'local-mac-address' is checked next,
  * because that is the default address.  If that isn't set, then the obsolete
- * 'address' is checked, just in case we're using an old device tree.
+ * 'address' is checked, just in case we're using an old device tree.  And
+ * finally, we check for a method of indirect MAC address lookup, via
+ * 'mac-address-lookup'.
  *
  * Note that the 'address' property is supposed to contain a virtual address of
  * the register set, but some DTS files have redefined that property to be the
@@ -1299,7 +1374,11 @@ void *fwnode_get_mac_address(struct fwnode_handle *fwnode, char *addr, int alen)
 	if (res)
 		return res;
 
-	return fwnode_get_mac_addr(fwnode, "address", addr, alen);
+	res = fwnode_get_mac_addr(fwnode, "address", addr, alen);
+	if (res)
+		return res;
+
+	return fwnode_lookup_mac_addr(fwnode, addr, alen);
 }
 EXPORT_SYMBOL(fwnode_get_mac_address);
 
diff --git a/include/linux/property.h b/include/linux/property.h
index ac8a1ebc4c1b..aca5dbb51e19 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -14,6 +14,7 @@
 #define _LINUX_PROPERTY_H_
 
 #include <linux/fwnode.h>
+#include <linux/list.h>
 #include <linux/types.h>
 
 struct device;
@@ -285,6 +286,28 @@ const void *device_get_match_data(struct device *dev);
 
 int device_get_phy_mode(struct device *dev);
 
+/**
+ * struct device_mac_addr_provider - MAC address provider
+ *
+ * Provide methods by which the rest of the kernel can retrieve MAC addresses,
+ * e.g., from a firmware table.
+ *
+ * @entry: list head, for keeping track of all providers
+ * @prefix: string which uniquely identifies the provider
+ * @lookup: Look up a MAC address by key. The provider may associate this @key
+ *	with a stored MAC address; if a match is found, the provider copies the
+ *	associated MAC address to @mac. If not found, a non-zero error code is
+ *	returned.
+ */
+struct device_mac_addr_provider {
+	struct list_head entry;
+	const char *prefix;
+	int (*lookup)(const char *key, u8 *mac);
+};
+
+void device_register_mac_addr_provider(struct device_mac_addr_provider *prov);
+void device_unregister_mac_addr_provider(struct device_mac_addr_provider *prov);
+
 void *device_get_mac_address(struct device *dev, char *addr, int alen);
 
 int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
-- 
2.18.0.865.gffc8e1a3cd6-goog

[RFC PATCH v1 3/3] firmware: vpd: add MAC address parser

[Brian Norris]

Device trees can now specify their MAC address via something like:

	mac-address-lookup = "vpd:wifi_mac0";

instead of requiring boot firmware to parse and splice the FDT itself.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/firmware/google/vpd.c | 67 +++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
index e9db895916c3..b2b08497db32 100644
--- a/drivers/firmware/google/vpd.c
+++ b/drivers/firmware/google/vpd.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/ctype.h>
+#include <linux/if_ether.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
@@ -24,6 +25,7 @@
 #include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
 
@@ -173,6 +175,36 @@ static ssize_t vpd_section_read(struct file *filp, struct kobject *kobp,
 				       sec->bin_attr.size);
 }
 
+static const char *__vpd_get_entry(struct vpd_section *sec, const char *key)
+{
+	struct vpd_attrib_info *info;
+
+	list_for_each_entry(info, &sec->attribs, list)
+		if (!strcmp(info->key, key))
+			return info->value;
+
+	return NULL;
+}
+
+static const char *vpd_get_entry(const char *key)
+{
+	const char *value;
+
+	if (rw_vpd.enabled) {
+		value = __vpd_get_entry(&rw_vpd, key);
+		if (value)
+			return value;
+	}
+
+	if (ro_vpd.enabled) {
+		value = __vpd_get_entry(&ro_vpd, key);
+		if (value)
+			return value;
+	}
+
+	return NULL;
+}
+
 static int vpd_section_create_attribs(struct vpd_section *sec)
 {
 	s32 consumed;
@@ -286,6 +318,37 @@ static int vpd_sections_init(phys_addr_t physaddr)
 	return 0;
 }
 
+/*
+ * 'key' is typically something like 'wifi_mac0' or 'ether_mac1'. Values are 12
+ * character strings of MAC address digits, in hex.
+ */
+static int vpd_lookup_mac_address(const char *key, u8 *mac)
+{
+	const char *entry;
+	int ret;
+
+	if (!key)
+		return -EINVAL;
+
+	entry = vpd_get_entry(key);
+	if (!entry)
+		return -ENOENT;
+
+	if (strlen(entry) != ETH_ALEN * 2)
+		return -EINVAL;
+
+	ret = hex2bin(mac, entry, ETH_ALEN);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static struct device_mac_addr_provider vpd_mac_addr_provider = {
+	.prefix		= "google-vpd",
+	.lookup		= &vpd_lookup_mac_address,
+};
+
 static int vpd_probe(struct coreboot_device *dev)
 {
 	int ret;
@@ -300,11 +363,15 @@ static int vpd_probe(struct coreboot_device *dev)
 		return ret;
 	}
 
+	device_register_mac_addr_provider(&vpd_mac_addr_provider);
+
 	return 0;
 }
 
 static int vpd_remove(struct coreboot_device *dev)
 {
+	device_unregister_mac_addr_provider(&vpd_mac_addr_provider);
+
 	vpd_section_destroy(&ro_vpd);
 	vpd_section_destroy(&rw_vpd);
 
-- 
2.18.0.865.gffc8e1a3cd6-goog

Re: [RFC PATCH v1 0/3] device property: Support MAC address in VPD

[Florian Fainelli]

On 08/14/2018 03:37 PM, Brian Norris wrote:
> Hi all,
> 
> Preface: I'm sure there are at least a few minor issues with this
> patchset as-is. But I'd appreciate ("RFC") if I can get general feedback
> on the approach here; perhaps there are alternatives, or perhaps I've
> missed similar proposals in the past. (My problems don't feel all that
> unique.)
> 
> ---
> 
> Today, we have generic support for 'mac-address' and 'local-mac-address'
> properties in both Device Tree nodes and in generic Device Properties,
> such that network device drivers can pick up a hardware address from
> there, in cases where the MAC address isn't baked into the network card.
> This method of MAC address retrieval presumes that either:
> (a) there's a unique device tree (or similar) stored on a given device
>     or
> (b) some other entity (e.g., boot firmware) will modify device nodes
>     runtime to place that MAC address into the appropriate device
>     properties.
> 
> Option (a) is not feasbile for many systems.
> 
> Option (b) can work, but there are some reasons why one might not want
> to do that:
>  (1) This requires that system firmware understand the device tree
>      structure, sometimes to the point of memorizing path names (e.g.,
>      /soc/wifi@xxxxxxxx). At least for Device Tree, these path names are
>      not necessarily an ABI, and so this introduces unneeded fragility.

The path to a node is something that is well defined and should be
stable given that the high level function of the node and its unit
address are not supposed to change. Under which circumstances, besides
incorrect specification of either of these two things, do they not
consist an ABI? Not refuting your statement here, just curious when/how
this can happen?

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

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

[1]: https://patchwork.ozlabs.org/cover/956062/
[2]: https://lkml.org/lkml/2018/3/24/312

> 
> Thanks,
> Brian
> 
> 
> Brian Norris (3):
>   dt-bindings: net: Add 'mac-address-lookup' property
>   device property: Support complex MAC address lookup
>   firmware: vpd: add MAC address parser
> 
>  .../devicetree/bindings/net/ethernet.txt      | 12 +++
>  drivers/base/property.c                       | 83 ++++++++++++++++++-
>  drivers/firmware/google/vpd.c                 | 67 +++++++++++++++
>  include/linux/property.h                      | 23 +++++
>  4 files changed, 183 insertions(+), 2 deletions(-)
> 


-- 
Florian

Re: [RFC PATCH v1 0/3] device property: Support MAC address in VPD

[Brian Norris]

+ Srinivas, as there are NVMEM questions

Hi Florian,

Thanks for the quick reply.

On Tue, Aug 14, 2018 at 04:00:28PM -0700, Florian Fainelli wrote:
> On 08/14/2018 03:37 PM, Brian Norris wrote:
> > Today, we have generic support for 'mac-address' and 'local-mac-address'
> > properties in both Device Tree nodes and in generic Device Properties,
> > such that network device drivers can pick up a hardware address from
> > there, in cases where the MAC address isn't baked into the network card.
> > This method of MAC address retrieval presumes that either:
> > (a) there's a unique device tree (or similar) stored on a given device
> >     or
> > (b) some other entity (e.g., boot firmware) will modify device nodes
> >     runtime to place that MAC address into the appropriate device
> >     properties.
> > 
> > Option (a) is not feasbile for many systems.
> > 
> > Option (b) can work, but there are some reasons why one might not want
> > to do that:
> >  (1) This requires that system firmware understand the device tree
> >      structure, sometimes to the point of memorizing path names (e.g.,
> >      /soc/wifi@xxxxxxxx). At least for Device Tree, these path names are
> >      not necessarily an ABI, and so this introduces unneeded fragility.
> 
> The path to a node is something that is well defined and should be
> stable given that the high level function of the node and its unit
> address are not supposed to change. Under which circumstances, besides
> incorrect specification of either of these two things, do they not
> consist an ABI? Not refuting your statement here, just curious when/how
> this can happen?

I can think of a few reasons:

 * it's not really standardized whether to use a /soc/ node or to
   just put top-level SoC blocks directly under /. There might be
   "recommendations", but I certainly have seen it both ways.

 * the "high level function name" is not set in stone AFAICT. Or at
   least, I've never seen a list of documented names. So while on one
   system I might see 'wlan@', another might use 'wifi@'.

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

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

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

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!

> [1]: https://patchwork.ozlabs.org/cover/956062/
> [2]: https://lkml.org/lkml/2018/3/24/312

Brian

[1] Fortunately, I've only needed this on DT so far.

Re: [RFC PATCH v1 0/3] device property: Support MAC address in VPD

[Florian Fainelli]

On 08/14/2018 05:22 PM, Brian Norris wrote:
> + Srinivas, as there are NVMEM questions
> 
> Hi Florian,
> 
> Thanks for the quick reply.
> 
> On Tue, Aug 14, 2018 at 04:00:28PM -0700, Florian Fainelli wrote:
>> On 08/14/2018 03:37 PM, Brian Norris wrote:
>>> Today, we have generic support for 'mac-address' and 'local-mac-address'
>>> properties in both Device Tree nodes and in generic Device Properties,
>>> such that network device drivers can pick up a hardware address from
>>> there, in cases where the MAC address isn't baked into the network card.
>>> This method of MAC address retrieval presumes that either:
>>> (a) there's a unique device tree (or similar) stored on a given device
>>>     or
>>> (b) some other entity (e.g., boot firmware) will modify device nodes
>>>     runtime to place that MAC address into the appropriate device
>>>     properties.
>>>
>>> Option (a) is not feasbile for many systems.
>>>
>>> Option (b) can work, but there are some reasons why one might not want
>>> to do that:
>>>  (1) This requires that system firmware understand the device tree
>>>      structure, sometimes to the point of memorizing path names (e.g.,
>>>      /soc/wifi@xxxxxxxx). At least for Device Tree, these path names are
>>>      not necessarily an ABI, and so this introduces unneeded fragility.
>>
>> The path to a node is something that is well defined and should be
>> stable given that the high level function of the node and its unit
>> address are not supposed to change. Under which circumstances, besides
>> incorrect specification of either of these two things, do they not
>> consist an ABI? Not refuting your statement here, just curious when/how
>> this can happen?
> 
> I can think of a few reasons:
> 
>  * it's not really standardized whether to use a /soc/ node or to
>    just put top-level SoC blocks directly under /. There might be
>    "recommendations", but I certainly have seen it both ways.

That type of stability and standardization, ok

> 
>  * the "high level function name" is not set in stone AFAICT. Or at
>    least, I've never seen a list of documented names. So while on one
>    system I might see 'wlan@', another might use 'wifi@'.

There are some recommended function names defined in devicetree-spec,
not everything is covered since the spec is still lagging a bit behind
as far as recommending names for what a modern SoC/board would embed
(with some definition of "modern").

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

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

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

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

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

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

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

> 
>> [1]: https://patchwork.ozlabs.org/cover/956062/
>> [2]: https://lkml.org/lkml/2018/3/24/312
> 
> Brian
> 
> [1] Fortunately, I've only needed this on DT so far.
> 


-- 
Florian

Re: [RFC PATCH v1 0/3] device property: Support MAC address in VPD

[Brian Norris]

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

Re: [RFC PATCH v1 0/3] device property: Support MAC address in VPD

[Stephen Boyd]

Quoting Brian Norris (2018-08-14 18:44:36)
> Hi,
> 
> On Tue, Aug 14, 2018 at 05:52:49PM -0700, Florian Fainelli wrote:
> > On 08/14/2018 05:22 PM, Brian Norris wrote:
> 
> > >> 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).

Yes. Aliases are the way to do this. It obviates much of this discussion
about finding things in DT by directly pointing to the node the
bootloader wants to go modify.

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

Where does it go on ACPI systems? Does the firmware stick it into some
ACPI table by reading from VPD?

Re: [RFC PATCH v1 0/3] device property: Support MAC address in VPD

[Brian Norris]

(sorry for the delayed response here)

Hi,

On Wed, Aug 15, 2018 at 03:07:00PM -0700, Stephen Boyd wrote:
> Quoting Brian Norris (2018-08-14 18:44:36)
> > 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).
> 
> Yes. Aliases are the way to do this. It obviates much of this discussion
> about finding things in DT by directly pointing to the node the
> bootloader wants to go modify.

Sure, that does seem to help.

> > > > 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...
> 
> Where does it go on ACPI systems? Does the firmware stick it into some
> ACPI table by reading from VPD?

I'm not aware of any ACPI system that needs to do something quite like
this. The closest I see is a Coreboot board for some Chromeboxes [1],
which handles Ethernet MAC addresses -- it does this by reading similar
VPD fields, but then it just goes and programs the MAC directly into the
Ethernet card's registers. This is a Realtek PCI Ethernet chip, and
apparently it even needs firmware to program its product and vendor ID
for it.

I'm not really sure what lesson to get out of that though. That
discoverable hardware is a giant dirty mess (even PCI gets a lot of help
from system firmware!)?

Or I guess maybe the lesson is: ACPI systems will always find a way to
do it in firmware, no matter how much bloat it costs. So I don't really
need to consider "does this solution work with ACPI"? But just, does it
work with {device,fwnode}_get_mac_address() -- e.g., by teaching them to
call of_get_nvmem_mac_address()?

Brian

[1] Here, and similar variants in other boards throughout the tree:
https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/beltino/lan.c

Re: [RFC PATCH v1 0/3] device property: Support MAC address in VPD

[Rob Herring]

On Tue, Aug 14, 2018 at 06:44:36PM -0700, Brian Norris wrote:
> Hi,

I should have read the rest of this thread first...

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

That sounds like an inadequately documented binding.

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

Uggg, it would be nice to clean that up. 

There's several aliases I'd like to get rid of (some platforms went a 
little crazy with them) and I'd like to start requiring alias names to 
be documented. I created an issue for the spec. Patches welcomeTM. :)

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

In the /aliases node, but yes.

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

Seems to me that nvmem needs to be extended to allow providers to 
retrieve and interpret data. Not everything is at some fixed offset and 
size. Something like this is valid dts:

nvmem = <&phandle> "a-string";

But that's pretty uncommon (I can't think of a binding that actually 
uses that). Perhaps the provider has an array of keys defined and the 
consumer just provides the index.

Or we could do '<key>-nvmem = <&phandle>', but parsing that is a bit 
more complicated.

Rob

Re: [RFC PATCH v1 0/3] device property: Support MAC address in VPD

[Brian Norris]

Hi, sorry for the delay, and thanks for the response.

I'm still not sure whether I'll pursue this series further, but I'd like
to at least narrow down what it'd look like.

On Wed, Aug 15, 2018 at 04:16:01PM -0600, Rob Herring wrote:
> On Tue, Aug 14, 2018 at 06:44:36PM -0700, Brian Norris wrote:
> > On Tue, Aug 14, 2018 at 05:52:49PM -0700, Florian Fainelli wrote:
> > > On 08/14/2018 05:22 PM, Brian Norris wrote:
> > > >> 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.
> 
> Uggg, it would be nice to clean that up. 

Is it fair to just change them, since they were never documented? Of
course, only after documenting the "right" way.

> There's several aliases I'd like to get rid of (some platforms went a 
> little crazy with them) and I'd like to start requiring alias names to 
> be documented. I created an issue for the spec. Patches welcomeTM. :)

So e.g., new text in Documentation/devicetree/bindings/net/ethernet.txt
and Documentation/devicetree/bindings/net/wireless/ieee80211.txt about
'aliases' entries? I can do that, especially if I give up on a new
binding like in this series.

> > 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?
> 
> In the /aliases node, but yes.

Oops, yes.

> Seems to me that nvmem needs to be extended to allow providers to 
> retrieve and interpret data. Not everything is at some fixed offset and 
> size. Something like this is valid dts:
> 
> nvmem = <&phandle> "a-string";
> 
> But that's pretty uncommon (I can't think of a binding that actually 
> uses that). Perhaps the provider has an array of keys defined and the 
> consumer just provides the index.

In the case of VPD, all keys are 0-terminated strings (there's also a
length field, but the key is still 0-terminated), so that scheme could
work. (I'm not sure an indexed provider is extremely relevant right now,
although it probably could be supported if I expand the of_nvmem
retrieval to support a generic of_xlate() override anyway.) The
information represented is almost the same as in my proposal, except that:
(a) now I have to give the VPD a phandle -- so far, I've avoided that,
    as it's just an auto-enumerated device underneath the
    /firmware/coreboot device (see drivers/firmware/google/vpd.c)
(b) this is no longer directly useful to ACPI systems -- I'm not
    actually sure how (if at all) nvmem provider/consumer is supposed to
    work there

But maybe this isn't really that useful to ACPI, and it's sufficient to
just have fwnode_get_mac_address() call of_get_nvmem_mac_address() when
we're using DT.

> Or we could do '<key>-nvmem = <&phandle>', but parsing that is a bit 
> more complicated.

That doesn't seem to have much advantage to me.

Brian

Re: [RFC PATCH v1 0/3] device property: Support MAC address in VPD

[Srinivas Kandagatla]

Sorry for the delay!! For some reason I missed this email thread totally!

On 31/08/18 02:26, Brian Norris wrote:
>> Seems to me that nvmem needs to be extended to allow providers to
>> retrieve and interpret data. Not everything is at some fixed offset and
>> size. Something like this is valid dts:
>>
>> nvmem = <&phandle> "a-string";
>>

There has been some discussion on extending nvmem support to MTD and 
non-DT users in https://patchwork.kernel.org/cover/10562365/

One of the thing which we discussed in this thread is adding compatible 
strings to cells mainly to
1> Differentiate between actual cells and partitions for MTD case.

2> Allow provider specific bindings for each cell, in VPD instance key 
string & value length could be one them.

This means that we would endup adding xlate callback support to the 
nvmem_config.

AFAIU, From consumer side old bindings should still work!

 From non-dt or ACPI side these cells can be parsed by the provider 
driver and add it to the nvmem_config.

Does this make sense? Or Did I miss anything obvious ?


>> But that's pretty uncommon (I can't think of a binding that actually
>> uses that). Perhaps the provider has an array of keys defined and the
>> consumer just provides the index.
> In the case of VPD, all keys are 0-terminated strings (there's also a
> length field, but the key is still 0-terminated), so that scheme could
> work. (I'm not sure an indexed provider is extremely relevant right now,
> although it probably could be supported if I expand the of_nvmem
> retrieval to support a generic of_xlate() override anyway.) The
> information represented is almost the same as in my proposal, except that:
> (a) now I have to give the VPD a phandle -- so far, I've avoided that,
>      as it's just an auto-enumerated device underneath the
>      /firmware/coreboot device (see drivers/firmware/google/vpd.c)
> (b) this is no longer directly useful to ACPI systems -- I'm not
>      actually sure how (if at all) nvmem provider/consumer is supposed to
>      work there
> 
> But maybe this isn't really that useful to ACPI, and it's sufficient to
> just have fwnode_get_mac_address() call of_get_nvmem_mac_address() when
> we're using DT.
> 
>> Or we could do '<key>-nvmem = <&phandle>', but parsing that is a bit
>> more complicated.
> That doesn't seem to have much advantage to me.

Re: [RFC PATCH v1 0/3] device property: Support MAC address in VPD

[Brian Norris]

Hi Srinivas,

On Fri, Aug 31, 2018 at 09:43:30AM +0100, Srinivas Kandagatla wrote:

> On 31/08/18 02:26, Brian Norris wrote:
> > > Seems to me that nvmem needs to be extended to allow providers to
> > > retrieve and interpret data. Not everything is at some fixed offset and
> > > size. Something like this is valid dts:
> > > 
> > > nvmem = <&phandle> "a-string";
> > > 
> 
> There has been some discussion on extending nvmem support to MTD and non-DT
> users in https://patchwork.kernel.org/cover/10562365/
> 
> One of the thing which we discussed in this thread is adding compatible
> strings to cells mainly to
> 1> Differentiate between actual cells and partitions for MTD case.

I'm not particularly worried about the MTD case. As I mentioned earlier,
while VPD is stored on flash (and could be exposed as an MTD), coreboot
places these tables in memory, and we currently just read them from
there instead of from flash.

> 2> Allow provider specific bindings for each cell, in VPD instance key
> string & value length could be one them.

I'm not sure we'd need to have a binding for value length -- VPD encodes
the length itself, and for many properties, the length is understood by
both sides anyway (2x6 bytes for a MAC address).

> This means that we would endup adding xlate callback support to the
> nvmem_config.

OK, but that's not in the current series, correct?

> AFAIU, From consumer side old bindings should still work!

I'm still trying to wrap my head around all the existing and proposed
behaviors of nvmem, but I see a few things lacking (IIUC):

(1) for the new "lookup" method, you would only support a single MAC
    address, identified by looking up for "mac-address" -- this means
    you can't support two devices (e.g., we have systems with VPD
    entries for "ethernet_mac0" and "etherent_mac1")
(2) the consumer API isn't very flexible -- it assumes that the data you
    read out of an NVMEM cell is directly usable as a MAC address; this
    fails for VPD, because VPD uses ASCII encoding. To resolve this,
    you'd need the consumer/provider relationship to know something
    about the data type -- to know that we should decode the ASCII
    values

> From non-dt or ACPI side these cells can be parsed by the provider driver
> and add it to the nvmem_config.

I think that might work, except for the above problems. But perhaps I'm
misreading.

> Does this make sense? Or Did I miss anything obvious ?
> 
> 
> > > But that's pretty uncommon (I can't think of a binding that actually
> > > uses that). Perhaps the provider has an array of keys defined and the
> > > consumer just provides the index.
> > In the case of VPD, all keys are 0-terminated strings (there's also a
> > length field, but the key is still 0-terminated), so that scheme could
> > work. (I'm not sure an indexed provider is extremely relevant right now,
> > although it probably could be supported if I expand the of_nvmem
> > retrieval to support a generic of_xlate() override anyway.) The
> > information represented is almost the same as in my proposal, except that:

@Rob:
I forgot about problem (2) above -- NVMEM is not very expressive about
the *type* of information. My proposal makes it explicit that the
provider is presenting MAC addresses. To make a generic VPD NVMEM
provider, I'd need to do ASCII decoding on some fields but not on
others.

Brian

> > (a) now I have to give the VPD a phandle -- so far, I've avoided that,
> >      as it's just an auto-enumerated device underneath the
> >      /firmware/coreboot device (see drivers/firmware/google/vpd.c)
> > (b) this is no longer directly useful to ACPI systems -- I'm not
> >      actually sure how (if at all) nvmem provider/consumer is supposed to
> >      work there
> > 
> > But maybe this isn't really that useful to ACPI, and it's sufficient to
> > just have fwnode_get_mac_address() call of_get_nvmem_mac_address() when
> > we're using DT.
> > 
> > > Or we could do '<key>-nvmem = <&phandle>', but parsing that is a bit
> > > more complicated.
> > That doesn't seem to have much advantage to me.