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
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 <[email protected]>
---
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
Don't overwrite the errno from spi_nor_read_raw().
Signed-off-by: Tudor Ambarus <[email protected]>
---
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
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 <[email protected]>
Suggested-by: Boris Brezillon <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
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 = ¶m_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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
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
> -----Original Message-----
> From: [email protected] [mailto:[email protected]]
> Sent: Friday, November 9, 2018 10:27 PM
> To: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; Yogesh
> Narayan Gaur <[email protected]>; [email protected];
> [email protected]
> 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 <[email protected]>
Tested-by: Yogesh Gaur <[email protected]>
> Suggested-by: Boris Brezillon <[email protected]>
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
> 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 = ¶m_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
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 <[email protected]>
Applied to http://git.infradead.org/linux-mtd.git mtd/fixes, thanks.
Boris
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 <[email protected]>
Applied to http://git.infradead.org/linux-mtd.git mtd/fixes, thanks.
Boris
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 <[email protected]>
> Signed-off-by: Tudor Ambarus <[email protected]>
Applied to http://git.infradead.org/linux-mtd.git mtd/fixes, thanks.
Boris
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 <[email protected]>
> Suggested-by: Boris Brezillon <[email protected]>
> Signed-off-by: Tudor Ambarus <[email protected]>
Applied to http://git.infradead.org/linux-mtd.git mtd/fixes, thanks.
Boris
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 <[email protected]>
Applied to http://git.infradead.org/linux-mtd.git spi-nor/next, thanks.
Boris