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

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.