Thread (18 messages) 18 messages, 3 authors, 2022-08-25

Re: [PATCH 6/8] nvmem: transformations: ethernet address offset support

From: Rafał Miłecki <zajec5@gmail.com>
Date: 2022-01-25 12:14:43
Also in: linux-arm-kernel, linux-devicetree, lkml

On 28.12.2021 15:25, Michael Walle wrote:
quoted hunk ↗ jump to hunk
An nvmem cell might just contain a base MAC address. To generate a
address of a specific interface, add a transformation to add an offset
to this base address.

Add a generic implementation and the first user of it, namely the sl28
vpd storage.

Signed-off-by: Michael Walle <redacted>
---
  drivers/nvmem/transformations.c | 45 +++++++++++++++++++++++++++++++++
  1 file changed, 45 insertions(+)
diff --git a/drivers/nvmem/transformations.c b/drivers/nvmem/transformations.c
index 61642a9feefb..15cd26da1f83 100644
--- a/drivers/nvmem/transformations.c
+++ b/drivers/nvmem/transformations.c
@@ -12,7 +12,52 @@ struct nvmem_transformations {
  	nvmem_cell_post_process_t pp;
  };
  
+/**
+ * nvmem_transform_mac_address_offset() - Add an offset to a mac address cell
+ *
+ * A simple transformation which treats the index argument as an offset and add
+ * it to a mac address. This is useful, if the nvmem cell stores a base
+ * ethernet address.
+ *
+ * @index: nvmem cell index
+ * @data: nvmem data
+ * @bytes: length of the data
+ *
+ * Return: 0 or negative error code on failure.
+ */
+static int nvmem_transform_mac_address_offset(int index, unsigned int offset,
+					      void *data, size_t bytes)
+{
+	if (bytes != ETH_ALEN)
+		return -EINVAL;
+
+	if (index < 0)
+		return -EINVAL;
+
+	if (!is_valid_ether_addr(data))
+		return -EINVAL;
+
+	eth_addr_add(data, index);
+
+	return 0;
+}
+
+static int nvmem_kontron_sl28_vpd_pp(void *priv, const char *id, int index,
+				     unsigned int offset, void *data,
+				     size_t bytes)
+{
+	if (!id)
+		return 0;
+
+	if (!strcmp(id, "mac-address"))
+		return nvmem_transform_mac_address_offset(index, offset, data,
+							  bytes);
+
+	return 0;
+}
+
  static const struct nvmem_transformations nvmem_transformations[] = {
+	{ .compatible = "kontron,sl28-vpd", .pp = nvmem_kontron_sl28_vpd_pp },
  	{}
  };
I think it's a rather bad solution that won't scale well at all.

You'll end up with a lot of NVMEM device specific strings and code in a
NVMEM core.

You'll have a lot of duplicated code (many device specific functions
calling e.g. nvmem_transform_mac_address_offset()).

I think it also ignores fact that one NVMEM device can be reused in
multiple platforms / device models using different (e.g. vendor / device
specific) cells.


What if we have:
1. Foo company using "kontron,sl28-vpd" with NVMEM cells:
    a. "mac-address"
    b. "mac-address-2"
    c. "mac-address-3"
2. Bar company using "kontron,sl28-vpd" with NVMEM cell:
    a. "mac-address"

In the first case you don't want any transformation.


If you consider using transformations for ASCII formats too then it
causes another conflict issue. Consider two devices:

1. Foo company device with BIN format of MAC
2. Bar company device with ASCII format of MAC

Both may use exactly the same binding:

partition@0 {
         compatible = "nvmem-cells";
         reg = <0x0 0x100000>;
         label = "bootloader";

         #address-cells = <1>;
         #size-cells = <1>;

         mac-address@100 {
                 reg = <0x100 0x6>;
         };
};

how are you going to handle them with proposed implementation? You can't
support both if you share "nvmem-cells" compatible string.


I think that what can solve those problems is assing "compatible" to
NVMEM cells.

Let me think about details of that possible solution.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help