[PATCH 0/4] nvmem: cell post-processing & U-Boot env MAC support

[PATCH 0/4] nvmem: cell post-processing & U-Boot env MAC support

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

U-Boot environment variables are stored using ASCII format. One of
important entries is "ethaddr" which contains base MAC address.

That NVMEM cell requires some extra processing when reading:
1. ASCII needs translating into binary MAC format
2. Final MAC needs to be calculated depending on cell index

This patchset was originally based on top of layouts implementation
which sadly ended up dropped for now. To proceed I rebased it on top of
the current NVMEM subsystem code. Michael's patch has applied cleanly
and this approach *will not* make U-Boot env transition to layouts any
harder so I believe it's fine to take those patches without waiting for
layouts updated implementation.

Michael Walle (1):
  nvmem: core: add per-cell post processing

Rafał Miłecki (3):
  nvmem: core: allow nvmem_cell_post_process_t callbacks to adjust
    buffer
  dt-bindings: nvmem: u-boot,env: add MAC's #nvmem-cell-cells
  nvmem: u-boot-env: post-process "ethaddr" env variable

 .../devicetree/bindings/nvmem/u-boot,env.yaml |  7 +++-
 drivers/nvmem/Kconfig                         |  1 +
 drivers/nvmem/core.c                          | 38 +++++++++++++++----
 drivers/nvmem/imx-ocotp.c                     |  8 ++--
 drivers/nvmem/u-boot-env.c                    | 25 ++++++++++++
 include/linux/nvmem-provider.h                |  7 +++-
 6 files changed, 71 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH 1/4] nvmem: core: add per-cell post processing

[Rafał Miłecki]

From: Michael Walle <michael@walle.cc>

Instead of relying on the name the consumer is using for the cell, like
it is done for the nvmem .cell_post_process configuration parameter,
provide a per-cell post processing hook. This can then be populated by
the NVMEM provider (or the NVMEM layout) when adding the cell.

Signed-off-by: Michael Walle <michael@walle.cc>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/nvmem/core.c           | 17 +++++++++++++++++
 include/linux/nvmem-provider.h |  3 +++
 2 files changed, 20 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 6783cd8478d7..c5c9a4654241 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -52,6 +52,7 @@ struct nvmem_cell_entry {
 	int			bytes;
 	int			bit_offset;
 	int			nbits;
+	nvmem_cell_post_process_t read_post_process;
 	struct device_node	*np;
 	struct nvmem_device	*nvmem;
 	struct list_head	node;
@@ -465,6 +466,7 @@ static int nvmem_cell_info_to_nvmem_cell_entry_nodup(struct nvmem_device *nvmem,
 	cell->offset = info->offset;
 	cell->bytes = info->bytes;
 	cell->name = info->name;
+	cell->read_post_process = info->read_post_process;
 
 	cell->bit_offset = info->bit_offset;
 	cell->nbits = info->nbits;
@@ -1429,6 +1431,13 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem,
 	if (cell->bit_offset || cell->nbits)
 		nvmem_shift_read_buffer_in_place(cell, buf);
 
+	if (cell->read_post_process) {
+		rc = cell->read_post_process(nvmem->priv, id, index,
+					     cell->offset, buf, cell->bytes);
+		if (rc)
+			return rc;
+	}
+
 	if (nvmem->cell_post_process) {
 		rc = nvmem->cell_post_process(nvmem->priv, id, index,
 					      cell->offset, buf, cell->bytes);
@@ -1537,6 +1546,14 @@ static int __nvmem_cell_entry_write(struct nvmem_cell_entry *cell, void *buf, si
 	    (cell->bit_offset == 0 && len != cell->bytes))
 		return -EINVAL;
 
+	/*
+	 * Any cells which have a read_post_process hook are read-only because
+	 * we cannot reverse the operation and it might affect other cells,
+	 * too.
+	 */
+	if (cell->read_post_process)
+		return -EINVAL;
+
 	if (cell->bit_offset || cell->nbits) {
 		buf = nvmem_cell_prepare_write_buffer(cell, buf, len);
 		if (IS_ERR(buf))
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index b3c14ce87a65..f87fd64eee8f 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -54,6 +54,8 @@ struct nvmem_keepout {
  * @bit_offset:	Bit offset if cell is smaller than a byte.
  * @nbits:	Number of bits.
  * @np:		Optional device_node pointer.
+ * @read_post_process:	Callback for optional post processing of cell data
+ *			on reads.
  */
 struct nvmem_cell_info {
 	const char		*name;
@@ -62,6 +64,7 @@ struct nvmem_cell_info {
 	unsigned int		bit_offset;
 	unsigned int		nbits;
 	struct device_node	*np;
+	nvmem_cell_post_process_t read_post_process;
 };
 
 /**
-- 
2.34.1

[PATCH 2/4] nvmem: core: allow nvmem_cell_post_process_t callbacks to adjust buffer

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Sometimes reading NVMEM cell value involves some data reformatting. it
may require resizing available buffer. Support that.

It's required e.g. to provide properly formatted MAC address in case
it's stored in a non-binary format (e.g. using ASCII).

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/nvmem/core.c           | 23 ++++++++++++++---------
 drivers/nvmem/imx-ocotp.c      |  8 ++++----
 include/linux/nvmem-provider.h |  4 ++--
 3 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index c5c9a4654241..18fbfbf61ec3 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1418,35 +1418,36 @@ static void nvmem_shift_read_buffer_in_place(struct nvmem_cell_entry *cell, void
 
 static int __nvmem_cell_read(struct nvmem_device *nvmem,
 			     struct nvmem_cell_entry *cell,
-			     void *buf, size_t *len, const char *id, int index)
+			     void **buf, size_t *len, const char *id, int index)
 {
+	size_t bytes = cell->bytes;
 	int rc;
 
-	rc = nvmem_reg_read(nvmem, cell->offset, buf, cell->bytes);
+	rc = nvmem_reg_read(nvmem, cell->offset, *buf, bytes);
 
 	if (rc)
 		return rc;
 
 	/* shift bits in-place */
 	if (cell->bit_offset || cell->nbits)
-		nvmem_shift_read_buffer_in_place(cell, buf);
+		nvmem_shift_read_buffer_in_place(cell, *buf);
 
 	if (cell->read_post_process) {
 		rc = cell->read_post_process(nvmem->priv, id, index,
-					     cell->offset, buf, cell->bytes);
+					     cell->offset, buf, &bytes);
 		if (rc)
 			return rc;
 	}
 
 	if (nvmem->cell_post_process) {
 		rc = nvmem->cell_post_process(nvmem->priv, id, index,
-					      cell->offset, buf, cell->bytes);
+					      cell->offset, buf, &bytes);
 		if (rc)
 			return rc;
 	}
 
 	if (len)
-		*len = cell->bytes;
+		*len = bytes;
 
 	return 0;
 }
@@ -1464,7 +1465,7 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem,
 void *nvmem_cell_read(struct nvmem_cell *cell, size_t *len)
 {
 	struct nvmem_device *nvmem = cell->entry->nvmem;
-	u8 *buf;
+	void *buf;
 	int rc;
 
 	if (!nvmem)
@@ -1474,7 +1475,7 @@ void *nvmem_cell_read(struct nvmem_cell *cell, size_t *len)
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
 
-	rc = __nvmem_cell_read(nvmem, cell->entry, buf, len, cell->id, cell->index);
+	rc = __nvmem_cell_read(nvmem, cell->entry, &buf, len, cell->id, cell->index);
 	if (rc) {
 		kfree(buf);
 		return ERR_PTR(rc);
@@ -1791,11 +1792,15 @@ ssize_t nvmem_device_cell_read(struct nvmem_device *nvmem,
 	if (!nvmem)
 		return -EINVAL;
 
+	/* Cells with read_post_process hook may realloc buffer we can't allow here */
+	if (info->read_post_process)
+		return -EINVAL;
+
 	rc = nvmem_cell_info_to_nvmem_cell_entry_nodup(nvmem, info, &cell);
 	if (rc)
 		return rc;
 
-	rc = __nvmem_cell_read(nvmem, &cell, buf, &len, NULL, 0);
+	rc = __nvmem_cell_read(nvmem, &cell, &buf, &len, NULL, 0);
 	if (rc)
 		return rc;
 
diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c
index e37a82f98ba6..0e0ab27cbfe3 100644
--- a/drivers/nvmem/imx-ocotp.c
+++ b/drivers/nvmem/imx-ocotp.c
@@ -223,18 +223,18 @@ static int imx_ocotp_read(void *context, unsigned int offset,
 }
 
 static int imx_ocotp_cell_pp(void *context, const char *id, int index,
-			     unsigned int offset, void *data, size_t bytes)
+			     unsigned int offset, void **data, size_t *bytes)
 {
 	struct ocotp_priv *priv = context;
 
 	/* Deal with some post processing of nvmem cell data */
 	if (id && !strcmp(id, "mac-address")) {
 		if (priv->params->reverse_mac_address) {
-			u8 *buf = data;
+			u8 *buf = *data;
 			int i;
 
-			for (i = 0; i < bytes/2; i++)
-				swap(buf[i], buf[bytes - i - 1]);
+			for (i = 0; i < *bytes / 2; i++)
+				swap(buf[i], buf[*bytes - i - 1]);
 		}
 	}
 
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index f87fd64eee8f..9c212f7bb7d1 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -19,8 +19,8 @@ typedef int (*nvmem_reg_read_t)(void *priv, unsigned int offset,
 typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset,
 				 void *val, size_t bytes);
 /* used for vendor specific post processing of cell data */
-typedef int (*nvmem_cell_post_process_t)(void *priv, const char *id, int index,
-					 unsigned int offset, void *buf, size_t bytes);
+typedef int (*nvmem_cell_post_process_t)(void *priv, const char *id, int index, unsigned int offset,
+					 void **buf, size_t *bytes);
 
 enum nvmem_type {
 	NVMEM_TYPE_UNKNOWN = 0,
-- 
2.34.1

Re: [PATCH 2/4] nvmem: core: allow nvmem_cell_post_process_t callbacks to adjust buffer

[Srinivas Kandagatla]

On 22/02/2023 17:22, Rafał Miłecki wrote:
> @@ -1791,11 +1792,15 @@ ssize_t nvmem_device_cell_read(struct nvmem_device *nvmem,
>   	if (!nvmem)
>   		return -EINVAL;
>   
> +	/* Cells with read_post_process hook may realloc buffer we can't allow here */
> +	if (info->read_post_process)
> +		return -EINVAL;
This should probably go in 1/4 patch. Other than that series looks good 
to me.

--srini

Re: [PATCH 2/4] nvmem: core: allow nvmem_cell_post_process_t callbacks to adjust buffer

[Miquel Raynal]

Hi Srinivas,

srinivas.kandagatla@linaro.org wrote on Thu, 9 Mar 2023 10:12:24 +0000:

> On 22/02/2023 17:22, Rafał Miłecki wrote:
> > @@ -1791,11 +1792,15 @@ ssize_t nvmem_device_cell_read(struct nvmem_device *nvmem,
> >   	if (!nvmem)
> >   		return -EINVAL;  
> >   > +	/* Cells with read_post_process hook may realloc buffer we can't allow here */  
> > +	if (info->read_post_process)
> > +		return -EINVAL;  
> This should probably go in 1/4 patch. Other than that series looks good to me.

FYI patch 1/4 is also carried by the nvmem-layouts series, so it's
probably best to keep these 2 patches separated to simplify the merging.

Thanks,
Miquèl

Re: [PATCH 2/4] nvmem: core: allow nvmem_cell_post_process_t callbacks to adjust buffer

[Srinivas Kandagatla]

On 09/03/2023 10:32, Miquel Raynal wrote:
> Hi Srinivas,
> 
> srinivas.kandagatla@linaro.org wrote on Thu, 9 Mar 2023 10:12:24 +0000:
> 
>> On 22/02/2023 17:22, Rafał Miłecki wrote:
>>> @@ -1791,11 +1792,15 @@ ssize_t nvmem_device_cell_read(struct nvmem_device *nvmem,
>>>    	if (!nvmem)
>>>    		return -EINVAL;
>>>    > +	/* Cells with read_post_process hook may realloc buffer we can't allow here */
>>> +	if (info->read_post_process)
>>> +		return -EINVAL;
>> This should probably go in 1/4 patch. Other than that series looks good to me.
> 
> FYI patch 1/4 is also carried by the nvmem-layouts series, so it's
> probably best to keep these 2 patches separated to simplify the merging.
that is intermediate thing, but Ideally this change belongs to 1/4 
patch, so once I apply these patches then we can always rebase layout 
series on top of nvmem-next

--srini
> 
> Thanks,
> Miquèl

Re: [PATCH 2/4] nvmem: core: allow nvmem_cell_post_process_t callbacks to adjust buffer

[Miquel Raynal]

Hi Srinivas,

srinivas.kandagatla@linaro.org wrote on Thu, 9 Mar 2023 10:53:07 +0000:

> On 09/03/2023 10:32, Miquel Raynal wrote:
> > Hi Srinivas,
> > 
> > srinivas.kandagatla@linaro.org wrote on Thu, 9 Mar 2023 10:12:24 +0000:
> >   
> >> On 22/02/2023 17:22, Rafał Miłecki wrote:  
> >>> @@ -1791,11 +1792,15 @@ ssize_t nvmem_device_cell_read(struct nvmem_device *nvmem,
> >>>    	if (!nvmem)
> >>>    		return -EINVAL;  
> >>>    > +	/* Cells with read_post_process hook may realloc buffer we can't allow here */  
> >>> +	if (info->read_post_process)
> >>> +		return -EINVAL;  
> >> This should probably go in 1/4 patch. Other than that series looks good to me.  
> > 
> > FYI patch 1/4 is also carried by the nvmem-layouts series, so it's
> > probably best to keep these 2 patches separated to simplify the merging.  
> that is intermediate thing, but Ideally this change belongs to 1/4 patch, so once I apply these patches then we can always rebase layout series on top of nvmem-next

Well, I still don't see the need for this patch because we have no use
for it *after* the introduction of layouts. Yes in some cases changing
the size of a cell might maybe be needed, but right now the use case is
to provide a MAC address, we know beforehand the size of the cell, so
there is no need, currently, for this hack.

Whatever. If you want it, just merge it. But *please*, I would like
to see these layouts in, so what's the plan?

Thanks,
Miquèl

Re: [PATCH 2/4] nvmem: core: allow nvmem_cell_post_process_t callbacks to adjust buffer

[Srinivas Kandagatla]

On 09/03/2023 11:23, Miquel Raynal wrote:
> Hi Srinivas,
> 
> srinivas.kandagatla@linaro.org wrote on Thu, 9 Mar 2023 10:53:07 +0000:
> 
>> On 09/03/2023 10:32, Miquel Raynal wrote:
>>> Hi Srinivas,
>>>
>>> srinivas.kandagatla@linaro.org wrote on Thu, 9 Mar 2023 10:12:24 +0000:
>>>    
>>>> On 22/02/2023 17:22, Rafał Miłecki wrote:
>>>>> @@ -1791,11 +1792,15 @@ ssize_t nvmem_device_cell_read(struct nvmem_device *nvmem,
>>>>>     	if (!nvmem)
>>>>>     		return -EINVAL;
>>>>>     > +	/* Cells with read_post_process hook may realloc buffer we can't allow here */
>>>>> +	if (info->read_post_process)
>>>>> +		return -EINVAL;
>>>> This should probably go in 1/4 patch. Other than that series looks good to me.
>>>
>>> FYI patch 1/4 is also carried by the nvmem-layouts series, so it's
>>> probably best to keep these 2 patches separated to simplify the merging.
>> that is intermediate thing, but Ideally this change belongs to 1/4 patch, so once I apply these patches then we can always rebase layout series on top of nvmem-next
> 
> Well, I still don't see the need for this patch because we have no use
> for it *after* the introduction of layouts. Yes in some cases changing
> the size of a cell might maybe be needed, but right now the use case is
> to provide a MAC address, we know beforehand the size of the cell, so
> there is no need, currently, for this hack.
> 
Am confused, should I ignore this series ?

> Whatever. If you want it, just merge it. But *please*, I would like

:-)

> to see these layouts in, so what's the plan?

Am on it, you sent v3 just 24hrs ago :-)


--srini
> 
> Thanks,
> Miquèl

Re: [PATCH 2/4] nvmem: core: allow nvmem_cell_post_process_t callbacks to adjust buffer

[Rafał Miłecki]

On 2023-03-09 12:44, Srinivas Kandagatla wrote:
> On 09/03/2023 11:23, Miquel Raynal wrote:
>> Hi Srinivas,
>> 
>> srinivas.kandagatla@linaro.org wrote on Thu, 9 Mar 2023 10:53:07 
>> +0000:
>> 
>>> On 09/03/2023 10:32, Miquel Raynal wrote:
>>>> Hi Srinivas,
>>>> 
>>>> srinivas.kandagatla@linaro.org wrote on Thu, 9 Mar 2023 10:12:24 
>>>> +0000:
>>>> 
>>>>> On 22/02/2023 17:22, Rafał Miłecki wrote:
>>>>>> @@ -1791,11 +1792,15 @@ ssize_t nvmem_device_cell_read(struct 
>>>>>> nvmem_device *nvmem,
>>>>>>     	if (!nvmem)
>>>>>>     		return -EINVAL;
>>>>>>     > +	/* Cells with read_post_process hook may realloc buffer we 
>>>>>> can't allow here */
>>>>>> +	if (info->read_post_process)
>>>>>> +		return -EINVAL;
>>>>> This should probably go in 1/4 patch. Other than that series looks 
>>>>> good to me.
>>>> 
>>>> FYI patch 1/4 is also carried by the nvmem-layouts series, so it's
>>>> probably best to keep these 2 patches separated to simplify the 
>>>> merging.
>>> that is intermediate thing, but Ideally this change belongs to 1/4 
>>> patch, so once I apply these patches then we can always rebase layout 
>>> series on top of nvmem-next
>> 
>> Well, I still don't see the need for this patch because we have no use
>> for it *after* the introduction of layouts. Yes in some cases changing
>> the size of a cell might maybe be needed, but right now the use case 
>> is
>> to provide a MAC address, we know beforehand the size of the cell, so
>> there is no need, currently, for this hack.
>> 
> Am confused, should I ignore this series ?

I'm confused no less.

I think we have 3 different opinions and no agreement on how to proceed.


Rafał (me):
NVMEM cells should be registered as they are in the raw format. No size
adjustments should happen while registering them. If NVMEM cell requires
some read post-processing then its size should be adjusted *while*
reading.


Michael:
.read_post_process() should be realloc the buffer


Miquel:
While registering NVMEM cell its size should be already adjusted to
match what .read_post_process() is about to return.


I'm really sorry if I got anyone's view wrong.

Re: [PATCH 2/4] nvmem: core: allow nvmem_cell_post_process_t callbacks to adjust buffer

[Miquel Raynal]

Hello,

rafal@milecki.pl wrote on Thu, 09 Mar 2023 12:52:37 +0100:

> On 2023-03-09 12:44, Srinivas Kandagatla wrote:
> > On 09/03/2023 11:23, Miquel Raynal wrote:  
> >> Hi Srinivas,  
> >> >> srinivas.kandagatla@linaro.org wrote on Thu, 9 Mar 2023 10:53:07 >> +0000:  
> >> >>> On 09/03/2023 10:32, Miquel Raynal wrote:  
> >>>> Hi Srinivas,  
> >>>> >>>> srinivas.kandagatla@linaro.org wrote on Thu, 9 Mar 2023 10:12:24 >>>> +0000:  
> >>>> >>>>> On 22/02/2023 17:22, Rafał Miłecki wrote:  
> >>>>>> @@ -1791,11 +1792,15 @@ ssize_t nvmem_device_cell_read(struct >>>>>> nvmem_device *nvmem,
> >>>>>>     	if (!nvmem)
> >>>>>>     		return -EINVAL;  
> >>>>>>     > +	/* Cells with read_post_process hook may realloc buffer we >>>>>> can't allow here */  
> >>>>>> +	if (info->read_post_process)
> >>>>>> +		return -EINVAL;  
> >>>>> This should probably go in 1/4 patch. Other than that series looks >>>>> good to me.  
> >>>> >>>> FYI patch 1/4 is also carried by the nvmem-layouts series, so it's  
> >>>> probably best to keep these 2 patches separated to simplify the >>>> merging.  
> >>> that is intermediate thing, but Ideally this change belongs to 1/4 >>> patch, so once I apply these patches then we can always rebase layout >>> series on top of nvmem-next  
> >> >> Well, I still don't see the need for this patch because we have no use  
> >> for it *after* the introduction of layouts. Yes in some cases changing
> >> the size of a cell might maybe be needed, but right now the use case >> is
> >> to provide a MAC address, we know beforehand the size of the cell, so
> >> there is no need, currently, for this hack.  
> >> > Am confused, should I ignore this series ?  

I think this series makes sense and addresses a need. But this issue
can also be solved with the layouts. Rafał does not want (I still
don't get the reason) to use that solution. Whatever. But if you apply
this series, it requires to modify the layouts series, thus postponing
it even more. I would prefer to merge that big series first and then
merge an update of this patch (which changes in the two layout drivers
the cell size argument type).

> I'm confused no less.
> 
> I think we have 3 different opinions and no agreement on how to proceed.
> 
> 
> Rafał (me):
> NVMEM cells should be registered as they are in the raw format. No size
> adjustments should happen while registering them. If NVMEM cell requires
> some read post-processing then its size should be adjusted *while*
> reading.

This implementation only works if you reduce the size of the cell.

While writing this, I am realizing that we would actually expect
a check on the nvmem side if the size was enlarged because this would
be a bug.

> Michael:
> .read_post_process() should be realloc the buffer

This would be more robust. But if we start with 1, we can improve it
later, I don't mind as long as an error is returned in case of misuse.

> Miquel:
> While registering NVMEM cell its size should be already adjusted to
> match what .read_post_process() is about to return.

Sounds like the simplest solution to me and covers all the uses we
have to day, but honestly, I won't fight for it.

> I'm really sorry if I got anyone's view wrong.

LGTM.

> > Whatever. If you want it, just merge it. But *please*, I would like  
>
> :-)
>
> > to see these layouts in, so what's the plan?  
>
> Am on it, you sent v3 just 24hrs ago :-)

Yes, sorry for being pushy. I just wanted to highlight that the two
series conflict together, but my answer was clumsy. Take the time you
need, that's how it's supposed to work anyway.

Thanks,
Miquèl

Re: [PATCH 2/4] nvmem: core: allow nvmem_cell_post_process_t callbacks to adjust buffer

[Rafał Miłecki]

On 2023-03-09 14:10, Miquel Raynal wrote:
> Hello,
> 
> rafal@milecki.pl wrote on Thu, 09 Mar 2023 12:52:37 +0100:
> 
>> On 2023-03-09 12:44, Srinivas Kandagatla wrote:
>> > On 09/03/2023 11:23, Miquel Raynal wrote:
>> >> Hi Srinivas,
>> >> >> srinivas.kandagatla@linaro.org wrote on Thu, 9 Mar 2023 10:53:07 >> +0000:
>> >> >>> On 09/03/2023 10:32, Miquel Raynal wrote:
>> >>>> Hi Srinivas,
>> >>>> >>>> srinivas.kandagatla@linaro.org wrote on Thu, 9 Mar 2023 10:12:24 >>>> +0000:
>> >>>> >>>>> On 22/02/2023 17:22, Rafał Miłecki wrote:
>> >>>>>> @@ -1791,11 +1792,15 @@ ssize_t nvmem_device_cell_read(struct >>>>>> nvmem_device *nvmem,
>> >>>>>>     	if (!nvmem)
>> >>>>>>     		return -EINVAL;
>> >>>>>>     > +	/* Cells with read_post_process hook may realloc buffer we >>>>>> can't allow here */
>> >>>>>> +	if (info->read_post_process)
>> >>>>>> +		return -EINVAL;
>> >>>>> This should probably go in 1/4 patch. Other than that series looks >>>>> good to me.
>> >>>> >>>> FYI patch 1/4 is also carried by the nvmem-layouts series, so it's
>> >>>> probably best to keep these 2 patches separated to simplify the >>>> merging.
>> >>> that is intermediate thing, but Ideally this change belongs to 1/4 >>> patch, so once I apply these patches then we can always rebase layout >>> series on top of nvmem-next
>> >> >> Well, I still don't see the need for this patch because we have no use
>> >> for it *after* the introduction of layouts. Yes in some cases changing
>> >> the size of a cell might maybe be needed, but right now the use case >> is
>> >> to provide a MAC address, we know beforehand the size of the cell, so
>> >> there is no need, currently, for this hack.
>> >> > Am confused, should I ignore this series ?
> 
> I think this series makes sense and addresses a need. But this issue
> can also be solved with the layouts. Rafał does not want (I still
> don't get the reason) to use that solution. Whatever. But if you apply
> this series, it requires to modify the layouts series, thus postponing
> it even more. I would prefer to merge that big series first and then
> merge an update of this patch (which changes in the two layout drivers
> the cell size argument type).

I'm going to argue those are two independent things.

I can add .read_post_process() without making this driver a layout.
I can make it layout without adding .read_post_process().

I said multiple time that I AM GOING to convert this driver into a 
layout.


>> I'm confused no less.
>> 
>> I think we have 3 different opinions and no agreement on how to 
>> proceed.
>> 
>> 
>> Rafał (me):
>> NVMEM cells should be registered as they are in the raw format. No 
>> size
>> adjustments should happen while registering them. If NVMEM cell 
>> requires
>> some read post-processing then its size should be adjusted *while*
>> reading.
> 
> This implementation only works if you reduce the size of the cell.

Which is enough for MAC. And I was asked to use simple solution.
I also was asked to support reallocationg which was the reason for
my rework.


> While writing this, I am realizing that we would actually expect
> a check on the nvmem side if the size was enlarged because this would
> be a bug.
> 
>> Michael:
>> .read_post_process() should be realloc the buffer
> 
> This would be more robust. But if we start with 1, we can improve it
> later, I don't mind as long as an error is returned in case of misuse.
> 
>> Miquel:
>> While registering NVMEM cell its size should be already adjusted to
>> match what .read_post_process() is about to return.
> 
> Sounds like the simplest solution to me and covers all the uses we
> have to day, but honestly, I won't fight for it.
> 
>> I'm really sorry if I got anyone's view wrong.
> 
> LGTM.
> 
>> > Whatever. If you want it, just merge it. But *please*, I would like
>> 
>> :-)
>> 
>> > to see these layouts in, so what's the plan?
>> 
>> Am on it, you sent v3 just 24hrs ago :-)
> 
> Yes, sorry for being pushy. I just wanted to highlight that the two
> series conflict together, but my answer was clumsy. Take the time you
> need, that's how it's supposed to work anyway.

AFAIR there is a minor conflict caused by the API change that allows
reallocations. I decided to rebase my patchwork because Michael claimed
it's going to take at least few more weeks to rework layouts patchset.

Now we have layouts patchset ready anyway and I'll have to rebase
again. Well, few more wasted hours, shouldn't make much difference.

Re: [PATCH 2/4] nvmem: core: allow nvmem_cell_post_process_t callbacks to adjust buffer

[Srinivas Kandagatla]

On 09/03/2023 11:52, Rafał Miłecki wrote:
> 
> Rafał (me):
> NVMEM cells should be registered as they are in the raw format. No size
> adjustments should happen while registering them. If NVMEM cell requires
> some read post-processing then its size should be adjusted *while*
> reading.
> 
> 
> Michael:
> .read_post_process() should be realloc the buffer
> 
> 
> Miquel:
> While registering NVMEM cell its size should be already adjusted to
> match what .read_post_process() is about to return.
This is the behavior that I would expect,  this is one time thing and 
cell sizes should be fixed before adding them.


--srini

[PATCH 3/4] dt-bindings: nvmem: u-boot,env: add MAC's #nvmem-cell-cells

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

U-Boot's "ethaddr" environment variable is very often used to store
*base* MAC address. It's used as a base for calculating addresses for
multiple interfaces. It's done by adding proper values. Actual offsets
are picked by manufacturers and vary across devices.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/nvmem/u-boot,env.yaml | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
index cbc5c69fd405..36d97fb87865 100644
--- a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
+++ b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
@@ -50,7 +50,11 @@ properties:
 
   ethaddr:
     type: object
-    description: Ethernet interface's MAC address
+    description: Ethernet interfaces base MAC address.
+    properties:
+      "#nvmem-cell-cells":
+        description: The first argument is a MAC address offset.
+        const: 1
 
 additionalProperties: false
 
@@ -72,6 +76,7 @@ examples:
             reg = <0x40000 0x10000>;
 
             mac: ethaddr {
+                #nvmem-cell-cells = <1>;
             };
         };
     };
-- 
2.34.1

[PATCH 4/4] nvmem: u-boot-env: post-process "ethaddr" env variable

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

U-Boot environment variables are stored in ASCII format so "ethaddr"
requires parsing into binary to make it work with Ethernet interfaces.

This includes support for indexes to support #nvmem-cell-cells = <1>.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/nvmem/Kconfig      |  1 +
 drivers/nvmem/u-boot-env.c | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 189ea85bd67d..71322ea7cf53 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -346,6 +346,7 @@ config NVMEM_U_BOOT_ENV
 	tristate "U-Boot environment variables support"
 	depends on OF && MTD
 	select CRC32
+	select GENERIC_NET_UTILS
 	help
 	  U-Boot stores its setup as environment variables. This driver adds
 	  support for verifying & exporting such data. It also exposes variables
diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c
index 29b1d87a3c51..1f5d8aba98bf 100644
--- a/drivers/nvmem/u-boot-env.c
+++ b/drivers/nvmem/u-boot-env.c
@@ -4,6 +4,8 @@
  */
 
 #include <linux/crc32.h>
+#include <linux/etherdevice.h>
+#include <linux/if_ether.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/mtd/mtd.h>
@@ -70,6 +72,27 @@ static int u_boot_env_read(void *context, unsigned int offset, void *val,
 	return 0;
 }
 
+static int u_boot_env_read_post_process_ethaddr(void *context, const char *id, int index,
+						unsigned int offset, void **data, size_t *bytes)
+{
+	u8 mac[ETH_ALEN];
+
+	if (*bytes != 3 * ETH_ALEN - 1)
+		return -EINVAL;
+
+	if (!mac_pton(*data, mac))
+		return -EINVAL;
+
+	if (index)
+		eth_addr_add(mac, index);
+
+	/* We need *smaller* buffer so don't bother to krealloc() */
+	ether_addr_copy(*data, mac);
+	*bytes = ETH_ALEN;
+
+	return 0;
+}
+
 static int u_boot_env_add_cells(struct u_boot_env *priv, uint8_t *buf,
 				size_t data_offset, size_t data_len)
 {
@@ -101,6 +124,8 @@ static int u_boot_env_add_cells(struct u_boot_env *priv, uint8_t *buf,
 		priv->cells[idx].offset = data_offset + value - data;
 		priv->cells[idx].bytes = strlen(value);
 		priv->cells[idx].np = of_get_child_by_name(dev->of_node, priv->cells[idx].name);
+		if (!strcmp(var, "ethaddr"))
+			priv->cells[idx].read_post_process = u_boot_env_read_post_process_ethaddr;
 	}
 
 	if (WARN_ON(idx != priv->ncells))
-- 
2.34.1