Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v3 2/4] drivers/staging/fsl-mc: Fix DPIO error path issues
To:     roy.pledge@nxp.com, devel@driverdev.osuosl.org,
        linux-arm-kernel@lists.infradead.org
Cc:     ruxandra.radulescu@nxp.com, arnd@arndb.de,
        gregkh@linuxfoundation.org, horia.geanta@nxp.com,
        linux-kernel@vger.kernel.org, leoyang.li@nxp.com,
        stuyoder@gmail.com, catalin.marinas@arm.com,
        laurentiu.tudor@nxp.com
References: <1522091134-24646-1-git-send-email-roy.pledge@nxp.com>
 <1522091134-24646-3-git-send-email-roy.pledge@nxp.com>
From:   Robin Murphy <robin.murphy@arm.com>
Message-ID: <5e91615b-b2b4-4fe3-4d25-1ae56624976b@arm.com>
Date:   Tue, 27 Mar 2018 12:05:15 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <1522091134-24646-3-git-send-email-roy.pledge@nxp.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Roy,

On 26/03/18 20:05, Roy Pledge wrote:
> The error path in the dpaa2_dpio_probe() function was not properly
> unmapping the QBMan device memory on the error path. This was also
> missing from the dpaa2_dpio_release() function.
> 
> Also addresses a memory leak of the device private data structure.
> 
> Signed-off-by: Roy Pledge <roy.pledge@nxp.com>
> ---
>   drivers/staging/fsl-mc/bus/dpio/dpio-driver.c | 49 +++++++++++++++++++--------
>   1 file changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/fsl-mc/bus/dpio/dpio-driver.c b/drivers/staging/fsl-mc/bus/dpio/dpio-driver.c
> index e00f473..e7a0009 100644
> --- a/drivers/staging/fsl-mc/bus/dpio/dpio-driver.c
> +++ b/drivers/staging/fsl-mc/bus/dpio/dpio-driver.c
> @@ -28,6 +28,7 @@ MODULE_DESCRIPTION("DPIO Driver");
>   
>   struct dpio_priv {
>   	struct dpaa2_io *io;
> +	struct dpaa2_io_desc desc;
>   };
>   
>   static irqreturn_t dpio_irq_handler(int irq_num, void *arg)
> @@ -85,7 +86,6 @@ static int register_dpio_irq_handlers(struct fsl_mc_device *dpio_dev, int cpu)
>   static int dpaa2_dpio_probe(struct fsl_mc_device *dpio_dev)
>   {
>   	struct dpio_attr dpio_attrs;
> -	struct dpaa2_io_desc desc;
>   	struct dpio_priv *priv;
>   	int err = -ENOMEM;
>   	struct device *dev = &dpio_dev->dev;
> @@ -117,7 +117,7 @@ static int dpaa2_dpio_probe(struct fsl_mc_device *dpio_dev)
>   		dev_err(dev, "dpio_get_attributes() failed %d\n", err);
>   		goto err_get_attr;
>   	}
> -	desc.qman_version = dpio_attrs.qbman_version;
> +	priv->desc.qman_version = dpio_attrs.qbman_version;
>   
>   	err = dpio_enable(dpio_dev->mc_io, 0, dpio_dev->mc_handle);
>   	if (err) {
> @@ -126,9 +126,9 @@ static int dpaa2_dpio_probe(struct fsl_mc_device *dpio_dev)
>   	}
>   
>   	/* initialize DPIO descriptor */
> -	desc.receives_notifications = dpio_attrs.num_priorities ? 1 : 0;
> -	desc.has_8prio = dpio_attrs.num_priorities == 8 ? 1 : 0;
> -	desc.dpio_id = dpio_dev->obj_desc.id;
> +	priv->desc.receives_notifications = dpio_attrs.num_priorities ? 1 : 0;
> +	priv->desc.has_8prio = dpio_attrs.num_priorities == 8 ? 1 : 0;
> +	priv->desc.dpio_id = dpio_dev->obj_desc.id;
>   
>   	/* get the cpu to use for the affinity hint */
>   	if (next_cpu == -1)
> @@ -139,19 +139,28 @@ static int dpaa2_dpio_probe(struct fsl_mc_device *dpio_dev)
>   	if (!cpu_possible(next_cpu)) {
>   		dev_err(dev, "probe failed. Number of DPIOs exceeds NR_CPUS.\n");
>   		err = -ERANGE;
> -		goto err_allocate_irqs;
> +		goto err_too_many_cpu;
>   	}
> -	desc.cpu = next_cpu;
> +	priv->desc.cpu = next_cpu;
>   
>   	/*
>   	 * Set the CENA regs to be the cache inhibited area of the portal to
>   	 * avoid coherency issues if a user migrates to another core.
>   	 */
> -	desc.regs_cena = memremap(dpio_dev->regions[1].start,
> -				  resource_size(&dpio_dev->regions[1]),
> -				  MEMREMAP_WC);
> -	desc.regs_cinh = ioremap(dpio_dev->regions[1].start,
> -				 resource_size(&dpio_dev->regions[1]));
> +	priv->desc.regs_cena = memremap(dpio_dev->regions[1].start,
> +					resource_size(&dpio_dev->regions[1]),
> +					MEMREMAP_WC);

Since you already have some devres-managed resources in this driver, 
maybe use devm_memremap? (and perhaps convert the existing ioremap too)

> +	if (!priv->desc.regs_cena) {
> +		dev_err(dev, "memremap failed\n");
> +		goto err_too_many_cpu;
> +	}
> +
> +	priv->desc.regs_cinh = ioremap(dpio_dev->regions[1].start,
> +				       resource_size(&dpio_dev->regions[1]));
> +	if (!priv->desc.regs_cinh) {
> +		dev_err(dev, "ioremap failed\n");
> +		goto err_ioremap_failed;
> +	}
>   
>   	err = fsl_mc_allocate_irqs(dpio_dev);
>   	if (err) {
> @@ -159,11 +168,11 @@ static int dpaa2_dpio_probe(struct fsl_mc_device *dpio_dev)
>   		goto err_allocate_irqs;
>   	}
>   
> -	err = register_dpio_irq_handlers(dpio_dev, desc.cpu);
> +	err = register_dpio_irq_handlers(dpio_dev, priv->desc.cpu);
>   	if (err)
>   		goto err_register_dpio_irq;
>   
> -	priv->io = dpaa2_io_create(&desc);
> +	priv->io = dpaa2_io_create(&priv->desc);
>   	if (!priv->io) {
>   		dev_err(dev, "dpaa2_io_create failed\n");
>   		goto err_dpaa2_io_create;
> @@ -171,7 +180,7 @@ static int dpaa2_dpio_probe(struct fsl_mc_device *dpio_dev)
>   
>   	dev_info(dev, "probed\n");
>   	dev_dbg(dev, "   receives_notifications = %d\n",
> -		desc.receives_notifications);
> +		priv->desc.receives_notifications);
>   	dpio_close(dpio_dev->mc_io, 0, dpio_dev->mc_handle);
>   	fsl_mc_portal_free(dpio_dev->mc_io);
>   
> @@ -182,6 +191,10 @@ static int dpaa2_dpio_probe(struct fsl_mc_device *dpio_dev)
>   err_register_dpio_irq:
>   	fsl_mc_free_irqs(dpio_dev);
>   err_allocate_irqs:
> +	iounmap(priv->desc.regs_cinh);
> +err_ioremap_failed:
> +	memunmap(priv->desc.regs_cena);

...then you wouldn't need to worry about this.

> +err_too_many_cpu:
>   	dpio_disable(dpio_dev->mc_io, 0, dpio_dev->mc_handle);
>   err_get_attr:
>   	dpio_close(dpio_dev->mc_io, 0, dpio_dev->mc_handle);
> @@ -189,6 +202,7 @@ static int dpaa2_dpio_probe(struct fsl_mc_device *dpio_dev)
>   	fsl_mc_portal_free(dpio_dev->mc_io);
>   err_mcportal:
>   	dev_set_drvdata(dev, NULL);
> +	devm_kfree(dev, priv);

The whole point of devm_* is that you don't need to do this - the devres 
entries are freed automatically by the driver core upon probe failure or 
driver detach, i.e. priv was never leaking.

>   err_priv_alloc:
>   	return err;
>   }
> @@ -230,8 +244,13 @@ static int dpaa2_dpio_remove(struct fsl_mc_device *dpio_dev)
>   
>   	dpio_close(dpio_dev->mc_io, 0, dpio_dev->mc_handle);
>   
> +	iounmap(priv->desc.regs_cinh);
> +	memunmap(priv->desc.regs_cena);
> +
>   	fsl_mc_portal_free(dpio_dev->mc_io);
>   
> +	devm_kfree(dev, priv);
> +
>   	dev_set_drvdata(dev, NULL);

Note that clearing drvdata is something else the driver core already 
does at about the same time as cleaning up devres entries (see 
__device_release_driver() and the failure path of really_probe(), in 
drivers/base/dd.c), so this has been similarly superfluous all along.

Robin.