* Re: [PATCH][mtd-next] mtd: nand: remove redundant check of len
@ 2017-12-13 20:38 ` Boris Brezillon
0 siblings, 0 replies; 11+ messages in thread
From: Boris Brezillon @ 2017-12-13 20:38 UTC (permalink / raw)
To: Colin Ian King
Cc: Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut,
Cyrille Pitchen, linux-mtd, kernel-janitors, linux-kernel
On Wed, 13 Dec 2017 20:30:04 +0000
Colin Ian King <colin.king@canonical.com> wrote:
> On 13/12/17 20:24, Boris Brezillon wrote:
> > On Wed, 13 Dec 2017 20:17:43 +0000
> > Colin King <colin.king@canonical.com> wrote:
> >
> >> From: Colin Ian King <colin.king@canonical.com>
> >>
> >> The check of len being zero is redundant as it has already been
> >> sanity checked for this value at the start of the function. Hence
> >> it is impossible for this test to be true and so the redundant
> >> code can be removed.
> >
> > Nope, it's not the same test, the initial test is
> >
> > if (len && !buf)
>
> Ah, the current tip from linux-next has:
>
> 1912 if (!len || !buf)
> 1913 return -EINVAL;
>
> ..so I guess that's why it got picked up by static analysis.
Hm, that's weird, that's not what I see [1] in linux-next.
[1]https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/mtd/nand/nand_base.c#n1488
>
> >
> > not
> >
> > if (len)
> >
> > So this test is not redundant.
> >
> >>
> >> Detected by CoverityScan, CID#1462748 ("Logically dead code")
> >>
> >> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> >> ---
> >> drivers/mtd/nand/nand_base.c | 4 ----
> >> 1 file changed, 4 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> >> index afd5e18db81c..9daaa23db943 100644
> >> --- a/drivers/mtd/nand/nand_base.c
> >> +++ b/drivers/mtd/nand/nand_base.c
> >> @@ -1507,10 +1507,6 @@ static int nand_read_param_page_op(struct nand_chip *chip, u8 page, void *buf,
> >> };
> >> struct nand_operation op = NAND_OPERATION(instrs);
> >>
> >> - /* Drop the DATA_IN instruction if len is set to 0. */
> >> - if (!len)
> >> - op.ninstrs--;
> >> -
> >> return nand_exec_op(chip, &op);
> >> }
> >>
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH][mtd-next] mtd: nand: remove redundant check of len
2017-12-13 20:38 ` Boris Brezillon
(?)
@ 2017-12-13 20:44 ` Colin Ian King
2017-12-13 20:47 ` Boris Brezillon
-1 siblings, 1 reply; 11+ messages in thread
From: Colin Ian King @ 2017-12-13 20:44 UTC (permalink / raw)
To: Boris Brezillon
Cc: Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut,
Cyrille Pitchen, linux-mtd, kernel-janitors, linux-kernel
On 13/12/17 20:38, Boris Brezillon wrote:
> On Wed, 13 Dec 2017 20:30:04 +0000
> Colin Ian King <colin.king@canonical.com> wrote:
>
>> On 13/12/17 20:24, Boris Brezillon wrote:
>>> On Wed, 13 Dec 2017 20:17:43 +0000
>>> Colin King <colin.king@canonical.com> wrote:
>>>
>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>
>>>> The check of len being zero is redundant as it has already been
>>>> sanity checked for this value at the start of the function. Hence
>>>> it is impossible for this test to be true and so the redundant
>>>> code can be removed.
>>>
>>> Nope, it's not the same test, the initial test is
>>>
>>> if (len && !buf)
>>
>> Ah, the current tip from linux-next has:
>>
>> 1912 if (!len || !buf)
>> 1913 return -EINVAL;
>>
>> ..so I guess that's why it got picked up by static analysis.
>
> Hm, that's weird, that's not what I see [1] in linux-next.
I see my mistake, I fixed the *wrong* function, I'll send a v2. Doh.
>
> [1]https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/mtd/nand/nand_base.c#n1488
>
>>
>>>
>>> not
>>>
>>> if (len)
>>>
>>> So this test is not redundant.
>>>
>>>>
>>>> Detected by CoverityScan, CID#1462748 ("Logically dead code")
>>>>
>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>>> ---
>>>> drivers/mtd/nand/nand_base.c | 4 ----
>>>> 1 file changed, 4 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>>>> index afd5e18db81c..9daaa23db943 100644
>>>> --- a/drivers/mtd/nand/nand_base.c
>>>> +++ b/drivers/mtd/nand/nand_base.c
>>>> @@ -1507,10 +1507,6 @@ static int nand_read_param_page_op(struct nand_chip *chip, u8 page, void *buf,
>>>> };
>>>> struct nand_operation op = NAND_OPERATION(instrs);
>>>>
>>>> - /* Drop the DATA_IN instruction if len is set to 0. */
>>>> - if (!len)
>>>> - op.ninstrs--;
>>>> -
>>>> return nand_exec_op(chip, &op);
>>>> }
>>>>
>>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH][mtd-next] mtd: nand: remove redundant check of len
2017-12-13 20:44 ` Colin Ian King
@ 2017-12-13 20:47 ` Boris Brezillon
0 siblings, 0 replies; 11+ messages in thread
From: Boris Brezillon @ 2017-12-13 20:47 UTC (permalink / raw)
To: Colin Ian King
Cc: Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut,
Cyrille Pitchen, linux-mtd, kernel-janitors, linux-kernel
On Wed, 13 Dec 2017 20:44:45 +0000
Colin Ian King <colin.king@canonical.com> wrote:
> On 13/12/17 20:38, Boris Brezillon wrote:
> > On Wed, 13 Dec 2017 20:30:04 +0000
> > Colin Ian King <colin.king@canonical.com> wrote:
> >
> >> On 13/12/17 20:24, Boris Brezillon wrote:
> >>> On Wed, 13 Dec 2017 20:17:43 +0000
> >>> Colin King <colin.king@canonical.com> wrote:
> >>>
> >>>> From: Colin Ian King <colin.king@canonical.com>
> >>>>
> >>>> The check of len being zero is redundant as it has already been
> >>>> sanity checked for this value at the start of the function. Hence
> >>>> it is impossible for this test to be true and so the redundant
> >>>> code can be removed.
> >>>
> >>> Nope, it's not the same test, the initial test is
> >>>
> >>> if (len && !buf)
> >>
> >> Ah, the current tip from linux-next has:
> >>
> >> 1912 if (!len || !buf)
> >> 1913 return -EINVAL;
> >>
> >> ..so I guess that's why it got picked up by static analysis.
> >
> > Hm, that's weird, that's not what I see [1] in linux-next.
>
> I see my mistake, I fixed the *wrong* function, I'll send a v2. Doh.
Yep, just noticed that too. No need to send a patch though.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][mtd-next] mtd: nand: remove redundant check of len
@ 2017-12-13 20:47 ` Boris Brezillon
0 siblings, 0 replies; 11+ messages in thread
From: Boris Brezillon @ 2017-12-13 20:47 UTC (permalink / raw)
To: Colin Ian King
Cc: Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut,
Cyrille Pitchen, linux-mtd, kernel-janitors, linux-kernel
On Wed, 13 Dec 2017 20:44:45 +0000
Colin Ian King <colin.king@canonical.com> wrote:
> On 13/12/17 20:38, Boris Brezillon wrote:
> > On Wed, 13 Dec 2017 20:30:04 +0000
> > Colin Ian King <colin.king@canonical.com> wrote:
> >
> >> On 13/12/17 20:24, Boris Brezillon wrote:
> >>> On Wed, 13 Dec 2017 20:17:43 +0000
> >>> Colin King <colin.king@canonical.com> wrote:
> >>>
> >>>> From: Colin Ian King <colin.king@canonical.com>
> >>>>
> >>>> The check of len being zero is redundant as it has already been
> >>>> sanity checked for this value at the start of the function. Hence
> >>>> it is impossible for this test to be true and so the redundant
> >>>> code can be removed.
> >>>
> >>> Nope, it's not the same test, the initial test is
> >>>
> >>> if (len && !buf)
> >>
> >> Ah, the current tip from linux-next has:
> >>
> >> 1912 if (!len || !buf)
> >> 1913 return -EINVAL;
> >>
> >> ..so I guess that's why it got picked up by static analysis.
> >
> > Hm, that's weird, that's not what I see [1] in linux-next.
>
> I see my mistake, I fixed the *wrong* function, I'll send a v2. Doh.
Yep, just noticed that too. No need to send a patch though.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][mtd-next] mtd: nand: remove redundant check of len
2017-12-13 20:38 ` Boris Brezillon
@ 2017-12-13 20:46 ` Boris Brezillon
-1 siblings, 0 replies; 11+ messages in thread
From: Boris Brezillon @ 2017-12-13 20:46 UTC (permalink / raw)
To: Colin Ian King
Cc: Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut,
Cyrille Pitchen, linux-mtd, kernel-janitors, linux-kernel
On Wed, 13 Dec 2017 21:38:44 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> On Wed, 13 Dec 2017 20:30:04 +0000
> Colin Ian King <colin.king@canonical.com> wrote:
>
> > On 13/12/17 20:24, Boris Brezillon wrote:
> > > On Wed, 13 Dec 2017 20:17:43 +0000
> > > Colin King <colin.king@canonical.com> wrote:
> > >
> > >> From: Colin Ian King <colin.king@canonical.com>
> > >>
> > >> The check of len being zero is redundant as it has already been
> > >> sanity checked for this value at the start of the function. Hence
> > >> it is impossible for this test to be true and so the redundant
> > >> code can be removed.
> > >
> > > Nope, it's not the same test, the initial test is
> > >
> > > if (len && !buf)
> >
> > Ah, the current tip from linux-next has:
> >
> > 1912 if (!len || !buf)
> > 1913 return -EINVAL;
> >
> > ..so I guess that's why it got picked up by static analysis.
>
> Hm, that's weird, that's not what I see [1] in linux-next.
This being said, the test in nand_readid_op() is wrong [1], so maybe
this was the thing you were trying to fix.
No need to send a new patch, I'll squash the fix in the commit
introducing the function.
Thanks,
Boris
[1]http://code.bulix.org/kxivhd-240572
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][mtd-next] mtd: nand: remove redundant check of len
@ 2017-12-13 20:46 ` Boris Brezillon
0 siblings, 0 replies; 11+ messages in thread
From: Boris Brezillon @ 2017-12-13 20:46 UTC (permalink / raw)
To: Colin Ian King
Cc: Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut,
Cyrille Pitchen, linux-mtd, kernel-janitors, linux-kernel
On Wed, 13 Dec 2017 21:38:44 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> On Wed, 13 Dec 2017 20:30:04 +0000
> Colin Ian King <colin.king@canonical.com> wrote:
>
> > On 13/12/17 20:24, Boris Brezillon wrote:
> > > On Wed, 13 Dec 2017 20:17:43 +0000
> > > Colin King <colin.king@canonical.com> wrote:
> > >
> > >> From: Colin Ian King <colin.king@canonical.com>
> > >>
> > >> The check of len being zero is redundant as it has already been
> > >> sanity checked for this value at the start of the function. Hence
> > >> it is impossible for this test to be true and so the redundant
> > >> code can be removed.
> > >
> > > Nope, it's not the same test, the initial test is
> > >
> > > if (len && !buf)
> >
> > Ah, the current tip from linux-next has:
> >
> > 1912 if (!len || !buf)
> > 1913 return -EINVAL;
> >
> > ..so I guess that's why it got picked up by static analysis.
>
> Hm, that's weird, that's not what I see [1] in linux-next.
This being said, the test in nand_readid_op() is wrong [1], so maybe
this was the thing you were trying to fix.
No need to send a new patch, I'll squash the fix in the commit
introducing the function.
Thanks,
Boris
[1]http://code.bulix.org/kxivhd-240572
^ permalink raw reply [flat|nested] 11+ messages in thread