Re: [PATCH] mtd: spi-nor: Kill check with no effect

[Cyrille Pitchen]

Le 17/09/2017 à 16:13, Richard Weinberger a écrit :
> header.major is of type u8 and cannot be negative.
> 
> Detected by CoverityScan CID#1417858 ("Integer handling issues")
> 
> Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable
> Parameters (SFDP) tables")
> Signed-off-by: Richard Weinberger 

Applied to the spi-nor/next branch of l2-mtd

I've replaced header.major by header.minor in the commit message as
reported by Boris.

Thanks!

> ---
> Cyrille,
> 
> I'm not sure what exactly you wanted to test.
> Maybe it makes sense casting header.major to s8 before checking against < 0?
> 
> Thanks,
> //richard
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d71765739a93..4b86decdf13e 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2252,8 +2252,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  
>   /* Check the SFDP header version. */
>   if (le32_to_cpu(header.signature) != SFDP_SIGNATURE ||
> - header.major != SFDP_JESD216_MAJOR ||
> - header.minor < SFDP_JESD216_MINOR)
> + header.major != SFDP_JESD216_MAJOR)
>   return -EINVAL;
>  
>   /*
> 

Re: [PATCH] mtd: spi-nor: Kill check with no effect

[Cyrille Pitchen]

Le 17/09/2017 à 16:13, Richard Weinberger a écrit :
> header.major is of type u8 and cannot be negative.
> 
> Detected by CoverityScan CID#1417858 ("Integer handling issues")
> 
> Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable
> Parameters (SFDP) tables")
> Signed-off-by: Richard Weinberger 

Applied to the spi-nor/next branch of l2-mtd

I've replaced header.major by header.minor in the commit message as
reported by Boris.

Thanks!

> ---
> Cyrille,
> 
> I'm not sure what exactly you wanted to test.
> Maybe it makes sense casting header.major to s8 before checking against < 0?
> 
> Thanks,
> //richard
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d71765739a93..4b86decdf13e 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2252,8 +2252,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  
>   /* Check the SFDP header version. */
>   if (le32_to_cpu(header.signature) != SFDP_SIGNATURE ||
> - header.major != SFDP_JESD216_MAJOR ||
> - header.minor < SFDP_JESD216_MINOR)
> + header.major != SFDP_JESD216_MAJOR)
>   return -EINVAL;
>  
>   /*
> 

Re: [PATCH] mtd: spi-nor: Kill check with no effect

[Cyrille Pitchen]

Hi Richard,

Le 18/09/2017 à 11:44, Boris Brezillon a écrit :
> On Sun, 17 Sep 2017 16:13:52 +0200
> Richard Weinberger  wrote:
> 
>> header.major is of type u8 and cannot be negative.
> 
> I guess you meant header.minor here.
> 
>>
>> Detected by CoverityScan CID#1417858 ("Integer handling issues")
>>
>> Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable
>> Parameters (SFDP) tables")
>> Signed-off-by: Richard Weinberger 
>> ---
>> Cyrille,
>>
>> I'm not sure what exactly you wanted to test.
>> Maybe it makes sense casting header.major to s8 before checking against < 0?
>>

Actually I hesitated between checking the exact match with one of the 3
major.minor pairs defined for each version of the JESD216 specification:

JESD216: 1.0
JESD216 rev A: 1.5
JESD216 rev B (the latest one): 1.6

I was wondering whether I should accept 1.1, 1.2, 1.3 and 1.4
(unofficial) pairs too and also later versions, which should be backward
compatible with JESD216 rev B, like rev B is backward compatible with
rev A and so on...

Finally I've chosen to accept everything after 1.0 to give a chance to
next revisions to be supported as much as possible by the current code.
The major number should not change but actually the test upon the minor
value is useless as every unsigned number is greater than or equal to 0 ;)

So once Boris comment taken into account:

Acked-by: Cyrille Pitchen 

Best regards,

Cyrille

>> Thanks,
>> //richard
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index d71765739a93..4b86decdf13e 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -2252,8 +2252,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>>  
>>  /* Check the SFDP header version. */
>>  if (le32_to_cpu(header.signature) != SFDP_SIGNATURE ||
>> -header.major != SFDP_JESD216_MAJOR ||
>> -header.minor < SFDP_JESD216_MINOR)
>> +header.major != SFDP_JESD216_MAJOR)
>>  return -EINVAL;
>>  
>>  /*
> 
> 

Re: [PATCH] mtd: spi-nor: Kill check with no effect

[Cyrille Pitchen]

Hi Richard,

Le 18/09/2017 à 11:44, Boris Brezillon a écrit :
> On Sun, 17 Sep 2017 16:13:52 +0200
> Richard Weinberger  wrote:
> 
>> header.major is of type u8 and cannot be negative.
> 
> I guess you meant header.minor here.
> 
>>
>> Detected by CoverityScan CID#1417858 ("Integer handling issues")
>>
>> Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable
>> Parameters (SFDP) tables")
>> Signed-off-by: Richard Weinberger 
>> ---
>> Cyrille,
>>
>> I'm not sure what exactly you wanted to test.
>> Maybe it makes sense casting header.major to s8 before checking against < 0?
>>

Actually I hesitated between checking the exact match with one of the 3
major.minor pairs defined for each version of the JESD216 specification:

JESD216: 1.0
JESD216 rev A: 1.5
JESD216 rev B (the latest one): 1.6

I was wondering whether I should accept 1.1, 1.2, 1.3 and 1.4
(unofficial) pairs too and also later versions, which should be backward
compatible with JESD216 rev B, like rev B is backward compatible with
rev A and so on...

Finally I've chosen to accept everything after 1.0 to give a chance to
next revisions to be supported as much as possible by the current code.
The major number should not change but actually the test upon the minor
value is useless as every unsigned number is greater than or equal to 0 ;)

So once Boris comment taken into account:

Acked-by: Cyrille Pitchen 

Best regards,

Cyrille

>> Thanks,
>> //richard
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index d71765739a93..4b86decdf13e 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -2252,8 +2252,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>>  
>>  /* Check the SFDP header version. */
>>  if (le32_to_cpu(header.signature) != SFDP_SIGNATURE ||
>> -header.major != SFDP_JESD216_MAJOR ||
>> -header.minor < SFDP_JESD216_MINOR)
>> +header.major != SFDP_JESD216_MAJOR)
>>  return -EINVAL;
>>  
>>  /*
> 
> 

Re: [PATCH] mtd: spi-nor: Kill check with no effect

[Richard Weinberger]

Am Montag, 18. September 2017, 11:44:51 CEST schrieb Boris Brezillon:
> On Sun, 17 Sep 2017 16:13:52 +0200
> 
> Richard Weinberger  wrote:
> > header.major is of type u8 and cannot be negative.
> 
> I guess you meant header.minor here.

Correct. :)

Thanks,
//richard

Re: [PATCH] mtd: spi-nor: Kill check with no effect

[Richard Weinberger]

Am Montag, 18. September 2017, 11:44:51 CEST schrieb Boris Brezillon:
> On Sun, 17 Sep 2017 16:13:52 +0200
> 
> Richard Weinberger  wrote:
> > header.major is of type u8 and cannot be negative.
> 
> I guess you meant header.minor here.

Correct. :)

Thanks,
//richard

Re: [PATCH] mtd: spi-nor: Kill check with no effect

[Boris Brezillon]

On Sun, 17 Sep 2017 16:13:52 +0200
Richard Weinberger  wrote:

> header.major is of type u8 and cannot be negative.

I guess you meant header.minor here.

> 
> Detected by CoverityScan CID#1417858 ("Integer handling issues")
> 
> Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable
> Parameters (SFDP) tables")
> Signed-off-by: Richard Weinberger 
> ---
> Cyrille,
> 
> I'm not sure what exactly you wanted to test.
> Maybe it makes sense casting header.major to s8 before checking against < 0?
> 
> Thanks,
> //richard
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d71765739a93..4b86decdf13e 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2252,8 +2252,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  
>   /* Check the SFDP header version. */
>   if (le32_to_cpu(header.signature) != SFDP_SIGNATURE ||
> - header.major != SFDP_JESD216_MAJOR ||
> - header.minor < SFDP_JESD216_MINOR)
> + header.major != SFDP_JESD216_MAJOR)
>   return -EINVAL;
>  
>   /*

Re: [PATCH] mtd: spi-nor: Kill check with no effect

[Boris Brezillon]

On Sun, 17 Sep 2017 16:13:52 +0200
Richard Weinberger  wrote:

> header.major is of type u8 and cannot be negative.

I guess you meant header.minor here.

> 
> Detected by CoverityScan CID#1417858 ("Integer handling issues")
> 
> Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable
> Parameters (SFDP) tables")
> Signed-off-by: Richard Weinberger 
> ---
> Cyrille,
> 
> I'm not sure what exactly you wanted to test.
> Maybe it makes sense casting header.major to s8 before checking against < 0?
> 
> Thanks,
> //richard
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d71765739a93..4b86decdf13e 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2252,8 +2252,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  
>   /* Check the SFDP header version. */
>   if (le32_to_cpu(header.signature) != SFDP_SIGNATURE ||
> - header.major != SFDP_JESD216_MAJOR ||
> - header.minor < SFDP_JESD216_MINOR)
> + header.major != SFDP_JESD216_MAJOR)
>   return -EINVAL;
>  
>   /*

[PATCH] mtd: spi-nor: Kill check with no effect

[Richard Weinberger]

header.major is of type u8 and cannot be negative.

Detected by CoverityScan CID#1417858 ("Integer handling issues")

Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable
Parameters (SFDP) tables")
Signed-off-by: Richard Weinberger 
---
Cyrille,

I'm not sure what exactly you wanted to test.
Maybe it makes sense casting header.major to s8 before checking against < 0?

Thanks,
//richard
---
 drivers/mtd/spi-nor/spi-nor.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d71765739a93..4b86decdf13e 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2252,8 +2252,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 
/* Check the SFDP header version. */
if (le32_to_cpu(header.signature) != SFDP_SIGNATURE ||
-   header.major != SFDP_JESD216_MAJOR ||
-   header.minor < SFDP_JESD216_MINOR)
+   header.major != SFDP_JESD216_MAJOR)
return -EINVAL;
 
/*
-- 
2.13.5

[PATCH] mtd: spi-nor: Kill check with no effect

[Richard Weinberger]

header.major is of type u8 and cannot be negative.

Detected by CoverityScan CID#1417858 ("Integer handling issues")

Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable
Parameters (SFDP) tables")
Signed-off-by: Richard Weinberger 
---
Cyrille,

I'm not sure what exactly you wanted to test.
Maybe it makes sense casting header.major to s8 before checking against < 0?

Thanks,
//richard
---
 drivers/mtd/spi-nor/spi-nor.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d71765739a93..4b86decdf13e 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2252,8 +2252,7 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 
/* Check the SFDP header version. */
if (le32_to_cpu(header.signature) != SFDP_SIGNATURE ||
-   header.major != SFDP_JESD216_MAJOR ||
-   header.minor < SFDP_JESD216_MINOR)
+   header.major != SFDP_JESD216_MAJOR)
return -EINVAL;
 
/*
-- 
2.13.5