Message-ID: <08d6657c-afa9-8739-196b-1e66d802e614@nokia.com>
Date:   Mon, 20 Dec 2021 10:46:43 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH v2] mtd: spi-nor: Check for zero erase size in
 spi_nor_find_best_erase_type()
Content-Language: en-US
To:     Tudor.Ambarus@microchip.com, linux-mtd@lists.infradead.org
Cc:     p.yadav@ti.com, michael@walle.cc, miquel.raynal@bootlin.com,
        richard@nod.at, vigneshr@ti.com, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
References: <20211119081412.29732-1-alexander.sverdlin@nokia.com>
 <0cabce03-bc22-eb3d-fa77-a1f5f787784d@microchip.com>
From:   Alexander Sverdlin <alexander.sverdlin@nokia.com>
In-Reply-To: <0cabce03-bc22-eb3d-fa77-a1f5f787784d@microchip.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TUZPNzZ0eTFPNE0zK1hJQkxmanVSNUF0Nlp5eUJyR3Q5SzhmRmhtLzMrZS92?=
 =?utf-8?B?cGxPTUswNGpTQ2JMVUZNOXdFbzBIZ1ZKOW1Sa3g3Vm91MW10RnpRMHM5dDhH?=
 =?utf-8?B?TkdicDRINnp0RzRmUjdGVU9WUHN3ZVNmMEZhNkRpV0pTS0ttRjdldXAxS3c1?=
 =?utf-8?B?a0hscXljSDErTi93ejFxS28xWFRKM3pDcThuWExzYmQ0eFkzK2RSd1VSeEVt?=
 =?utf-8?B?cjB5OEJ1NDFCZndBTzJ6ZTZSbjgxNjFXbVJqTHAzWlVRaUlZRm1MZnYxTk82?=
 =?utf-8?B?SjBESFpPYUxpa1dEbFpEWXAxdjBTSlozclNqL2hpOFNRQWowTHl2NGhiT25p?=
 =?utf-8?B?TkJCSjRaVkxGcDB5KzBlWWZXN3czYnA1MGlOUHRIck9jeENYZlJOQVZaamRm?=
 =?utf-8?B?U1RlcldEbGtQSlg2SUMxc1cwY2NnTzRNTi91WXk0WVVRSjBTVEQ0MG5YTnY4?=
 =?utf-8?B?TW45RUVsYWNpeUx6aDdva2JtZk5aTjRoOTg1M0REQzhtM0dvRHgwQ3QyTXVZ?=
 =?utf-8?B?OVZubE15ZlR0LzkvT0xoZWlxeWh6eDZoby9yNHpKVmtGU2xuSzlrdFJONDVS?=
 =?utf-8?B?VWtZd0J6U3hUMWxiNG9mWTRrY2VBOEFkOWdTdXdkUTNJM1RKZDFtVkRjNk5B?=
 =?utf-8?B?SXFnNHE3SlFGQzFxMUZIQkdPOE94b08rVGFtOGh6YVNKckt5Z3lTYTFrc3F1?=
 =?utf-8?B?R0h3SzNTaTZQdi90d3dJVTNiWk1YU3YvS3lkYzhlTFdvaG9WY1NBbjJtdi9K?=
 =?utf-8?B?UHdkZzB1MWgrQ1dXZytkQUxlUVZlUWxIVG41eDJ3bStOZExud2pwUFEvOG5B?=
 =?utf-8?B?OURwdFV5SDdOc25xZkNLcUora2pLVm80Y29qMllxbzBKRTFpdXEyejF4WnBs?=
 =?utf-8?B?RW5xRndibFBsV1Y5VTdjSzRCeE5qOVNJTTJ1aG9rd21vZnY0RklnMFpkL0U0?=
 =?utf-8?B?T2VTV2NvVER3ZlBWWkhuNStVU2hUbERVUFcyV214anphU0JDTTRyaXZNY0lO?=
 =?utf-8?B?L0drMWRvZUZUTU1XVUV3azdlbnBKVkduNHNvM0xpWWdGTXJ4ZWl0ZVowQ1pS?=
 =?utf-8?B?Y2MvbEJuMHRzRE1ma0E1N05jOWdNNU1QajU2NStRb2ZlN1BrYmRGQk5LVjR1?=
 =?utf-8?B?dWl4UmkxMmZUVmUzMEFha0p1R3A3NXdkdGxUMXBraXRiNVVKNzYvdjhJSW5E?=
 =?utf-8?B?a0YvMURTbjNpd3l3S3BzcnNxaTVwTU1QenJ6dnNaWkJySXlVQUsxZnRQQ0pw?=
 =?utf-8?B?R0R6ZXRHYkV4S2dmbnFSL3ozeDQ0Y2R5OG9KTHd3SGk5YjBCK1JBNk00cFZY?=
 =?utf-8?B?bCsxWmVERE5JeXp5VnM1Z2ExdlFDSnRoNWFYUElSUk5TbEIzSHhENlV0cElr?=
 =?utf-8?B?bUR5NldSd1ozdWpySUtKTVk5SEhQV1A2aVAycjEybEtlQVB6VFovR0dtZFVN?=
 =?utf-8?B?UytzTDVIelBlQ0VyakViSkQxQzhzRk9zUWtKRS9lTys1eTNwQzNXS2xVWGdp?=
 =?utf-8?B?UVJETlZpVnJ1Q1pxdExkK1Y3TGN5c096TU5naW5UdDI0blVMZDA2S3FuMjg5?=
 =?utf-8?B?SXltMFpaNjNCUkdKSlFhVS8rVUVSWUp1c080MmgrMDFZOFNGUGRmSGV3WHkz?=
 =?utf-8?B?cnEyV2lpZ21CSEJsMStzTnNPZUduUndJUWpVVm5KUUlUZFcrejY3U3llT0xS?=
 =?utf-8?B?RUY5TEZaOG5qY1I3WmozNFAveVBQNDdxNkZQUHlabTNUT0NjTSsrcWc2Uk9i?=
 =?utf-8?B?ZExieWxOamZrU01jc21YSGdhb1A4R1hacStSQWdza21NWHlTMVVLTWJYeHNS?=
 =?utf-8?B?VDU3MUpWWWplODdvMFpxa3pKaWNqNloyNVZ1WVlQTllKSEdwY3F6a2ZxYkhK?=
 =?utf-8?B?cjhjcGhnUUhTRURlUE1DMStwcVdHZVpWZXNWbk1TSG9kS0NxNXpnczJwbDZs?=
 =?utf-8?B?UXpPZzFSYnlZL1IyNVUxeFVnSExWSVJSUHhaUkJJc3QvVTZIamlrcGlEdWJq?=
 =?utf-8?B?bHN3Vnh1REVVbEgvNGExUVRERWlRRGJIdkpKODN0R0RtU1c0ZEFGK3VpTFVR?=
 =?utf-8?B?Smt0RVdiSnhNbGZocVowc3ZFUmJ1bmpRRTNnMW9XM1gyQjlGMjYrN2dNRGFL?=
 =?utf-8?B?ektsZmkyK205ZVJHL0R1dUhmb2hYZzcra0pNQ2pBa1ZRK0I0cXFuQSt6cWZL?=
 =?utf-8?B?cGxtNG56SndHS1VoM3duQm9sSmFOeDZNaldCbkN5dW5lVlU3a29IdHRra3Nv?=
 =?utf-8?B?OGtsN1hiemtRajZ2WVlNcDZxTE9nPT0=?=
X-OriginatorOrg: nokia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3bc506e2-8cfb-48bb-f635-08d9c39da3ae
X-MS-Exchange-CrossTenant-AuthSource: AM0PR07MB4531.eurprd07.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Dec 2021 09:46:47.5882
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 5d471751-9675-428d-917b-70f44f9630b0
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: zn6ASIKSP2g4WHvCYLQpy8RxZ+eEsYKLFDoF8T3R0OZH/tbQ1huRRInJ78R1OSp1+fSgSh8tpSgM+ygLAa2+1h2rtjHpzb11n+UuAaoDzuA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR07MB3876
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hello Tudor,

On 18/12/2021 02:31, Tudor.Ambarus@microchip.com wrote:
>> Erase can be zeroed in spi_nor_parse_4bait() or
>> spi_nor_init_non_uniform_erase_map(). In practice it happened with
>> mt25qu256a, which supports 4K, 32K, 64K erases with 3b address commands,
>> but only 4K and 64K erase with 4b address commands.
> 
> :D
> 
>>
>> Fixes: dc92843159a7 ("mtd: spi-nor: fix erase_type array to indicate current map conf")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>> ---
>> Changes in v2:
>> erase->opcode -> erase->size
>>
>>  drivers/mtd/spi-nor/core.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 88dd090..183ea9d 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -1400,6 +1400,8 @@ spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>>                         continue;
>>
>>                 erase = &map->erase_type[i];
>> +               if (!erase->size)
>> +                       continue;
> 
> I need a bit of context here. Does mt25qu256a has a uniform erase layout?

You caught me, the bug will not be visible with this flash type without the patch
which has been ignored for long time:
https://www.spinics.net/lists/linux-mtd/msg11510.html

I however run the above patch because of the reasons described in the commit message.
Nevertheless, the bug fixed now remains a bug no matter what triggers it.

> i.e. Does your flash has sectors of more than one size or does not allow
> the 4K and 64K erase types to be applied on all sectors in the 4B case?
> If no, you should have been in the spi_nor_has_uniform_erase() case, and
> if this case does not suit you, maybe we should update the code for this
> specific case instead.
> 
> On a short look I see that this flash defines just BFPT and 4BAIT table,
> so no SMPT. It looks like you're forcing the flash to behave as it had defined
> SMPT. Am I wrong?
> 
> Also, should we update the region's erase mask instead and mask out the
> unsupported erase type? I would love to hear more about your use case.

-- 
Best regards,
Alexander Sverdlin.