drives/soc/fsl: Move DPIO driver out of staging

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 <[email protected]>

---
  drivers/staging/fsl-mc/bus/dpio/dpio-driver.c | 49 +++++++++++++++++++--------
  1 file changed, 34 insertions(+), 15 deletions(-)

drivers/staging/fsl-mc/bus/dpio/dpio-driver.c +7 -7

index e00f473..e7a0009 100644
@@ -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

drivers/staging/fsl-mc/bus/dpio/dpio-driver.c +14 -3

+	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);
 

drivers/staging/fsl-mc/bus/dpio/dpio-driver.c +0 -0

  err_priv_alloc:
  	return err;
  }

@@ -230,8 +244,13 @@ static int dpaa2_dpio_remove(struct fsl_mc_device *dpio_dev)
 

Hi Roy,

Quoted 63 lines · Roy Pledge

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 <[email protected]>

---
   drivers/staging/fsl-mc/bus/dpio/dpio-driver.c | 49 +++++++++++++++++++--------
   1 file changed, 34 insertions(+), 15 deletions(-)

drivers/staging/fsl-mc/bus/dpio/dpio-driver.c +7 -7

index e00f473..e7a0009 100644
@@ -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;
 

drivers/staging/fsl-mc/bus/dpio/dpio-driver.c static int dpaa2_dpio_probe(struct fsl_mc_device *dpio_dev) · line 152

Quoted 12 lines

drivers/staging/fsl-mc/bus/dpio/dpio-driver.c +3 -5

   	/*
   	 * 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)

Quoted 39 lines

drivers/staging/fsl-mc/bus/dpio/dpio-driver.c +14 -3

+	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);
 

drivers/staging/fsl-mc/bus/dpio/dpio-driver.c static int dpaa2_dpio_probe(struct fsl_mc_device *dpio_dev) · line 196

Quoted 6 lines

drivers/staging/fsl-mc/bus/dpio/dpio-driver.c +3 -0

@@ -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.

drivers/staging/fsl-mc/bus/dpio/dpio-driver.c static int dpaa2_dpio_probe(struct fsl_mc_device *dpio_dev) · line 205

Quoted 9 lines

drivers/staging/fsl-mc/bus/dpio/dpio-driver.c +2 -0

+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.

Quoted 13 lines

drivers/staging/fsl-mc/bus/dpio/dpio-driver.c +3 -0

   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);
 

drivers/staging/fsl-mc/bus/dpio/dpio-driver.c static int dpaa2_dpio_remove(struct fsl_mc_device *dpio_dev) · line 254

Quoted 3 lines

drivers/staging/fsl-mc/bus/dpio/dpio-driver.c +2 -0

+	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.