| Message ID | 20181108110653.21063-4-tudor.ambarus@microchip.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mtd: spi-nor: fixes found when debugging smpt | expand |
On Thu, 8 Nov 2018 11:07:11 +0000 <Tudor.Ambarus@microchip.com> 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 <boris.brezillon@bootlin.com> I don't remember suggesting this :-). > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > 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 13:54:47 +0100 Boris Brezillon <boris.brezillon@bootlin.com> 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 > <Tudor.Ambarus@microchip.com> 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 <boris.brezillon@bootlin.com> > > 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 <tudor.ambarus@microchip.com> >> --- >> 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 <Tudor.Ambarus@microchip.com> wrote: > On 11/08/2018 02:54 PM, Boris Brezillon wrote: > > On Thu, 8 Nov 2018 11:07:11 +0000 > > <Tudor.Ambarus@microchip.com> 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 <boris.brezillon@bootlin.com> > > > > 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 <tudor.ambarus@microchip.com> > >> --- > >> 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 > <Tudor.Ambarus@microchip.com> wrote: > >> On 11/08/2018 02:54 PM, Boris Brezillon wrote: >>> On Thu, 8 Nov 2018 11:07:11 +0000 >>> <Tudor.Ambarus@microchip.com> 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 <boris.brezillon@bootlin.com> >>> >>> 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 <tudor.ambarus@microchip.com> >>>> --- >>>> 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 <Tudor.Ambarus@microchip.com> 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 > <Tudor.Ambarus@microchip.com> 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.
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;
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 <boris.brezillon@bootlin.com> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/mtd/spi-nor/spi-nor.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)