2018-05-05 20:26:34

by Mathieu Malaterre

Subject: [PATCH] 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(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index b05aa8e81303..f34f2363925a 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -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

2018-05-06 13:59:51

by Andy Shevchenko

Subject: Re: [PATCH] nvmem: properly handle returned value nvmem_reg_read

On Sat, May 5, 2018 at 11:24 PM, Mathieu Malaterre <[email protected]> wrote:
> 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.

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

>
> Signed-off-by: Mathieu Malaterre <[email protected]>
> ---
> drivers/nvmem/core.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index b05aa8e81303..f34f2363925a 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -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
>



--
With Best Regards,
Andy Shevchenko

2018-05-08 09:39:22

by Srinivas Kandagatla

Subject: Re: [PATCH] nvmem: properly handle returned value nvmem_reg_read

Thanks for the patch,

On 05/05/18 21:24, Mathieu Malaterre wrote:
> 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(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index b05aa8e81303..f34f2363925a 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -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;
You should return an error pointer here, not NULL.

thanks,
srini


> }
>
> /**
>

2018-05-09 18:59:29

by Mathieu Malaterre

Subject: [PATCH v2] 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]>
---
v2: prefer ERR_PTR(-EINVAL) over a simple return NULL

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

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index b05aa8e81303..f7b6c85cf393 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -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);
}

/**
--
2.11.0

2018-05-10 10:00:12

by Srinivas Kandagatla

Subject: Re: [PATCH v2] nvmem: properly handle returned value nvmem_reg_read

On 09/05/18 19:57, Mathieu Malaterre wrote:
> 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(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index b05aa8e81303..f7b6c85cf393 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -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);
You should return ERR_PTR(rc) not EINVAL here.
errors should always propagate to caller!

thanks,
srini
> }
>
> /**
>

2018-05-10 18:40:48

by Mathieu Malaterre

Subject: [PATCH v3] nvmem: properly handle returned value from nvmem_reg_read

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(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index b05aa8e81303..1e28597138c8 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -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);
}

/**
--
2.11.0

2018-05-10 18:43:14

by Mathieu Malaterre

Subject: Re: [PATCH v2] nvmem: properly handle returned value nvmem_reg_read

On Thu, May 10, 2018 at 11:58 AM, Srinivas Kandagatla
<[email protected]> wrote:
>
>
> On 09/05/18 19:57, Mathieu Malaterre wrote:
>>
>> 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(+)
>>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index b05aa8e81303..f7b6c85cf393 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -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);
>
> 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.

> thanks,
> srini
>>
>> }
>> /**
>>
>