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;
From:   Roy Pledge <roy.pledge@nxp.com>
To:     Robin Murphy <robin.murphy@arm.com>,
        "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>
CC:     Ruxandra Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com>,
        "arnd@arndb.de" <arnd@arndb.de>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        =?iso-8859-2?Q?Horia_Geant=E3?= <horia.geanta@nxp.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Leo Li <leoyang.li@nxp.com>,
        "stuyoder@gmail.com" <stuyoder@gmail.com>,
        "catalin.marinas@arm.com" <catalin.marinas@arm.com>,
        Laurentiu Tudor <laurentiu.tudor@nxp.com>
Subject: Re: [PATCH v3 2/4] drivers/staging/fsl-mc: Fix DPIO error path issues
Thread-Topic: [PATCH v3 2/4] drivers/staging/fsl-mc: Fix DPIO error path
 issues
Thread-Index: AQHTxTV7QTuW6ZX0tUiE2xLwpXsWdQ==
Date:   Tue, 27 Mar 2018 13:19:48 +0000
Message-ID: <DB6PR04MB2999389243117938BCBA141686AC0@DB6PR04MB2999.eurprd04.prod.outlook.com>
References: <1522091134-24646-1-git-send-email-roy.pledge@nxp.com>
 <1522091134-24646-3-git-send-email-roy.pledge@nxp.com>
 <5e91615b-b2b4-4fe3-4d25-1ae56624976b@arm.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="iso-8859-2"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 933baa8a-5420-44a7-5add-08d593e56aad
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Mar 2018 13:19:48.8864
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR04MB2965
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 3/27/2018 7:05 AM, Robin Murphy wrote:=0A=
> Hi Roy,=0A=
>=0A=
> On 26/03/18 20:05, Roy Pledge wrote:=0A=
>> The error path in the dpaa2_dpio_probe() function was not properly=0A=
>> unmapping the QBMan device memory on the error path. This was also=0A=
>> missing from the dpaa2_dpio_release() function.=0A=
>>=0A=
>> Also addresses a memory leak of the device private data structure.=0A=
>>=0A=
>> Signed-off-by: Roy Pledge <roy.pledge@nxp.com>=0A=
>> ---=0A=
>>    drivers/staging/fsl-mc/bus/dpio/dpio-driver.c | 49 ++++++++++++++++++=
+--------=0A=
>>    1 file changed, 34 insertions(+), 15 deletions(-)=0A=
>>=0A=
>> diff --git a/drivers/staging/fsl-mc/bus/dpio/dpio-driver.c b/drivers/sta=
ging/fsl-mc/bus/dpio/dpio-driver.c=0A=
>> index e00f473..e7a0009 100644=0A=
>> --- a/drivers/staging/fsl-mc/bus/dpio/dpio-driver.c=0A=
>> +++ b/drivers/staging/fsl-mc/bus/dpio/dpio-driver.c=0A=
>> @@ -28,6 +28,7 @@ MODULE_DESCRIPTION("DPIO Driver");=0A=
>>    =0A=
>>    struct dpio_priv {=0A=
>>    	struct dpaa2_io *io;=0A=
>> +	struct dpaa2_io_desc desc;=0A=
>>    };=0A=
>>    =0A=
>>    static irqreturn_t dpio_irq_handler(int irq_num, void *arg)=0A=
>> @@ -85,7 +86,6 @@ static int register_dpio_irq_handlers(struct fsl_mc_de=
vice *dpio_dev, int cpu)=0A=
>>    static int dpaa2_dpio_probe(struct fsl_mc_device *dpio_dev)=0A=
>>    {=0A=
>>    	struct dpio_attr dpio_attrs;=0A=
>> -	struct dpaa2_io_desc desc;=0A=
>>    	struct dpio_priv *priv;=0A=
>>    	int err =3D -ENOMEM;=0A=
>>    	struct device *dev =3D &dpio_dev->dev;=0A=
>> @@ -117,7 +117,7 @@ static int dpaa2_dpio_probe(struct fsl_mc_device *dp=
io_dev)=0A=
>>    		dev_err(dev, "dpio_get_attributes() failed %d\n", err);=0A=
>>    		goto err_get_attr;=0A=
>>    	}=0A=
>> -	desc.qman_version =3D dpio_attrs.qbman_version;=0A=
>> +	priv->desc.qman_version =3D dpio_attrs.qbman_version;=0A=
>>    =0A=
>>    	err =3D dpio_enable(dpio_dev->mc_io, 0, dpio_dev->mc_handle);=0A=
>>    	if (err) {=0A=
>> @@ -126,9 +126,9 @@ static int dpaa2_dpio_probe(struct fsl_mc_device *dp=
io_dev)=0A=
>>    	}=0A=
>>    =0A=
>>    	/* initialize DPIO descriptor */=0A=
>> -	desc.receives_notifications =3D dpio_attrs.num_priorities ? 1 : 0;=0A=
>> -	desc.has_8prio =3D dpio_attrs.num_priorities =3D=3D 8 ? 1 : 0;=0A=
>> -	desc.dpio_id =3D dpio_dev->obj_desc.id;=0A=
>> +	priv->desc.receives_notifications =3D dpio_attrs.num_priorities ? 1 : =
0;=0A=
>> +	priv->desc.has_8prio =3D dpio_attrs.num_priorities =3D=3D 8 ? 1 : 0;=
=0A=
>> +	priv->desc.dpio_id =3D dpio_dev->obj_desc.id;=0A=
>>    =0A=
>>    	/* get the cpu to use for the affinity hint */=0A=
>>    	if (next_cpu =3D=3D -1)=0A=
>> @@ -139,19 +139,28 @@ static int dpaa2_dpio_probe(struct fsl_mc_device *=
dpio_dev)=0A=
>>    	if (!cpu_possible(next_cpu)) {=0A=
>>    		dev_err(dev, "probe failed. Number of DPIOs exceeds NR_CPUS.\n");=
=0A=
>>    		err =3D -ERANGE;=0A=
>> -		goto err_allocate_irqs;=0A=
>> +		goto err_too_many_cpu;=0A=
>>    	}=0A=
>> -	desc.cpu =3D next_cpu;=0A=
>> +	priv->desc.cpu =3D next_cpu;=0A=
>>    =0A=
>>    	/*=0A=
>>    	 * Set the CENA regs to be the cache inhibited area of the portal to=
=0A=
>>    	 * avoid coherency issues if a user migrates to another core.=0A=
>>    	 */=0A=
>> -	desc.regs_cena =3D memremap(dpio_dev->regions[1].start,=0A=
>> -				  resource_size(&dpio_dev->regions[1]),=0A=
>> -				  MEMREMAP_WC);=0A=
>> -	desc.regs_cinh =3D ioremap(dpio_dev->regions[1].start,=0A=
>> -				 resource_size(&dpio_dev->regions[1]));=0A=
>> +	priv->desc.regs_cena =3D memremap(dpio_dev->regions[1].start,=0A=
>> +					resource_size(&dpio_dev->regions[1]),=0A=
>> +					MEMREMAP_WC);=0A=
> Since you already have some devres-managed resources in this driver,=0A=
> maybe use devm_memremap? (and perhaps convert the existing ioremap too)=
=0A=
That would make since - will do.=0A=
>=0A=
>> +	if (!priv->desc.regs_cena) {=0A=
>> +		dev_err(dev, "memremap failed\n");=0A=
>> +		goto err_too_many_cpu;=0A=
>> +	}=0A=
>> +=0A=
>> +	priv->desc.regs_cinh =3D ioremap(dpio_dev->regions[1].start,=0A=
>> +				       resource_size(&dpio_dev->regions[1]));=0A=
>> +	if (!priv->desc.regs_cinh) {=0A=
>> +		dev_err(dev, "ioremap failed\n");=0A=
>> +		goto err_ioremap_failed;=0A=
>> +	}=0A=
>>    =0A=
>>    	err =3D fsl_mc_allocate_irqs(dpio_dev);=0A=
>>    	if (err) {=0A=
>> @@ -159,11 +168,11 @@ static int dpaa2_dpio_probe(struct fsl_mc_device *=
dpio_dev)=0A=
>>    		goto err_allocate_irqs;=0A=
>>    	}=0A=
>>    =0A=
>> -	err =3D register_dpio_irq_handlers(dpio_dev, desc.cpu);=0A=
>> +	err =3D register_dpio_irq_handlers(dpio_dev, priv->desc.cpu);=0A=
>>    	if (err)=0A=
>>    		goto err_register_dpio_irq;=0A=
>>    =0A=
>> -	priv->io =3D dpaa2_io_create(&desc);=0A=
>> +	priv->io =3D dpaa2_io_create(&priv->desc);=0A=
>>    	if (!priv->io) {=0A=
>>    		dev_err(dev, "dpaa2_io_create failed\n");=0A=
>>    		goto err_dpaa2_io_create;=0A=
>> @@ -171,7 +180,7 @@ static int dpaa2_dpio_probe(struct fsl_mc_device *dp=
io_dev)=0A=
>>    =0A=
>>    	dev_info(dev, "probed\n");=0A=
>>    	dev_dbg(dev, "   receives_notifications =3D %d\n",=0A=
>> -		desc.receives_notifications);=0A=
>> +		priv->desc.receives_notifications);=0A=
>>    	dpio_close(dpio_dev->mc_io, 0, dpio_dev->mc_handle);=0A=
>>    	fsl_mc_portal_free(dpio_dev->mc_io);=0A=
>>    =0A=
>> @@ -182,6 +191,10 @@ static int dpaa2_dpio_probe(struct fsl_mc_device *d=
pio_dev)=0A=
>>    err_register_dpio_irq:=0A=
>>    	fsl_mc_free_irqs(dpio_dev);=0A=
>>    err_allocate_irqs:=0A=
>> +	iounmap(priv->desc.regs_cinh);=0A=
>> +err_ioremap_failed:=0A=
>> +	memunmap(priv->desc.regs_cena);=0A=
> ...then you wouldn't need to worry about this.=0A=
>=0A=
>> +err_too_many_cpu:=0A=
>>    	dpio_disable(dpio_dev->mc_io, 0, dpio_dev->mc_handle);=0A=
>>    err_get_attr:=0A=
>>    	dpio_close(dpio_dev->mc_io, 0, dpio_dev->mc_handle);=0A=
>> @@ -189,6 +202,7 @@ static int dpaa2_dpio_probe(struct fsl_mc_device *dp=
io_dev)=0A=
>>    	fsl_mc_portal_free(dpio_dev->mc_io);=0A=
>>    err_mcportal:=0A=
>>    	dev_set_drvdata(dev, NULL);=0A=
>> +	devm_kfree(dev, priv);=0A=
> The whole point of devm_* is that you don't need to do this - the devres=
=0A=
> entries are freed automatically by the driver core upon probe failure or=
=0A=
> driver detach, i.e. priv was never leaking.=0A=
Right - Not sure what I was thinking yesterday. I think I saw the alloc =0A=
without free and instinctively=0A=
added it.=A0 I will remove this.=0A=
>=0A=
>>    err_priv_alloc:=0A=
>>    	return err;=0A=
>>    }=0A=
>> @@ -230,8 +244,13 @@ static int dpaa2_dpio_remove(struct fsl_mc_device *=
dpio_dev)=0A=
>>    =0A=
>>    	dpio_close(dpio_dev->mc_io, 0, dpio_dev->mc_handle);=0A=
>>    =0A=
>> +	iounmap(priv->desc.regs_cinh);=0A=
>> +	memunmap(priv->desc.regs_cena);=0A=
>> +=0A=
>>    	fsl_mc_portal_free(dpio_dev->mc_io);=0A=
>>    =0A=
>> +	devm_kfree(dev, priv);=0A=
>> +=0A=
>>    	dev_set_drvdata(dev, NULL);=0A=
> Note that clearing drvdata is something else the driver core already=0A=
> does at about the same time as cleaning up devres entries (see=0A=
> __device_release_driver() and the failure path of really_probe(), in=0A=
> drivers/base/dd.c), so this has been similarly superfluous all along.=0A=
Yes I'll clean this up too.=A0 Thanks for the comments, this will make =0A=
things better for sure.=0A=
>=0A=
> Robin.=0A=
>=0A=
=0A=