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

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

[Tudor.Ambarus]

From: Tudor Ambarus <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 <alexander.sverdlin@nokia.com>
Signed-off-by: Tudor Ambarus <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

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

[Tudor.Ambarus]

Hi, Alexander,

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

Thanks,
ta

On 11/22/2018 02:36 PM, Tudor Ambarus - M18064 wrote:
> From: Tudor Ambarus <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 <alexander.sverdlin@nokia.com>
> Signed-off-by: Tudor Ambarus <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;
> 

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

[Sverdlin, Alexander (Nokia - DE/Ulm)]

Hello Tudor,

On 22/11/2018 13:36, Tudor.Ambarus@microchip.com wrote:
> From: Tudor Ambarus <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 <alexander.sverdlin@nokia.com>
> Signed-off-by: Tudor Ambarus <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);

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,
             ^~~~~~~~~~~~~~~~~~~~~~

> +
>  	spi_nor_region_mark_end(&region[i - 1]);
>  
>  	return 0;

-- 
Best regards,
Alexander Sverdlin.

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

[Tudor.Ambarus]

On 11/22/2018 06:14 PM, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> Hello Tudor,
> 
> On 22/11/2018 13:36, Tudor.Ambarus@microchip.com wrote:
>> From: Tudor Ambarus <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 <alexander.sverdlin@nokia.com>
>> Signed-off-by: Tudor Ambarus <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);
> 
> 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,
>              ^~~~~~~~~~~~~~~~~~~~~~
> 
>> +


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;

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

[Sverdlin, Alexander (Nokia - DE/Ulm)]

Hello Tudor,

On 22/11/2018 13:36, Tudor.Ambarus@microchip.com wrote:
> From: Tudor Ambarus <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 <alexander.sverdlin@nokia.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

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. 

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

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

[Boris Brezillon]

On Fri, 23 Nov 2018 09:42:55 +0000
"Sverdlin, Alexander (Nokia - DE/Ulm)" <alexander.sverdlin@nokia.com>
wrote:

> Hello Tudor,
> 
> On 22/11/2018 13:36, Tudor.Ambarus@microchip.com wrote:
> > From: Tudor Ambarus <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 <alexander.sverdlin@nokia.com>
> > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>  
> 
> 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.

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.

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

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

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

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

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

[Boris Brezillon]

On Fri, 23 Nov 2018 11:17:29 +0100
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> On Fri, 23 Nov 2018 09:42:55 +0000
> "Sverdlin, Alexander (Nokia - DE/Ulm)" <alexander.sverdlin@nokia.com>
> wrote:
> 
> > Hello Tudor,
> > 
> > On 22/11/2018 13:36, Tudor.Ambarus@microchip.com wrote:
> > > From: Tudor Ambarus <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 <alexander.sverdlin@nokia.com>
> > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>  
> > 
> > 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.
> 
> 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.

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

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

[Tudor.Ambarus]

Hi, Alexander,

On 11/23/2018 11:42 AM, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> Hello Tudor,
> 
> On 22/11/2018 13:36, Tudor.Ambarus@microchip.com wrote:
>> From: Tudor Ambarus <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 <alexander.sverdlin@nokia.com>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> 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.

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

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

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

[Sverdlin, Alexander (Nokia - DE/Ulm)]

Hello Tudor,

On 23/11/2018 11:32, Tudor.Ambarus@microchip.com wrote:
>> 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? 

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.

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

-- 
Best regards,
Alexander Sverdlin.

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

[Tudor.Ambarus]

Hi, Alexander,

On 11/23/2018 01:33 PM, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> 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