Re: [PATCH] PCI: xgene-msi: Use bitmap_zalloc() when applicable

["Krzysztof Wilczyński" <kw@linux.com>]

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