Re: [PATCH] PCI: xilinx-nwl: Simplify code and fix a memory leak

[Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>]

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