Re: [PATCH v15 3/8] mtd: spi-nor: sfdp: Clarify that nor->read_{opcode, dummy} are uninitialized

Pratyush Yadav
2022-05-31T11:18:56Z

On 10/05/22 07:10AM, tkuw584924@gmail.com wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>
> nor->read_{opcode, dummy} are uninitialized (value zero) at SFDP parsing
> time. Clarify that in the code.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> drivers/mtd/spi-nor/sfdp.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 61ae8c8c5237..058ce218d2af 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -178,12 +178,10 @@ static int spi_nor_read_raw(struct spi_nor *nor, u32 addr, size_t len, u8 *buf)
> static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
> size_t len, void *buf)
> {
> - u8 addr_nbytes, read_opcode, read_dummy;
> + u8 addr_nbytes;
> int ret;
>
> - read_opcode = nor->read_opcode;
> addr_nbytes = nor->addr_nbytes;
> - read_dummy = nor->read_dummy;
>
> nor->read_opcode = SPINOR_OP_RDSFDP;
> nor->addr_nbytes = 3;
> @@ -191,9 +189,10 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
>
> ret = spi_nor_read_raw(nor, addr, len, buf);
>
> - nor->read_opcode = read_opcode;
> nor->addr_nbytes = addr_nbytes;
> - nor->read_dummy = read_dummy;
> + /* Restore setup to its uninitialized state. */
> + nor->read_opcode = 0;
> + nor->read_dummy = 0;

Hmm, I don't like this too much. Why do you need to explicitly set the
default value here? What's wrong with this function just not caring
about what is initialized by this point and what is not? It is really
not its business caring about default values of these parameters. Modify
them, run the command, and restore them, regardless of default or
initialized values.

>
> return ret;
> }
> @@ -690,7 +689,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> u32 addr;
> int err;
> u8 i;
> - u8 addr_nbytes, read_opcode, read_dummy;
> + u8 addr_nbytes;
> u8 read_data_mask, map_id;
>
> /* Use a kmalloc'ed bounce buffer to guarantee it is DMA-able. */
> @@ -699,8 +698,6 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> return ERR_PTR(-ENOMEM);
>
> addr_nbytes = nor->addr_nbytes;
> - read_dummy = nor->read_dummy;
> - read_opcode = nor->read_opcode;
>
> map_id = 0;
> /* Determine if there are any optional Detection Command Descriptors */
> @@ -757,8 +754,9 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> out:
> kfree(buf);
> nor->addr_nbytes = addr_nbytes;
> - nor->read_dummy = read_dummy;
> - nor->read_opcode = read_opcode;
> + /* Restore setup to its uninitialized state. */
> + nor->read_dummy = 0;
> + nor->read_opcode = 0;
> return ret;
> }
>
> --
> 2.25.1
>

--
Regards,
Pratyush Yadav
Texas Instruments Inc.