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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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