[PATCH master 0/7] regmap: fix size of regmap-backed cdev and nvmem

[PATCH master 0/7] regmap: fix size of regmap-backed cdev and nvmem

[Ahmad Fatoum]

struct regmap::max_register is in units of struct regmap::reg_stride.
To get the total number or registers, we need to divide by reg_stride
before adding one, but we ended up adding one before division.

This is wrong at different places across the tree, leading to the last
fuse to be inaccessible when not using the regmap API directly, i.e.
when using a NVMEM registered by nvmem_register_regmap or the cdev
instantiated in /dev.

Ahmad Fatoum (6):
  regmap: fix calculation of regmap size when reg_stride != 1
  nvmem: bsec: correct regmap's max_register
  nvmem: startfive-otp: correct regmap's max_register
  nvmem: imx-ocotp-ele: correct regmap's max_register
  nvmem: ocotp: correct regmap's max_register
  nvmem: ocotp: align OCOTP bank count with Linux

Robin van der Gracht (1):
  nvmem: regmap: Fix nvmem size

 drivers/base/regmap/regmap.c  | 28 ++++++++++++++++++++++++++--
 drivers/nvmem/bsec.c          |  2 +-
 drivers/nvmem/imx-ocotp-ele.c |  2 +-
 drivers/nvmem/ocotp.c         | 24 ++++++++++++------------
 drivers/nvmem/regmap.c        |  4 ++--
 drivers/nvmem/starfive-otp.c  |  2 +-
 6 files changed, 43 insertions(+), 19 deletions(-)

-- 
2.39.2

[PATCH master 1/7] regmap: fix calculation of regmap size when reg_stride != 1

[Ahmad Fatoum]

struct regmap::max_register is in units of struct regmap::reg_stride.
To get the total number or registers, we need to divide by reg_stride
before adding one, but we ended up adding one before division.

regmap_size_bytes() is currently only used for sizing the cdev and
rendered the last element of the cdev inaccessible.

Fixes: 7a53e162de2a ("Add initial regmap support")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/base/regmap/regmap.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 4d896c677b28..7ad527954c43 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -412,10 +412,34 @@ static struct cdev_operations regmap_fops = {
 	.write	= regmap_cdev_write,
 };
 
+/*
+ * regmap_count_registers - returns the total number of registers
+ *
+ * @map:	The map
+ *
+ * Returns the total number of registers in a regmap
+ */
+static size_t regmap_count_registers(struct regmap *map)
+{
+	/*
+	 * max_register is in units of reg_stride, so we need to divide
+	 * by the register stride before adding one to arrive at the
+	 * total number of registers.
+	 */
+	return (map->max_register / map->reg_stride) + 1;
+}
+
+/*
+ * regmap_size_bytes - computes the size of the regmap in bytes
+ *
+ * @map:	The map
+ *
+ * Returns the number of bytes needed to hold all values in the
+ * regmap.
+ */
 size_t regmap_size_bytes(struct regmap *map)
 {
-	return regmap_round_val_bytes(map) * (map->max_register + 1) /
-		map->reg_stride;
+	return regmap_round_val_bytes(map) * regmap_count_registers(map);
 }
 
 /*
-- 
2.39.2

[PATCH master 2/7] nvmem: bsec: correct regmap's max_register

[Ahmad Fatoum]

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 <robin@protonic.nl>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 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;
 
-- 
2.39.2

[PATCH master 3/7] nvmem: startfive-otp: correct regmap's max_register

[Ahmad Fatoum]

The max_register must be a multiple of the register stride, which is not
the case for 0x200 - 1 == 0x1ff. Instead, we should be setting
(0x1ff * 4), so fix the calculation to do this.

Fixes: 88f3d13bafc6 ("nvmem: add StarFive OTP support")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/nvmem/starfive-otp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvmem/starfive-otp.c b/drivers/nvmem/starfive-otp.c
index d22350b10ef2..47b94b1399a7 100644
--- a/drivers/nvmem/starfive-otp.c
+++ b/drivers/nvmem/starfive-otp.c
@@ -172,7 +172,7 @@ static int starfive_otp_probe(struct device *dev)
 	config.reg_bits = 32;
 	config.val_bits = 32;
 	config.reg_stride = 4;
-	config.max_register = total_fuses;
+	config.max_register = (total_fuses - 1) * config.reg_stride;
 
 	priv = xzalloc(sizeof(*priv));
 
-- 
2.39.2

[PATCH master 4/7] nvmem: imx-ocotp-ele: correct regmap's max_register

[Ahmad Fatoum]

The max_register must be a multiple of the register stride, which is not
the case for 2048 - 1 == 2047. Instead, we should be setting 2044, so
fix the calculation to do this.

Fixes: e75746d53cca ("nvmem: Add support for i.MX ELE ocotp")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/nvmem/imx-ocotp-ele.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvmem/imx-ocotp-ele.c b/drivers/nvmem/imx-ocotp-ele.c
index dbdb70d46fad..b748a30b1fc7 100644
--- a/drivers/nvmem/imx-ocotp-ele.c
+++ b/drivers/nvmem/imx-ocotp-ele.c
@@ -139,7 +139,7 @@ static int imx_ele_ocotp_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 = priv->data->size - 1;
+	priv->map_config.max_register = priv->data->size - priv->map_config.reg_stride;
 
 	priv->map = regmap_init(dev, &imx_ocotp_regmap_bus, priv, &priv->map_config);
 	if (IS_ERR(priv->map))
-- 
2.39.2

[PATCH master 5/7] nvmem: ocotp: correct regmap's max_register

[Ahmad Fatoum]

The Fuse words of the i.MX are 32-bit wide each and their number vary by
SoC. For the i.MX, there are 128 fuse words, which if linearized, are a
total 512 bytes in size.

The barebox driver calls the total size num_regs, which is misleading
and then sets the max_register to be the total_size - 1, which is wrong
as it's supposed to be e a multiple of the register stride, which is not
the case for (512 / 4) - 1 == 127.

Instead, we should be setting 508. Fix the calculation to do this and
while at it, rename the driver data member to its Linux counterpart.

Fixes: 2ee6bb35043d ("ARM: i.MX: ocotp: Switch to regmap support")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/nvmem/ocotp.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/nvmem/ocotp.c b/drivers/nvmem/ocotp.c
index 641c825986cb..76c50a964435 100644
--- a/drivers/nvmem/ocotp.c
+++ b/drivers/nvmem/ocotp.c
@@ -107,7 +107,7 @@ enum imx_ocotp_format_mac_direction {
 struct ocotp_priv;
 
 struct imx_ocotp_data {
-	int num_regs;
+	int nregs;
 	u32 (*addr_to_offset)(u32 addr);
 	void (*format_mac)(u8 *dst, const u8 *src,
 			   enum imx_ocotp_format_mac_direction dir);
@@ -769,7 +769,7 @@ static int imx_ocotp_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->num_regs - 1;
+	priv->map_config.max_register = priv->map_config.reg_stride * (data->nregs - 1);
 
 	priv->map = regmap_init(dev, &imx_ocotp_regmap_bus, priv, &priv->map_config);
 	if (IS_ERR(priv->map))
@@ -853,7 +853,7 @@ static u32 vf610_addr_to_offset(u32 addr)
 }
 
 static struct imx_ocotp_data imx6q_ocotp_data = {
-	.num_regs = 512,
+	.nregs = 128,
 	.addr_to_offset = imx6q_addr_to_offset,
 	.mac_offsets_num = 1,
 	.mac_offsets = { MAC_OFFSET_0 },
@@ -864,7 +864,7 @@ static struct imx_ocotp_data imx6q_ocotp_data = {
 };
 
 static struct imx_ocotp_data imx6sl_ocotp_data = {
-	.num_regs = 256,
+	.nregs = 64,
 	.addr_to_offset = imx6sl_addr_to_offset,
 	.mac_offsets_num = 1,
 	.mac_offsets = { MAC_OFFSET_0 },
@@ -875,7 +875,7 @@ static struct imx_ocotp_data imx6sl_ocotp_data = {
 };
 
 static struct imx_ocotp_data imx6ul_ocotp_data = {
-	.num_regs = 512,
+	.nregs = 128,
 	.addr_to_offset = imx6q_addr_to_offset,
 	.mac_offsets_num = 2,
 	.mac_offsets = { MAC_OFFSET_0, IMX6UL_MAC_OFFSET_1 },
@@ -886,7 +886,7 @@ static struct imx_ocotp_data imx6ul_ocotp_data = {
 };
 
 static struct imx_ocotp_data imx6ull_ocotp_data = {
-	.num_regs = 256,
+	.nregs = 64,
 	.addr_to_offset = imx6q_addr_to_offset,
 	.mac_offsets_num = 2,
 	.mac_offsets = { MAC_OFFSET_0, IMX6UL_MAC_OFFSET_1 },
@@ -897,7 +897,7 @@ static struct imx_ocotp_data imx6ull_ocotp_data = {
 };
 
 static struct imx_ocotp_data vf610_ocotp_data = {
-	.num_regs = 512,
+	.nregs = 128,
 	.addr_to_offset = vf610_addr_to_offset,
 	.mac_offsets_num = 2,
 	.mac_offsets = { MAC_OFFSET_0, MAC_OFFSET_1 },
@@ -914,7 +914,7 @@ static struct imx8m_featctrl_data imx8mp_featctrl_data = {
 };
 
 static struct imx_ocotp_data imx8mp_ocotp_data = {
-	.num_regs = 1024,
+	.nregs = 256,
 	.addr_to_offset = imx6sl_addr_to_offset,
 	.mac_offsets_num = 2,
 	.mac_offsets = { 0x90, 0x94 },
@@ -923,7 +923,7 @@ static struct imx_ocotp_data imx8mp_ocotp_data = {
 };
 
 static struct imx_ocotp_data imx8mq_ocotp_data = {
-	.num_regs = 2048,
+	.nregs = 512,
 	.addr_to_offset = imx6sl_addr_to_offset,
 	.mac_offsets_num = 1,
 	.mac_offsets = { 0x90 },
@@ -939,7 +939,7 @@ static struct imx8m_featctrl_data imx8mm_featctrl_data = {
 };
 
 static struct imx_ocotp_data imx8mm_ocotp_data = {
-	.num_regs = 2048,
+	.nregs = 512,
 	.addr_to_offset = imx6sl_addr_to_offset,
 	.mac_offsets_num = 1,
 	.mac_offsets = { 0x90 },
@@ -956,7 +956,7 @@ static struct imx8m_featctrl_data imx8mn_featctrl_data = {
 };
 
 static struct imx_ocotp_data imx8mn_ocotp_data = {
-	.num_regs = 2048,
+	.nregs = 512,
 	.addr_to_offset = imx6sl_addr_to_offset,
 	.mac_offsets_num = 1,
 	.mac_offsets = { 0x90 },
@@ -968,7 +968,7 @@ static struct imx_ocotp_data imx8mn_ocotp_data = {
 };
 
 static struct imx_ocotp_data imx7d_ocotp_data = {
-	.num_regs = 2048,
+	.nregs = 512,
 	.addr_to_offset = imx6sl_addr_to_offset,
 	.mac_offsets_num = 1,
 	.mac_offsets = { MAC_OFFSET_0, IMX6UL_MAC_OFFSET_1 },
-- 
2.39.2

[PATCH master 6/7] nvmem: ocotp: align OCOTP bank count with Linux

[Ahmad Fatoum]

Now that struct imx_ocotp_data::nregs is equivalent to the Linux driver's
struct ocotp_params::nregs, the differences between the Linux driver and
barebox are easy to spot. Some of the differences were only quite recently
added to Linux in v6.7-rc1 (i.MX6UL, i.MX6ULL), but others have been
that way since they were first added to the Linux driver.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
Cc: Rouven Czerwinski <rcz@pengutronix.de>
Cc: Christian Eggers <ceggers@arri.de>
---
 drivers/nvmem/ocotp.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/nvmem/ocotp.c b/drivers/nvmem/ocotp.c
index 76c50a964435..6480fd95238e 100644
--- a/drivers/nvmem/ocotp.c
+++ b/drivers/nvmem/ocotp.c
@@ -875,7 +875,7 @@ static struct imx_ocotp_data imx6sl_ocotp_data = {
 };
 
 static struct imx_ocotp_data imx6ul_ocotp_data = {
-	.nregs = 128,
+	.nregs = 144,
 	.addr_to_offset = imx6q_addr_to_offset,
 	.mac_offsets_num = 2,
 	.mac_offsets = { MAC_OFFSET_0, IMX6UL_MAC_OFFSET_1 },
@@ -886,7 +886,7 @@ static struct imx_ocotp_data imx6ul_ocotp_data = {
 };
 
 static struct imx_ocotp_data imx6ull_ocotp_data = {
-	.nregs = 64,
+	.nregs = 80,
 	.addr_to_offset = imx6q_addr_to_offset,
 	.mac_offsets_num = 2,
 	.mac_offsets = { MAC_OFFSET_0, IMX6UL_MAC_OFFSET_1 },
@@ -914,7 +914,7 @@ static struct imx8m_featctrl_data imx8mp_featctrl_data = {
 };
 
 static struct imx_ocotp_data imx8mp_ocotp_data = {
-	.nregs = 256,
+	.nregs = 384,
 	.addr_to_offset = imx6sl_addr_to_offset,
 	.mac_offsets_num = 2,
 	.mac_offsets = { 0x90, 0x94 },
@@ -923,7 +923,7 @@ static struct imx_ocotp_data imx8mp_ocotp_data = {
 };
 
 static struct imx_ocotp_data imx8mq_ocotp_data = {
-	.nregs = 512,
+	.nregs = 256,
 	.addr_to_offset = imx6sl_addr_to_offset,
 	.mac_offsets_num = 1,
 	.mac_offsets = { 0x90 },
@@ -939,7 +939,7 @@ static struct imx8m_featctrl_data imx8mm_featctrl_data = {
 };
 
 static struct imx_ocotp_data imx8mm_ocotp_data = {
-	.nregs = 512,
+	.nregs = 256,
 	.addr_to_offset = imx6sl_addr_to_offset,
 	.mac_offsets_num = 1,
 	.mac_offsets = { 0x90 },
@@ -956,7 +956,7 @@ static struct imx8m_featctrl_data imx8mn_featctrl_data = {
 };
 
 static struct imx_ocotp_data imx8mn_ocotp_data = {
-	.nregs = 512,
+	.nregs = 256,
 	.addr_to_offset = imx6sl_addr_to_offset,
 	.mac_offsets_num = 1,
 	.mac_offsets = { 0x90 },
@@ -968,7 +968,7 @@ static struct imx_ocotp_data imx8mn_ocotp_data = {
 };
 
 static struct imx_ocotp_data imx7d_ocotp_data = {
-	.nregs = 512,
+	.nregs = 64,
 	.addr_to_offset = imx6sl_addr_to_offset,
 	.mac_offsets_num = 1,
 	.mac_offsets = { MAC_OFFSET_0, IMX6UL_MAC_OFFSET_1 },
-- 
2.39.2

[PATCH master 7/7] nvmem: regmap: Fix nvmem size

[Ahmad Fatoum]

From: Robin van der Gracht <robin@protonic.nl>

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.

Fixes: b4abbd8a6cbb ("nvmem: add nvmem_regmap_register helper")
Signed-off-by: Robin van der Gracht <robin@protonic.nl>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v2 was here: https://lore.barebox.org/barebox/20240102105330.GA1318922@pengutronix.de/T/#t

v2 -> v3:
  - use regmap_size_bytes() without multiplication with stride after
    fixing its implementation.
---
 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 fa5405d7a86a..24712fbb0f33 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))
 		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);
 	config.cell_post_process = cell_post_process;
 	config.reg_write = nvmem_regmap_write;
 	config.reg_read = nvmem_regmap_read;
-- 
2.39.2

Re: [PATCH master 0/7] regmap: fix size of regmap-backed cdev and nvmem

[Sascha Hauer]

On Tue, Jan 02, 2024 at 06:00:53PM +0100, Ahmad Fatoum wrote:
> struct regmap::max_register is in units of struct regmap::reg_stride.
> To get the total number or registers, we need to divide by reg_stride
> before adding one, but we ended up adding one before division.
> 
> This is wrong at different places across the tree, leading to the last
> fuse to be inaccessible when not using the regmap API directly, i.e.
> when using a NVMEM registered by nvmem_register_regmap or the cdev
> instantiated in /dev.
> 
> Ahmad Fatoum (6):
>   regmap: fix calculation of regmap size when reg_stride != 1
>   nvmem: bsec: correct regmap's max_register
>   nvmem: startfive-otp: correct regmap's max_register
>   nvmem: imx-ocotp-ele: correct regmap's max_register
>   nvmem: ocotp: correct regmap's max_register
>   nvmem: ocotp: align OCOTP bank count with Linux
> 
> Robin van der Gracht (1):
>   nvmem: regmap: Fix nvmem size

Applied, thanks

Sascha

> 
>  drivers/base/regmap/regmap.c  | 28 ++++++++++++++++++++++++++--
>  drivers/nvmem/bsec.c          |  2 +-
>  drivers/nvmem/imx-ocotp-ele.c |  2 +-
>  drivers/nvmem/ocotp.c         | 24 ++++++++++++------------
>  drivers/nvmem/regmap.c        |  4 ++--
>  drivers/nvmem/starfive-otp.c  |  2 +-
>  6 files changed, 43 insertions(+), 19 deletions(-)
> 
> -- 
> 2.39.2
> 
> 
> 

-- 
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 master 2/7] nvmem: bsec: correct regmap's max_register

[Robin van der Gracht]

Hi Ahmad,

Comments are below.

On Tue,  2 Jan 2024 18:00:55 +0100
Ahmad Fatoum <a.fatoum@pengutronix.de> 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 <robin@protonic.nl>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  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 
0000017c: xxxxxxxx

I can now also read register 100:
md -l -s /dev/stm32-bsec 400+4 
00000190: 00000000                                           ....

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

Re: [PATCH master 2/7] nvmem: bsec: correct regmap's max_register

[Ahmad Fatoum]

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 <a.fatoum@pengutronix.de> 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 <robin@protonic.nl>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  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 
> 0000017c: xxxxxxxx
> 
> I can now also read register 100:
> md -l -s /dev/stm32-bsec 400+4 
> 00000190: 00000000                                           ....
> 
> 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-5555 |

Re: [PATCH master 2/7] nvmem: bsec: correct regmap's max_register

[Robin van der Gracht]

Hello Ahmad,

On Mon, 8 Jan 2024 11:44:00 +0100
Ahmad Fatoum <a.fatoum@pengutronix.de> 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 <a.fatoum@pengutronix.de> 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 <robin@protonic.nl>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >>  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 
> > 0000017c: xxxxxxxx
> > 
> > I can now also read register 100:
> > md -l -s /dev/stm32-bsec 400+4 
> > 00000190: 00000000                                           ....
> > 
> > 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

[Robin van der Gracht]

On Mon, 8 Jan 2024 12:17:09 +0100
Robin van der Gracht <robin@protonic.nl> 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 <robin@protonic.nl>

Re: [PATCH master 2/7] nvmem: bsec: correct regmap's max_register

[Ahmad Fatoum]

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

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