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

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:

quoted

On 10/22/21 3:59 PM, Michael Walle wrote:

quoted

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:

quoted

On 10/22/21 3:10 PM, Pratyush Yadav wrote:

quoted

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:

quoted

Am 2021-08-17 12:24, schrieb Pratyush Yadav:

quoted

On 27/07/21 07:52AM, Tudor Ambarus wrote:

quoted

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.

quoted

quoted

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.

quoted

quoted

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

quoted

quoted

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