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@xxxxxxxxxxxxx wrote:
>> From: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
> 
> 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;
>