From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
To: "Krzysztof Wilczyński" <kw@linux.com>
Cc: toan@os.amperecomputing.com, lorenzo.pieralisi@arm.com,
robh@kernel.org, bhelgaas@google.com, linux-pci@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] PCI: xgene-msi: Use bitmap_zalloc() when applicable
Date: Mon, 8 Nov 2021 20:57:39 +0100 [thread overview]
Message-ID: <2ee153cc-961e-abaf-2c02-a15b4c0b7986@wanadoo.fr> (raw)
In-Reply-To: <YYh1vrNCavFKuskW@rocinante>
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
>
next prev parent reply other threads:[~2021-11-08 19:57 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-23 20:02 [PATCH] PCI: xgene-msi: Use bitmap_zalloc() when applicable Christophe JAILLET
2021-11-06 21:36 ` Krzysztof Wilczyński
2021-11-07 7:18 ` Christophe JAILLET
2021-11-08 0:56 ` Krzysztof Wilczyński
2021-11-08 19:57 ` Christophe JAILLET [this message]
2021-11-29 17:40 ` Lorenzo Pieralisi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2ee153cc-961e-abaf-2c02-a15b4c0b7986@wanadoo.fr \
--to=christophe.jaillet@wanadoo.fr \
--cc=bhelgaas@google.com \
--cc=kernel-janitors@vger.kernel.org \
--cc=kw@linux.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=robh@kernel.org \
--cc=toan@os.amperecomputing.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).