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

[Christophe JAILLET]

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@xxxxxxxxxx/

> 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@xxxxxxxxx>
>
> 	Krzysztof