Hi Christophe!

[...]

quoted

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.

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel