Re: [PATCH] PCI: brcmstb: Declare a bitmap as a bitmap, not as a plain 'unsigned long'

From: Bjorn Helgaas <helgaas@kernel.org>
To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: nsaenz@kernel.org, jim2101024@gmail.com, f.fainelli@gmail.com,
	bcm-kernel-feedback-list@broadcom.com, lorenzo.pieralisi@arm.com,
	robh@kernel.org, kw@linux.com, bhelgaas@google.com,
	linux-rpi-kernel@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] PCI: brcmstb: Declare a bitmap as a bitmap, not as a plain 'unsigned long'
Date: Tue, 30 Nov 2021 11:04:17 -0600	[thread overview]
Message-ID: <20211130170417.GA2744534@bhelgaas> (raw)
In-Reply-To: <e6d9da2112aab2939d1507b90962d07bfd735b4c.1636273671.git.christophe.jaillet@wanadoo.fr>

On Sun, Nov 07, 2021 at 09:32:58AM +0100, Christophe JAILLET wrote:
> The 'used' field of 'struct brcm_msi' is used as a bitmap. So it should
> be declared as so (i.e. unsigned long *).
> 
> This fixes an harmless Coverity warning about array vs singleton usage.
> 
> This bitmap can be BRCM_INT_PCI_MSI_LEGACY_NR or BRCM_INT_PCI_MSI_NR long.
> So, while at it, document it, should it help someone in the future.
> 
> Addresses-Coverity: "Out-of-bounds access (ARRAY_VS_SINGLETON)"
> Suggested-by: Krzysztof Wilczynski <kw@linux.com>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> The BUILD_BUG_ON is surely a bit to much of paranoia :)
> 
> I'm also not really pleased about the layout of the DECLARE_BITMAP. This
> looks odd, but I couldn't find something nicer :(
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 1fc7bd49a7ad..15d394ac7478 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -266,8 +266,9 @@ struct brcm_msi {
>  	struct mutex		lock; /* guards the alloc/free operations */
>  	u64			target_addr;
>  	int			irq;
> -	/* used indicates which MSI interrupts have been alloc'd */
> -	unsigned long		used;
> +	/* Used indicates which MSI interrupts have been alloc'd. 'nr' bellow is
> +	   the real size of the bitmap. It depends on the chip. */

I hate to bike-shed this even more, but IMO we should just drop the
comment above completely.  It's not the usual commenting style, no
other drivers provide similar explanation, and "below" is misspelled,
which will lead to a future fixup patch.

> +	DECLARE_BITMAP		(used, BRCM_INT_PCI_MSI_NR);
>  	bool			legacy;
>  	/* Some chips have MSIs in bits [31..24] of a shared register. */
>  	int			legacy_shift;
> @@ -534,7 +535,7 @@ static int brcm_msi_alloc(struct brcm_msi *msi)
>  	int hwirq;
>  
>  	mutex_lock(&msi->lock);
> -	hwirq = bitmap_find_free_region(&msi->used, msi->nr, 0);
> +	hwirq = bitmap_find_free_region(msi->used, msi->nr, 0);
>  	mutex_unlock(&msi->lock);
>  
>  	return hwirq;
> @@ -543,7 +544,7 @@ static int brcm_msi_alloc(struct brcm_msi *msi)
>  static void brcm_msi_free(struct brcm_msi *msi, unsigned long hwirq)
>  {
>  	mutex_lock(&msi->lock);
> -	bitmap_release_region(&msi->used, hwirq, 0);
> +	bitmap_release_region(msi->used, hwirq, 0);
>  	mutex_unlock(&msi->lock);
>  }
>  
> @@ -661,6 +662,12 @@ static int brcm_pcie_enable_msi(struct brcm_pcie *pcie)
>  	msi->irq = irq;
>  	msi->legacy = pcie->hw_rev < BRCM_PCIE_HW_REV_33;
>  
> +	/*
> +	 * Sanity check to make sure that the 'used' bitmap in struct brcm_msi
> +	 * is large enough.
> +	 */
> +	BUILD_BUG_ON(BRCM_INT_PCI_MSI_LEGACY_NR > BRCM_INT_PCI_MSI_NR);
> +
>  	if (msi->legacy) {
>  		msi->intr_base = msi->base + PCIE_INTR2_CPU_BASE;
>  		msi->nr = BRCM_INT_PCI_MSI_LEGACY_NR;
> -- 
> 2.30.2
> 