[PATCH 2/7] mtd: spi-nor: core: Report correct name in case of ID collisions

[Tudor.Ambarus at microchip.com]

On 7/5/21 7:09 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 05/07/21 01:24PM, Tudor.Ambarus at microchip.com wrote:
>> On 7/5/21 4:13 PM, Pratyush Yadav wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 02/07/21 05:41PM, Tudor Ambarus wrote:
>>>> Provide a way to report the correct flash name in case of ID collisions.
>>>> There will be a single flash_info entry when flash IDs collide, and the
>>>> differentiation between the flash types will be made at runtime
>>>> if possible.
>>>
>>> I am not sure if having one entry for all flashes with a collision is a
>>> good idea. For example, say we have one flash which supports SFDP and
>>> that's all we need for it. Another flash with the same ID does not
>>> support SFDP so it needs the SPI_NOR_DUAL_READ, etc. flags. How would
>>> you handle this case with the same entry? You would have to set all the
>>> flags in the disambiguation function. And nor->info is declared as const
>>> so you can't change the flags in there. Any code checking for
>>> info->flags would not work properly for these type of flashes. Wouldn't
>>> it be better to have multiple entries with the same ID and just pick the
>>> one we need in the disambiguation function?
>>
>> Your scenario is hit in patches 3/7 and 4/7. In case of ID collisions between
>> a SFDP and a non-SFDP compliant flash, I propose to set SPI_NOR_PARSE_SFDP, as
>> well as all the other required flash info flags that statically init the flash.
>> The SFDP flash will init all its NOR flags at runtime, while the non-SFDP flash
>> will issue a RDSFDP command that will fail, and then it will init all the NOR
>> flags based on the flash info flags.
> 
> spi_nor_info_init_params() is called before spi_nor_sfdp_init_params().
> So if flash A sets SPI_NOR_QUAD_READ, flash B will also inherit it even
> though it might not actually support quad mode reads. You would need to
> refactor the parameter initialization path to do SFDP first and skip the
> flags based init.
> 
> But that doesn't solve all the problems. What about the flags that can't
> be discovered by SFDP? I see SPI_NOR_NO_ERASE, SPI_NOR_BP3_SR_BIT6, etc.
> that are not set by the SFDP path. Some of those might be discoverable,
> and we just don't do it yet, but I am not convinced all of them can be.
> For example, I don't see any way to discover SPI_NOR_NO_ERASE via SFDP.
> Erase type 1 field in BFPT is not optional and has to be specified.
> 
> How will you differentiate from the SFDP-discoverable and
> non-SFDP-discoverable flags? How can you which belongs to which flash? I
> know this is a bit far fetched but let's solve this problem once and for
> all.
> 

Yeah, that's my goal, to solve all these once and for all. I've started working on it,
and refactored the params init sequence. I have a WIP branch at:
https://github.com/ambarus/linux-0day/commits/spi-nor/next-id-collisions.
I'll post everything after I finish, we can ignore all this for the moment.

Thanks Pratyush for your insights!