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.
Signed-off-by: Christophe JAILLET <[email protected]>
---
drivers/pci/controller/pcie-xilinx-nwl.c | 30 ++++++------------------
1 file changed, 7 insertions(+), 23 deletions(-)
diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
index a72b4f9a2b00..40d070e54ad2 100644
--- 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;
}
static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
--
2.30.2
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?
> --- 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 <[email protected]>
Krzysztof
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 <[email protected]>
>
> Krzysztof
>
On Sun, 7 Nov 2021 11:04:43 +0100, Christophe JAILLET wrote:
> 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.
>
> [...]
Applied to pci/xilinx-nwl, thanks!
[1/1] PCI: xilinx-nwl: Simplify code and fix a memory leak
https://git.kernel.org/lpieralisi/pci/c/e2b86f9777
Thanks,
Lorenzo
On Sun, Nov 07, 2021 at 11:04:43AM +0100, Christophe JAILLET wrote:
> Allocate space for 'bitmap' in 'struct nwl_msi' at build time instead of
> dynamically allocating the memory at runtime.
Definitely a good change. To be pedantic, I don't think this converts
the alloc to *build* time. It converts it to probe-time, when
nwl_pcie_probe() calls devm_pci_alloc_host_bridge().
> 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.
>
> Signed-off-by: Christophe JAILLET <[email protected]>
> ---
> drivers/pci/controller/pcie-xilinx-nwl.c | 30 ++++++------------------
> 1 file changed, 7 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
> index a72b4f9a2b00..40d070e54ad2 100644
> --- 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;
> }
>
> static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
> --
> 2.30.2
>