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

Michael Walle
2022-05-12T21:39:02Z

Am 2022-05-12 23:33, schrieb Michael Walle:
> Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
>> 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;
>>
>> 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. */
>
> Who is modifying this?

Ahh scrap that, this very function. I need to go to sleep.

I just saw your next patch which will also get rid of the
addr_nbytes. I'd still like to get rid of this state sometime
and pass it as an additional parameter. But:

Reviewed-by: Michael Walle <michael@walle.cc>