| Message ID | 32f3bc1fbfbd6ee0815e565012904758ca9eff7e.1635019243.git.christophe.jaillet@wanadoo.fr |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: xgene-msi: Use bitmap_zalloc() when applicable | expand |
Hi Christophe! > 'xgene_msi->bitmap' is a bitmap. So use 'bitmap_zalloc()' to simplify code, > improve the semantic and avoid some open-coded arithmetic in allocator > arguments. > > Also change the corresponding 'kfree()' into 'bitmap_free()' to keep > consistency. I believe, after having a brief look, that we might have a few other candidates that we could also update: drivers/pci/controller/dwc/pcie-designware-ep.c 717: ep->ib_window_map = devm_kcalloc(dev, 724: ep->ob_window_map = devm_kcalloc(dev, drivers/pci/controller/pcie-iproc-msi.c 592: msi->bitmap = devm_kcalloc(pcie->dev, BITS_TO_LONGS(msi->nr_msi_vecs), drivers/pci/controller/pcie-xilinx-nwl.c 470: bit = bitmap_find_free_region(msi->bitmap, INT_PCI_MSI_NR, 567: msi->bitmap = kzalloc(size, GFP_KERNEL); 637: msi->bitmap = NULL; drivers/pci/controller/pcie-iproc-msi.c 262: hwirq = bitmap_find_free_region(msi->bitmap, msi->nr_msi_vecs, 290: bitmap_release_region(msi->bitmap, hwirq, drivers/pci/controller/pcie-xilinx-nwl.c 470: bit = bitmap_find_free_region(msi->bitmap, INT_PCI_MSI_NR, 494: bitmap_release_region(msi->bitmap, data->hwirq, drivers/pci/controller/pcie-brcmstb.c 537: hwirq = bitmap_find_free_region(&msi->used, msi->nr, 0); 546: bitmap_release_region(&msi->used, hwirq, 0); drivers/pci/controller/pcie-xilinx.c 240: hwirq = bitmap_find_free_region(port->msi_map, XILINX_NUM_MSI_IRQS, order_base_2(nr_irqs)); 263: bitmap_release_region(port->msi_map, d->hwirq, order_base_2(nr_irqs)); Some of the above could also potentially benefit from being converted to use the DECLARE_BITMAP() macro to create the bitmap that is then being embedded into some struct used to capture details and state, rather than store a pointer to later allocate memory dynamically. Some controller drivers already do this, so we could convert rest where appropriate. What do you think? We also have this nudge from Coverity that we could fix, as per: 532 static int brcm_msi_alloc(struct brcm_msi *msi) 533 { 534 int hwirq; 535 536 mutex_lock(&msi->lock); 1. address_of: Taking address with &msi->used yields a singleton pointer. CID 1468487 (#1 of 1): Out-of-bounds access (ARRAY_VS_SINGLETON)2. callee_ptr_arith: Passing &msi->used to function bitmap_find_free_region which uses it as an array. This might corrupt or misinterpret adjacent memory locations. [show details] 537 hwirq = bitmap_find_free_region(&msi->used, msi->nr, 0); 538 mutex_unlock(&msi->lock); 539 540 return hwirq; 541 } 543 static void brcm_msi_free(struct brcm_msi *msi, unsigned long hwirq) 544 { 545 mutex_lock(&msi->lock); 1. address_of: Taking address with &msi->used yields a singleton pointer. CID 1468424 (#1 of 1): Out-of-bounds access (ARRAY_VS_SINGLETON)2. callee_ptr_arith: Passing &msi->used to function bitmap_release_region which uses it as an array. This might corrupt or misinterpret adjacent memory locations. [show details] 546 bitmap_release_region(&msi->used, hwirq, 0); 547 mutex_unlock(&msi->lock); 548 } We could look at addressing this too at the same time. [...] > - int size = BITS_TO_LONGS(NR_MSI_VEC) * sizeof(long); > - > - xgene_msi->bitmap = kzalloc(size, GFP_KERNEL); > + xgene_msi->bitmap = bitmap_zalloc(NR_MSI_VEC, GFP_KERNEL); > if (!xgene_msi->bitmap) > return -ENOMEM; > > @@ -360,7 +358,7 @@ static int xgene_msi_remove(struct platform_device *pdev) > > kfree(msi->msi_groups); > > - kfree(msi->bitmap); > + bitmap_free(msi->bitmap); > msi->bitmap = NULL; > > xgene_free_domains(msi); Thank you! Reviewed-by: Krzysztof Wilczyński <kw@linux.com> Krzysztof
Le 06/11/2021 à 22:36, Krzysztof Wilczyński a écrit : > Hi Christophe! > >> 'xgene_msi->bitmap' is a bitmap. So use 'bitmap_zalloc()' to simplify code, >> improve the semantic and avoid some open-coded arithmetic in allocator >> arguments. >> >> Also change the corresponding 'kfree()' into 'bitmap_free()' to keep >> consistency. > > I believe, after having a brief look, that we might have a few other > candidates that we could also update: > > drivers/pci/controller/dwc/pcie-designware-ep.c > 717: ep->ib_window_map = devm_kcalloc(dev, > 724: ep->ob_window_map = devm_kcalloc(dev, > > drivers/pci/controller/pcie-iproc-msi.c > 592: msi->bitmap = devm_kcalloc(pcie->dev, BITS_TO_LONGS(msi->nr_msi_vecs), > > drivers/pci/controller/pcie-xilinx-nwl.c > 470: bit = bitmap_find_free_region(msi->bitmap, INT_PCI_MSI_NR, > 567: msi->bitmap = kzalloc(size, GFP_KERNEL); > 637: msi->bitmap = NULL; > > drivers/pci/controller/pcie-iproc-msi.c > 262: hwirq = bitmap_find_free_region(msi->bitmap, msi->nr_msi_vecs, > 290: bitmap_release_region(msi->bitmap, hwirq, > > drivers/pci/controller/pcie-xilinx-nwl.c > 470: bit = bitmap_find_free_region(msi->bitmap, INT_PCI_MSI_NR, > 494: bitmap_release_region(msi->bitmap, data->hwirq, > > drivers/pci/controller/pcie-brcmstb.c > 537: hwirq = bitmap_find_free_region(&msi->used, msi->nr, 0); > 546: bitmap_release_region(&msi->used, hwirq, 0); > > drivers/pci/controller/pcie-xilinx.c > 240: hwirq = bitmap_find_free_region(port->msi_map, XILINX_NUM_MSI_IRQS, order_base_2(nr_irqs)); > 263: bitmap_release_region(port->msi_map, d->hwirq, order_base_2(nr_irqs)); > > Some of the above could also potentially benefit from being converted to > use the DECLARE_BITMAP() macro to create the bitmap that is then being > embedded into some struct used to capture details and state, rather than > store a pointer to later allocate memory dynamically. Some controller > drivers already do this, so we could convert rest where appropriate. > > What do you think? Hi, my first goal was to simplify code that was not already spotted by a cocci script proposed by Joe Perches (see [1]). I'll give a closer look at the opportunities spotted by Joe if they have not already been fixed in the meantime. Concerning the use of DECLARE_BITMAP instead of alloc/free memory, it can be more tricky to spot it. Will try to give a look at it. [1]: https://lore.kernel.org/kernel-janitors/994b268f-ea33-bf82-96ab-c20057ba4930@wanadoo.fr/ > > We also have this nudge from Coverity that we could fix, as per: > > 532 static int brcm_msi_alloc(struct brcm_msi *msi) > 533 { > 534 int hwirq; > 535 > 536 mutex_lock(&msi->lock); > 1. address_of: Taking address with &msi->used yields a singleton pointer. > CID 1468487 (#1 of 1): Out-of-bounds access (ARRAY_VS_SINGLETON)2. callee_ptr_arith: Passing &msi->used to function bitmap_find_free_region which uses it as an array. This might corrupt or misinterpret adjacent memory locations. [show details] > 537 hwirq = bitmap_find_free_region(&msi->used, msi->nr, 0); > 538 mutex_unlock(&msi->lock); > 539 > 540 return hwirq; > 541 } > > 543 static void brcm_msi_free(struct brcm_msi *msi, unsigned long hwirq) > 544 { > 545 mutex_lock(&msi->lock); > 1. address_of: Taking address with &msi->used yields a singleton pointer. > CID 1468424 (#1 of 1): Out-of-bounds access (ARRAY_VS_SINGLETON)2. callee_ptr_arith: Passing &msi->used to function bitmap_release_region which uses it as an array. This might corrupt or misinterpret adjacent memory locations. [show details] > 546 bitmap_release_region(&msi->used, hwirq, 0); > 547 mutex_unlock(&msi->lock); > 548 } > > We could look at addressing this too at the same time. I'll give it a look. > > [...] >> - int size = BITS_TO_LONGS(NR_MSI_VEC) * sizeof(long); >> - >> - xgene_msi->bitmap = kzalloc(size, GFP_KERNEL); >> + xgene_msi->bitmap = bitmap_zalloc(NR_MSI_VEC, GFP_KERNEL); >> if (!xgene_msi->bitmap) >> return -ENOMEM; >> >> @@ -360,7 +358,7 @@ static int xgene_msi_remove(struct platform_device *pdev) >> >> kfree(msi->msi_groups); >> >> - kfree(msi->bitmap); >> + bitmap_free(msi->bitmap); >> msi->bitmap = NULL; >> >> xgene_free_domains(msi); > > Thank you! > > Reviewed-by: Krzysztof Wilczyński <kw@linux.com> > > Krzysztof >
Hi Christophe! [...] > > I believe, after having a brief look, that we might have a few other > > candidates that we could also update: > > > > drivers/pci/controller/dwc/pcie-designware-ep.c > > 717: ep->ib_window_map = devm_kcalloc(dev, > > 724: ep->ob_window_map = devm_kcalloc(dev, > > drivers/pci/controller/pcie-iproc-msi.c > > 592: msi->bitmap = devm_kcalloc(pcie->dev, BITS_TO_LONGS(msi->nr_msi_vecs), > > drivers/pci/controller/pcie-xilinx-nwl.c > > 470: bit = bitmap_find_free_region(msi->bitmap, INT_PCI_MSI_NR, > > 567: msi->bitmap = kzalloc(size, GFP_KERNEL); > > 637: msi->bitmap = NULL; > > drivers/pci/controller/pcie-iproc-msi.c > > 262: hwirq = bitmap_find_free_region(msi->bitmap, msi->nr_msi_vecs, > > 290: bitmap_release_region(msi->bitmap, hwirq, > > drivers/pci/controller/pcie-xilinx-nwl.c > > 470: bit = bitmap_find_free_region(msi->bitmap, INT_PCI_MSI_NR, > > 494: bitmap_release_region(msi->bitmap, data->hwirq, > > drivers/pci/controller/pcie-brcmstb.c > > 537: hwirq = bitmap_find_free_region(&msi->used, msi->nr, 0); > > 546: bitmap_release_region(&msi->used, hwirq, 0); > > drivers/pci/controller/pcie-xilinx.c > > 240: hwirq = bitmap_find_free_region(port->msi_map, XILINX_NUM_MSI_IRQS, order_base_2(nr_irqs)); > > 263: bitmap_release_region(port->msi_map, d->hwirq, order_base_2(nr_irqs)); > > > > Some of the above could also potentially benefit from being converted to > > use the DECLARE_BITMAP() macro to create the bitmap that is then being > > embedded into some struct used to capture details and state, rather than > > store a pointer to later allocate memory dynamically. Some controller > > drivers already do this, so we could convert rest where appropriate. > > > > What do you think? > > Hi, > > my first goal was to simplify code that was not already spotted by a cocci > script proposed by Joe Perches (see [1]). Ahh, OK. I didn't know that Joe worked on adding new Coccinelle script to deal with the bitmap allocations and such. I assumed you did some code review and found some issues. I had a quick look at what the Coccinelle script found, and it seems I have missed when I did some search on my own: drivers/pci/controller/pcie-rcar-ep.c > I'll give a closer look at the opportunities spotted by Joe if they have not > already been fixed in the meantime. As per the thread you linked to, I can see that neither the new Coccinelle script nor the changes from Joe were accepted yet, or I couldn't see anything yet (at least not in the PCI tree). > Concerning the use of DECLARE_BITMAP instead of alloc/free memory, it can be > more tricky to spot it. Will try to give a look at it. A lot more code to read, indeed. However, the benefits are quite nice: simpler code, easier error handling and reducing probability of leaking memory. I think, a lot of the drivers we have in our tree could (and a lot already do) leverage the DECLARE_BITMAP() macro reserving space during build time over dealing with memory allocations and such. > > We also have this nudge from Coverity that we could fix, as per: > > > > 532 static int brcm_msi_alloc(struct brcm_msi *msi) > > 533 { > > 534 int hwirq; > > 535 > > 536 mutex_lock(&msi->lock); > > 1. address_of: Taking address with &msi->used yields a singleton pointer. > > CID 1468487 (#1 of 1): Out-of-bounds access (ARRAY_VS_SINGLETON)2. callee_ptr_arith: Passing &msi->used to function bitmap_find_free_region which uses it as an array. This might corrupt or misinterpret adjacent memory locations. [show details] > > 537 hwirq = bitmap_find_free_region(&msi->used, msi->nr, 0); > > 538 mutex_unlock(&msi->lock); > > 539 > > 540 return hwirq; > > 541 } > > 543 static void brcm_msi_free(struct brcm_msi *msi, unsigned long hwirq) > > 544 { > > 545 mutex_lock(&msi->lock); > > 1. address_of: Taking address with &msi->used yields a singleton pointer. > > CID 1468424 (#1 of 1): Out-of-bounds access (ARRAY_VS_SINGLETON)2. callee_ptr_arith: Passing &msi->used to function bitmap_release_region which uses it as an array. This might corrupt or misinterpret adjacent memory locations. [show details] > > 546 bitmap_release_region(&msi->used, hwirq, 0); > > 547 mutex_unlock(&msi->lock); > > 548 } > > > > We could look at addressing this too at the same time. > > I'll give it a look. Thank you! Krzysztof
Le 08/11/2021 à 01:56, Krzysztof Wilczyński a écrit : > Hi Christophe! > > [...] >>> I believe, after having a brief look, that we might have a few other >>> candidates that we could also update: >>> >>> drivers/pci/controller/dwc/pcie-designware-ep.c >>> 717: ep->ib_window_map = devm_kcalloc(dev, >>> 724: ep->ob_window_map = devm_kcalloc(dev, >>> drivers/pci/controller/pcie-iproc-msi.c >>> 592: msi->bitmap = devm_kcalloc(pcie->dev, BITS_TO_LONGS(msi->nr_msi_vecs), >>> drivers/pci/controller/pcie-xilinx-nwl.c >>> 470: bit = bitmap_find_free_region(msi->bitmap, INT_PCI_MSI_NR, >>> 567: msi->bitmap = kzalloc(size, GFP_KERNEL); >>> 637: msi->bitmap = NULL; >>> drivers/pci/controller/pcie-iproc-msi.c >>> 262: hwirq = bitmap_find_free_region(msi->bitmap, msi->nr_msi_vecs, >>> 290: bitmap_release_region(msi->bitmap, hwirq, >>> drivers/pci/controller/pcie-xilinx-nwl.c >>> 470: bit = bitmap_find_free_region(msi->bitmap, INT_PCI_MSI_NR, >>> 494: bitmap_release_region(msi->bitmap, data->hwirq, >>> drivers/pci/controller/pcie-brcmstb.c >>> 537: hwirq = bitmap_find_free_region(&msi->used, msi->nr, 0); >>> 546: bitmap_release_region(&msi->used, hwirq, 0); >>> drivers/pci/controller/pcie-xilinx.c >>> 240: hwirq = bitmap_find_free_region(port->msi_map, XILINX_NUM_MSI_IRQS, order_base_2(nr_irqs)); >>> 263: bitmap_release_region(port->msi_map, d->hwirq, order_base_2(nr_irqs)); >>> >>> Some of the above could also potentially benefit from being converted to >>> use the DECLARE_BITMAP() macro to create the bitmap that is then being >>> embedded into some struct used to capture details and state, rather than >>> store a pointer to later allocate memory dynamically. Some controller >>> drivers already do this, so we could convert rest where appropriate. >>> >>> What do you think? >> >> Hi, >> >> my first goal was to simplify code that was not already spotted by a cocci >> script proposed by Joe Perches (see [1]). > > Ahh, OK. I didn't know that Joe worked on adding new Coccinelle script to > deal with the bitmap allocations and such. I assumed you did some code > review and found some issues. > > I had a quick look at what the Coccinelle script found, and it seems I have > missed when I did some search on my own: > > drivers/pci/controller/pcie-rcar-ep.c > >> I'll give a closer look at the opportunities spotted by Joe if they have not >> already been fixed in the meantime. > > As per the thread you linked to, I can see that neither the new Coccinelle > script nor the changes from Joe were accepted yet, or I couldn't see > anything yet (at least not in the PCI tree). No patch has been proposed yet, only the script and its output has been sent on @kernel-janitor. There was also a discussion about the need to update the corresponding kfree() into bitmap_free() to keep consistency. Doing it with coccinelle could be challenging. devm_ function are not impacted, but for the others, it can be more tricky and would likely need manual update. CJ > >> Concerning the use of DECLARE_BITMAP instead of alloc/free memory, it can be >> more tricky to spot it. Will try to give a look at it. > > A lot more code to read, indeed. However, the benefits are quite nice: > simpler code, easier error handling and reducing probability of leaking > memory. > > I think, a lot of the drivers we have in our tree could (and a lot already > do) leverage the DECLARE_BITMAP() macro reserving space during build time > over dealing with memory allocations and such. > >>> We also have this nudge from Coverity that we could fix, as per: >>> >>> 532 static int brcm_msi_alloc(struct brcm_msi *msi) >>> 533 { >>> 534 int hwirq; >>> 535 >>> 536 mutex_lock(&msi->lock); >>> 1. address_of: Taking address with &msi->used yields a singleton pointer. >>> CID 1468487 (#1 of 1): Out-of-bounds access (ARRAY_VS_SINGLETON)2. callee_ptr_arith: Passing &msi->used to function bitmap_find_free_region which uses it as an array. This might corrupt or misinterpret adjacent memory locations. [show details] >>> 537 hwirq = bitmap_find_free_region(&msi->used, msi->nr, 0); >>> 538 mutex_unlock(&msi->lock); >>> 539 >>> 540 return hwirq; >>> 541 } >>> 543 static void brcm_msi_free(struct brcm_msi *msi, unsigned long hwirq) >>> 544 { >>> 545 mutex_lock(&msi->lock); >>> 1. address_of: Taking address with &msi->used yields a singleton pointer. >>> CID 1468424 (#1 of 1): Out-of-bounds access (ARRAY_VS_SINGLETON)2. callee_ptr_arith: Passing &msi->used to function bitmap_release_region which uses it as an array. This might corrupt or misinterpret adjacent memory locations. [show details] >>> 546 bitmap_release_region(&msi->used, hwirq, 0); >>> 547 mutex_unlock(&msi->lock); >>> 548 } >>> >>> We could look at addressing this too at the same time. >> >> I'll give it a look. > > Thank you! > > Krzysztof >
On Sat, 23 Oct 2021 22:02:05 +0200, Christophe JAILLET wrote: > 'xgene_msi->bitmap' is a bitmap. So use 'bitmap_zalloc()' to simplify code, > improve the semantic and avoid some open-coded arithmetic in allocator > arguments. > > Also change the corresponding 'kfree()' into 'bitmap_free()' to keep > consistency. > > [...] Applied to pci/xgene, thanks! [1/1] PCI: xgene-msi: Use bitmap_zalloc() when applicable https://git.kernel.org/lpieralisi/pci/c/1ed9b961be Thanks, Lorenzo
diff --git a/drivers/pci/controller/pci-xgene-msi.c b/drivers/pci/controller/pci-xgene-msi.c index c50ff279903c..bfa259781b69 100644 --- a/drivers/pci/controller/pci-xgene-msi.c +++ b/drivers/pci/controller/pci-xgene-msi.c @@ -269,9 +269,7 @@ static void xgene_free_domains(struct xgene_msi *msi) static int xgene_msi_init_allocator(struct xgene_msi *xgene_msi) { - int size = BITS_TO_LONGS(NR_MSI_VEC) * sizeof(long); - - xgene_msi->bitmap = kzalloc(size, GFP_KERNEL); + xgene_msi->bitmap = bitmap_zalloc(NR_MSI_VEC, GFP_KERNEL); if (!xgene_msi->bitmap) return -ENOMEM; @@ -360,7 +358,7 @@ static int xgene_msi_remove(struct platform_device *pdev) kfree(msi->msi_groups); - kfree(msi->bitmap); + bitmap_free(msi->bitmap); msi->bitmap = NULL; xgene_free_domains(msi);
'xgene_msi->bitmap' is a bitmap. So use 'bitmap_zalloc()' to simplify code, improve the semantic and avoid some open-coded arithmetic in allocator arguments. Also change the corresponding 'kfree()' into 'bitmap_free()' to keep consistency. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/pci/controller/pci-xgene-msi.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)