From: "Krzysztof Wilczyński" <kw@linux.com> To: Christophe JAILLET <christophe.jaillet@wanadoo.fr> 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: Sat, 6 Nov 2021 22:36:05 +0100 [thread overview] Message-ID: <YYb1RXjnXSV8xF/0@rocinante> (raw) In-Reply-To: <32f3bc1fbfbd6ee0815e565012904758ca9eff7e.1635019243.git.christophe.jaillet@wanadoo.fr> 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? 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. [...] > - 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@linux.com> Krzysztof
WARNING: multiple messages have this Message-ID (diff)
From: "Krzysztof Wilczyński" <kw@linux.com> To: Christophe JAILLET <christophe.jaillet@wanadoo.fr> 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: Sat, 6 Nov 2021 22:36:05 +0100 [thread overview] Message-ID: <YYb1RXjnXSV8xF/0@rocinante> (raw) In-Reply-To: <32f3bc1fbfbd6ee0815e565012904758ca9eff7e.1635019243.git.christophe.jaillet@wanadoo.fr> 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? 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. [...] > - 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@linux.com> Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-11-06 21:36 UTC|newest] Thread overview: 12+ 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-10-23 20:02 ` Christophe JAILLET 2021-11-06 21:36 ` Krzysztof Wilczyński [this message] 2021-11-06 21:36 ` Krzysztof Wilczyński 2021-11-07 7:18 ` Christophe JAILLET 2021-11-07 7:18 ` Christophe JAILLET 2021-11-08 0:56 ` Krzysztof Wilczyński 2021-11-08 0:56 ` Krzysztof Wilczyński 2021-11-08 19:57 ` Christophe JAILLET 2021-11-08 19:57 ` Christophe JAILLET 2021-11-29 17:40 ` Lorenzo Pieralisi 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=YYb1RXjnXSV8xF/0@rocinante \ --to=kw@linux.com \ --cc=bhelgaas@google.com \ --cc=christophe.jaillet@wanadoo.fr \ --cc=kernel-janitors@vger.kernel.org \ --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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.