Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 18 Dec 2020 19:45:50 +0530
From:   Pratyush Yadav <p.yadav@ti.com>
To:     Vignesh Raghavendra <vigneshr@ti.com>
CC:     Tudor Ambarus <tudor.ambarus@microchip.com>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Boris Brezillon <bbrezillon@kernel.org>,
        Marek Vasut <marek.vasut@gmail.com>,
        <linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] mtd: spi-nor: sfdp: Fix SMPT parsing when 4BAIT table is
 before SMPT
Message-ID: <20201218141550.f2ik6lw65ly4lca5@ti.com>
References: <20201212115817.5122-1-vigneshr@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20201212115817.5122-1-vigneshr@ti.com>
User-Agent: NeoMutt/20171215
Precedence: bulk

Hi Vignesh,

On 12/12/20 05:28PM, Vignesh Raghavendra wrote:
> If 4BAIT table appears before SMPT then, nor->addr_width is set to 4 before
> SMPT table is parsed. This causes SMPT parser code to use addr_width of 4
> for reading current sector map configuration in spi_nor_get_map_in_use()
> if SMPT_CMD_ADDRESS_LEN reads SMPT_CMD_ADDRESS_LEN_USE_CURRENT (see
> spi_nor_smpt_addr_width()). Instead code should be using the value
> presented by BFPT_DWORD1_ADDRESS_BYTES field. On S28HS family of devices
> this bug leads to selection of invalid sector mapping thus causing erase
> and write failures.
> 
> Fix this by parsing 4BAIT address table at the very end after all other
> parameter tables are parsed.

I'm not convinced this is the best way to go about this. What if we have 
another table tomorrow that introduces some other side effect? Will we 
special-case all those as well? IMO a better way of going about this 
would be to make these parsers not affect the current state of the 
flash, but instead store that information as a possible future state of 
the flash. Then we can use that to finally decide what we want to do in 
spi_nor_set_addr_width(). In fact, that information is already stored 
via SNOR_F_HAS_4BAIT (though I'm not too opposed to adding a 
nor->params->addr_width).

I see this as a cleaner alternative to your approach. We do need to be 
careful about any obscure bugs here like with [0] though.

[0] https://lore.kernel.org/linux-mtd/20201006132346.12652-1-bert@biot.com/

-- 
Regards,
Pratyush Yadav
Texas Instruments India