From: Colin Ian King <[email protected]>
The comparison of hcd->irq to less than zero for an error check will
never be true because hcd->irq is an unsigned int. Fix this by
assigning the int retval to the return of platform_get_irq and checking
this for the -ve error condition and assigning hcd->irq to retval.
Addresses-Coverity: ("Unsigned compared against 0")
Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
Signed-off-by: Colin Ian King <[email protected]>
---
drivers/usb/host/ehci-mv.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/ehci-mv.c b/drivers/usb/host/ehci-mv.c
index 0d61f43c29dc..cffdc8d01b2a 100644
--- a/drivers/usb/host/ehci-mv.c
+++ b/drivers/usb/host/ehci-mv.c
@@ -166,11 +166,10 @@ static int mv_ehci_probe(struct platform_device *pdev)
hcd->rsrc_len = resource_size(r);
hcd->regs = ehci_mv->op_regs;
- hcd->irq = platform_get_irq(pdev, 0);
- if (hcd->irq < 0) {
- retval = hcd->irq;
+ retval = platform_get_irq(pdev, 0);
+ if (retval < 0)
goto err_disable_clk;
- }
+ hcd->irq = retval;
ehci = hcd_to_ehci(hcd);
ehci->caps = (struct ehci_caps __iomem *) ehci_mv->cap_regs;
--
2.25.1
On Fri, May 15, 2020 at 05:54:53PM +0100, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> The comparison of hcd->irq to less than zero for an error check will
> never be true because hcd->irq is an unsigned int. Fix this by
> assigning the int retval to the return of platform_get_irq and checking
> this for the -ve error condition and assigning hcd->irq to retval.
>
> Addresses-Coverity: ("Unsigned compared against 0")
> Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
> Signed-off-by: Colin Ian King <[email protected]>
> ---
Thanks to Coverity for spotting this. Any reason why it didn't spot
exactly the same mistake in the ohci-da8xx.c driver?
Also, why wasn't the patch CC'ed for the stable series?
Alan Stern
On 15/05/2020 18:21, Alan Stern wrote:
> On Fri, May 15, 2020 at 05:54:53PM +0100, Colin King wrote:
>> From: Colin Ian King <[email protected]>
>>
>> The comparison of hcd->irq to less than zero for an error check will
>> never be true because hcd->irq is an unsigned int. Fix this by
>> assigning the int retval to the return of platform_get_irq and checking
>> this for the -ve error condition and assigning hcd->irq to retval.
>>
>> Addresses-Coverity: ("Unsigned compared against 0")
>> Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
>> Signed-off-by: Colin Ian King <[email protected]>
>> ---
>
> Thanks to Coverity for spotting this. Any reason why it didn't spot
> exactly the same mistake in the ohci-da8xx.c driver?
No idea, it is curious that it can spot one error but miss another.
Sometimes I see these issues on the next scan, so it maybe the database
diff'ing is awry.
>
> Also, why wasn't the patch CC'ed for the stable series?
My bad on that. Human error
>
> Alan Stern
>
On Fri, May 15, 2020 at 06:26:04PM +0100, Colin Ian King wrote:
> On 15/05/2020 18:21, Alan Stern wrote:
> > On Fri, May 15, 2020 at 05:54:53PM +0100, Colin King wrote:
> >> From: Colin Ian King <[email protected]>
> >>
> >> The comparison of hcd->irq to less than zero for an error check will
> >> never be true because hcd->irq is an unsigned int. Fix this by
> >> assigning the int retval to the return of platform_get_irq and checking
> >> this for the -ve error condition and assigning hcd->irq to retval.
> >>
> >> Addresses-Coverity: ("Unsigned compared against 0")
> >> Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
> >> Signed-off-by: Colin Ian King <[email protected]>
> >> ---
> >
> > Thanks to Coverity for spotting this. Any reason why it didn't spot
> > exactly the same mistake in the ohci-da8xx.c driver?
>
> No idea, it is curious that it can spot one error but miss another.
> Sometimes I see these issues on the next scan, so it maybe the database
> diff'ing is awry.
>
> >
> > Also, why wasn't the patch CC'ed for the stable series?
>
> My bad on that. Human error
Actually the question itself was my mistake. I didn't notice that your
patch was a fix to something that was just merged and hadn't been CC'ed
to stable.
Alan Stern
Hi Alan & Colin：
On 2020/5/16 1:21, Alan Stern wrote:
> On Fri, May 15, 2020 at 05:54:53PM +0100, Colin King wrote:
>> From: Colin Ian King <[email protected]>
>>
>> The comparison of hcd->irq to less than zero for an error check will
>> never be true because hcd->irq is an unsigned int. Fix this by
>> assigning the int retval to the return of platform_get_irq and checking
>> this for the -ve error condition and assigning hcd->irq to retval.
>>
>> Addresses-Coverity: ("Unsigned compared against 0")
>> Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
>> Signed-off-by: Colin Ian King <[email protected]>
>> ---
> Thanks to Coverity for spotting this. Any reason why it didn't spot
> exactly the same mistake in the ohci-da8xx.c driver?
I just looked at the code and wondered whether it would be more
appropriate to modify the header file "hcd.h". Since 'irq' might be an
negative, why not just modify the variables in the 'struct usb_hcd',
'unsigned int irq'--> 'int irq'? After all, it's a public one.
Thanks,
Tang Bin
>
>
On Sat, May 16, 2020 at 07:23:42AM +0800, Tang Bin wrote:
> Hi Alan & Colin：
>
> On 2020/5/16 1:21, Alan Stern wrote:
> > On Fri, May 15, 2020 at 05:54:53PM +0100, Colin King wrote:
> > > From: Colin Ian King <[email protected]>
> > >
> > > The comparison of hcd->irq to less than zero for an error check will
> > > never be true because hcd->irq is an unsigned int. Fix this by
> > > assigning the int retval to the return of platform_get_irq and checking
> > > this for the -ve error condition and assigning hcd->irq to retval.
> > >
> > > Addresses-Coverity: ("Unsigned compared against 0")
> > > Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
> > > Signed-off-by: Colin Ian King <[email protected]>
> > > ---
> > Thanks to Coverity for spotting this. Any reason why it didn't spot
> > exactly the same mistake in the ohci-da8xx.c driver?
>
> I just looked at the code and wondered whether it would be more appropriate
> to modify the header file "hcd.h". Since 'irq' might be an negative, why
> not just modify the variables in the 'struct usb_hcd', 'unsigned int
> irq'--> 'int irq'? After all, it's a public one.
I think the code in the drivers should be changed. The reason is if
platform_get_irq() returns a negative value, then that value should be
what the probe function returns.
Alan Stern
On Fri, May 15, 2020 at 01:21:21PM -0400, Alan Stern wrote:
> On Fri, May 15, 2020 at 05:54:53PM +0100, Colin King wrote:
> > From: Colin Ian King <[email protected]>
> >
> > The comparison of hcd->irq to less than zero for an error check will
> > never be true because hcd->irq is an unsigned int. Fix this by
> > assigning the int retval to the return of platform_get_irq and checking
> > this for the -ve error condition and assigning hcd->irq to retval.
> >
> > Addresses-Coverity: ("Unsigned compared against 0")
> > Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
> > Signed-off-by: Colin Ian King <[email protected]>
> > ---
>
> Thanks to Coverity for spotting this. Any reason why it didn't spot
> exactly the same mistake in the ohci-da8xx.c driver?
I think Coverity only runs on a limited kernel configuration and does
not build everything :(
On 16/05/2020 07:30, Greg Kroah-Hartman wrote:
> On Fri, May 15, 2020 at 01:21:21PM -0400, Alan Stern wrote:
>> On Fri, May 15, 2020 at 05:54:53PM +0100, Colin King wrote:
>>> From: Colin Ian King <[email protected]>
>>>
>>> The comparison of hcd->irq to less than zero for an error check will
>>> never be true because hcd->irq is an unsigned int. Fix this by
>>> assigning the int retval to the return of platform_get_irq and checking
>>> this for the -ve error condition and assigning hcd->irq to retval.
>>>
>>> Addresses-Coverity: ("Unsigned compared against 0")
>>> Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
>>> Signed-off-by: Colin Ian King <[email protected]>
>>> ---
>>
>> Thanks to Coverity for spotting this. Any reason why it didn't spot
>> exactly the same mistake in the ohci-da8xx.c driver?
>
> I think Coverity only runs on a limited kernel configuration and does
> not build everything :(
>
I do x86-64 make allmodconfig builds, so it does a fair amount of
coverage on the builds
Colin