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

[<Tudor.Ambarus@microchip.com>]

On 10/24/21 8:05 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-10-22 15:25, schrieb Tudor.Ambarus@microchip.com:
>> On 10/22/21 3:59 PM, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2021-10-22 14:42, schrieb Tudor.Ambarus@microchip.com:
>>>> On 10/22/21 3:10 PM, Pratyush Yadav wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>> know
>>>>> the content is safe
>>>>>
>>>>> On 22/10/21 01:21PM, Michael Walle wrote:
>>>>>> Am 2021-08-17 12:24, schrieb Pratyush Yadav:
>>>>>>> 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.
>>>>>>
>>>>>> Maybe we should just forbid to look at the SNOR_F_ flags in these
>>>>>> functions. Instead the information could also be deduced by looking
>>>>>> at
>>>>>> the SPI_NOR_ flags.
>>>>
>>>> not true.
>>>
>>> I guess you mean that some init flash init functions might set it,
>>> too.
>>> Right? IMHO this goes into the spaghetti code direction again.
>>> spi_nor_late_init_params() must not look at the SNOR_F_ flags. There
>>> are too much inter-function dependencies and it is really hard to
>>> follow the call chain.
>>
>> I don't think I understand what you are referring to. The flash_info
>> flags
>> are used just to set their SNOR_F correspondents. Take
>> SNOR_F_4B_OPCODES for
>> example. A flash that can't discover 4b opcodes support when parsing
>> SFDP,
>> will set the SPI_NOR_4B_OPCODES flash_info flag. Flashes that can
>> discover it,
>> will ignore/not set the SPI_NOR_4B_OPCODES flag, and let the SFDP do
>> its thing:
>> the SFDP code will set the SNOR_F_4B_OPCODES flag. The only flags that
>> are used
>> across the SPI NOR core are the nor->flags (SNOR_F).
> 
> Sorry, I wasn't clear enough. I was talking about checking (already set)
> SNOR_F
> flags in all these init functions ({flash,manufacturer)->post_sfdp(),
> (flash,manufacturer)->late_init, ..). If we didn't allow this, then
> we would avoid all these "subtle bugs" which happen because some of
> these
> functions depend on another being called first. I.e. all the called
> functions within spi_nor_init_params() may only add SNOR_F flags, but
> must
> not actually use them. [I see that spi_nor_sfdp_init_params() will
> remove SNOR_F_4B_OPCODES. Mhh.]

Keeping the setting of SNOR_F flags based on the flash_info flags as late
as we can assures the reader that SNOR_F flags are not used across the
init params chain. It also indicates that the flash can't set the SNOR_F
capability by parsing SFDP, so it needed explicit flags at declaration.
It is clearer who sets and when.

> 
> Btw. I wonder what is the difference between a member being in "struct
> spi_nor"
> and being in "struct spi_nor_flash_parameter". Apparently, it should
> contain the properties which can be set/changed in the fixups or by
> parsing the SFPD. But then there are also the flags which are also
> changed in the fixups.
> 
> Maybe we should pass the "struct nor" as const and a second parameter
> "struct spi_nor_flash_parameter *params" which can be updated by the
> called function. This way we can be sure the functions won't change
> anything else. I don't suggest to do that right now, just to start
> a discussion.

I'll let this for other time, maybe a separate thread.

> 
>>> The locking ops should also go into the (static) flash init which only
>>> depends on the SPI_NOR_ flags. If SNOR_F_HAS_LOCK will be added at
>>
>> we don't have to specify the locking methods, we have generic ones. A
>> single
>> flag is enough.
> 
> Hm, I'm not really sure we need that SNOR_F_HAS_LOCK flag at all.
> "if (nor->params->locking_ops)" should be the same. At least the current
> code
> which checks the info->flags for SPI_NOR_HAS_LOCK will set the default
> locking ops instead of just setting the SNOR_F_HAS_LOCK.

SNOR_F_HAS_LOCK is kept for consistency. The core driver operates just on SNOR_F
flags. Here it's the same idea, I would like to set the locking ops as late in
the init chain as possible, to indicate that the locking ops are not needed earlier.

> 
>>> in an init function, then this code should also take care of setting
>>
>> what init function? We're just setting some flags.
> 
> all the functions called in spi_nor_init_params().
> 
>>> the correct locking ops. Maybe both can be set together with a helper.
> 
> -michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel