[PATCH v2 0/5] mtd: spi-nor: fixes found when debugging smpt

[PATCH v2 0/5] mtd: spi-nor: fixes found when debugging smpt

[Tudor.Ambarus]

Bunch of fixes that we found while debugging the roll back to
SPINOR_OP_READ_1_1_4_4B in case smpt parser fails.

Boris, Yogesh,

Now I'm looking for a fix in case the smpt detection commands
define variable address length and dummy bytes. Will follow.

v2:
- drop patches 3 and 6
- update fixes tag for the first patch
- add fixes tags for other patches
- typo, blank line, rename variable, add comment

Tudor Ambarus (5):
  mtd: spi-nor: don't drop sfdp data if optional parsers fail
  mtd: spi-nor: fix iteration over smpt array
  mtd: spi-nor: don't overwrite errno in spi_nor_get_map_in_use()
  mtd: spi_nor: pass DMA-able buffer to spi_nor_read_raw()
  mtd: spi-nor: remove unneeded smpt zeroization

 drivers/mtd/spi-nor/spi-nor.c | 85 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 63 insertions(+), 22 deletions(-)

-- 
2.9.4

[PATCH v2 1/5] mtd: spi-nor: don't drop sfdp data if optional parsers fail

[Tudor.Ambarus]

JESD216C states that just the Basic Flash Parameter Table is mandatory.
Already defined (or future) additional parameter headers and tables are
optional.

Don't drop already collected sfdp data in case an optional table
parser fails. In case of failing, each optional parser is responsible
to roll back to the previously known spi_nor data.

Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
Reported-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v2: update Fixes tag to point to correct commit

 drivers/mtd/spi-nor/spi-nor.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 4a96ee719e5a..2cdf96013689 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -3130,7 +3130,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 	if (err)
 		goto exit;
 
-	/* Parse other parameter headers. */
+	/* Parse optional parameter tables. */
 	for (i = 0; i < header.nph; i++) {
 		param_header = &param_headers[i];
 
@@ -3143,8 +3143,17 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 			break;
 		}
 
-		if (err)
-			goto exit;
+		if (err) {
+			dev_warn(dev, "Failed to parse optional parameter table: %04x\n",
+				 SFDP_PARAM_HEADER_ID(param_header));
+			/*
+			 * Let's not drop all information we extracted so far
+			 * if optional table parsers fail. In case of failing,
+			 * each optional parser is responsible to roll back to
+			 * the previously known spi_nor data.
+			 */
+			err = 0;
+		}
 	}
 
 exit:
-- 
2.9.4

RE: [PATCH v2 1/5] mtd: spi-nor: don't drop sfdp data if optional parsers fail

[Yogesh Narayan Gaur]

> -----Original Message-----
> From: Tudor.Ambarus@microchip.com [mailto:Tudor.Ambarus@microchip.com]
> Sent: Friday, November 9, 2018 10:27 PM
> To: boris.brezillon@bootlin.com; marek.vasut@gmail.com;
> dwmw2@infradead.org; computersforpeace@gmail.com; richard@nod.at
> Cc: linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Yogesh
> Narayan Gaur <yogeshnarayan.gaur@nxp.com>; cyrille.pitchen@wedev4u.fr;
> Tudor.Ambarus@microchip.com
> Subject: [PATCH v2 1/5] mtd: spi-nor: don't drop sfdp data if optional parsers fail
> 
> JESD216C states that just the Basic Flash Parameter Table is mandatory.
> Already defined (or future) additional parameter headers and tables are optional.
> 
> Don't drop already collected sfdp data in case an optional table parser fails. In
> case of failing, each optional parser is responsible to roll back to the previously
> known spi_nor data.
> 
> Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
> Reported-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
Tested-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>

> Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> v2: update Fixes tag to point to correct commit
> 
>  drivers/mtd/spi-nor/spi-nor.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index
> 4a96ee719e5a..2cdf96013689 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -3130,7 +3130,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  	if (err)
>  		goto exit;
> 
> -	/* Parse other parameter headers. */
> +	/* Parse optional parameter tables. */
>  	for (i = 0; i < header.nph; i++) {
>  		param_header = &param_headers[i];
> 
> @@ -3143,8 +3143,17 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  			break;
>  		}
> 
> -		if (err)
> -			goto exit;
> +		if (err) {
> +			dev_warn(dev, "Failed to parse optional parameter
> table: %04x\n",
> +				 SFDP_PARAM_HEADER_ID(param_header));
> +			/*
> +			 * Let's not drop all information we extracted so far
> +			 * if optional table parsers fail. In case of failing,
> +			 * each optional parser is responsible to roll back to
> +			 * the previously known spi_nor data.
> +			 */
> +			err = 0;
> +		}
>  	}
> 
>  exit:
> --
> 2.9.4

Re: [v2,1/5] mtd: spi-nor: don't drop sfdp data if optional parsers fail

[Boris Brezillon]

On Fri, 2018-11-09 at 16:56:48 UTC,  wrote:
> JESD216C states that just the Basic Flash Parameter Table is mandatory.
> Already defined (or future) additional parameter headers and tables are
> optional.
> 
> Don't drop already collected sfdp data in case an optional table
> parser fails. In case of failing, each optional parser is responsible
> to roll back to the previously known spi_nor data.
> 
> Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
> Reported-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
> Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Applied to http://git.infradead.org/linux-mtd.git mtd/fixes, thanks.

Boris

[PATCH v2 2/5] mtd: spi-nor: fix iteration over smpt array

[Tudor.Ambarus]

Iterate over smpt array using its starting address and length
instead of the blind iterations that used data found in the array.

This prevents possible memory accesses outside of the smpt array
boundaries in case software, or manufacturers, misrepresent smpt
array fields.

Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v2: add Fixes tag, add a blank line

 drivers/mtd/spi-nor/spi-nor.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 2cdf96013689..98e433e8e4c2 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2860,12 +2860,15 @@ static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
  * spi_nor_get_map_in_use() - get the configuration map in use
  * @nor:	pointer to a 'struct spi_nor'
  * @smpt:	pointer to the sector map parameter table
+ * @smpt_len:	sector map parameter table length
  */
-static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt)
+static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
+					 u8 smpt_len)
 {
 	const u32 *ret = NULL;
-	u32 i, addr;
+	u32 addr;
 	int err;
+	u8 i;
 	u8 addr_width, read_opcode, read_dummy;
 	u8 read_data_mask, data_byte, map_id;
 
@@ -2874,9 +2877,11 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt)
 	read_opcode = nor->read_opcode;
 
 	map_id = 0;
-	i = 0;
 	/* Determine if there are any optional Detection Command Descriptors */
-	while (!(smpt[i] & SMPT_DESC_TYPE_MAP)) {
+	for (i = 0; i < smpt_len; i += 2) {
+		if (smpt[i] & SMPT_DESC_TYPE_MAP)
+			break;
+
 		read_data_mask = SMPT_CMD_READ_DATA(smpt[i]);
 		nor->addr_width = spi_nor_smpt_addr_width(nor, smpt[i]);
 		nor->read_dummy = spi_nor_smpt_read_dummy(nor, smpt[i]);
@@ -2892,18 +2897,33 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt)
 		 * Configuration that is currently in use.
 		 */
 		map_id = map_id << 1 | !!(data_byte & read_data_mask);
-		i = i + 2;
 	}
 
-	/* Find the matching configuration map */
-	while (SMPT_MAP_ID(smpt[i]) != map_id) {
+	/*
+	 * If command descriptors are provided, they always precede map
+	 * descriptors in the table. There is no need to start the iteration
+	 * over smpt array all over again.
+	 *
+	 * Find the matching configuration map.
+	 */
+	while (i < smpt_len) {
+		if (SMPT_MAP_ID(smpt[i]) == map_id) {
+			ret = smpt + i;
+			break;
+		}
+
+		/*
+		 * If there are no more configuration map descriptors and no
+		 * configuration ID matched the configuration identifier, the
+		 * sector address map is unknown.
+		 */
 		if (smpt[i] & SMPT_DESC_END)
-			goto out;
+			break;
+
 		/* increment the table index to the next map */
 		i += SMPT_MAP_REGION_COUNT(smpt[i]) + 1;
 	}
 
-	ret = smpt + i;
 	/* fall through */
 out:
 	nor->addr_width = addr_width;
@@ -3025,7 +3045,7 @@ static int spi_nor_parse_smpt(struct spi_nor *nor,
 	for (i = 0; i < smpt_header->length; i++)
 		smpt[i] = le32_to_cpu(smpt[i]);
 
-	sector_map = spi_nor_get_map_in_use(nor, smpt);
+	sector_map = spi_nor_get_map_in_use(nor, smpt, smpt_header->length);
 	if (!sector_map) {
 		ret = -EINVAL;
 		goto out;
-- 
2.9.4

Re: [v2,2/5] mtd: spi-nor: fix iteration over smpt array

[Boris Brezillon]

On Fri, 2018-11-09 at 16:56:50 UTC,  wrote:
> Iterate over smpt array using its starting address and length
> instead of the blind iterations that used data found in the array.
> 
> This prevents possible memory accesses outside of the smpt array
> boundaries in case software, or manufacturers, misrepresent smpt
> array fields.
> 
> Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
> Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Applied to http://git.infradead.org/linux-mtd.git mtd/fixes, thanks.

Boris

[PATCH v2 3/5] mtd: spi-nor: don't overwrite errno in spi_nor_get_map_in_use()

[Tudor.Ambarus]

Don't overwrite the errno from spi_nor_read_raw().

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 98e433e8e4c2..04a1c5b825e6 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2861,11 +2861,13 @@ static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
  * @nor:	pointer to a 'struct spi_nor'
  * @smpt:	pointer to the sector map parameter table
  * @smpt_len:	sector map parameter table length
+ *
+ * Return: pointer to the map in use, ERR_PTR(-errno) otherwise.
  */
 static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
 					 u8 smpt_len)
 {
-	const u32 *ret = NULL;
+	const u32 *ret;
 	u32 addr;
 	int err;
 	u8 i;
@@ -2889,8 +2891,10 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
 		addr = smpt[i + 1];
 
 		err = spi_nor_read_raw(nor, addr, 1, &data_byte);
-		if (err)
+		if (err) {
+			ret = ERR_PTR(err);
 			goto out;
+		}
 
 		/*
 		 * Build an index value that is used to select the Sector Map
@@ -2906,6 +2910,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
 	 *
 	 * Find the matching configuration map.
 	 */
+	ret = ERR_PTR(-EINVAL);
 	while (i < smpt_len) {
 		if (SMPT_MAP_ID(smpt[i]) == map_id) {
 			ret = smpt + i;
@@ -3046,8 +3051,8 @@ static int spi_nor_parse_smpt(struct spi_nor *nor,
 		smpt[i] = le32_to_cpu(smpt[i]);
 
 	sector_map = spi_nor_get_map_in_use(nor, smpt, smpt_header->length);
-	if (!sector_map) {
-		ret = -EINVAL;
+	if (IS_ERR(sector_map)) {
+		ret = PTR_ERR(sector_map);
 		goto out;
 	}
 
-- 
2.9.4

Re: [v2, 3/5] mtd: spi-nor: don't overwrite errno in spi_nor_get_map_in_use()

[Boris Brezillon]

On Fri, 2018-11-09 at 16:56:52 UTC,  wrote:
> Don't overwrite the errno from spi_nor_read_raw().
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Applied to http://git.infradead.org/linux-mtd.git mtd/fixes, thanks.

Boris

[PATCH v2 4/5] mtd: spi_nor: pass DMA-able buffer to spi_nor_read_raw()

[Tudor.Ambarus]

spi_nor_read_raw() calls nor->read() which might be implemented
by the m25p80 driver. m25p80 uses the spi-mem layer which requires
DMA-able in/out buffers. Pass kmalloc'ed dma buffer to spi_nor_read_raw().

Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v2: drop GFP_DMA, rename buf, add comment

 drivers/mtd/spi-nor/spi-nor.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 04a1c5b825e6..458ca8321999 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2161,7 +2161,7 @@ spi_nor_set_pp_settings(struct spi_nor_pp_command *pp,
  * @nor:	pointer to a 'struct spi_nor'
  * @addr:	offset in the serial flash memory
  * @len:	number of bytes to read
- * @buf:	buffer where the data is copied into
+ * @buf:	buffer where the data is copied into (dma-safe memory)
  *
  * Return: 0 on success, -errno otherwise.
  */
@@ -2868,11 +2868,17 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
 					 u8 smpt_len)
 {
 	const u32 *ret;
+	u8 *buf;
 	u32 addr;
 	int err;
 	u8 i;
 	u8 addr_width, read_opcode, read_dummy;
-	u8 read_data_mask, data_byte, map_id;
+	u8 read_data_mask, map_id;
+
+	/* Use a kmalloc'ed bounce buffer to guarantee it is DMA-able. */
+	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		return ERR_PTR(-ENOMEM);
 
 	addr_width = nor->addr_width;
 	read_dummy = nor->read_dummy;
@@ -2890,7 +2896,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
 		nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
 		addr = smpt[i + 1];
 
-		err = spi_nor_read_raw(nor, addr, 1, &data_byte);
+		err = spi_nor_read_raw(nor, addr, 1, buf);
 		if (err) {
 			ret = ERR_PTR(err);
 			goto out;
@@ -2900,7 +2906,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
 		 * Build an index value that is used to select the Sector Map
 		 * Configuration that is currently in use.
 		 */
-		map_id = map_id << 1 | !!(data_byte & read_data_mask);
+		map_id = map_id << 1 | !!(*buf & read_data_mask);
 	}
 
 	/*
@@ -2931,6 +2937,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
 
 	/* fall through */
 out:
+	kfree(buf);
 	nor->addr_width = addr_width;
 	nor->read_dummy = read_dummy;
 	nor->read_opcode = read_opcode;
-- 
2.9.4

Re: [v2,4/5] mtd: spi_nor: pass DMA-able buffer to spi_nor_read_raw()

[Boris Brezillon]

On Fri, 2018-11-09 at 16:56:54 UTC,  wrote:
> spi_nor_read_raw() calls nor->read() which might be implemented
> by the m25p80 driver. m25p80 uses the spi-mem layer which requires
> DMA-able in/out buffers. Pass kmalloc'ed dma buffer to spi_nor_read_raw().
> 
> Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Applied to http://git.infradead.org/linux-mtd.git mtd/fixes, thanks.

Boris

[PATCH v2 5/5] mtd: spi-nor: remove unneeded smpt zeroization

[Tudor.Ambarus]

The entire smpt array is initialized with data read from sfdp,
there is no need to init it with zeroes before.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 458ca8321999..f9c657867e5a 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -3044,7 +3044,7 @@ static int spi_nor_parse_smpt(struct spi_nor *nor,
 
 	/* Read the Sector Map Parameter Table. */
 	len = smpt_header->length * sizeof(*smpt);
-	smpt = kzalloc(len, GFP_KERNEL);
+	smpt = kmalloc(len, GFP_KERNEL);
 	if (!smpt)
 		return -ENOMEM;
 
-- 
2.9.4

Re: [v2,5/5] mtd: spi-nor: remove unneeded smpt zeroization

[Boris Brezillon]

On Fri, 2018-11-09 at 16:56:56 UTC,  wrote:
> The entire smpt array is initialized with data read from sfdp,
> there is no need to init it with zeroes before.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Applied to http://git.infradead.org/linux-mtd.git spi-nor/next, thanks.

Boris