* Re: [PATCH] nvmem: properly handle returned value nvmem_reg_read
2018-05-05 20:24 [PATCH] nvmem: properly handle returned value nvmem_reg_read Mathieu Malaterre
@ 2018-05-06 13:58 ` Andy Shevchenko
2018-05-08 9:30 ` Srinivas Kandagatla
2018-05-09 18:57 ` [PATCH v2] " Mathieu Malaterre
2 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2018-05-06 13:58 UTC (permalink / raw)
To: Mathieu Malaterre; +Cc: Srinivas Kandagatla, Linux Kernel Mailing List
On Sat, May 5, 2018 at 11:24 PM, Mathieu Malaterre <malat@debian.org> 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 <malat@debian.org>
> ---
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] nvmem: properly handle returned value nvmem_reg_read
2018-05-05 20:24 [PATCH] nvmem: properly handle returned value nvmem_reg_read Mathieu Malaterre
2018-05-06 13:58 ` Andy Shevchenko
@ 2018-05-08 9:30 ` Srinivas Kandagatla
2018-05-09 18:57 ` [PATCH v2] " Mathieu Malaterre
2 siblings, 0 replies; 7+ messages in thread
From: Srinivas Kandagatla @ 2018-05-08 9:30 UTC (permalink / raw)
To: Mathieu Malaterre; +Cc: linux-kernel
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 <malat@debian.org>
> ---
> 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
> }
>
> /**
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] nvmem: properly handle returned value nvmem_reg_read
2018-05-05 20:24 [PATCH] nvmem: properly handle returned value nvmem_reg_read Mathieu Malaterre
2018-05-06 13:58 ` Andy Shevchenko
2018-05-08 9:30 ` Srinivas Kandagatla
@ 2018-05-09 18:57 ` Mathieu Malaterre
2018-05-10 9:58 ` Srinivas Kandagatla
2018-05-10 18:39 ` [PATCH v3] nvmem: properly handle returned value from nvmem_reg_read Mathieu Malaterre
2 siblings, 2 replies; 7+ messages in thread
From: Mathieu Malaterre @ 2018-05-09 18:57 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: Mathieu Malaterre, linux-kernel
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 <malat@debian.org>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] nvmem: properly handle returned value nvmem_reg_read
2018-05-09 18:57 ` [PATCH v2] " Mathieu Malaterre
@ 2018-05-10 9:58 ` Srinivas Kandagatla
2018-05-10 18:40 ` Mathieu Malaterre
2018-05-10 18:39 ` [PATCH v3] nvmem: properly handle returned value from nvmem_reg_read Mathieu Malaterre
1 sibling, 1 reply; 7+ messages in thread
From: Srinivas Kandagatla @ 2018-05-10 9:58 UTC (permalink / raw)
To: Mathieu Malaterre; +Cc: linux-kernel
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 <malat@debian.org>
> ---
> 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
> }
>
> /**
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] nvmem: properly handle returned value nvmem_reg_read
2018-05-10 9:58 ` Srinivas Kandagatla
@ 2018-05-10 18:40 ` Mathieu Malaterre
0 siblings, 0 replies; 7+ messages in thread
From: Mathieu Malaterre @ 2018-05-10 18:40 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: LKML
On Thu, May 10, 2018 at 11:58 AM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> 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 <malat@debian.org>
>> ---
>> 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
>>
>> }
>> /**
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3] nvmem: properly handle returned value from nvmem_reg_read
2018-05-09 18:57 ` [PATCH v2] " Mathieu Malaterre
2018-05-10 9:58 ` Srinivas Kandagatla
@ 2018-05-10 18:39 ` Mathieu Malaterre
1 sibling, 0 replies; 7+ messages in thread
From: Mathieu Malaterre @ 2018-05-10 18:39 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: Mathieu Malaterre, linux-kernel
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 <malat@debian.org>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread