Date:   Tue, 11 Jan 2022 09:54:12 -0600
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Yang Li <yang.lee@linux.alibaba.com>
Cc:     kys@microsoft.com, haiyangz@microsoft.com, sthemmin@microsoft.com,
        wei.liu@kernel.org, decui@microsoft.com, lorenzo.pieralisi@arm.com,
        robh@kernel.org, kw@linux.com, bhelgaas@google.com,
        linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Abaci Robot <abaci@linux.alibaba.com>,
        Sunil Muthuswamy <sunilmut@microsoft.com>
Subject: Re: [PATCH -next] PCI: hv: Unsigned comparison with less than zero
Message-ID: <20220111155412.GA142851@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220111012622.19447-1-yang.lee@linux.alibaba.com>
Precedence: bulk

[+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 <abaci@linux.alibaba.com>
> Signed-off-by: Yang Li <yang.lee@linux.alibaba.com>
> ---
>  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
>