[PATCH v2] nvmem: regmap: Fix nvmem size

[PATCH v2] nvmem: regmap: Fix nvmem size

[Robin van der Gracht]

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 <robin@protonic.nl>
---

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

Re: [PATCH v2] nvmem: regmap: Fix nvmem size

[Ahmad Fatoum]

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-5555 |

Re: [PATCH v2] nvmem: regmap: Fix nvmem size

[Robin van der Gracht]

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

[Ahmad Fatoum]

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-5555 |

Re: [PATCH v2] nvmem: regmap: Fix nvmem size

[Sascha Hauer]

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 <robin@protonic.nl>
> ---
> 
> 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-5555 |

Re: [PATCH v2] nvmem: regmap: Fix nvmem size

[Sascha Hauer]

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 <robin@protonic.nl>
> > ---
> > 
> > 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-5555 |

Re: [PATCH v2] nvmem: regmap: Fix nvmem size

[Ahmad Fatoum]

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-5555 |