Re: [PATCH v2] nvmem: regmap: Fix nvmem size
Hello Robin, On 21.12.23 15:02, Ahmad Fatoum wrote: > On 20.12.23 13:38, Robin van der Gracht wrote: >> Hi Ahmad, >> >> On 2023-12-20 10:00, Ahmad Fatoum wrote: >>> Hello Robin, >>> >>> Thanks for the fix. >>> >>> On 20.12.23 09:29, Robin van der Gracht wrote: - if (roffset + rbytes > stride * regmap_get_max_register(map)) + if (roffset + rbytes > regmap_size_bytes(map) * stride) >>> >>> Shouldn't stride on the right hand side be dropped? >> >> roffset = register index * stride. >> >> I.e. 380 for register with index 95. >> >> For stm32mp1x bsec: >> map->format.val_bytes = 4 >> map->reg_stride = 4 >> >> regmap_size_bytes() = map->format.val_bytes * (95 + 1) / map->reg_stride = 96 > > Ouch. What have I been thinking when I named regmap_size_bytes() this way... > Change looks ok then, but we should really rename regmap_size_bytes()... After talking it over with Sascha, I see that the name is correct with regard to how the function was initially used. What's wrong it the implementation and that goes back to 2016.. I just sent out a series fixing this. md -s /dev/stm32-bsec 380 now works. Please verify if it's behaving as you expect. Thanks, Ahmad > >> >> So the result with the stride on the right size is correct. >> >> I moved stride from left to right to be consistent with the size calculation >> in nvmem_regmap_register_with_pp() >> >> Kind regards, >> Robin >> >> >>> >>> Cheers, >>> Ahmad >>> return -EINVAL; for (i = roffset; i < roffset + rbytes; i += stride) { @@ -78,7 +78,7 @@ nvmem_regmap_register_with_pp(struct regmap *map, const char *name, config.priv = map; config.stride = 1; config.word_size = 1; - config.size = regmap_get_max_register(map) * regmap_get_reg_stride(map); + config.size = regmap_size_bytes(map) * regmap_get_reg_stride(map); config.cell_post_process = cell_post_process; config.reg_write = nvmem_regmap_write; config.reg_read = nvmem_regmap_read; >> > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v2] nvmem: regmap: Fix nvmem size
On Tue, Jan 02, 2024 at 11:31:43AM +0100, Sascha Hauer wrote: > On Wed, Dec 20, 2023 at 09:29:08AM +0100, Robin van der Gracht wrote: > > We should add 1 to the max_register index since counting is zero based. > > > > i.e. the stm32mp151 bsec has registers 0 - 95 with reg_stride 4. > > Size should be (95 + 1) * 4 = 384 bytes otherwise we can't access bsec > > register 95 (last one). > > > > regmap_size_bytes() does take the +1 into account so we can use that. > > > > Signed-off-by: Robin van der Gracht > > --- > > > > v2: Fix the size calculation in nvmem_regmap_read() as well. > > > > drivers/nvmem/regmap.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > Applied, thanks And un-applied again, see Ahmads patch Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v2] nvmem: regmap: Fix nvmem size
On Wed, Dec 20, 2023 at 09:29:08AM +0100, Robin van der Gracht wrote: > We should add 1 to the max_register index since counting is zero based. > > i.e. the stm32mp151 bsec has registers 0 - 95 with reg_stride 4. > Size should be (95 + 1) * 4 = 384 bytes otherwise we can't access bsec > register 95 (last one). > > regmap_size_bytes() does take the +1 into account so we can use that. > > Signed-off-by: Robin van der Gracht > --- > > v2: Fix the size calculation in nvmem_regmap_read() as well. > > drivers/nvmem/regmap.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Applied, thanks Sascha > > diff --git a/drivers/nvmem/regmap.c b/drivers/nvmem/regmap.c > index fa5405d7a8..b923c5787d 100644 > --- a/drivers/nvmem/regmap.c > +++ b/drivers/nvmem/regmap.c > @@ -38,7 +38,7 @@ static int nvmem_regmap_read(void *ctx, unsigned offset, > void *buf, size_t bytes > skip_bytes = offset & (stride - 1); > rbytes = roundup(bytes + skip_bytes, stride); > > - if (roffset + rbytes > stride * regmap_get_max_register(map)) > + if (roffset + rbytes > regmap_size_bytes(map) * stride) > return -EINVAL; > > for (i = roffset; i < roffset + rbytes; i += stride) { > @@ -78,7 +78,7 @@ nvmem_regmap_register_with_pp(struct regmap *map, const > char *name, > config.priv = map; > config.stride = 1; > config.word_size = 1; > - config.size = regmap_get_max_register(map) * regmap_get_reg_stride(map); > + config.size = regmap_size_bytes(map) * regmap_get_reg_stride(map); > config.cell_post_process = cell_post_process; > config.reg_write = nvmem_regmap_write; > config.reg_read = nvmem_regmap_read; > -- > 2.40.1 > > > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v2] nvmem: regmap: Fix nvmem size
On 20.12.23 13:38, Robin van der Gracht wrote: > Hi Ahmad, > > On 2023-12-20 10:00, Ahmad Fatoum wrote: >> Hello Robin, >> >> Thanks for the fix. >> >> On 20.12.23 09:29, Robin van der Gracht wrote: >>> - if (roffset + rbytes > stride * regmap_get_max_register(map)) >>> + if (roffset + rbytes > regmap_size_bytes(map) * stride) >> >> Shouldn't stride on the right hand side be dropped? > > roffset = register index * stride. > > I.e. 380 for register with index 95. > > For stm32mp1x bsec: > map->format.val_bytes = 4 > map->reg_stride = 4 > > regmap_size_bytes() = map->format.val_bytes * (95 + 1) / map->reg_stride = 96 Ouch. What have I been thinking when I named regmap_size_bytes() this way... Change looks ok then, but we should really rename regmap_size_bytes()... > > So the result with the stride on the right size is correct. > > I moved stride from left to right to be consistent with the size calculation > in nvmem_regmap_register_with_pp() > > Kind regards, > Robin > > >> >> Cheers, >> Ahmad >> >>> return -EINVAL; >>> >>> for (i = roffset; i < roffset + rbytes; i += stride) { >>> @@ -78,7 +78,7 @@ nvmem_regmap_register_with_pp(struct regmap *map, const >>> char *name, >>> config.priv = map; >>> config.stride = 1; >>> config.word_size = 1; >>> - config.size = regmap_get_max_register(map) * >>> regmap_get_reg_stride(map); >>> + config.size = regmap_size_bytes(map) * regmap_get_reg_stride(map); >>> config.cell_post_process = cell_post_process; >>> config.reg_write = nvmem_regmap_write; >>> config.reg_read = nvmem_regmap_read; > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v2] nvmem: regmap: Fix nvmem size
Hi Ahmad, On 2023-12-20 10:00, Ahmad Fatoum wrote: Hello Robin, Thanks for the fix. On 20.12.23 09:29, Robin van der Gracht wrote: - if (roffset + rbytes > stride * regmap_get_max_register(map)) + if (roffset + rbytes > regmap_size_bytes(map) * stride) Shouldn't stride on the right hand side be dropped? roffset = register index * stride. I.e. 380 for register with index 95. For stm32mp1x bsec: map->format.val_bytes = 4 map->reg_stride = 4 regmap_size_bytes() = map->format.val_bytes * (95 + 1) / map->reg_stride = 96 So the result with the stride on the right size is correct. I moved stride from left to right to be consistent with the size calculation in nvmem_regmap_register_with_pp() Kind regards, Robin Cheers, Ahmad return -EINVAL; for (i = roffset; i < roffset + rbytes; i += stride) { @@ -78,7 +78,7 @@ nvmem_regmap_register_with_pp(struct regmap *map, const char *name, config.priv = map; config.stride = 1; config.word_size = 1; - config.size = regmap_get_max_register(map) * regmap_get_reg_stride(map); + config.size = regmap_size_bytes(map) * regmap_get_reg_stride(map); config.cell_post_process = cell_post_process; config.reg_write = nvmem_regmap_write; config.reg_read = nvmem_regmap_read;
Re: [PATCH v2] nvmem: regmap: Fix nvmem size
Hello Robin, Thanks for the fix. On 20.12.23 09:29, Robin van der Gracht wrote: > - if (roffset + rbytes > stride * regmap_get_max_register(map)) > + if (roffset + rbytes > regmap_size_bytes(map) * stride) Shouldn't stride on the right hand side be dropped? Cheers, Ahmad > return -EINVAL; > > for (i = roffset; i < roffset + rbytes; i += stride) { > @@ -78,7 +78,7 @@ nvmem_regmap_register_with_pp(struct regmap *map, const > char *name, > config.priv = map; > config.stride = 1; > config.word_size = 1; > - config.size = regmap_get_max_register(map) * regmap_get_reg_stride(map); > + config.size = regmap_size_bytes(map) * regmap_get_reg_stride(map); > config.cell_post_process = cell_post_process; > config.reg_write = nvmem_regmap_write; > config.reg_read = nvmem_regmap_read; -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH v2] nvmem: regmap: Fix nvmem size
We should add 1 to the max_register index since counting is zero based. i.e. the stm32mp151 bsec has registers 0 - 95 with reg_stride 4. Size should be (95 + 1) * 4 = 384 bytes otherwise we can't access bsec register 95 (last one). regmap_size_bytes() does take the +1 into account so we can use that. Signed-off-by: Robin van der Gracht --- v2: Fix the size calculation in nvmem_regmap_read() as well. drivers/nvmem/regmap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/nvmem/regmap.c b/drivers/nvmem/regmap.c index fa5405d7a8..b923c5787d 100644 --- a/drivers/nvmem/regmap.c +++ b/drivers/nvmem/regmap.c @@ -38,7 +38,7 @@ static int nvmem_regmap_read(void *ctx, unsigned offset, void *buf, size_t bytes skip_bytes = offset & (stride - 1); rbytes = roundup(bytes + skip_bytes, stride); - if (roffset + rbytes > stride * regmap_get_max_register(map)) + if (roffset + rbytes > regmap_size_bytes(map) * stride) return -EINVAL; for (i = roffset; i < roffset + rbytes; i += stride) { @@ -78,7 +78,7 @@ nvmem_regmap_register_with_pp(struct regmap *map, const char *name, config.priv = map; config.stride = 1; config.word_size = 1; - config.size = regmap_get_max_register(map) * regmap_get_reg_stride(map); + config.size = regmap_size_bytes(map) * regmap_get_reg_stride(map); config.cell_post_process = cell_post_process; config.reg_write = nvmem_regmap_write; config.reg_read = nvmem_regmap_read; -- 2.40.1