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@xxxxxxxxxx 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@xxxxxxxxxx wrote on Thu, 9 Mar 2023 10:53:07 >> +0000:
> > >> >>> On 09/03/2023 10:32, Miquel Raynal wrote:
> > >>>> Hi Srinivas,
> > >>>> >>>> srinivas.kandagatla@xxxxxxxxxx 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.