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

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

[Christophe JAILLET]

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. */
+	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

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

[Krzysztof Wilczyński]

Hi Christophe!

[...]
> This bitmap can be BRCM_INT_PCI_MSI_LEGACY_NR or BRCM_INT_PCI_MSI_NR long.

Ahh.  OK.  Given this an option would be to: do nothing (keep current
status quo); allocate memory dynamically passing the "msi->nr" after it
has been set accordingly; use BRCM_INT_PCI_MSI_NR and waste a little bit
of space.

Perhaps moving to using the DECLARE_BITMAP() would be fine in this case
too, at least to match style of other drivers more closely.

Jim, Florian and Lorenzo - is this something that would be OK with you,
or you would rather keep things as they were?

> Addresses-Coverity: "Out-of-bounds access (ARRAY_VS_SINGLETON)"

This tag would have to be written as:

  Addresses-Coverity: ("Out-of-bounds access (ARRAY_VS_SINGLETON)")

[...]
> +	DECLARE_BITMAP		(used, BRCM_INT_PCI_MSI_NR);

Probably not the most elegant solution, but I would keep it as:

  DECLARE_BITMAP(used, BRCM_INT_PCI_MSI_NR);

Otherwise aligning either before or after the open bracket will cause
either an error or a warning issued by checkpatch.pl accordingly about
the style.  Other users of this (a vast majoirty) macro don't do any
specific alignment at large

[...]
> +	/*
> +	 * 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);

A healthy paranoia, I see. :-)

	Krzysztof

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

[Florian Fainelli]

On 11/7/2021 5:34 PM, Krzysztof Wilczyński wrote:
> Hi Christophe!
> 
> [...]
>> This bitmap can be BRCM_INT_PCI_MSI_LEGACY_NR or BRCM_INT_PCI_MSI_NR long.
> 
> Ahh.  OK.  Given this an option would be to: do nothing (keep current
> status quo); allocate memory dynamically passing the "msi->nr" after it
> has been set accordingly; use BRCM_INT_PCI_MSI_NR and waste a little bit
> of space.
> 
> Perhaps moving to using the DECLARE_BITMAP() would be fine in this case
> too, at least to match style of other drivers more closely.
> 
> Jim, Florian and Lorenzo - is this something that would be OK with you,
> or you would rather keep things as they were?

I would be tempted to leave the code as-is, but if we do we are probably 
bound to seeing patches like Christophe's in the future to address the 
problem, unless we place a coverity specific comment in the source tree, 
which is probably frowned upon.

The addition of the BUILD_BUG_ON() is a good addition though.
-- 
Florian

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

[Christophe JAILLET]

Le 08/11/2021 à 17:28, Florian Fainelli a écrit :
> 
> 
> On 11/7/2021 5:34 PM, Krzysztof Wilczyński wrote:
>> Hi Christophe!
>>
>> [...]
>>> This bitmap can be BRCM_INT_PCI_MSI_LEGACY_NR or BRCM_INT_PCI_MSI_NR 
>>> long.
>>
>> Ahh.  OK.  Given this an option would be to: do nothing (keep current
>> status quo); allocate memory dynamically passing the "msi->nr" after it
>> has been set accordingly; use BRCM_INT_PCI_MSI_NR and waste a little bit
>> of space.
>>
>> Perhaps moving to using the DECLARE_BITMAP() would be fine in this case
>> too, at least to match style of other drivers more closely.
>>
>> Jim, Florian and Lorenzo - is this something that would be OK with you,
>> or you would rather keep things as they were?
> 
> I would be tempted to leave the code as-is, but if we do we are probably 
> bound to seeing patches like Christophe's in the future to address the 

Even if I don't find this report in the Coverity database, it should 
from around April 2018.
So, if you have not already received several patches for that, I doubt 
that you will receive many in the future.


My own feeling is that using a long (and not a long *) as a bitmap, and 
accessing it with &long may look spurious to a reader.
That said, it works.

So, I let you decide if the patch is of any use. Should I need to tweak 
or resend it, let me know.


CJ

> problem, unless we place a coverity specific comment in the source tree, 
> which is probably frowned upon.
> 
> The addition of the BUILD_BUG_ON() is a good addition though.

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

[Krzysztof Wilczyński]

Hello,

[...]
> > > Jim, Florian and Lorenzo - is this something that would be OK with you,
> > > or you would rather keep things as they were?
> > 
> > I would be tempted to leave the code as-is, but if we do we are probably
> > bound to seeing patches like Christophe's in the future to address the
> 
> Even if I don't find this report in the Coverity database, it should from
> around April 2018.
> So, if you have not already received several patches for that, I doubt that
> you will receive many in the future.
> 
> 
> My own feeling is that using a long (and not a long *) as a bitmap, and
> accessing it with &long may look spurious to a reader.
> That said, it works.
> 
> So, I let you decide if the patch is of any use. Should I need to tweak or
> resend it, let me know.

I would be pro taking it, not only to addresses the Coverity complaint, but
also to align the code with other drivers a little bit more.  Only if
the driver maintainers have no objection, that is.

	Krzysztof

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

[Florian Fainelli]

On 11/7/21 1:32 AM, 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>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

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

[Florian Fainelli]

On 11/8/21 3:30 PM, Krzysztof Wilczyński wrote:
> Hello,
> 
> [...]
>>>> Jim, Florian and Lorenzo - is this something that would be OK with you,
>>>> or you would rather keep things as they were?
>>>
>>> I would be tempted to leave the code as-is, but if we do we are probably
>>> bound to seeing patches like Christophe's in the future to address the
>>
>> Even if I don't find this report in the Coverity database, it should from
>> around April 2018.
>> So, if you have not already received several patches for that, I doubt that
>> you will receive many in the future.
>>
>>
>> My own feeling is that using a long (and not a long *) as a bitmap, and
>> accessing it with &long may look spurious to a reader.
>> That said, it works.
>>
>> So, I let you decide if the patch is of any use. Should I need to tweak or
>> resend it, let me know.
> 
> I would be pro taking it, not only to addresses the Coverity complaint, but
> also to align the code with other drivers a little bit more.  Only if
> the driver maintainers have no objection, that is.

Driver consistency is a strong argument, fine with me then.
-- 
Florian

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

[Krzysztof Wilczyński]

Hi Florian,

[...]
> >>>> Jim, Florian and Lorenzo - is this something that would be OK with you,
> >>>> or you would rather keep things as they were?
> >>>
> >>> I would be tempted to leave the code as-is, but if we do we are probably
> >>> bound to seeing patches like Christophe's in the future to address the
> >>
> >> Even if I don't find this report in the Coverity database, it should from
> >> around April 2018.
> >> So, if you have not already received several patches for that, I doubt that
> >> you will receive many in the future.
> >>
> >>
> >> My own feeling is that using a long (and not a long *) as a bitmap, and
> >> accessing it with &long may look spurious to a reader.
> >> That said, it works.
> >>
> >> So, I let you decide if the patch is of any use. Should I need to tweak or
> >> resend it, let me know.
> > 
> > I would be pro taking it, not only to addresses the Coverity complaint, but
> > also to align the code with other drivers a little bit more.  Only if
> > the driver maintainers have no objection, that is.
> 
> Driver consistency is a strong argument, fine with me then.

Thank you!

	Krzysztof

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

[Lorenzo Pieralisi]

On Sun, 7 Nov 2021 09:32:58 +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.

Applied to pci/brcmstb, thanks!

[1/1] PCI: brcmstb: Declare a bitmap as a bitmap, not as a plain 'unsigned long'
      https://git.kernel.org/lpieralisi/pci/c/3e46790d16

Thanks,
Lorenzo

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

[Bjorn Helgaas]

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
> 

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

[Dan Carpenter]

On Tue, Nov 30, 2021 at 11:04:17AM -0600, Bjorn Helgaas wrote:
> > +	/* 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.
> 

There a bunch of these...

$ git grep -wi bellow
Documentation/input/devices/ntrig.rst:|min_height,              |size threshold bellow which fingers are ignored      |
Documentation/networking/arcnet-hardware.rst:the I/O address space bellow 0x200 is RESERVED for mainboard, so
Documentation/networking/regulatory.rst:Bellow is a simple example, with a regulatory domain cached using the stack.
Documentation/security/digsig.rst:of the key: 5D2B05FC633EE3E8 in the example bellow.
Documentation/sound/alsa-configuration.rst:    bitmap of available external inputs for FX8010 (see bellow)
Documentation/sound/alsa-configuration.rst:    bitmap of available external outputs for FX8010 (see bellow)
arch/arm/mach-s3c/mach-mini2440.c: * This macro simplifies the table bellow
arch/arm/mm/kasan_init.c:        * At first we should unmap early shadow (clear_pgds() call bellow).
arch/arm/mm/pmsa-v7.c:                 * data access till we setup RAM bellow would be done
arch/mips/include/asm/sgi/mc.h:  * be the same size. The size encoding for supported SIMMs is bellow */
drivers/edac/sb_edac.c:  * algorithm bellow.
drivers/edac/sb_edac.c:  * The check bellow is probably enough to fill all cases where
drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c: * The 2 macros bellow represent the actual size in bytes that
drivers/gpu/drm/i915/display/intel_psr.c:                * comments bellow for more information
drivers/gpu/drm/i915/gt/shaders/README:The instructions bellow assume:
drivers/gpu/drm/rockchip/cdn-dp-reg.h:/* bellow registers need access by mailbox */
drivers/gpu/drm/sun4i/sun8i_mixer.h: * Sub-engines listed bellow are unused for now. The EN registers are here only
drivers/gpu/drm/tidss/tidss_dispc_regs.h: * macros bellow can be used.
drivers/gpu/drm/tidss/tidss_plane.c:     * (the two first checks bellow). At the end of a row the HW
drivers/net/ethernet/chelsio/cxgb/suni1x10gexp_regs.h:/* Refer to the Register Bit Masks bellow for the naming of each register and */
drivers/net/ethernet/freescale/fman/fman_port.c:        /* The code bellow is a trick so the FM will not release the buffer
drivers/net/wireless/ath/ath6kl/wmi.h:  /* lowest of bellow */
drivers/net/wireless/ath/ath6kl/wmi.h:  /* highest of bellow */
drivers/net/wireless/ath/ath6kl/wmi.h:  /* lowest of bellow */
drivers/net/wireless/ath/ath6kl/wmi.h:  /* highest of bellow */
drivers/net/wireless/ath/wcn36xx/wcn36xx.h: * used in both SMD channel and TX BD. See table bellow when it is used.
drivers/power/supply/adp5061.c:          * bellow this value, weak charge mode is entered
drivers/pwm/pwm-stm32.c:                scale = priv->max_arr; /* bellow resolution, use max scale */
drivers/thermal/ti-soc-thermal/dra752-bandgap.h: * All the macros bellow define the required bits for
drivers/thermal/ti-soc-thermal/dra752-bandgap.h: * All the macros bellow are definitions for handling the
drivers/thermal/ti-soc-thermal/omap4xxx-bandgap.h: * All the macros bellow define the required bits for
drivers/thermal/ti-soc-thermal/omap4xxx-bandgap.h: * All the macros bellow are definitions for handling the
drivers/thermal/ti-soc-thermal/omap4xxx-bandgap.h: * All the macros bellow define the required bits for
drivers/thermal/ti-soc-thermal/omap4xxx-bandgap.h: * All the macros bellow are definitions for handling the
drivers/thermal/ti-soc-thermal/omap5xxx-bandgap.h: * All the macros bellow define the required bits for
drivers/thermal/ti-soc-thermal/omap5xxx-bandgap.h: * All the macros bellow are definitions for handling the
drivers/video/fbdev/omap2/omapfb/dss/hdmi.h:    /* This lock should be taken when booleans bellow are touched. */
drivers/video/fbdev/via/lcd.c:  DEBUG_MSG(KERN_INFO "bellow viafb_lcd_set_mode!!\n");
sound/soc/codecs/tlv320aic31xx.c:               /* See bellow for details how fix this. */
tools/perf/builtin-help.c:      char *page_path; /* it leaks but we exec bellow */

regards,
dan carpenter