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.
Tudor Ambarus (7):
mtd: spi-nor: don't drop sfdp data if optional parsers fail
mtd: spi-nor: fix iteration over smpt array
mtd: spi-nor: add restriction for nmaps in smpt parser
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: ensure memory used for nor->read() is DMA safe
mtd: spi-nor: remove unneeded smpt zeroization
drivers/mtd/spi-nor/spi-nor.c | 98 ++++++++++++++++++++++++++++++++-----------
1 file changed, 74 insertions(+), 24 deletions(-)
--
2.9.4
Iterate over smpt array using its starting address and length
instead of the blindly 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.
Suggested-by: Boris Brezillon <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/mtd/spi-nor/spi-nor.c | 39 +++++++++++++++++++++++++++++----------
1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 2cdf96013689..59dcedb08691 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,10 @@ 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 +2896,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 +3044,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
The map selector is limited to a maximum of 8 bits, allowing
for a maximum of 256 possible map configurations. The total
number of map configurations should be addressable by the
total number of bits described by the detection commands.
For example: if there are five to eight possible sector map
configurations, at least three configuration detection commands
will be needed to extract three bits of configuration selection
information from the device in order to identify which configuration
is currently in use.
Suggested-by: Boris Brezillon <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/mtd/spi-nor/spi-nor.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 59dcedb08691..bd1866d714f2 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2868,7 +2868,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
const u32 *ret = NULL;
u32 addr;
int err;
- u8 i;
+ u8 i, ncmds, nmaps;
u8 addr_width, read_opcode, read_dummy;
u8 read_data_mask, data_byte, map_id;
@@ -2877,6 +2877,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
read_opcode = nor->read_opcode;
map_id = 0;
+ ncmds = 0;
/* Determine if there are any optional Detection Command Descriptors */
for (i = 0; i < smpt_len; i += 2) {
if (smpt[i] & SMPT_DESC_TYPE_MAP)
@@ -2896,6 +2897,7 @@ 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);
+ ncmds++;
}
/*
@@ -2905,7 +2907,16 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
*
* Find the matching configuration map.
*/
- while (i < smpt_len) {
+ for (nmaps = 0; i < smpt_len; nmaps++) {
+ /*
+ * The map selector is limited to a maximum of 8 bits, allowing
+ * for a maximum of 256 possible map configurations. The total
+ * number of map configurations should be addressable by the
+ * total number of bits described by the detection commands.
+ */
+ if (ncmds && nmaps >= (1 << (ncmds + 1)))
+ break;
+
if (SMPT_MAP_ID(smpt[i]) == map_id) {
ret = smpt + i;
break;
--
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: 5390a8df769e ("mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories")
Reported-by: Yogesh Gaur <[email protected]>
Suggested-by: Boris Brezillon <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
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
Use GFP_DMA to ensure that the memory we allocate for transfers in
nor->read() can be DMAed.
spi_nor_read_sfdp() calls spi_nor_read_raw(), which calls nor-read().
The latter might be implemented by the m25p80 driver which is on top
of the spi-mem layer. spi-mem requires DMA-able in/out buffers, let's
make sure they are.
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/mtd/spi-nor/spi-nor.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 2eaa21c85483..a13fc82bade5 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2238,7 +2238,7 @@ static int spi_nor_read_sfdp_dma_unsafe(struct spi_nor *nor, u32 addr,
void *dma_safe_buf;
int ret;
- dma_safe_buf = kmalloc(len, GFP_KERNEL);
+ dma_safe_buf = kmalloc(len, GFP_KERNEL | GFP_DMA);
if (!dma_safe_buf)
return -ENOMEM;
@@ -3053,7 +3053,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 = kzalloc(len, GFP_KERNEL | GFP_DMA);
if (!smpt)
return -ENOMEM;
@@ -3140,7 +3140,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
if (header.nph) {
psize = header.nph * sizeof(*param_headers);
- param_headers = kmalloc(psize, GFP_KERNEL);
+ param_headers = kmalloc(psize, GFP_KERNEL | GFP_DMA);
if (!param_headers)
return -ENOMEM;
--
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 a13fc82bade5..c71e1da1f562 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -3053,7 +3053,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 | GFP_DMA);
+ smpt = kmalloc(len, GFP_KERNEL | GFP_DMA);
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 bd1866d714f2..79ca1102faed 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, ncmds, nmaps;
@@ -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
@@ -2907,6 +2911,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);
for (nmaps = 0; i < smpt_len; nmaps++) {
/*
* The map selector is limited to a maximum of 8 bits, allowing
@@ -3056,8 +3061,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
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().
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/mtd/spi-nor/spi-nor.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 79ca1102faed..2eaa21c85483 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,16 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
u8 smpt_len)
{
const u32 *ret;
+ u8 *dma_safe;
u32 addr;
int err;
u8 i, ncmds, nmaps;
u8 addr_width, read_opcode, read_dummy;
- u8 read_data_mask, data_byte, map_id;
+ u8 read_data_mask, map_id;
+
+ dma_safe = kmalloc(sizeof(*dma_safe), GFP_KERNEL | GFP_DMA);
+ if (!dma_safe)
+ return ERR_PTR(-ENOMEM);
addr_width = nor->addr_width;
read_dummy = nor->read_dummy;
@@ -2890,7 +2895,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, dma_safe);
if (err) {
ret = ERR_PTR(err);
goto out;
@@ -2900,7 +2905,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 | !!(*dma_safe & read_data_mask);
ncmds++;
}
@@ -2941,6 +2946,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
/* fall through */
out:
+ kfree(dma_safe);
nor->addr_width = addr_width;
nor->read_dummy = read_dummy;
nor->read_opcode = read_opcode;
--
2.9.4
On Thu, 8 Nov 2018 11:07:09 +0000
<[email protected]> wrote:
> Iterate over smpt array using its starting address and length
> instead of the blindly iterations that used data found in the array.
^blind
>
> This prevents possible memory accesses outside of the smpt array
> boundaries in case software, or manufacturers, misrepresent smpt
> array fields.
>
> Suggested-by: Boris Brezillon <[email protected]>
> Signed-off-by: Tudor Ambarus <[email protected]>
I think we should consider this patch as a fix. Would you mind adding a
Fixes tag?
> ---
> drivers/mtd/spi-nor/spi-nor.c | 39 +++++++++++++++++++++++++++++----------
> 1 file changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 2cdf96013689..59dcedb08691 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,10 @@ 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;
nit: add a blank line here.
> 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 +2896,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 +3044,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;
On Thu, 8 Nov 2018 11:07:11 +0000
<[email protected]> wrote:
> The map selector is limited to a maximum of 8 bits, allowing
> for a maximum of 256 possible map configurations. The total
> number of map configurations should be addressable by the
> total number of bits described by the detection commands.
>
> For example: if there are five to eight possible sector map
> configurations, at least three configuration detection commands
> will be needed to extract three bits of configuration selection
> information from the device in order to identify which configuration
> is currently in use.
>
> Suggested-by: Boris Brezillon <[email protected]>
I don't remember suggesting this :-).
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
> drivers/mtd/spi-nor/spi-nor.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 59dcedb08691..bd1866d714f2 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2868,7 +2868,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> const u32 *ret = NULL;
> u32 addr;
> int err;
> - u8 i;
> + u8 i, ncmds, nmaps;
> u8 addr_width, read_opcode, read_dummy;
> u8 read_data_mask, data_byte, map_id;
>
> @@ -2877,6 +2877,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> read_opcode = nor->read_opcode;
>
> map_id = 0;
> + ncmds = 0;
> /* Determine if there are any optional Detection Command Descriptors */
> for (i = 0; i < smpt_len; i += 2) {
> if (smpt[i] & SMPT_DESC_TYPE_MAP)
> @@ -2896,6 +2897,7 @@ 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);
> + ncmds++;
> }
>
> /*
> @@ -2905,7 +2907,16 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> *
> * Find the matching configuration map.
> */
> - while (i < smpt_len) {
> + for (nmaps = 0; i < smpt_len; nmaps++) {
> + /*
> + * The map selector is limited to a maximum of 8 bits, allowing
> + * for a maximum of 256 possible map configurations. The total
> + * number of map configurations should be addressable by the
> + * total number of bits described by the detection commands.
> + */
> + if (ncmds && nmaps >= (1 << (ncmds + 1)))
> + break;
> +
Maybe I missed something but it sounds like this change is just
optimizing the SPMT parsing a bit, and to be honest, I'm not sure this
is really needed. Most of the time, smpt_len will be rather small, so
trying to bail out earlier is not bringing much perf improvements.
> if (SMPT_MAP_ID(smpt[i]) == map_id) {
> ret = smpt + i;
> break;
On Thu, 8 Nov 2018 11:07:16 +0000
<[email protected]> 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().
>
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
> drivers/mtd/spi-nor/spi-nor.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 79ca1102faed..2eaa21c85483 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,16 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> u8 smpt_len)
> {
> const u32 *ret;
> + u8 *dma_safe;
I'd prefer to have it named buf, data_buf or scratch_buf...
> u32 addr;
> int err;
> u8 i, ncmds, nmaps;
> u8 addr_width, read_opcode, read_dummy;
> - u8 read_data_mask, data_byte, map_id;
> + u8 read_data_mask, map_id;
> +
... and have a comment here explaining why your allocating the buffer
instead of using a local var placed on the stack.
> + dma_safe = kmalloc(sizeof(*dma_safe), GFP_KERNEL | GFP_DMA);
Please don't use GFP_DMA, kmalloc() should already return a DMA-safe
buffer (see [1]).
> + if (!dma_safe)
> + return ERR_PTR(-ENOMEM);
>
> addr_width = nor->addr_width;
> read_dummy = nor->read_dummy;
> @@ -2890,7 +2895,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, dma_safe);
> if (err) {
> ret = ERR_PTR(err);
> goto out;
> @@ -2900,7 +2905,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 | !!(*dma_safe & read_data_mask);
> ncmds++;
> }
>
> @@ -2941,6 +2946,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>
> /* fall through */
> out:
> + kfree(dma_safe);
> nor->addr_width = addr_width;
> nor->read_dummy = read_dummy;
> nor->read_opcode = read_opcode;
[1]https://elixir.bootlin.com/linux/latest/source/include/linux/gfp.h#L263
On Thu, 8 Nov 2018 11:07:18 +0000
<[email protected]> wrote:
> Use GFP_DMA to ensure that the memory we allocate for transfers in
> nor->read() can be DMAed.
See my comment on patch 5.
On Thu, 8 Nov 2018 13:54:47 +0100
Boris Brezillon <[email protected]> wrote:
> > - while (i < smpt_len) {
> > + for (nmaps = 0; i < smpt_len; nmaps++) {
> > + /*
> > + * The map selector is limited to a maximum of 8 bits, allowing
> > + * for a maximum of 256 possible map configurations. The total
> > + * number of map configurations should be addressable by the
> > + * total number of bits described by the detection commands.
> > + */
> > + if (ncmds && nmaps >= (1 << (ncmds + 1)))
> > + break;
> > +
>
> Maybe I missed something but it sounds like this change is just
> optimizing the SPMT parsing a bit, and to be honest, I'm not sure this
> is really needed. Most of the time, smpt_len will be rather small, so
> trying to bail out earlier is not bringing much perf improvements.
To make it clearer, I don't think the extra complexity is worth the tiny
perf improvement.
On 11/08/2018 02:54 PM, Boris Brezillon wrote:
> On Thu, 8 Nov 2018 11:07:11 +0000
> <[email protected]> wrote:
>
>> The map selector is limited to a maximum of 8 bits, allowing
>> for a maximum of 256 possible map configurations. The total
>> number of map configurations should be addressable by the
>> total number of bits described by the detection commands.
>>
>> For example: if there are five to eight possible sector map
>> configurations, at least three configuration detection commands
>> will be needed to extract three bits of configuration selection
>> information from the device in order to identify which configuration
>> is currently in use.
>>
>> Suggested-by: Boris Brezillon <[email protected]>
>
> I don't remember suggesting this :-).
I see this when you discussed with Yogesh:
+ for (nmaps = 0; i< smpt_len; nmaps++) {
+ if(!(smpt[i] & SMPT_DESC_TYPE_MAP)) {
+ i += 2;
+ continue;
+ }
+
+ if(!map_id_is_valid) {
+ if (nmaps) {
+ ret = NULL;
+ break;
+ }
If I understand correctly, you meant that if there are no detection commands,
and more than a configuration map, then return an error.
This is correct in my opinion. What I did was to generalize this idea. If smtp
defines more maps than you can select using detection commands, return error.
+
+ ret = smpt+i;
+ } else if (map_id == SMPT_MAP_ID(smpt[i])) {
+ ret = smpt+i;
+ break;
+ }
+
/* increment the table index to the next map */
i += SMPT_MAP_REGION_COUNT(smpt[i]) + 1;
}
>
>> Signed-off-by: Tudor Ambarus <[email protected]>
>> ---
>> drivers/mtd/spi-nor/spi-nor.c | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 59dcedb08691..bd1866d714f2 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -2868,7 +2868,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>> const u32 *ret = NULL;
>> u32 addr;
>> int err;
>> - u8 i;
>> + u8 i, ncmds, nmaps;
>> u8 addr_width, read_opcode, read_dummy;
>> u8 read_data_mask, data_byte, map_id;
>>
>> @@ -2877,6 +2877,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>> read_opcode = nor->read_opcode;
>>
>> map_id = 0;
>> + ncmds = 0;
>> /* Determine if there are any optional Detection Command Descriptors */
>> for (i = 0; i < smpt_len; i += 2) {
>> if (smpt[i] & SMPT_DESC_TYPE_MAP)
>> @@ -2896,6 +2897,7 @@ 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);
>> + ncmds++;
>> }
>>
>> /*
>> @@ -2905,7 +2907,16 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>> *
>> * Find the matching configuration map.
>> */
>> - while (i < smpt_len) {
>> + for (nmaps = 0; i < smpt_len; nmaps++) {
>> + /*
>> + * The map selector is limited to a maximum of 8 bits, allowing
>> + * for a maximum of 256 possible map configurations. The total
>> + * number of map configurations should be addressable by the
>> + * total number of bits described by the detection commands.
>> + */
>> + if (ncmds && nmaps >= (1 << (ncmds + 1)))
>> + break;
>> +
>
> Maybe I missed something but it sounds like this change is just
> optimizing the SPMT parsing a bit, and to be honest, I'm not sure this
> is really needed. Most of the time, smpt_len will be rather small, so
> trying to bail out earlier is not bringing much perf improvements.
It's rather a smtp validity check. I want to return an error if there are not
enough detection commands to identify the map id.
Cheers,
ta
On Thu, 8 Nov 2018 13:58:45 +0000
<[email protected]> wrote:
> On 11/08/2018 02:54 PM, Boris Brezillon wrote:
> > On Thu, 8 Nov 2018 11:07:11 +0000
> > <[email protected]> wrote:
> >
> >> The map selector is limited to a maximum of 8 bits, allowing
> >> for a maximum of 256 possible map configurations. The total
> >> number of map configurations should be addressable by the
> >> total number of bits described by the detection commands.
> >>
> >> For example: if there are five to eight possible sector map
> >> configurations, at least three configuration detection commands
> >> will be needed to extract three bits of configuration selection
> >> information from the device in order to identify which configuration
> >> is currently in use.
> >>
> >> Suggested-by: Boris Brezillon <[email protected]>
> >
> > I don't remember suggesting this :-).
>
> I see this when you discussed with Yogesh:
>
> + for (nmaps = 0; i< smpt_len; nmaps++) {
> + if(!(smpt[i] & SMPT_DESC_TYPE_MAP)) {
> + i += 2;
> + continue;
> + }
> +
> + if(!map_id_is_valid) {
> + if (nmaps) {
> + ret = NULL;
> + break;
> + }
>
> If I understand correctly, you meant that if there are no detection commands,
> and more than a configuration map, then return an error.
Yes, because in this case we have no way to know what config is
currently selected.
>
> This is correct in my opinion. What I did was to generalize this idea. If smtp
> defines more maps than you can select using detection commands, return error.
AFAICT you're no longer checking that map_id is valid (see below).
>
> +
> + ret = smpt+i;
> + } else if (map_id == SMPT_MAP_ID(smpt[i])) {
> + ret = smpt+i;
> + break;
> + }
> +
> /* increment the table index to the next map */
> i += SMPT_MAP_REGION_COUNT(smpt[i]) + 1;
> }
>
> >
> >> Signed-off-by: Tudor Ambarus <[email protected]>
> >> ---
> >> drivers/mtd/spi-nor/spi-nor.c | 15 +++++++++++++--
> >> 1 file changed, 13 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> >> index 59dcedb08691..bd1866d714f2 100644
> >> --- a/drivers/mtd/spi-nor/spi-nor.c
> >> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >> @@ -2868,7 +2868,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> >> const u32 *ret = NULL;
> >> u32 addr;
> >> int err;
> >> - u8 i;
> >> + u8 i, ncmds, nmaps;
> >> u8 addr_width, read_opcode, read_dummy;
> >> u8 read_data_mask, data_byte, map_id;
> >>
> >> @@ -2877,6 +2877,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> >> read_opcode = nor->read_opcode;
> >>
> >> map_id = 0;
> >> + ncmds = 0;
> >> /* Determine if there are any optional Detection Command Descriptors */
> >> for (i = 0; i < smpt_len; i += 2) {
> >> if (smpt[i] & SMPT_DESC_TYPE_MAP)
> >> @@ -2896,6 +2897,7 @@ 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);
> >> + ncmds++;
> >> }
> >>
> >> /*
> >> @@ -2905,7 +2907,16 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
> >> *
> >> * Find the matching configuration map.
> >> */
> >> - while (i < smpt_len) {
> >> + for (nmaps = 0; i < smpt_len; nmaps++) {
> >> + /*
> >> + * The map selector is limited to a maximum of 8 bits, allowing
> >> + * for a maximum of 256 possible map configurations. The total
> >> + * number of map configurations should be addressable by the
> >> + * total number of bits described by the detection commands.
> >> + */
> >> + if (ncmds && nmaps >= (1 << (ncmds + 1)))
> >> + break;
You're no longer checking that when ncmds == 0 nmaps has to be 1. So,
say the chip exposed no smpt commands and has several maps defined,
map_id will be 0 while it should have be "undefined". My version
would return an error, but yours tries to find map_id 0.
> >> +
> >
> > Maybe I missed something but it sounds like this change is just
> > optimizing the SPMT parsing a bit, and to be honest, I'm not sure this
> > is really needed. Most of the time, smpt_len will be rather small, so
> > trying to bail out earlier is not bringing much perf improvements.
>
> It's rather a smtp validity check. I want to return an error if there are not
> enough detection commands to identify the map id.
You would have failed the same way without this validity check after a
maximum of smpt_len iterations, right?
On 11/08/2018 04:15 PM, Boris Brezillon wrote:
> On Thu, 8 Nov 2018 13:58:45 +0000
> <[email protected]> wrote:
>
>> On 11/08/2018 02:54 PM, Boris Brezillon wrote:
>>> On Thu, 8 Nov 2018 11:07:11 +0000
>>> <[email protected]> wrote:
>>>
>>>> The map selector is limited to a maximum of 8 bits, allowing
>>>> for a maximum of 256 possible map configurations. The total
>>>> number of map configurations should be addressable by the
>>>> total number of bits described by the detection commands.
>>>>
>>>> For example: if there are five to eight possible sector map
>>>> configurations, at least three configuration detection commands
>>>> will be needed to extract three bits of configuration selection
>>>> information from the device in order to identify which configuration
>>>> is currently in use.
>>>>
>>>> Suggested-by: Boris Brezillon <[email protected]>
>>>
>>> I don't remember suggesting this :-).
>>
>> I see this when you discussed with Yogesh:
>>
>> + for (nmaps = 0; i< smpt_len; nmaps++) {
>> + if(!(smpt[i] & SMPT_DESC_TYPE_MAP)) {
>> + i += 2;
>> + continue;
>> + }
>> +
>> + if(!map_id_is_valid) {
>> + if (nmaps) {
>> + ret = NULL;
>> + break;
>> + }
>>
>> If I understand correctly, you meant that if there are no detection commands,
>> and more than a configuration map, then return an error.
>
> Yes, because in this case we have no way to know what config is
> currently selected.
>
>>
>> This is correct in my opinion. What I did was to generalize this idea. If smtp
>> defines more maps than you can select using detection commands, return error.
>
> AFAICT you're no longer checking that map_id is valid (see below).
>
>>
>> +
>> + ret = smpt+i;
>> + } else if (map_id == SMPT_MAP_ID(smpt[i])) {
>> + ret = smpt+i;
>> + break;
>> + }
>> +
>> /* increment the table index to the next map */
>> i += SMPT_MAP_REGION_COUNT(smpt[i]) + 1;
>> }
>>
>>>
>>>> Signed-off-by: Tudor Ambarus <[email protected]>
>>>> ---
>>>> drivers/mtd/spi-nor/spi-nor.c | 15 +++++++++++++--
>>>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>>> index 59dcedb08691..bd1866d714f2 100644
>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>> @@ -2868,7 +2868,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>> const u32 *ret = NULL;
>>>> u32 addr;
>>>> int err;
>>>> - u8 i;
>>>> + u8 i, ncmds, nmaps;
>>>> u8 addr_width, read_opcode, read_dummy;
>>>> u8 read_data_mask, data_byte, map_id;
>>>>
>>>> @@ -2877,6 +2877,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>> read_opcode = nor->read_opcode;
>>>>
>>>> map_id = 0;
>>>> + ncmds = 0;
>>>> /* Determine if there are any optional Detection Command Descriptors */
>>>> for (i = 0; i < smpt_len; i += 2) {
>>>> if (smpt[i] & SMPT_DESC_TYPE_MAP)
>>>> @@ -2896,6 +2897,7 @@ 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);
>>>> + ncmds++;
>>>> }
>>>>
>>>> /*
>>>> @@ -2905,7 +2907,16 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>> *
>>>> * Find the matching configuration map.
>>>> */
>>>> - while (i < smpt_len) {
>>>> + for (nmaps = 0; i < smpt_len; nmaps++) {
>>>> + /*
>>>> + * The map selector is limited to a maximum of 8 bits, allowing
>>>> + * for a maximum of 256 possible map configurations. The total
>>>> + * number of map configurations should be addressable by the
>>>> + * total number of bits described by the detection commands.
>>>> + */
>>>> + if (ncmds && nmaps >= (1 << (ncmds + 1)))
>>>> + break;
>
> You're no longer checking that when ncmds == 0 nmaps has to be 1. So,
> say the chip exposed no smpt commands and has several maps defined,
> map_id will be 0 while it should have be "undefined". My version
> would return an error, but yours tries to find map_id 0.
yep, I missed the ncmds == 0 case.
>
>>>> +
>>>
>>> Maybe I missed something but it sounds like this change is just
>>> optimizing the SPMT parsing a bit, and to be honest, I'm not sure this
>>> is really needed. Most of the time, smpt_len will be rather small, so
>>> trying to bail out earlier is not bringing much perf improvements.
>>
>> It's rather a smtp validity check. I want to return an error if there are not
>> enough detection commands to identify the map id.
>
> You would have failed the same way without this validity check after a
> maximum of smpt_len iterations, right?
>
Right. The correct fix would be to count nmaps in a loop, then do these checks,
and if all ok, search for the map_id in another loop :).
On Thu, 8 Nov 2018 14:48:11 +0000
<[email protected]> wrote:
> >>>> +
> >>>
> >>> Maybe I missed something but it sounds like this change is just
> >>> optimizing the SPMT parsing a bit, and to be honest, I'm not sure this
> >>> is really needed. Most of the time, smpt_len will be rather small, so
> >>> trying to bail out earlier is not bringing much perf improvements.
> >>
> >> It's rather a smtp validity check. I want to return an error if there are not
> >> enough detection commands to identify the map id.
> >
> > You would have failed the same way without this validity check after a
> > maximum of smpt_len iterations, right?
> >
>
> Right. The correct fix would be to count nmaps in a loop, then do these checks,
> and if all ok, search for the map_id in another loop :).
Or just error out when !ncmds && nmaps > 1.
If you insist on keeping the ncmds && nmaps >= (1 << (ncmds + 1))
check, that's fine, but it's not replacing the consistency check I was
doing ;-).
On 11/08/2018 04:54 PM, Boris Brezillon wrote:
> On Thu, 8 Nov 2018 14:48:11 +0000
> <[email protected]> wrote:
>
>>>>>> +
>>>>>
>>>>> Maybe I missed something but it sounds like this change is just
>>>>> optimizing the SPMT parsing a bit, and to be honest, I'm not sure this
>>>>> is really needed. Most of the time, smpt_len will be rather small, so
>>>>> trying to bail out earlier is not bringing much perf improvements.
>>>>
>>>> It's rather a smtp validity check. I want to return an error if there are not
>>>> enough detection commands to identify the map id.
>>>
>>> You would have failed the same way without this validity check after a
>>> maximum of smpt_len iterations, right?
>>>
>>
>> Right. The correct fix would be to count nmaps in a loop, then do these checks,
>> and if all ok, search for the map_id in another loop :).
>
> Or just error out when !ncmds && nmaps > 1.
Solves partially the problem.
>
> If you insist on keeping the ncmds && nmaps >= (1 << (ncmds + 1))
> check, that's fine, but it's not replacing the consistency check I was
> doing ;-).
>
I don't have a strong opinion on this, we can live without these checks as well.