2022-01-11 01:26:33

by Yang Li

[permalink] [raw]
Subject: [PATCH -next] PCI: hv: Unsigned comparison with less than zero

The return from the call to bitmap_find_free_region() is int, it can be
a negative error code, however this is being assigned to an unsigned
int variable 'index', so making 'index' an int.

Eliminate the following coccicheck warning:
./drivers/pci/controller/pci-hyperv.c:712:5-10: WARNING: Unsigned
expression compared with zero: index < 0

Reported-by: Abaci Robot <[email protected]>
Signed-off-by: Yang Li <[email protected]>
---
drivers/pci/controller/pci-hyperv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 26c9c8ec0989..20ea2ee330b8 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -701,7 +701,7 @@ static int hv_pci_vec_alloc_device_irq(struct irq_domain *domain,
irq_hw_number_t *hwirq)
{
struct hv_pci_chip_data *chip_data = domain->host_data;
- unsigned int index;
+ int index;

/* Find and allocate region from the SPI bitmap */
mutex_lock(&chip_data->map_lock);
--
2.20.1.7.g153144c



2022-01-11 15:54:24

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH -next] PCI: hv: Unsigned comparison with less than zero

[+cc Sunil]

On Tue, Jan 11, 2022 at 09:26:22AM +0800, Yang Li wrote:
> The return from the call to bitmap_find_free_region() is int, it can be
> a negative error code, however this is being assigned to an unsigned
> int variable 'index', so making 'index' an int.
>
> Eliminate the following coccicheck warning:
> ./drivers/pci/controller/pci-hyperv.c:712:5-10: WARNING: Unsigned
> expression compared with zero: index < 0

Definitely looks like a bug. Thanks very much for catching it!

Minor things:

1) Can you make the subject and commit log talk about the *bug* this
fixes, which is that an error return from
bitmap_find_free_region() is currently ignored and we instead
return a completely bogus *hwirq from
hv_pci_vec_alloc_device_irq()?

The warning is only secondary.

2) When fixing a bug, can you also mention the commit that
*introduced* the bug, so we can figure out where the fix needs to
be backported?

It looks like c10bdb758ca4 ("PCI: hv: Add arm64 Hyper-V vPCI
support"), so we should have a line like this:

Fixes: c10bdb758ca4 ("PCI: hv: Add arm64 Hyper-V vPCI support")

In this case, c10bdb758ca4 is still pending for the current merge
window, so we'll probably squash this fix into the commit so
there's no bisection window between c10bdb758ca4 and the fix.

3) Please cc the author of the original commit in case there's
something we're missing (I added Sunil here).

4) Make the subject line start with a verb so it matches the style
in drivers/pci/, where I try to make the subject line a sentence
that makes sense all by itself and tells what the patch does.

Thanks again for the fix!

Bjorn

> Reported-by: Abaci Robot <[email protected]>
> Signed-off-by: Yang Li <[email protected]>
> ---
> drivers/pci/controller/pci-hyperv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 26c9c8ec0989..20ea2ee330b8 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -701,7 +701,7 @@ static int hv_pci_vec_alloc_device_irq(struct irq_domain *domain,
> irq_hw_number_t *hwirq)
> {
> struct hv_pci_chip_data *chip_data = domain->host_data;
> - unsigned int index;
> + int index;
>
> /* Find and allocate region from the SPI bitmap */
> mutex_lock(&chip_data->map_lock);
> --
> 2.20.1.7.g153144c
>