Messages in this thread | | | Subject | Re: [PATCH] PCI: xgene-msi: Use bitmap_zalloc() when applicable | From | Christophe JAILLET <> | Date | Mon, 8 Nov 2021 20:57:39 +0100 |
| |
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 >
| |