| Message ID | 20180814223758.117433-1-briannorris@chromium.org |
|---|---|
| Headers | show |
| Series | device property: Support MAC address in VPD | expand |
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(-) >
+ 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.
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. >
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
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?
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
(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
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
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.
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.
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(-)