Re: [PATCH v2 17/35] mtd: spi-nor: Introduce spi_nor_nonsfdp_flags_init()

[<Tudor.Ambarus@microchip.com>]

On 8/17/21 1:24 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 27/07/21 07:52AM, Tudor Ambarus wrote:
>> Used to initialize the NOR flags for settings that are not defined
>> in the JESD216 SFDP standard, thus can not be retrieved when parsing
>> SFDP. No functional change.
> 
> I am worried if the order in which these flags are set can cause some
> subtle bugs.
> 
> I can see one instance of it with SNOR_F_HAS_LOCK.
> spi_nor_late_init_params() checks for SNOR_F_HAS_LOCK and if there are
> no locking ops specified, it sets the default locking ops. This works
> fine before this patch because the flag is set before the function is
> called. But now, the flag will be set _after_ the function is called,
> and so you will never be able to set the default flags.
> 
> This is one bug I can spot but I fear some others might be hiding
> somewhere as well. SPI NOR has accumulated a lot of spaghetti code over
> the years and I certainly felt it when working on my Octal DTR series.
> It caused an address width selection bug that was not obvious at all,
> and was not even caught during the rc cycles.
> 
> I think this series does clean up that spaghetti a lot. But you need to
> be careful of such bugs. I think you should definitely let this series
> cook in next for some time so it gets some exposure and hopefully some
> testing.
> 
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  drivers/mtd/spi-nor/core.c | 88 ++++++++++++++++++++++----------------
>>  1 file changed, 52 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 1f38fa8ab2fa..6a8617346764 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2687,6 +2687,56 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
>>               spi_nor_init_default_locking_ops(nor);
>>  }
>>
>> +/**
>> + * spi_nor_nonsfdp_flags_init() - Initialize NOR flags for settings that are not
>> + * defined in the JESD216 SFDP standard, thus can not be retrieved when parsing
>> + * SFDP.
>> + * @nor:     pointer to a 'struct spi_nor'
>> + */
>> +static void spi_nor_nonsfdp_flags_init(struct spi_nor *nor)
>> +{
>> +     const struct flash_info *info = nor->info;
>> +     struct device_node *np = spi_nor_get_flash_node(nor);
>> +
>> +     if (of_property_read_bool(np, "broken-flash-reset"))
>> +             nor->flags |= SNOR_F_BROKEN_RESET;
>> +
>> +     if (info->flags & SPI_NOR_SWP_IS_VOLATILE)
>> +             nor->flags |= SNOR_F_SWP_IS_VOLATILE;
>> +
>> +     if (info->flags & SPI_NOR_HAS_LOCK)
>> +             nor->flags |= SNOR_F_HAS_LOCK;
> 
> As mentioned above, this would cause a bug.

Good catch!

> 
>> +
>> +     if (info->flags & SPI_NOR_HAS_TB) {
>> +             nor->flags |= SNOR_F_HAS_SR_TB;
>> +             if (info->flags & SPI_NOR_TB_SR_BIT6)
>> +                     nor->flags |= SNOR_F_HAS_SR_TB_BIT6;
>> +     }
>> +
>> +     if (info->flags & SPI_NOR_4BIT_BP) {
>> +             nor->flags |= SNOR_F_HAS_4BIT_BP;
>> +             if (info->flags & SPI_NOR_BP3_SR_BIT6)
>> +                     nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
>> +     }
>> +
>> +     if (info->flags & NO_CHIP_ERASE)
>> +             nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
>> +
>> +     if (info->flags & USE_FSR)
>> +             nor->flags |= SNOR_F_USE_FSR;
>> +
>> +     if (info->flags & USE_CLSR)
>> +             nor->flags |= SNOR_F_USE_CLSR;
>> +
>> +     /*
>> +      * Make sure the XSR_RDY flag is set before calling
>> +      * spi_nor_wait_till_ready(). Xilinx S3AN share MFR
>> +      * with Atmel SPI NOR.
>> +      */
>> +     if (info->flags & SPI_NOR_XSR_RDY)
>> +             nor->flags |=  SNOR_F_READY_XSR_RDY;
>> +}
>> +
>>  /**
>>   * spi_nor_init_params() - Initialize the flash's parameters and settings.
>>   * @nor:     pointer to a 'struct spi_nor'.
>> @@ -2736,6 +2786,8 @@ static int spi_nor_init_params(struct spi_nor *nor)
>>
>>       spi_nor_late_init_params(nor);
>>
>> +     spi_nor_nonsfdp_flags_init(nor);
>> +
>>       return 0;
>>  }
>>
>> @@ -3078,7 +3130,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>       const struct flash_info *info;
>>       struct device *dev = nor->dev;
>>       struct mtd_info *mtd = &nor->mtd;
>> -     struct device_node *np = spi_nor_get_flash_node(nor);
>>       int ret;
>>       int i;
>>
>> @@ -3115,17 +3166,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>
>>       mutex_init(&nor->lock);
>>
>> -     /*
>> -      * Make sure the XSR_RDY flag is set before calling
>> -      * spi_nor_wait_till_ready(). Xilinx S3AN share MFR
>> -      * with Atmel SPI NOR.
>> -      */
>> -     if (info->flags & SPI_NOR_XSR_RDY)
>> -             nor->flags |=  SNOR_F_READY_XSR_RDY;
>> -
>> -     if (info->flags & SPI_NOR_HAS_LOCK)
>> -             nor->flags |= SNOR_F_HAS_LOCK;
>> -
>>       mtd->_write = spi_nor_write;
>>
>>       /* Init flash parameters based on flash_info struct and SFDP */
>> @@ -3147,27 +3187,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>       mtd->_get_device = spi_nor_get_device;
>>       mtd->_put_device = spi_nor_put_device;
>>
>> -     if (info->flags & USE_FSR)
>> -             nor->flags |= SNOR_F_USE_FSR;
>> -     if (info->flags & SPI_NOR_HAS_TB) {
>> -             nor->flags |= SNOR_F_HAS_SR_TB;
>> -             if (info->flags & SPI_NOR_TB_SR_BIT6)
>> -                     nor->flags |= SNOR_F_HAS_SR_TB_BIT6;
>> -     }
>> -
>> -     if (info->flags & NO_CHIP_ERASE)
>> -             nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
>> -     if (info->flags & USE_CLSR)
>> -             nor->flags |= SNOR_F_USE_CLSR;
>> -     if (info->flags & SPI_NOR_SWP_IS_VOLATILE)
>> -             nor->flags |= SNOR_F_SWP_IS_VOLATILE;
>> -
>> -     if (info->flags & SPI_NOR_4BIT_BP) {
>> -             nor->flags |= SNOR_F_HAS_4BIT_BP;
>> -             if (info->flags & SPI_NOR_BP3_SR_BIT6)
>> -                     nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
>> -     }
>> -
>>       if (info->flags & SPI_NOR_NO_ERASE)
>>               mtd->flags |= MTD_NO_ERASE;
>>
>> @@ -3175,9 +3194,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>       nor->page_size = nor->params->page_size;
>>       mtd->writebufsize = nor->page_size;
>>
>> -     if (of_property_read_bool(np, "broken-flash-reset"))
>> -             nor->flags |= SNOR_F_BROKEN_RESET;
>> -
> 
> As I pointed out above, I think this patch is certainly going in the
> right direction. We just need to be careful of the bugs that slip
> through.

Right, I'll self review all once I'll prepare v3. And I'll redo the testing,
this time trying to cover all the flash info flags. Would be great if Michael,
Vignesh and others can have a look on the series too. With 2 or 3 persons reviewing
and better test coverage, we should be fine. Would be great if we can have
these cleaning patches without introducing regressions, but if we introduce it's not
a tragedy, we will fix all once reported.

Cheers,
ta

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/