Re: [PATCH master 2/7] nvmem: bsec: correct regmap's max_register
Hello Robin, On 08.01.24 13:48, Robin van der Gracht wrote: > On Mon, 8 Jan 2024 12:17:09 +0100 > Robin van der Gracht wrote: > > ... >>> >>> barebox@Linux Automation MC-1 board:/ ls -l /dev/stm32-bsec >>> crw---384 /dev/stm32-bsec >> >> Thats more like it. I'll apply the full series and recheck. > > This works as expected. Thanks. > > Minor note: > > As you mention in your patch notes: > "struct regmap::max_register is in units of struct regmap::reg_stride" Argh. I see now that my wording was ambiguous. I meant to say is that max_register is a multiple of regmap::reg_stride, but one could understand the commit message the other way... > This used to be the value of the maximum register number (index). > The doc in include/linux/regmap.h line 33 mentions 'index'. Maybe that > needs some mentioning of stride as well.. regmap is expressive enough to support a scheme where register numbers increase by 1, but values by 4. This would have been the proper way to register the regmap. Alas, I didn't do it this way when I first added the bsec driver, so we'll have to live with this. > > Regardless: > Tested-by: Robin van der Gracht Thanks. I just Cc'd you on a documentation patch. Cheers, Ahmad -- 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 master 2/7] nvmem: bsec: correct regmap's max_register
On Mon, 8 Jan 2024 12:17:09 +0100 Robin van der Gracht wrote: ... > > > > barebox@Linux Automation MC-1 board:/ ls -l /dev/stm32-bsec > > crw---384 /dev/stm32-bsec > > Thats more like it. I'll apply the full series and recheck. This works as expected. Thanks. Minor note: As you mention in your patch notes: "struct regmap::max_register is in units of struct regmap::reg_stride" This used to be the value of the maximum register number (index). The doc in include/linux/regmap.h line 33 mentions 'index'. Maybe that needs some mentioning of stride as well.. Regardless: Tested-by: Robin van der Gracht
Re: [PATCH master 2/7] nvmem: bsec: correct regmap's max_register
Hello Ahmad,
On Mon, 8 Jan 2024 11:44:00 +0100
Ahmad Fatoum wrote:
> Hello Robin,
>
> On 08.01.24 11:29, Robin van der Gracht wrote:
> > Hi Ahmad,
> >
> > Comments are below.
> >
> > On Tue, 2 Jan 2024 18:00:55 +0100
> > Ahmad Fatoum wrote:
> >
> >> The max_register must be a multiple of the register stride, which is not
> >> the case for (384 / 4) - 1 == 95. Instead, we should be setting 380, so
> >> fix the calculation to do this.
> >>
> >> Fixes: 094ce0ee7cdf ("nvmem: bsec: correct regmap's max_register")
> >> Reported-by: Robin van der Gracht
> >> Signed-off-by: Ahmad Fatoum
> >> ---
> >> drivers/nvmem/bsec.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/nvmem/bsec.c b/drivers/nvmem/bsec.c
> >> index 889f14428d59..22e30c6c2e82 100644
> >> --- a/drivers/nvmem/bsec.c
> >> +++ b/drivers/nvmem/bsec.c
> >> @@ -218,7 +218,7 @@ static int stm32_bsec_probe(struct device *dev)
> >>priv->map_config.reg_bits = 32;
> >>priv->map_config.val_bits = 32;
> >>priv->map_config.reg_stride = 4;
> >> - priv->map_config.max_register = (data->size / 4) - 1;
> >> + priv->map_config.max_register = data->size -
> >> priv->map_config.reg_stride;
> >>
> >>priv->lower = data->lower;
> >>
> >
> > This patch gives a bsec device size of 1520 bytes. Which means I'm now
> > allowed to read/write beyond register 95 without an error.
> >
> > barebox@board:/ ls -l /dev/stm32-bsec
> > crw--- 1520 /dev/stm32-bsec
> >
> > The device size is now in bytes, but the read/write offsets given to
> > the md and mw commands is still in bytes/stride.
> >
> > I.e. to read register 95:
> > md -l -s /dev/stm32-bsec 380+4
> > 017c:
> >
> > I can now also read register 100:
> > md -l -s /dev/stm32-bsec 400+4
> > 0190:
> >
> > This doesn't seem right.
> >
> > max_register should probably stay 95. See doc[1]
> >
> > 1:https://git.pengutronix.de/cgit/barebox/tree/include/linux/regmap.h?h=v2023.12.0#n33
> >
>
> Did you apply the whole series? With the whole series applied I have:
Argh. No. I missed it was part of a series. I was only cc'd to this
one.
>
> barebox@Linux Automation MC-1 board:/ ls -l /dev/stm32-bsec
> crw---384 /dev/stm32-bsec
Thats more like it. I'll apply the full series and recheck.
Robin
Re: [PATCH master 2/7] nvmem: bsec: correct regmap's max_register
Hello Robin,
On 08.01.24 11:29, Robin van der Gracht wrote:
> Hi Ahmad,
>
> Comments are below.
>
> On Tue, 2 Jan 2024 18:00:55 +0100
> Ahmad Fatoum wrote:
>
>> The max_register must be a multiple of the register stride, which is not
>> the case for (384 / 4) - 1 == 95. Instead, we should be setting 380, so
>> fix the calculation to do this.
>>
>> Fixes: 094ce0ee7cdf ("nvmem: bsec: correct regmap's max_register")
>> Reported-by: Robin van der Gracht
>> Signed-off-by: Ahmad Fatoum
>> ---
>> drivers/nvmem/bsec.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvmem/bsec.c b/drivers/nvmem/bsec.c
>> index 889f14428d59..22e30c6c2e82 100644
>> --- a/drivers/nvmem/bsec.c
>> +++ b/drivers/nvmem/bsec.c
>> @@ -218,7 +218,7 @@ static int stm32_bsec_probe(struct device *dev)
>> priv->map_config.reg_bits = 32;
>> priv->map_config.val_bits = 32;
>> priv->map_config.reg_stride = 4;
>> -priv->map_config.max_register = (data->size / 4) - 1;
>> +priv->map_config.max_register = data->size -
>> priv->map_config.reg_stride;
>>
>> priv->lower = data->lower;
>>
>
> This patch gives a bsec device size of 1520 bytes. Which means I'm now
> allowed to read/write beyond register 95 without an error.
>
> barebox@board:/ ls -l /dev/stm32-bsec
> crw--- 1520 /dev/stm32-bsec
>
> The device size is now in bytes, but the read/write offsets given to
> the md and mw commands is still in bytes/stride.
>
> I.e. to read register 95:
> md -l -s /dev/stm32-bsec 380+4
> 017c:
>
> I can now also read register 100:
> md -l -s /dev/stm32-bsec 400+4
> 0190:
>
> This doesn't seem right.
>
> max_register should probably stay 95. See doc[1]
>
> 1:https://git.pengutronix.de/cgit/barebox/tree/include/linux/regmap.h?h=v2023.12.0#n33
Did you apply the whole series? With the whole series applied I have:
barebox@Linux Automation MC-1 board:/ ls -l /dev/stm32-bsec
crw---384 /dev/stm32-bsec
Because there are two issues (size in bsec driver is wrong, cdev size is
calculated wrong),
I need to decide which to fix first or squash them. I chose to make the size
intermittently
bigger (so reading valid offsets still work) instead of making it smaller and
breaking
bisect (or squashing all and making it less easy to review).
Cheers,
Ahmad
>
> Robin
>
--
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 master 2/7] nvmem: bsec: correct regmap's max_register
Hi Ahmad,
Comments are below.
On Tue, 2 Jan 2024 18:00:55 +0100
Ahmad Fatoum wrote:
> The max_register must be a multiple of the register stride, which is not
> the case for (384 / 4) - 1 == 95. Instead, we should be setting 380, so
> fix the calculation to do this.
>
> Fixes: 094ce0ee7cdf ("nvmem: bsec: correct regmap's max_register")
> Reported-by: Robin van der Gracht
> Signed-off-by: Ahmad Fatoum
> ---
> drivers/nvmem/bsec.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvmem/bsec.c b/drivers/nvmem/bsec.c
> index 889f14428d59..22e30c6c2e82 100644
> --- a/drivers/nvmem/bsec.c
> +++ b/drivers/nvmem/bsec.c
> @@ -218,7 +218,7 @@ static int stm32_bsec_probe(struct device *dev)
> priv->map_config.reg_bits = 32;
> priv->map_config.val_bits = 32;
> priv->map_config.reg_stride = 4;
> - priv->map_config.max_register = (data->size / 4) - 1;
> + priv->map_config.max_register = data->size -
> priv->map_config.reg_stride;
>
> priv->lower = data->lower;
>
This patch gives a bsec device size of 1520 bytes. Which means I'm now
allowed to read/write beyond register 95 without an error.
barebox@board:/ ls -l /dev/stm32-bsec
crw--- 1520 /dev/stm32-bsec
The device size is now in bytes, but the read/write offsets given to
the md and mw commands is still in bytes/stride.
I.e. to read register 95:
md -l -s /dev/stm32-bsec 380+4
017c:
I can now also read register 100:
md -l -s /dev/stm32-bsec 400+4
0190:
This doesn't seem right.
max_register should probably stay 95. See doc[1]
1:https://git.pengutronix.de/cgit/barebox/tree/include/linux/regmap.h?h=v2023.12.0#n33
Robin
