| Message ID | 20230222172245.6313-3-zajec5@gmail.com |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | nvmem: cell post-processing & U-Boot env MAC support | expand |
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
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
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
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
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
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.
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
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.
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 --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,