Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <455f4360-34fe-7fee-66d5-fd945fe1e086@gmail.com>
Date:   Tue, 25 Jan 2022 13:08:58 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:96.0) Gecko/20100101
 Thunderbird/96.0
Subject: Re: [PATCH 6/8] nvmem: transformations: ethernet address offset
 support
To:     Michael Walle <michael@walle.cc>, linux-mtd@lists.infradead.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org
Cc:     Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Rob Herring <robh+dt@kernel.org>,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Shawn Guo <shawnguo@kernel.org>, Li Yang <leoyang.li@nxp.com>,
        Frank Rowand <frowand.list@gmail.com>,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Ansuel Smith <ansuelsmth@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>
References: <20211228142549.1275412-1-michael@walle.cc>
 <20211228142549.1275412-7-michael@walle.cc>
From:   =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>
In-Reply-To: <20211228142549.1275412-7-michael@walle.cc>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 28.12.2021 15:25, Michael Walle wrote:
> 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 <michael@walle.cc>
> ---
>   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.