diff mbox series

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

Message ID 20230222172245.6313-3-zajec5@gmail.com (mailing list archive)
State New, archived
Headers show
Series nvmem: cell post-processing & U-Boot env MAC support | expand

Commit Message

Rafał Miłecki Feb. 22, 2023, 5:22 p.m. UTC
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(-)

Comments

Srinivas Kandagatla March 9, 2023, 10:12 a.m. UTC | #1
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
Miquel Raynal March 9, 2023, 10:32 a.m. UTC | #2
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
Srinivas Kandagatla March 9, 2023, 10:53 a.m. UTC | #3
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
Miquel Raynal March 9, 2023, 11:23 a.m. UTC | #4
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
Srinivas Kandagatla March 9, 2023, 11:44 a.m. UTC | #5
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
Rafał Miłecki March 9, 2023, 11:52 a.m. UTC | #6
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.
Miquel Raynal March 9, 2023, 1:10 p.m. UTC | #7
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
Rafał Miłecki March 9, 2023, 1:31 p.m. UTC | #8
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.
Srinivas Kandagatla March 10, 2023, 9:25 a.m. UTC | #9
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
diff mbox series

Patch

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,