[PATCH] mtd: spi-nor: fix erase_type array to indicate current map conf

Discussion:

T***@microchip.com

2018-11-22 12:36:59 UTC

From: Tudor Ambarus <***@microchip.com>

Bug reported for the out-of-tree S25FS128S flash memory.

BFPT table advertises all the erase types supported by all the
possible map configurations. Update the erase_type array to indicate
which erase types are applicable to the current map configuration.

Backward compatibility test done on sst26vf064b.

Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
Reported-by: Alexander Sverdlin <***@nokia.com>
Signed-off-by: Tudor Ambarus <***@microchip.com>
---
drivers/mtd/spi-nor/spi-nor.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 93c9bc8931fc..a71adcd6ddfa 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -3000,7 +3000,8 @@ static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
u64 offset;
u32 region_count;
int i, j;
- u8 erase_type, uniform_erase_type;
+ u8 uniform_erase_type, save_uniform_erase_type;
+ u8 erase_type, regions_erase_type;

region_count = SMPT_MAP_REGION_COUNT(*smpt);
/*
@@ -3014,6 +3015,7 @@ static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
map->regions = region;

uniform_erase_type = 0xff;
+ regions_erase_type = 0;
offset = 0;
/* Populate regions. */
for (i = 0; i < region_count; i++) {
@@ -3030,13 +3032,38 @@ static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
*/
uniform_erase_type &= erase_type;

+ /*
+ * regions_erase_type mask will indicate all the erase types
+ * supported in this configuration map.
+ */
+ regions_erase_type |= erase_type;
+
offset = (region[i].offset & ~SNOR_ERASE_FLAGS_MASK) +
region[i].size;
}

+ save_uniform_erase_type = map->uniform_erase_type;
map->uniform_erase_type = spi_nor_sort_erase_mask(map,
uniform_erase_type);

+ if (!regions_erase_type) {
+ /*
+ * Roll back to the previous uniform_erase_type mask, SMPT is
+ * broken.
+ */
+ map->uniform_erase_type = save_uniform_erase_type;
+ return -EINVAL;
+ }
+
+ /*
+ * BFPT table advertises all the erase types supported by all the
+ * possible map configurations. Update the erase_type array to indicate
+ * which erase types are applicable to the current map configuration.
+ */
+ for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
+ if (!(regions_erase_type & BIT(erase[i].idx)))
+ spi_nor_set_erase_type(&erase[i], 0, 0xFF);
+
spi_nor_region_mark_end(&region[i - 1]);

return 0;

--
2.9.4

T***@microchip.com

2018-11-22 12:38:22 UTC

Permalink

Hi, Alexander,

Can you please test this patch to see if it fixes your problem?

Thanks,
ta

Post by T***@microchip.com
Bug reported for the out-of-tree S25FS128S flash memory.
BFPT table advertises all the erase types supported by all the
possible map configurations. Update the erase_type array to indicate
which erase types are applicable to the current map configuration.
Backward compatibility test done on sst26vf064b.
Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
---
drivers/mtd/spi-nor/spi-nor.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 93c9bc8931fc..a71adcd6ddfa 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -3000,7 +3000,8 @@ static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
u64 offset;
u32 region_count;
int i, j;
- u8 erase_type, uniform_erase_type;
+ u8 uniform_erase_type, save_uniform_erase_type;
+ u8 erase_type, regions_erase_type;
region_count = SMPT_MAP_REGION_COUNT(*smpt);
/*
@@ -3014,6 +3015,7 @@ static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
map->regions = region;
uniform_erase_type = 0xff;
+ regions_erase_type = 0;
offset = 0;
/* Populate regions. */
for (i = 0; i < region_count; i++) {
@@ -3030,13 +3032,38 @@ static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
*/
uniform_erase_type &= erase_type;
+ /*
+ * regions_erase_type mask will indicate all the erase types
+ * supported in this configuration map.
+ */
+ regions_erase_type |= erase_type;
+
offset = (region[i].offset & ~SNOR_ERASE_FLAGS_MASK) +
region[i].size;
}
+ save_uniform_erase_type = map->uniform_erase_type;
map->uniform_erase_type = spi_nor_sort_erase_mask(map,
uniform_erase_type);
+ if (!regions_erase_type) {
+ /*
+ * Roll back to the previous uniform_erase_type mask, SMPT is
+ * broken.
+ */
+ map->uniform_erase_type = save_uniform_erase_type;
+ return -EINVAL;
+ }
+
+ /*
+ * BFPT table advertises all the erase types supported by all the
+ * possible map configurations. Update the erase_type array to indicate
+ * which erase types are applicable to the current map configuration.
+ */
+ for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
+ if (!(regions_erase_type & BIT(erase[i].idx)))
+ spi_nor_set_erase_type(&erase[i], 0, 0xFF);
+
spi_nor_region_mark_end(&region[i - 1]);
return 0;

Sverdlin, Alexander (Nokia - DE/Ulm)

2018-11-22 14:56:07 UTC

Permalink

Hello Tudor,

Post by T***@microchip.com
Can you please test this patch to see if it fixes your problem?

the patch applies neither to Linus's master nor to spi-nor/next (of l2-mtd tree) branches.
What have you used as a base for your patch?

Post by T***@microchip.com

Post by T***@microchip.com
Bug reported for the out-of-tree S25FS128S flash memory.

It turned out to be not relevant, S25FS128S works with upstream Linux as well, just
the entry is named s25fl129p1...

Post by T***@microchip.com

Post by T***@microchip.com
BFPT table advertises all the erase types supported by all the
possible map configurations. Update the erase_type array to indicate
which erase types are applicable to the current map configuration.
Backward compatibility test done on sst26vf064b.
Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")

--
Best regards,
Alexander Sverdlin.

T***@microchip.com

2018-11-22 15:01:19 UTC

Permalink

Hi, Alexander,

Post by Sverdlin, Alexander (Nokia - DE/Ulm)
Hello Tudor,

Post by T***@microchip.com
Can you please test this patch to see if it fixes your problem?

the patch applies neither to Linus's master nor to spi-nor/next (of l2-mtd tree) branches.
What have you used as a base for your patch?

http://git.infradead.org/linux-mtd.git, mtd/fixes branch

Post by Sverdlin, Alexander (Nokia - DE/Ulm)

Post by T***@microchip.com

Post by T***@microchip.com
Bug reported for the out-of-tree S25FS128S flash memory.

It turned out to be not relevant, S25FS128S works with upstream Linux as well, just
the entry is named s25fl129p1...

ok, will remove this comment if so.

Cheers,
ta

Sverdlin, Alexander (Nokia - DE/Ulm)

2018-11-22 16:14:34 UTC

Permalink

Hello Tudor,

have you noticed the following:

.../drivers/mtd/spi-nor/spi-nor.c: In function ‘spi_nor_init_non_uniform_erase_map’:
.../drivers/mtd/spi-nor/spi-nor.c:2940:27: error: passing argument 1 of ‘spi_nor_set_erase_type’ discards ‘const’ qualifier
from pointer target type [-Werror=discarded-qualifiers]
spi_nor_set_erase_type(&erase[i], 0, 0xFF);
^
.../drivers/mtd/spi-nor/spi-nor.c:2368:13: note: expected ‘struct spi_nor_erase_type *’ but argument is of type ‘const stru
ct spi_nor_erase_type *’
static void spi_nor_set_erase_type(struct spi_nor_erase_type *erase,
^~~~~~~~~~~~~~~~~~~~~~

Post by T***@microchip.com
+
spi_nor_region_mark_end(&region[i - 1]);
return 0;

--
Best regards,
Alexander Sverdlin.

T***@microchip.com

2018-11-22 16:22:34 UTC

Permalink

Post by Sverdlin, Alexander (Nokia - DE/Ulm)
Hello Tudor,

.../drivers/mtd/spi-nor/spi-nor.c:2940:27: error: passing argument 1 of ‘spi_nor_set_erase_type’ discards ‘const’ qualifier
from pointer target type [-Werror=discarded-qualifiers]
spi_nor_set_erase_type(&erase[i], 0, 0xFF);
^
.../drivers/mtd/spi-nor/spi-nor.c:2368:13: note: expected ‘struct spi_nor_erase_type *’ but argument is of type ‘const stru
ct spi_nor_erase_type *’
static void spi_nor_set_erase_type(struct spi_nor_erase_type *erase,
^~~~~~~~~~~~~~~~~~~~~~

Post by T***@microchip.com
+

Oops, no, thanks! Removing the const qualifier will do the trick:

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index a71adcd6ddfa..06086916ec3c 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2995,7 +2995,7 @@ static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
const u32 *smpt)
{
struct spi_nor_erase_map *map = &nor->erase_map;
- const struct spi_nor_erase_type *erase = map->erase_type;
+ struct spi_nor_erase_type *erase = map->erase_type;
struct spi_nor_erase_region *region;
u64 offset;
u32 region_count;

Sverdlin, Alexander (Nokia - DE/Ulm)

2018-11-23 09:42:55 UTC

Permalink

Hello Tudor,

I've tested this patch and it fixes the erasesize for S25FS128S and
our 128k partitions are writeable again with this patch.

Nevertheless, I think this is coincidence. I don't think that it
makes sense to OR all the erase types from all regions in one
bitmask and derive any uniform erasesize out of it.
This makes little sense for me in case of non-uniform maps.

I believe, the culprit here is one level higher, in the MTD partitioning
code (mtdpart.c) which has to be taught about non-uniform maps
but there is no infrastructure for this currently.

What is possible to fix still is to choose smallest, not biggest
erasesize for uniform case. I have such a patch, but I need hands
on on a S25FS128S configured in uniform mode.

Non uniform case requires MTD layer rework. We just cannot handle
this hardware with just one erasesize in mind.

Post by T***@microchip.com
---
drivers/mtd/spi-nor/spi-nor.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 93c9bc8931fc..a71adcd6ddfa 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -3000,7 +3000,8 @@ static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
u64 offset;
u32 region_count;
int i, j;
- u8 erase_type, uniform_erase_type;
+ u8 uniform_erase_type, save_uniform_erase_type;
+ u8 erase_type, regions_erase_type;
region_count = SMPT_MAP_REGION_COUNT(*smpt);
/*
@@ -3014,6 +3015,7 @@ static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
map->regions = region;
uniform_erase_type = 0xff;
+ regions_erase_type = 0;
offset = 0;
/* Populate regions. */
for (i = 0; i < region_count; i++) {
@@ -3030,13 +3032,38 @@ static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
*/
uniform_erase_type &= erase_type;
+ /*
+ * regions_erase_type mask will indicate all the erase types
+ * supported in this configuration map.
+ */
+ regions_erase_type |= erase_type;
+
offset = (region[i].offset & ~SNOR_ERASE_FLAGS_MASK) +
region[i].size;
}
+ save_uniform_erase_type = map->uniform_erase_type;
map->uniform_erase_type = spi_nor_sort_erase_mask(map,
uniform_erase_type);
+ if (!regions_erase_type) {
+ /*
+ * Roll back to the previous uniform_erase_type mask, SMPT is
+ * broken.
+ */
+ map->uniform_erase_type = save_uniform_erase_type;
+ return -EINVAL;
+ }
+
+ /*
+ * BFPT table advertises all the erase types supported by all the
+ * possible map configurations. Update the erase_type array to indicate
+ * which erase types are applicable to the current map configuration.
+ */
+ for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
+ if (!(regions_erase_type & BIT(erase[i].idx)))
+ spi_nor_set_erase_type(&erase[i], 0, 0xFF);
+
spi_nor_region_mark_end(&region[i - 1]);
return 0;

--
Best regards,
Alexander Sverdlin.

Boris Brezillon

2018-11-23 10:17:29 UTC

Permalink

On Fri, 23 Nov 2018 09:42:55 +0000

Post by Sverdlin, Alexander (Nokia - DE/Ulm)
Hello Tudor,

Keep in mind that mtdpart is only one part of the issue. As I said
previously, some MTD users (UBI) expect a single eraseblock size, so
it's not only mtdpart that you'd need to fix, but basically all MTD
users that don't check ->eraseregions.

Post by Sverdlin, Alexander (Nokia - DE/Ulm)
What is possible to fix still is to choose smallest, not biggest
erasesize for uniform case.

Yep, I agree. But we also have to be careful here, if some NORs were
already supported and were picking the biggest erasesize, we have to
preserve this behavior, otherwise we'll break things.

Post by Sverdlin, Alexander (Nokia - DE/Ulm)
I have such a patch, but I need hands
on on a S25FS128S configured in uniform mode.
Non uniform case requires MTD layer rework.

There's already the concept of ->eraseregions. Maybe I'm wrong but I
thought it would fit the non-uniform erase scheme we have in SPI NOR.

Post by Sverdlin, Alexander (Nokia - DE/Ulm)
We just cannot handle
this hardware with just one erasesize in mind.

We can, if we decide to always use the biggest non-uniform erasesize.
Of course that only works if the biggest erase size is always a
multiple of smaller ones, but I'm pretty sure this is always true.

Post by Sverdlin, Alexander (Nokia - DE/Ulm)

Boris Brezillon

2018-11-23 10:29:03 UTC

Permalink

On Fri, 23 Nov 2018 11:17:29 +0100

Post by Boris Brezillon
On Fri, 23 Nov 2018 09:42:55 +0000

Post by Sverdlin, Alexander (Nokia - DE/Ulm)
Hello Tudor,

Just checked, and it seems mtdpart already supports eraseregions [1].
It picks the biggest erasesize of the regions covered by the partition,
which is exactly what we want. So all we'd have to do is make spi-nor.c
define those regions.

[1]https://elixir.bootlin.com/linux/v4.20-rc3/source/drivers/mtd/mtdpart.c#L482

T***@microchip.com

2018-11-23 10:32:33 UTC

Permalink

Hi, Alexander,

Post by Sverdlin, Alexander (Nokia - DE/Ulm)
Hello Tudor,

I've tested this patch and it fixes the erasesize for S25FS128S and
our 128k partitions are writeable again with this patch.

Good. You said that the flash is configured in non-uniform mapping (hybrid
sector option). There is no uniform erase type that can erase the entire
flash memory by its own, so the erase should be done using the non-uniform
erase code (spi_nor_has_uniform_erase() should return false). Does the
non-uniform erase work as expected?

For example, if the hybrid section is at the top of the address space, when
using mtd_utils:
$ mtd_debug erase /dev/your_mtd 4096 126976
I would expect the use of the following erase types: 7*4k, 32k, 64k
Then write and read back to check if the erase was done correctly:
$ dd if=/dev/urandom of=in bs=4096 count=31
$ mtd_debug write /dev/your_mtd 4096 126976 in
$ mtd_debug read /dev/your_mtd 4096 126976 out
$ sha1sum in out
the output should be the same.

Feel free to add some prints in the code to be sure that non-uniform erase code
selects the correct erase types.

Post by Sverdlin, Alexander (Nokia - DE/Ulm)
Nevertheless, I think this is coincidence. I don't think that it
makes sense to OR all the erase types from all regions in one
bitmask and derive any uniform erasesize out of it.
This makes little sense for me in case of non-uniform maps.

I just masked out the erase types that are not supported by the current
configuration map. We should not use an erase type that is not supported
by any of the map regions. SMPT, if present, should have precedence over
BFPT when indicating supported erase types.

Cheers,
ta

Post by Sverdlin, Alexander (Nokia - DE/Ulm)
I believe, the culprit here is one level higher, in the MTD partitioning
code (mtdpart.c) which has to be taught about non-uniform maps
but there is no infrastructure for this currently.
What is possible to fix still is to choose smallest, not biggest
erasesize for uniform case. I have such a patch, but I need hands
on on a S25FS128S configured in uniform mode.
Non uniform case requires MTD layer rework. We just cannot handle
this hardware with just one erasesize in mind.

Sverdlin, Alexander (Nokia - DE/Ulm)

2018-11-23 11:33:07 UTC

Permalink

Hello Tudor,

Post by T***@microchip.com

Post by Sverdlin, Alexander (Nokia - DE/Ulm)
I've tested this patch and it fixes the erasesize for S25FS128S and
our 128k partitions are writeable again with this patch.

unfortunately, it doesn't. I was too fast reporting the success...
So erasesize is back to the value before non-uniform support patches,
partitions are not made read only any more, but actual write or
erase still fails.

I'll investigate further.

Taking into account the comments from Boris, we probably need this patch
anyway.

Post by T***@microchip.com
For example, if the hybrid section is at the top of the address space, when
$ mtd_debug erase /dev/your_mtd 4096 126976
I would expect the use of the following erase types: 7*4k, 32k, 64k
$ dd if=/dev/urandom of=in bs=4096 count=31
$ mtd_debug write /dev/your_mtd 4096 126976 in
$ mtd_debug read /dev/your_mtd 4096 126976 out
$ sha1sum in out
the output should be the same.
Feel free to add some prints in the code to be sure that non-uniform erase code
selects the correct erase types.

--
Best regards,
Alexander Sverdlin.

T***@microchip.com

2018-11-23 12:39:44 UTC

Permalink

Hi, Alexander,

Post by Sverdlin, Alexander (Nokia - DE/Ulm)
So erasesize is back to the value before non-uniform support patches,
partitions are not made read only any more, but actual write or
erase still fails.

How does the erase fail?

What map configuration is selected? Where are the hybrid blocks, at the bottom
or at the top of the address space?

Does the erase fail even when address is aligned with 64k and the size multiple
of 64k? Does the following fail?

$ mtd_debug erase /dev/your_mtd 0 65536

The non-uniform erase will fail if it can't select a sequence of commands that
can erase the entire requested chunk.

Cheers,
ta