| Message ID | e6d9da2112aab2939d1507b90962d07bfd735b4c.1636273671.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: brcmstb: Declare a bitmap as a bitmap, not as a plain 'unsigned long' | expand |
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
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.
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.
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
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>
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.
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
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
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 >
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
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;
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(-)