diff mbox series

mtd: cfi_cmdset_0001: Support the absence of protection registers

Message ID 20200417142325.2931423-1-jean-philippe@linaro.org
State New
Headers show
Series mtd: cfi_cmdset_0001: Support the absence of protection registers | expand

Commit Message

Jean-Philippe Brucker April 17, 2020, 2:23 p.m. UTC 
The flash controller implemented by the Arm Base platform behaves like
the Intel StrataFlash J3 device, but omits several features. In
particular it doesn't implement a protection register, so "Number of
Protection register fields" in the Primary Vendor-Specific Extended
Query, is 0.

The Intel StrataFlash J3 datasheet only lists 1 as a valid value for
NumProtectionFields. It describes the field as:

	"Number of Protection register fields in JEDEC ID space.
	“00h,” indicates that 256 protection bytes are available"

While a value of 0 may arguably not be architecturally valid, the
driver's current behavior is certainly wrong: if NumProtectionFields is
0, read_pri_intelext() adds a negative value to the unsigned extra_size,
and ends up in an infinite loop.

Fix it by ignoring a NumProtectionFields of 0.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
I guess this flash device has never been tested on Linux. The bug showed
up when trying to boot the latest arm64 defconfig, which enabled
CONFIG_MTD_PHYSMAP_OF, on the RevC FastModel. Without this config option
the device isn't probed.
---
 drivers/mtd/chips/cfi_cmdset_0001.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Sudeep Holla April 29, 2020, 2:47 p.m. UTC | #1 
On Fri, Apr 17, 2020 at 04:23:26PM +0200, Jean-Philippe Brucker wrote:
> The flash controller implemented by the Arm Base platform behaves like
> the Intel StrataFlash J3 device, but omits several features. In
> particular it doesn't implement a protection register, so "Number of
> Protection register fields" in the Primary Vendor-Specific Extended
> Query, is 0.
> 
> The Intel StrataFlash J3 datasheet only lists 1 as a valid value for
> NumProtectionFields. It describes the field as:
> 
> 	"Number of Protection register fields in JEDEC ID space.
> 	“00h,” indicates that 256 protection bytes are available"
> 
> While a value of 0 may arguably not be architecturally valid, the
> driver's current behavior is certainly wrong: if NumProtectionFields is
> 0, read_pri_intelext() adds a negative value to the unsigned extra_size,
> and ends up in an infinite loop.
> 
> Fix it by ignoring a NumProtectionFields of 0.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> I guess this flash device has never been tested on Linux. The bug showed
> up when trying to boot the latest arm64 defconfig, which enabled
> CONFIG_MTD_PHYSMAP_OF, on the RevC FastModel. Without this config option
> the device isn't probed.

Any progress with this patch ?

FWIW, this fixes boot on few arm64 Arm Ltd FastModels we use for
development including the above mentioned RevC FastModel. So,

Tested-by:  Sudeep Holla <sudeep.holla@arm.com>
Catalin Marinas April 30, 2020, 4:32 p.m. UTC | #2 
On Fri, Apr 17, 2020 at 04:23:26PM +0200, Jean-Philippe Brucker wrote:
> The flash controller implemented by the Arm Base platform behaves like
> the Intel StrataFlash J3 device, but omits several features. In
> particular it doesn't implement a protection register, so "Number of
> Protection register fields" in the Primary Vendor-Specific Extended
> Query, is 0.
> 
> The Intel StrataFlash J3 datasheet only lists 1 as a valid value for
> NumProtectionFields. It describes the field as:
> 
> 	"Number of Protection register fields in JEDEC ID space.
> 	“00h,” indicates that 256 protection bytes are available"
> 
> While a value of 0 may arguably not be architecturally valid, the
> driver's current behavior is certainly wrong: if NumProtectionFields is
> 0, read_pri_intelext() adds a negative value to the unsigned extra_size,
> and ends up in an infinite loop.
> 
> Fix it by ignoring a NumProtectionFields of 0.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

If you need another confirmation:

Tested-by: Catalin Marinas <catalin.marinas@arm.com>
Vignesh Raghavendra May 1, 2020, 6:24 a.m. UTC | #3 
Hi,

On 4/30/2020 10:02 PM, Catalin Marinas wrote:
> On Fri, Apr 17, 2020 at 04:23:26PM +0200, Jean-Philippe Brucker wrote:
>> The flash controller implemented by the Arm Base platform behaves like
>> the Intel StrataFlash J3 device, but omits several features. In
>> particular it doesn't implement a protection register, so "Number of
>> Protection register fields" in the Primary Vendor-Specific Extended
>> Query, is 0.
>>
>> The Intel StrataFlash J3 datasheet only lists 1 as a valid value for
>> NumProtectionFields. It describes the field as:
>>
>> 	"Number of Protection register fields in JEDEC ID space.
>> 	“00h,” indicates that 256 protection bytes are available"
>>
>> While a value of 0 may arguably not be architecturally valid, the
>> driver's current behavior is certainly wrong: if NumProtectionFields is
>> 0, read_pri_intelext() adds a negative value to the unsigned extra_size,
>> and ends up in an infinite loop.
>>
>> Fix it by ignoring a NumProtectionFields of 0.
>>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> 
> If you need another confirmation:
> 
> Tested-by: Catalin Marinas <catalin.marinas@arm.com>
> 

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git cfi/next

Thanks
Vignesh
diff mbox series

Patch

diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
index 142c0f9485fe1..42001c49833b9 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -420,8 +420,9 @@  read_pri_intelext(struct map_info *map, __u16 adr)
 		extra_size = 0;
 
 		/* Protection Register info */
-		extra_size += (extp->NumProtectionFields - 1) *
-			      sizeof(struct cfi_intelext_otpinfo);
+		if (extp->NumProtectionFields)
+			extra_size += (extp->NumProtectionFields - 1) *
+				      sizeof(struct cfi_intelext_otpinfo);
 	}
 
 	if (extp->MinorVersion >= '1') {
@@ -695,14 +696,16 @@  static int cfi_intelext_partition_fixup(struct mtd_info *mtd,
 	 */
 	if (extp && extp->MajorVersion == '1' && extp->MinorVersion >= '3'
 	    && extp->FeatureSupport & (1 << 9)) {
+		int offs = 0;
 		struct cfi_private *newcfi;
 		struct flchip *chip;
 		struct flchip_shared *shared;
-		int offs, numregions, numparts, partshift, numvirtchips, i, j;
+		int numregions, numparts, partshift, numvirtchips, i, j;
 
 		/* Protection Register info */
-		offs = (extp->NumProtectionFields - 1) *
-		       sizeof(struct cfi_intelext_otpinfo);
+		if (extp->NumProtectionFields)
+			offs = (extp->NumProtectionFields - 1) *
+			       sizeof(struct cfi_intelext_otpinfo);
 
 		/* Burst Read info */
 		offs += extp->extra[offs+1]+2;