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

From: Michael Walle <michael@walle.cc>
To: Tudor.Ambarus@microchip.com
Cc: macromorgan@hotmail.com, vigneshr@ti.com, jaimeliao@mxic.com.tw,
	richard@nod.at, esben@geanix.com, linux@rasmusvillemoes.dk,
	knaerzche@gmail.com, Nicolas.Ferre@microchip.com,
	linux-mtd@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org, code@reto-schneider.ch,
	miquel.raynal@bootlin.com, heiko.thiery@gmail.com, sr@denx.de,
	p.yadav@ti.com, mail@david-bauer.net, zhengxunli@mxic.com.tw
Subject: Re: [PATCH v2 17/35] mtd: spi-nor: Introduce spi_nor_nonsfdp_flags_init()
Date: Sun, 24 Oct 2021 19:05:01 +0200	[thread overview]
Message-ID: <b06ac5238d85f62ebe588f161e34cf59@walle.cc> (raw)
In-Reply-To: <9bf93cbf-fc57-54ae-2439-45e4d75dc0b7@microchip.com>

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.]

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.

>> 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.

>> 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 MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/