nvmem: properly handle returned value nvmem_reg_read

PATCH Superseded Mathieu Malaterre 3 participants 7 messages May 5, 2018 – May 10, 2018 NVMEM framework

Share & tools

Mathieu Malaterre

author

2018-05-05 20:24 UTC #0 lore raw

PATCH 1/1 nvmem: properly handle returned value nvmem_reg_read

Function nvmem_reg_read can return a non zero value indicating an error. This returned value must be read and error propagated to nvmem_cell_prepare_write_buffer. Silence the following gcc warning (W=1):

drivers/nvmem/core.c:1093:9: warning: variable ‘rc’ set but not used [-Wunused-but-set-variable]

Signed-off-by Mathieu Malaterre <[email protected]>

---
 drivers/nvmem/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/nvmem/core.c +7 -0

index b05aa8e81303..f34f2363925a 100644
@@ -1107,6 +1107,8 @@ static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
 
 		/* setup the first byte with lsb bits from nvmem */
 		rc = nvmem_reg_read(nvmem, cell->offset, &v, 1);
+		if (rc)
+			goto err;
 		*b++ |= GENMASK(bit_offset - 1, 0) & v;
 
 		/* setup rest of the byte if any */

@@ -1125,11 +1127,16 @@ static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
 		/* setup the last byte with msb bits from nvmem */
 		rc = nvmem_reg_read(nvmem,
 				    cell->offset + cell->bytes - 1, &v, 1);
+		if (rc)
+			goto err;
 		*p |= GENMASK(7, (nbits + bit_offset) % BITS_PER_BYTE) & v;
 
 	}
 
 	return buf;
+err:
+	kfree(buf);
+	return NULL;
 }
 
 /**

Signature · 1 line

2.11.0

Andy Shevchenko

2018-05-06 13:58 UTC #1 lore raw

Quoted 3 lines · Mathieu Malaterre

Function nvmem_reg_read can return a non zero value indicating an error. This returned value must be read and error propagated to nvmem_cell_prepare_write_buffer.

Must is to strong word here. How come it *must*?
Did you investigate the history of the changes that brought us to current code?

How had you tested your change?
While it looks subtle it is in one of the storage class devices core, which might be very sensible to changing workflow.

Quoted 3 lines

Silence the following gcc warning (W=1):
drivers/nvmem/core.c:1093:9: warning: variable ‘rc’ set but not used [-Wunused-but-set-variable]

This is least argument to accept this patch.

Quoted 39 lines

Signed-off-by Mathieu Malaterre <[email protected]>

---
 drivers/nvmem/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/nvmem/core.c +7 -1

index b05aa8e81303..f34f2363925a 100644
@@ -1107,6 +1107,8 @@ static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
 
                /* setup the first byte with lsb bits from nvmem */
                rc = nvmem_reg_read(nvmem, cell->offset, &v, 1);
+               if (rc)
+                       goto err;
                *b++ |= GENMASK(bit_offset - 1, 0) & v;
 
                /* setup rest of the byte if any */

@@ -1125,11 +1127,16 @@ static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
                /* setup the last byte with msb bits from nvmem */
                rc = nvmem_reg_read(nvmem,
                                    cell->offset + cell->bytes - 1, &v, 1);
+               if (rc)
+                       goto err;
                *p |= GENMASK(7, (nbits + bit_offset) % BITS_PER_BYTE) & v;
 
        }
 
        return buf;
+err:
+       kfree(buf);
+       return NULL;
 }
 
 /**
--

2.11.0

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Srinivas Kandagatla

2018-05-08 09:30 UTC #2 lore raw

In reply to Mathieu Malaterre

Thanks for the patch,

Quoted 28 lines · Mathieu Malaterre

Function nvmem_reg_read can return a non zero value indicating an error. This returned value must be read and error propagated to nvmem_cell_prepare_write_buffer. Silence the following gcc warning (W=1):
drivers/nvmem/core.c:1093:9: warning: variable ‘rc’ set but not used [-Wunused-but-set-variable]
Signed-off-by Mathieu Malaterre <[email protected]>
---
  drivers/nvmem/core.c | 7 +++++++
  1 file changed, 7 insertions(+)
drivers/nvmem/core.c +2 -0
index b05aa8e81303..f34f2363925a 100644
@@ -1107,6 +1107,8 @@ static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
 
  		/* setup the first byte with lsb bits from nvmem */
  		rc = nvmem_reg_read(nvmem, cell->offset, &v, 1);
+		if (rc)
+			goto err;
  		*b++ |= GENMASK(bit_offset - 1, 0) & v;
 
  		/* setup rest of the byte if any */
@@ -1125,11 +1127,16 @@ static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
  		/* setup the last byte with msb bits from nvmem */
  		rc = nvmem_reg_read(nvmem,
  				    cell->offset + cell->bytes - 1, &v, 1);

drivers/nvmem/core.c static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell, · line 1139

Quoted 10 lines

drivers/nvmem/core.c +5 -0

+		if (rc)
+			goto err;
  		*p |= GENMASK(7, (nbits + bit_offset) % BITS_PER_BYTE) & v;
 
  	}
 
  	return buf;
+err:
+	kfree(buf);
+	return NULL;

You should return an error pointer here, not NULL.

thanks,
srini

Quoted 4 lines

drivers/nvmem/core.c +0 -0
  }
 
  /**

Mathieu Malaterre

author

2018-05-09 18:57 UTC #3 lore raw

PATCH v2 1/1 nvmem: properly handle returned value nvmem_reg_read

In reply to Mathieu Malaterre

drivers/nvmem/core.c:1093:9: warning: variable ‘rc’ set but not used [-Wunused-but-set-variable]

Signed-off-by Mathieu Malaterre <[email protected]>

---
v2: prefer ERR_PTR(-EINVAL) over a simple return NULL

 drivers/nvmem/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/nvmem/core.c +7 -0

index b05aa8e81303..f7b6c85cf393 100644
@@ -1107,6 +1107,8 @@ static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
 
 		/* setup the first byte with lsb bits from nvmem */
 		rc = nvmem_reg_read(nvmem, cell->offset, &v, 1);
+		if (rc)
+			goto err;
 		*b++ |= GENMASK(bit_offset - 1, 0) & v;
 
 		/* setup rest of the byte if any */

@@ -1125,11 +1127,16 @@ static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
 		/* setup the last byte with msb bits from nvmem */
 		rc = nvmem_reg_read(nvmem,
 				    cell->offset + cell->bytes - 1, &v, 1);
+		if (rc)
+			goto err;
 		*p |= GENMASK(7, (nbits + bit_offset) % BITS_PER_BYTE) & v;
 
 	}
 
 	return buf;
+err:
+	kfree(buf);
+	return ERR_PTR(-EINVAL);
 }
 
 /**

Signature · 1 line

2.11.0

Srinivas Kandagatla

2018-05-10 09:58 UTC #4 lore raw

Quoted 30 lines · Mathieu Malaterre

Function nvmem_reg_read can return a non zero value indicating an error. This returned value must be read and error propagated to nvmem_cell_prepare_write_buffer. Silence the following gcc warning (W=1):
drivers/nvmem/core.c:1093:9: warning: variable ‘rc’ set but not used [-Wunused-but-set-variable]
Signed-off-by Mathieu Malaterre <[email protected]>
---
v2: prefer ERR_PTR(-EINVAL) over a simple return NULL
  drivers/nvmem/core.c | 7 +++++++
  1 file changed, 7 insertions(+)
drivers/nvmem/core.c +2 -0
index b05aa8e81303..f7b6c85cf393 100644
@@ -1107,6 +1107,8 @@ static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
 
  		/* setup the first byte with lsb bits from nvmem */
  		rc = nvmem_reg_read(nvmem, cell->offset, &v, 1);
+		if (rc)
+			goto err;
  		*b++ |= GENMASK(bit_offset - 1, 0) & v;
 
  		/* setup rest of the byte if any */
@@ -1125,11 +1127,16 @@ static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
  		/* setup the last byte with msb bits from nvmem */
  		rc = nvmem_reg_read(nvmem,
  				    cell->offset + cell->bytes - 1, &v, 1);

drivers/nvmem/core.c static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell, · line 1139

Quoted 10 lines

drivers/nvmem/core.c +5 -0

+		if (rc)
+			goto err;
  		*p |= GENMASK(7, (nbits + bit_offset) % BITS_PER_BYTE) & v;
 
  	}
 
  	return buf;
+err:
+	kfree(buf);
+	return ERR_PTR(-EINVAL);

You should return ERR_PTR(rc) not EINVAL here.
errors should always propagate to caller!

thanks,
srini

Quoted 4 lines

drivers/nvmem/core.c +0 -0
  }
 
  /**

Mathieu Malaterre

author

2018-05-10 18:41 UTC #5 lore raw

Quoted 40 lines · Srinivas Kandagatla

Quoted 37 lines · Mathieu Malaterre

drivers/nvmem/core.c:1093:9: warning: variable ‘rc’ set but not used
[-Wunused-but-set-variable]

Signed-off-by Mathieu Malaterre <[email protected]>

---
v2: prefer ERR_PTR(-EINVAL) over a simple return NULL

  drivers/nvmem/core.c | 7 +++++++
  1 file changed, 7 insertions(+)

drivers/nvmem/core.c +4 -0

index b05aa8e81303..f7b6c85cf393 100644
@@ -1107,6 +1107,8 @@ static void *nvmem_cell_prepare_write_buffer(struct
 nvmem_cell *cell,
                /* setup the first byte with lsb bits from nvmem */
                rc = nvmem_reg_read(nvmem, cell->offset, &v, 1);
+               if (rc)
+                       goto err;
                *b++ |= GENMASK(bit_offset - 1, 0) & v;
                /* setup rest of the byte if any */

@@ -1125,11 +1127,16 @@ static void
 *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
                /* setup the last byte with msb bits from nvmem */
                rc = nvmem_reg_read(nvmem,
                                    cell->offset + cell->bytes - 1, &v,
 1);
+               if (rc)
+                       goto err;
                *p |= GENMASK(7, (nbits + bit_offset) % BITS_PER_BYTE) &
 v;

Quoted 8 lines

drivers/nvmem/core.c static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell, · line 1139
Quoted 5 lines
drivers/nvmem/core.c +3 -0
        }
        return buf;
+err:
+       kfree(buf);
+       return ERR_PTR(-EINVAL);
You should return ERR_PTR(rc) not EINVAL here.
errors should always propagate to caller!

third time's a charm ?

Sorry about this, I was not paying attention.

Quoted 7 lines

thanks,
srini
Quoted 4 lines
drivers/nvmem/core.c +0 -0
  }
    /**

Mathieu Malaterre

author

2018-05-10 18:39 UTC #6 lore raw

PATCH v3 1/1 nvmem: properly handle returned value from nvmem_reg_read

In reply to Mathieu Malaterre

Function ‘nvmem_reg_read’ can return a non zero value indicating an error. This returned value should be read and error propagated to ‘nvmem_cell_prepare_write_buffer’. Silence a gcc warning (using W=1):

drivers/nvmem/core.c:1093:9: warning: variable ‘rc’ set but not used [-Wunused-but-set-variable]

Signed-off-by Mathieu Malaterre <[email protected]>

---
v3: return exact error code instead of EINVAL, tweak commit message a bit
v2: prefer ERR_PTR(-EINVAL) over a simple return NULL

 drivers/nvmem/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/nvmem/core.c +7 -0

index b05aa8e81303..1e28597138c8 100644
@@ -1107,6 +1107,8 @@ static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
 
 		/* setup the first byte with lsb bits from nvmem */
 		rc = nvmem_reg_read(nvmem, cell->offset, &v, 1);
+		if (rc)
+			goto err;
 		*b++ |= GENMASK(bit_offset - 1, 0) & v;
 
 		/* setup rest of the byte if any */

@@ -1125,11 +1127,16 @@ static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
 		/* setup the last byte with msb bits from nvmem */
 		rc = nvmem_reg_read(nvmem,
 				    cell->offset + cell->bytes - 1, &v, 1);
+		if (rc)
+			goto err;
 		*p |= GENMASK(7, (nbits + bit_offset) % BITS_PER_BYTE) & v;
 
 	}
 
 	return buf;
+err:
+	kfree(buf);
+	return ERR_PTR(rc);
 }
 
 /**

Signature · 1 line

2.11.0