2021-06-08 19:21:57

by Vadym Kochan

[permalink] [raw]
Subject: [PATCH v2 0/3] nvmem: add ONIE NVMEM cells parser

This series adds cells parser for the ONIE TLV attributes which are
stored on NVMEM device. It adds possibility to read the mac address (and
other info) by other drivers.

Because ONIE stores info in TLV format it should be parsed first and
then register the cells. Current NVMEM API allows to register cell
table with known cell's offset which is not guaranteed in case of TLV.

To make it properly handled the NVMEM parser object is introduced. The
parser needs to be registered before target NVMEM device is registered.
During the registration of NVMEM device the parser is called to parse
the device's cells and reister the cell table.

v2:
1) Series title changed, "provider" is replaced by "parser".

2) onie-cells.c renamed to onie-tlv-cells.c

3) Do not register onie cells parser via platform driver,
but via module_init().

4) Removed cells_remove callback from nvmem_parser_config because
now cells table and lookups are registered by core.c

5) Introduced nvmem_parser_data which is passed to parser
handler/driver to fill cells table & lookups.

6) core.c registers cells table & lookups to simplify the parser
handler logic.

7) parser's module refcount is incremented/decremented on each parser
bind/unbind to nvmem device.

8) core.c relies on parser name which is obtained via
device_property API, instead of of_node which was used
in previous version.

9) nvmem-cell-parser-name DT nvmem property was added to bind
cells parser to nvmem device during the nvmem registration.

P.S.
I marked it with v2 as relative to series which was sent as:

"nvmem: add ONIE NVMEM cells provider"

Vadym Kochan (3):
nvmem: core: introduce cells parser
dt-bindings: nvmem: document nvmem-cells-parser-name property
nvmem: add ONIE nvmem cells parser

.../devicetree/bindings/nvmem/nvmem.yaml | 5 +
drivers/nvmem/Kconfig | 9 +
drivers/nvmem/Makefile | 2 +
drivers/nvmem/core.c | 178 +++++++++++
drivers/nvmem/onie-tlv-cells.c | 302 ++++++++++++++++++
include/linux/nvmem-provider.h | 31 ++
6 files changed, 527 insertions(+)
create mode 100644 drivers/nvmem/onie-tlv-cells.c

--
2.17.1


2021-06-09 08:05:01

by Vadym Kochan

[permalink] [raw]
Subject: [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser

ONIE is a small operating system, pre-installed on bare metal network
switches, that provides an environment for automated provisioning.

This system requires that NVMEM (EEPROM) device holds various system
information (mac address, platform name, etc) in a special TLV layout.

The driver registers ONIE TLV attributes as NVMEM cells which can be
accessed by other platform driver. Also it allows to use
of_get_mac_address() to retrieve mac address for the netdev.

Signed-off-by: Vadym Kochan <[email protected]>
---
v2:
1) onie-cells.c renamed to onie-tlv-cells.c

2) Do not register onie cells parser via platform driver,
but via module_init().

3) Simplified cells table & lookups registration by just filling
all this in nvmem_parser_data.

drivers/nvmem/Kconfig | 9 +
drivers/nvmem/Makefile | 2 +
drivers/nvmem/onie-tlv-cells.c | 302 +++++++++++++++++++++++++++++++++
3 files changed, 313 insertions(+)
create mode 100644 drivers/nvmem/onie-tlv-cells.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index dd2019006838..a08ff087361b 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -288,4 +288,13 @@ config NVMEM_BRCM_NVRAM
This driver provides support for Broadcom's NVRAM that can be accessed
using I/O mapping.

+config NVMEM_ONIE_TLV_CELLS
+ tristate "ONIE TLV cells support"
+ help
+ This is a driver to provide cells from ONIE TLV structure stored
+ on NVME device.
+
+ This driver can also be built as a module. If so, the module
+ will be called nvmem-onie-tlv-cells.
+
endif
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index bbea1410240a..f70d7b817377 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -59,3 +59,5 @@ obj-$(CONFIG_NVMEM_RMEM) += nvmem-rmem.o
nvmem-rmem-y := rmem.o
obj-$(CONFIG_NVMEM_BRCM_NVRAM) += nvmem_brcm_nvram.o
nvmem_brcm_nvram-y := brcm_nvram.o
+obj-$(CONFIG_NVMEM_ONIE_TLV_CELLS) += nvmem-onie-tlv-cells.o
+nvmem-onie-tlv-cells-y := onie-tlv-cells.o
diff --git a/drivers/nvmem/onie-tlv-cells.c b/drivers/nvmem/onie-tlv-cells.c
new file mode 100644
index 000000000000..85b1c92da0c5
--- /dev/null
+++ b/drivers/nvmem/onie-tlv-cells.c
@@ -0,0 +1,302 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ONIE NVMEM cells provider
+ *
+ * Author: Vadym Kochan <[email protected]>
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/kref.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/nvmem-provider.h>
+#include <linux/slab.h>
+
+#define ONIE_NVMEM_TLV_MAX_LEN 2048
+
+#define ONIE_NVMEM_HDR_ID "TlvInfo"
+
+struct onie_tlv_hdr {
+ u8 id[8];
+ u8 version;
+ __be16 data_len;
+} __packed;
+
+struct onie_tlv {
+ u8 type;
+ u8 len;
+ u8 val[0];
+} __packed;
+
+struct onie_nvmem_attr {
+ struct list_head head;
+ const char *name;
+ unsigned int offset;
+ unsigned int len;
+};
+
+struct onie_tlv_parser {
+ unsigned int attr_count;
+ struct list_head attrs;
+ struct device *dev;
+
+ struct nvmem_cell_lookup *lookup;
+ int nlookups;
+};
+
+static struct nvmem_parser *nvmem_parser;
+
+static bool onie_nvmem_hdr_is_valid(struct onie_tlv_hdr *hdr)
+{
+ if (memcmp(hdr->id, ONIE_NVMEM_HDR_ID, sizeof(hdr->id)) != 0)
+ return false;
+ if (hdr->version != 0x1)
+ return false;
+
+ return true;
+}
+
+static void onie_nvmem_attrs_free(struct onie_tlv_parser *parser)
+{
+ struct onie_nvmem_attr *attr, *tmp;
+
+ list_for_each_entry_safe(attr, tmp, &parser->attrs, head) {
+ list_del(&attr->head);
+ kfree(attr);
+ }
+}
+
+static const char *onie_nvmem_attr_name(u8 type)
+{
+ switch (type) {
+ case 0x21: return "product-name";
+ case 0x22: return "part-number";
+ case 0x23: return "serial-number";
+ case 0x24: return "mac-address";
+ case 0x25: return "manufacture-date";
+ case 0x26: return "device-version";
+ case 0x27: return "label-revision";
+ case 0x28: return "platforn-name";
+ case 0x29: return "onie-version";
+ case 0x2A: return "num-macs";
+ case 0x2B: return "manufacturer";
+ case 0x2C: return "country-code";
+ case 0x2D: return "vendor";
+ case 0x2E: return "diag-version";
+ case 0x2F: return "service-tag";
+ case 0xFD: return "vendor-extension";
+ case 0xFE: return "crc32";
+
+ default: return "unknown";
+ }
+}
+
+static int onie_nvmem_tlv_parse(struct onie_tlv_parser *parser, u8 *data, u16 len)
+{
+ unsigned int hlen = sizeof(struct onie_tlv_hdr);
+ unsigned int offset = 0;
+ int err;
+
+ parser->attr_count = 0;
+
+ while (offset < len) {
+ struct onie_nvmem_attr *attr;
+ struct onie_tlv *tlv;
+
+ tlv = (struct onie_tlv *)(data + offset);
+
+ if (offset + tlv->len >= len) {
+ pr_err("TLV len is too big(0x%x) at 0x%x\n",
+ tlv->len, hlen + offset);
+
+ /* return success in case something was parsed */
+ return 0;
+ }
+
+ attr = kmalloc(sizeof(*attr), GFP_KERNEL);
+ if (!attr) {
+ err = -ENOMEM;
+ goto err_attr_alloc;
+ }
+
+ attr->name = onie_nvmem_attr_name(tlv->type);
+ /* skip 'type' and 'len' */
+ attr->offset = hlen + offset + 2;
+ attr->len = tlv->len;
+
+ list_add(&attr->head, &parser->attrs);
+ parser->attr_count++;
+
+ offset += sizeof(*tlv) + tlv->len;
+ }
+
+ if (!parser->attr_count)
+ return -EINVAL;
+
+ return 0;
+
+err_attr_alloc:
+ onie_nvmem_attrs_free(parser);
+ return err;
+}
+
+static int onie_nvmem_decode(struct onie_tlv_parser *parser, struct nvmem_device *nvmem)
+{
+ struct onie_tlv_hdr hdr;
+ u8 *data;
+ u16 len;
+ int ret;
+
+ ret = nvmem_device_read(nvmem, 0, sizeof(hdr), &hdr);
+ if (ret < 0)
+ return ret;
+
+ if (!onie_nvmem_hdr_is_valid(&hdr)) {
+ pr_err("invalid ONIE TLV header\n");
+ return -EINVAL;
+ }
+
+ len = be16_to_cpu(hdr.data_len);
+
+ if (len > ONIE_NVMEM_TLV_MAX_LEN)
+ len = ONIE_NVMEM_TLV_MAX_LEN;
+
+ data = kmalloc(len, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ ret = nvmem_device_read(nvmem, sizeof(hdr), len, data);
+ if (ret < 0)
+ goto err_data_read;
+
+ ret = onie_nvmem_tlv_parse(parser, data, len);
+ if (ret)
+ goto err_info_parse;
+
+ kfree(data);
+
+ return 0;
+
+err_info_parse:
+err_data_read:
+ kfree(data);
+ return ret;
+}
+
+static int onie_nvmem_cells_parse(struct onie_tlv_parser *parser,
+ struct nvmem_device *nvmem,
+ struct nvmem_cell_table *table)
+{
+ struct nvmem_cell_info *cells;
+ struct onie_nvmem_attr *attr;
+ unsigned int ncells = 0;
+ int err;
+
+ INIT_LIST_HEAD(&parser->attrs);
+ parser->attr_count = 0;
+
+ err = onie_nvmem_decode(parser, nvmem);
+ if (err)
+ return err;
+
+ cells = kmalloc_array(parser->attr_count, sizeof(*cells), GFP_KERNEL);
+ if (!cells) {
+ err = -ENOMEM;
+ goto err_cells_alloc;
+ }
+
+ parser->lookup = kmalloc_array(parser->attr_count,
+ sizeof(struct nvmem_cell_lookup),
+ GFP_KERNEL);
+ if (!parser->lookup) {
+ err = -ENOMEM;
+ goto err_lookup_alloc;
+ }
+
+ list_for_each_entry(attr, &parser->attrs, head) {
+ struct nvmem_cell_lookup *lookup;
+ struct nvmem_cell_info *cell;
+
+ cell = &cells[ncells];
+
+ lookup = &parser->lookup[ncells];
+ lookup->con_id = NULL;
+
+ cell->offset = attr->offset;
+ cell->name = attr->name;
+ cell->bytes = attr->len;
+ cell->bit_offset = 0;
+ cell->nbits = 0;
+
+ lookup->cell_name = cell->name;
+ lookup->con_id = cell->name;
+
+ ncells++;
+ }
+
+ table->ncells = ncells;
+ table->cells = cells;
+
+ parser->nlookups = ncells;
+
+ onie_nvmem_attrs_free(parser);
+
+ return 0;
+
+err_lookup_alloc:
+ kfree(cells);
+err_cells_alloc:
+ onie_nvmem_attrs_free(parser);
+
+ return err;
+}
+
+static int onie_cells_parse(struct nvmem_device *nvmem,
+ struct nvmem_parser_data *data)
+{
+ struct onie_tlv_parser parser;
+ int err;
+
+ err = onie_nvmem_cells_parse(&parser, nvmem, &data->table);
+ if (err) {
+ pr_err("failed to parse ONIE attributes\n");
+ return err;
+ }
+
+ data->nlookups = parser.nlookups;
+ data->lookup = parser.lookup;
+
+ return 0;
+}
+
+static int __init onie_tlv_init(void)
+{
+ struct nvmem_parser_config parser_config = { };
+
+ parser_config.cells_parse = onie_cells_parse;
+ parser_config.owner = THIS_MODULE;
+ parser_config.name = "onie-tlv-cells";
+
+ nvmem_parser = nvmem_parser_register(&parser_config);
+ if (IS_ERR(nvmem_parser)) {
+ pr_err("failed to register %s parser\n", parser_config.name);
+ return PTR_ERR(nvmem_parser);
+ }
+
+ pr_info("registered %s parser\n", parser_config.name);
+
+ return 0;
+}
+
+static void __exit onie_tlv_exit(void)
+{
+ nvmem_parser_unregister(nvmem_parser);
+}
+
+module_init(onie_tlv_init);
+module_exit(onie_tlv_exit);
+
+MODULE_AUTHOR("Vadym Kochan <[email protected]>");
+MODULE_DESCRIPTION("ONIE TLV NVMEM cells parser");
+MODULE_LICENSE("GPL");
--
2.17.1

2021-08-06 23:11:23

by Jan Lübbe

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser

Hi,

On Tue, 2021-06-08 at 22:03 +0300, Vadym Kochan wrote:

...
> + case 0x24: return "mac-address";
...
> + case 0x2A: return "num-macs";

Is suspect these properties define which range of MACs is assigned to the
device. How would you use them to assign MAC addresses to multiple interfaces?
The nvmem-cells property in the network device's node can only refer to one
cell, and not to i.e the cells value + 1.

I think it would be useful to have a way to express this setup for systems with
many interfaces, but am unsure of where this should be described. Maybe a "mac-
address-offset" property in the generic ethernet controller binding?

Regards,
Jan
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2021-09-08 10:05:41

by Vadym Kochan

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser


Hi Jan,

Jan Lübbe <[email protected]> writes:

> Hi,
>
> On Tue, 2021-06-08 at 22:03 +0300, Vadym Kochan wrote:
>
> ...
>> + case 0x24: return "mac-address";
> ...

This is a base mac,

>> + case 0x2A: return "num-macs";
>
> Is suspect these properties define which range of MACs is assigned to the

Yes

> device. How would you use them to assign MAC addresses to multiple interfaces?
> The nvmem-cells property in the network device's node can only refer to one
> cell, and not to i.e the cells value + 1.
>

Currently in net/ethernet/marvell/prestera/prestera_main.c it is
incremented and hard-coded by the driver.

> I think it would be useful to have a way to express this setup for systems with
> many interfaces, but am unsure of where this should be described. Maybe a "mac-
> address-offset" property in the generic ethernet controller binding?
>
> Regards,
> Jan

May be something like eth_address_provider should be introduced in
net/ethernet/ ?

This provider can provide something like eth_provider_address_next() which
will consider "mac-address-num" (or other specific fields).

2021-09-12 21:09:09

by John Thomson

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser

Hi Vadym,

On Wed, 8 Sep 2021, at 09:56, Vadym Kochan wrote:
>
> Hi Jan,
>
> Jan Lübbe <[email protected]> writes:

>
> > I think it would be useful to have a way to express this setup for systems with
> > many interfaces, but am unsure of where this should be described. Maybe a "mac-
> > address-offset" property in the generic ethernet controller binding?
> >
> > Regards,
> > Jan
>
> May be something like eth_address_provider should be introduced in
> net/ethernet/ ?
>
> This provider can provide something like eth_provider_address_next() which
> will consider "mac-address-num" (or other specific fields).
>

A patch series proposed the devicetree property
mac-address-increment, but it did not get support at the time:
of_net: add mac-address-increment support
https://lore.kernel.org/all/[email protected]/
dt-bindings: net: Document use of mac-address-increment
https://lore.kernel.org/all/[email protected]/

Cheers,
--
John Thomson

2021-09-13 15:27:04

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser



On 12/09/2021 22:06, John Thomson wrote:
> Hi Vadym,
>
> On Wed, 8 Sep 2021, at 09:56, Vadym Kochan wrote:
>>
>> Hi Jan,
>>
>> Jan Lübbe <[email protected]> writes:
> …
>>
>>> I think it would be useful to have a way to express this setup for systems with
>>> many interfaces, but am unsure of where this should be described. Maybe a "mac-
>>> address-offset" property in the generic ethernet controller binding?
>>>
>>> Regards,
>>> Jan
>>
>> May be something like eth_address_provider should be introduced in
>> net/ethernet/ ?
>>
>> This provider can provide something like eth_provider_address_next() which
>> will consider "mac-address-num" (or other specific fields).
>>
>
> A patch series proposed the devicetree property
> mac-address-increment, but it did not get support at the time:

Please have a look at some recent nvmem patches
https://lkml.org/lkml/2021/9/8/270 that adds support for vendor specific
post-processing of nvmem-cells.

Am hoping that increment usecase (along with other variants) should also
be dealt in similar way.


--srini
> of_net: add mac-address-increment support
> https://lore.kernel.org/all/[email protected]/
> dt-bindings: net: Document use of mac-address-increment
> https://lore.kernel.org/all/[email protected]/
>



> Cheers,
>