Messages in this thread | | | Subject | Re: [PATCH] PCI: xilinx-nwl: Simplify code and fix a memory leak | From | Christophe JAILLET <> | Date | Mon, 8 Nov 2021 07:37:29 +0100 |
| |
Le 08/11/2021 à 01:31, Krzysztof Wilczyński a écrit : > Hi Christophe, > >> Allocate space for 'bitmap' in 'struct nwl_msi' at build time instead of >> dynamically allocating the memory at runtime. >> >> This simplifies code (especially error handling paths) and avoid some >> open-coded arithmetic in allocator arguments >> >> This also fixes a potential memory leak. The bitmap was never freed. It is >> now part of a managed resource. > > Just to confirm - you mean potentially leaking when the driver would be > unloaded? Not the error handling path, correct?
Correct, the leak would happen on driver unload only.
CJ
> >> --- a/drivers/pci/controller/pcie-xilinx-nwl.c >> +++ b/drivers/pci/controller/pcie-xilinx-nwl.c >> @@ -146,7 +146,7 @@ >> >> struct nwl_msi { /* MSI information */ >> struct irq_domain *msi_domain; >> - unsigned long *bitmap; >> + DECLARE_BITMAP(bitmap, INT_PCI_MSI_NR); >> struct irq_domain *dev_domain; >> struct mutex lock; /* protect bitmap variable */ >> int irq_msi0; >> @@ -335,12 +335,10 @@ static void nwl_pcie_leg_handler(struct irq_desc *desc) >> >> static void nwl_pcie_handle_msi_irq(struct nwl_pcie *pcie, u32 status_reg) >> { >> - struct nwl_msi *msi; >> + struct nwl_msi *msi = &pcie->msi; >> unsigned long status; >> u32 bit; >> >> - msi = &pcie->msi; >> - >> while ((status = nwl_bridge_readl(pcie, status_reg)) != 0) { >> for_each_set_bit(bit, &status, 32) { >> nwl_bridge_writel(pcie, 1 << bit, status_reg); >> @@ -560,30 +558,21 @@ static int nwl_pcie_enable_msi(struct nwl_pcie *pcie) >> struct nwl_msi *msi = &pcie->msi; >> unsigned long base; >> int ret; >> - int size = BITS_TO_LONGS(INT_PCI_MSI_NR) * sizeof(long); >> >> mutex_init(&msi->lock); >> >> - msi->bitmap = kzalloc(size, GFP_KERNEL); >> - if (!msi->bitmap) >> - return -ENOMEM; >> - >> /* Get msi_1 IRQ number */ >> msi->irq_msi1 = platform_get_irq_byname(pdev, "msi1"); >> - if (msi->irq_msi1 < 0) { >> - ret = -EINVAL; >> - goto err; >> - } >> + if (msi->irq_msi1 < 0) >> + return -EINVAL; >> >> irq_set_chained_handler_and_data(msi->irq_msi1, >> nwl_pcie_msi_handler_high, pcie); >> >> /* Get msi_0 IRQ number */ >> msi->irq_msi0 = platform_get_irq_byname(pdev, "msi0"); >> - if (msi->irq_msi0 < 0) { >> - ret = -EINVAL; >> - goto err; >> - } >> + if (msi->irq_msi0 < 0) >> + return -EINVAL; >> >> irq_set_chained_handler_and_data(msi->irq_msi0, >> nwl_pcie_msi_handler_low, pcie); >> @@ -592,8 +581,7 @@ static int nwl_pcie_enable_msi(struct nwl_pcie *pcie) >> ret = nwl_bridge_readl(pcie, I_MSII_CAPABILITIES) & MSII_PRESENT; >> if (!ret) { >> dev_err(dev, "MSI not present\n"); >> - ret = -EIO; >> - goto err; >> + return -EIO; >> } >> >> /* Enable MSII */ >> @@ -632,10 +620,6 @@ static int nwl_pcie_enable_msi(struct nwl_pcie *pcie) >> nwl_bridge_writel(pcie, MSGF_MSI_SR_LO_MASK, MSGF_MSI_MASK_LO); >> >> return 0; >> -err: >> - kfree(msi->bitmap); >> - msi->bitmap = NULL; >> - return ret; > > Thank you! > > Reviewed-by: Krzysztof Wilczyński <kw@linux.com> > > Krzysztof >
| |