[PATCH 1/3] dt-bindings: nvmem: u-boot,env: Add support for u-boot,env-size

[PATCH 1/3] dt-bindings: nvmem: u-boot,env: Add support for u-boot,env-size

[Christian Marangi]

Add support for u-boot,env-size new property.

Permit to declare a custom size of the U-Boot env that differs than the
partition size where the U-Boot env is located.

U-Boot env is validated by calculating the CRC32 on the entire env
and in some specific case, the env size might differ from the partition
size resulting in wrong CRC32 calculation than the expected one saved at
the start of the partition.

This happens when U-Boot is compiled by hardcoding a specific env size
but the env is actually placed in a bigger partition, resulting in needing
to provide a custom value.

Declaring this property, this value will be used for NVMEM size instead of
the mtd partition.

Add also an example to make it clear the scenario of mismatched
partition size and actual U-Boot env.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../devicetree/bindings/nvmem/u-boot,env.yaml | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
index 36d97fb87865..3970725a2c57 100644
--- a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
+++ b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
@@ -44,6 +44,24 @@ properties:
   reg:
     maxItems: 1
 
+  u-boot,env-size:
+    description: |
+      Permit to declare a custom size of the U-Boot env that differs than the
+      partition size where the U-Boot env is located.
+
+      U-Boot env is validated by calculating the CRC32 on the entire env
+      and in some specific case, the env size might differ from the partition
+      size resulting in wrong CRC32 calculation than the expected one saved at
+      the start of the partition.
+
+      This happens when U-Boot is compiled by hardcoding a specific env size
+      but the env is actually placed in a bigger partition, resulting in needing
+      to provide a custom value.
+
+      Declaring this property, this value will be used for NVMEM size instead of
+      the mtd partition.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
   bootcmd:
     type: object
     description: Command to use for automatic booting
@@ -99,3 +117,32 @@ examples:
             };
         };
     };
+  - |
+    partitions {
+        compatible = "fixed-partitions";
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        partition@0 {
+            reg = <0x0 0xc80000>;
+            label = "qcadata";
+            read-only;
+        };
+
+        partition@c80000 {
+            label = "APPSBL";
+            reg = <0xc80000 0x500000>;
+            read-only;
+        };
+
+        partition@1180000 {
+            compatible = "u-boot,env";
+            reg = <0x1180000 0x80000>;
+
+            u-boot,env-size = <0x40000>;
+
+            mac1: ethaddr {
+                #nvmem-cell-cells = <1>;
+            };
+        };
+    };
-- 
2.40.1

[PATCH 2/3] nvmem: u-boot-env: Permit to declare custom env-size

[Christian Marangi]

Permit to declare a custom size of the U-Boot env that differs than the
partition size where the U-Boot env is located.

U-Boot env is validated by calculating the CRC32 on the entire env
and in some specific case, the env size might differ from the partition
size resulting in wrong CRC32 calculation than the expected one saved at
the start of the partition.

This happens when U-Boot is compiled by hardcoding a specific env size
but the env is actually placed in a bigger partition, resulting in needing
to provide a custom value.

To declare a custom env-size, the new property 'u-boot,env-size' is
introduced to handle this special case.

If the property is not declared, the mtd size is used instead.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/nvmem/u-boot-env.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c
index ee9fd9989b6e..b64c37308789 100644
--- a/drivers/nvmem/u-boot-env.c
+++ b/drivers/nvmem/u-boot-env.c
@@ -24,6 +24,7 @@ enum u_boot_env_format {
 struct u_boot_env {
 	struct device *dev;
 	enum u_boot_env_format format;
+	unsigned int u_boot_env_size;
 
 	struct mtd_info *mtd;
 
@@ -149,14 +150,14 @@ static int u_boot_env_parse(struct u_boot_env *priv)
 	uint8_t *buf;
 	int err;
 
-	buf = kcalloc(1, priv->mtd->size, GFP_KERNEL);
+	buf = kcalloc(1, priv->u_boot_env_size, GFP_KERNEL);
 	if (!buf) {
 		err = -ENOMEM;
 		goto err_out;
 	}
 
-	err = mtd_read(priv->mtd, 0, priv->mtd->size, &bytes, buf);
-	if ((err && !mtd_is_bitflip(err)) || bytes != priv->mtd->size) {
+	err = mtd_read(priv->mtd, 0, priv->u_boot_env_size, &bytes, buf);
+	if ((err && !mtd_is_bitflip(err)) || bytes != priv->u_boot_env_size) {
 		dev_err(dev, "Failed to read from mtd: %d\n", err);
 		goto err_kfree;
 	}
@@ -179,8 +180,8 @@ static int u_boot_env_parse(struct u_boot_env *priv)
 		break;
 	}
 	crc32 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));
-	crc32_data_len = priv->mtd->size - crc32_data_offset;
-	data_len = priv->mtd->size - data_offset;
+	crc32_data_len = priv->u_boot_env_size - crc32_data_offset;
+	data_len = priv->u_boot_env_size - data_offset;
 
 	calc = crc32(~0, buf + crc32_data_offset, crc32_data_len) ^ ~0L;
 	if (calc != crc32) {
@@ -189,7 +190,7 @@ static int u_boot_env_parse(struct u_boot_env *priv)
 		goto err_kfree;
 	}
 
-	buf[priv->mtd->size - 1] = '\0';
+	buf[priv->u_boot_env_size - 1] = '\0';
 	err = u_boot_env_add_cells(priv, buf, data_offset, data_len);
 	if (err)
 		dev_err(dev, "Failed to add cells: %d\n", err);
@@ -224,6 +225,10 @@ static int u_boot_env_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->mtd);
 	}
 
+	/* In absence of a custom env size, use the full mtd partition size */
+	if (of_property_read_u32(np, "u-boot,env-size", &priv->u_boot_env_size))
+		priv->u_boot_env_size = priv->mtd->size;
+
 	err = u_boot_env_parse(priv);
 	if (err)
 		return err;
@@ -232,7 +237,7 @@ static int u_boot_env_probe(struct platform_device *pdev)
 	config.cells = priv->cells;
 	config.ncells = priv->ncells;
 	config.priv = priv;
-	config.size = priv->mtd->size;
+	config.size = priv->u_boot_env_size;
 
 	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
 }
-- 
2.40.1

[PATCH 3/3] nvmem: u-boot-env: Handle "reduced" ASCII address declaration

[Christian Marangi]

Parsing ASCII values is hard as hex values can be declared in many ways
and OEM never follow a genera rules. This is especially problematic for
parsing MAC address writte in ASCII format where an hex values can be
written in 2 different format:
- a Full format with leading 0, 01 02 06 0A
- a Reduced format with trimmed leading 0, 1 2 6 A

The current U-Boot-env driver assume that the Full format is always used
by parsting the NVMEM cell size and expect always the form of
XX:XX:XX:XX:XX:XX to pass it directly to mac_pton that expects this
format.

Reality is that it's common for OEM to use the reduced format resulting
in MAC address in the format of XX:a:XX:XX:XX:XX:XX or a:XX:XX:XX:XX:XX
or XX:XX:XX:XX:XX:a.

To handle these special format additional care is needed.

Some additional logic are added to "normalize" the MAC address in ASCII
to a format that is accepted by mac_pton.

As using the NVMEM cell size is not enough anymore, some additional
validation are done by checking if we have at least a format of
X:X:X:X:X:X by checking if we have at least 5 ':' char while checking
the NVMEM cell. The size validation is changed to check a range of
ETH_ALEN + 5 (assuming a min valid MAC address of X:X:X:X:X:X) and
the full form by checking 3 * ETH_ALEN -1 (for the full format
XX:XX:XX:XX:XX:XX)

The parsing function try to be as redable as possible while saving some
code duplication and skip the use of memcpy function as the post_process
is called very little time just on driver probe.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/nvmem/u-boot-env.c | 50 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c
index b64c37308789..e33565e872f8 100644
--- a/drivers/nvmem/u-boot-env.c
+++ b/drivers/nvmem/u-boot-env.c
@@ -76,12 +76,58 @@ static int u_boot_env_read(void *context, unsigned int offset, void *val,
 static int u_boot_env_read_post_process_ethaddr(void *context, const char *id, int index,
 						unsigned int offset, void *buf, size_t bytes)
 {
+	u8 *src_mac = buf;
 	u8 mac[ETH_ALEN];
 
-	if (bytes != 3 * ETH_ALEN - 1)
+	if (bytes < ETH_ALEN + 5 || bytes > 3 * ETH_ALEN - 1)
 		return -EINVAL;
 
-	if (!mac_pton(buf, mac))
+	/* ASCII address might need to be normalized, try to parse it */
+	if (bytes != 3 * ETH_ALEN - 1) {
+		u8 tmp_mac[3 * ETH_ALEN - 1];
+		int i = 0, j = 0;
+
+		while (i < bytes) {
+			/* First check if we need to handle a reduced section
+			 * or we are handling the last byte.
+			 * Example of reduced section:
+			 * - a:XX:XX:XX:XX:XX
+			 * - XX:a:XX:XX:XX:XX
+			 * - XX:XX:XX:XX:XX:a
+			 */
+			if (src_mac[i + 1] == ':' || i + 1 == bytes) {
+				tmp_mac[j] = '0';
+				tmp_mac[j + 1] = src_mac[i];
+				if (src_mac[i + 1] == ':')
+					tmp_mac[j + 2] = src_mac[i + 1];
+				i += 2;
+			/* Handle a full section or we are handling the last 2 byte.
+			 * Example of full section:
+			 * - aa:XX:XX:XX:XX:XX
+			 * - XX:aa:XX:XX:XX:XX
+			 * - XX:XX:XX:XX:XX:aa
+			 */
+			} else if (src_mac[i + 2] == ':' || i + 2 == bytes) {
+				tmp_mac[j] = src_mac[i];
+				tmp_mac[j + 1] = src_mac[i + 1];
+				if (src_mac[i + 2] == ':')
+					tmp_mac[j + 2] = src_mac[i + 2];
+				i += 3;
+			/* Return -EINVAL if we have a not valid ascii address.
+			 * We can use the logic of missing ':' to validate a
+			 * correct ASCII address.
+			 */
+			} else {
+				return -EINVAL;
+			}
+			j += 3;
+		}
+
+		/* Parse the normalized mac instead of the nvmem cell */
+		src_mac = tmp_mac;
+	}
+
+	if (!mac_pton(src_mac, mac))
 		return -EINVAL;
 
 	if (index)
-- 
2.40.1

Re: [PATCH 1/3] dt-bindings: nvmem: u-boot,env: Add support for u-boot,env-size

[Rob Herring]

On Mon, Jul 24, 2023 at 10:26:30AM +0200, Christian Marangi wrote:
> Add support for u-boot,env-size new property.
> 
> Permit to declare a custom size of the U-Boot env that differs than the
> partition size where the U-Boot env is located.
> 
> U-Boot env is validated by calculating the CRC32 on the entire env
> and in some specific case, the env size might differ from the partition
> size resulting in wrong CRC32 calculation than the expected one saved at
> the start of the partition.

Why can't you just change the partition size? There is no size really 
because it is just defined in DT.
> 
> This happens when U-Boot is compiled by hardcoding a specific env size
> but the env is actually placed in a bigger partition, resulting in needing
> to provide a custom value.

If u-boot is compiled that way, then shouldn't it have that size 
contained within it? What happens when the DT doesn't match?

> 
> Declaring this property, this value will be used for NVMEM size instead of
> the mtd partition.
> 
> Add also an example to make it clear the scenario of mismatched
> partition size and actual U-Boot env.

If we do have this, then perhaps there is a generic need for a data 
size property.

> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../devicetree/bindings/nvmem/u-boot,env.yaml | 47 +++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
> index 36d97fb87865..3970725a2c57 100644
> --- a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
> +++ b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
> @@ -44,6 +44,24 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  u-boot,env-size:
> +    description: |
> +      Permit to declare a custom size of the U-Boot env that differs than the
> +      partition size where the U-Boot env is located.
> +
> +      U-Boot env is validated by calculating the CRC32 on the entire env
> +      and in some specific case, the env size might differ from the partition
> +      size resulting in wrong CRC32 calculation than the expected one saved at
> +      the start of the partition.
> +
> +      This happens when U-Boot is compiled by hardcoding a specific env size
> +      but the env is actually placed in a bigger partition, resulting in needing
> +      to provide a custom value.
> +
> +      Declaring this property, this value will be used for NVMEM size instead of
> +      the mtd partition.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
>    bootcmd:
>      type: object
>      description: Command to use for automatic booting
> @@ -99,3 +117,32 @@ examples:
>              };
>          };
>      };
> +  - |
> +    partitions {
> +        compatible = "fixed-partitions";
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        partition@0 {
> +            reg = <0x0 0xc80000>;
> +            label = "qcadata";
> +            read-only;
> +        };
> +
> +        partition@c80000 {
> +            label = "APPSBL";
> +            reg = <0xc80000 0x500000>;
> +            read-only;
> +        };
> +
> +        partition@1180000 {
> +            compatible = "u-boot,env";
> +            reg = <0x1180000 0x80000>;
> +
> +            u-boot,env-size = <0x40000>;
> +
> +            mac1: ethaddr {
> +                #nvmem-cell-cells = <1>;
> +            };
> +        };
> +    };
> -- 
> 2.40.1
> 

Re: [PATCH 1/3] dt-bindings: nvmem: u-boot,env: Add support for u-boot,env-size

[Christian Marangi]

On Wed, Jul 26, 2023 at 10:36:00AM -0600, Rob Herring wrote:
> On Mon, Jul 24, 2023 at 10:26:30AM +0200, Christian Marangi wrote:
> > Add support for u-boot,env-size new property.
> > 
> > Permit to declare a custom size of the U-Boot env that differs than the
> > partition size where the U-Boot env is located.
> > 
> > U-Boot env is validated by calculating the CRC32 on the entire env
> > and in some specific case, the env size might differ from the partition
> > size resulting in wrong CRC32 calculation than the expected one saved at
> > the start of the partition.
> 
> Why can't you just change the partition size? There is no size really 
> because it is just defined in DT.

Hi,
partition may also come by parser at runtime and are not only provided
by DT. One example is cmdlinepart where partition comes from cmd line or
smem-part from qcom smem.

We support this case by declaring these partition in DT and NVMEM
connects the dynamic partition to OF in DT (to reference and use the
NVMEM cell in other nodes)

> > 
> > This happens when U-Boot is compiled by hardcoding a specific env size
> > but the env is actually placed in a bigger partition, resulting in needing
> > to provide a custom value.
> 
> If u-boot is compiled that way, then shouldn't it have that size 
> contained within it? What happens when the DT doesn't match?
> 

When DT doesn't match the expected compiled size, the CRC32 validation
fail.
U-Boot store the env size crc32 in the first few bytes of the uboot env
partition. And we walidate that the saved crc32 actually match the
calculated one to make sure we have a not corrupted env size. If crc32
validation fail the uboot env nvmem module fails to probe.

> > 
> > Declaring this property, this value will be used for NVMEM size instead of
> > the mtd partition.
> > 
> > Add also an example to make it clear the scenario of mismatched
> > partition size and actual U-Boot env.
> 
> If we do have this, then perhaps there is a generic need for a data 
> size property.
> 

Can you give some example of this? I'm more than happy to follow
standard property instead of use custom ones.

> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  .../devicetree/bindings/nvmem/u-boot,env.yaml | 47 +++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
> > index 36d97fb87865..3970725a2c57 100644
> > --- a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
> > +++ b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
> > @@ -44,6 +44,24 @@ properties:
> >    reg:
> >      maxItems: 1
> >  
> > +  u-boot,env-size:
> > +    description: |
> > +      Permit to declare a custom size of the U-Boot env that differs than the
> > +      partition size where the U-Boot env is located.
> > +
> > +      U-Boot env is validated by calculating the CRC32 on the entire env
> > +      and in some specific case, the env size might differ from the partition
> > +      size resulting in wrong CRC32 calculation than the expected one saved at
> > +      the start of the partition.
> > +
> > +      This happens when U-Boot is compiled by hardcoding a specific env size
> > +      but the env is actually placed in a bigger partition, resulting in needing
> > +      to provide a custom value.
> > +
> > +      Declaring this property, this value will be used for NVMEM size instead of
> > +      the mtd partition.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> >    bootcmd:
> >      type: object
> >      description: Command to use for automatic booting
> > @@ -99,3 +117,32 @@ examples:
> >              };
> >          };
> >      };
> > +  - |
> > +    partitions {
> > +        compatible = "fixed-partitions";
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +
> > +        partition@0 {
> > +            reg = <0x0 0xc80000>;
> > +            label = "qcadata";
> > +            read-only;
> > +        };
> > +
> > +        partition@c80000 {
> > +            label = "APPSBL";
> > +            reg = <0xc80000 0x500000>;
> > +            read-only;
> > +        };
> > +
> > +        partition@1180000 {
> > +            compatible = "u-boot,env";
> > +            reg = <0x1180000 0x80000>;
> > +
> > +            u-boot,env-size = <0x40000>;
> > +
> > +            mac1: ethaddr {
> > +                #nvmem-cell-cells = <1>;
> > +            };
> > +        };
> > +    };
> > -- 
> > 2.40.1
> > 

-- 
	Ansuel