FPGA Manager devres cleanup

From: Russ Weight <[email protected]>

The stratix10-soc driver uses fpga_mgr_create() function and is therefore responsible to call fpga_mgr_free() to release the class driver resources. Add a missing call to fpga_mgr_free in the s10_remove() function.

Signed-off-by Russ Weight <[email protected]>

Reviewed-by Xu Yilun <[email protected]>

Signed-off-by Moritz Fischer <[email protected]>

---
 drivers/fpga/stratix10-soc.c | 1 +
 1 file changed, 1 insertion(+)

drivers/fpga/stratix10-soc.c +1 -0

index 657a70c5fc99..9e34bbbce26e 100644
@@ -454,6 +454,7 @@ static int s10_remove(struct platform_device *pdev)
 	struct s10_priv *priv = mgr->priv;
 
 	fpga_mgr_unregister(mgr);
+	fpga_mgr_free(mgr);
 	stratix10_svc_free_channel(priv->chan);
 
 	return 0;

Signature · 1 line

2.31.1

Quoted 24 lines · Moritz Fischer

From: Russ Weight <[email protected]>

The stratix10-soc driver uses fpga_mgr_create() function and is therefore responsible to call fpga_mgr_free() to release the class driver resources. Add a missing call to fpga_mgr_free in the s10_remove() function.

Signed-off-by Russ Weight <[email protected]>

Reviewed-by Xu Yilun <[email protected]>

Signed-off-by Moritz Fischer <[email protected]>

---
 drivers/fpga/stratix10-soc.c | 1 +
 1 file changed, 1 insertion(+)

drivers/fpga/stratix10-soc.c +1 -0

index 657a70c5fc99..9e34bbbce26e 100644
@@ -454,6 +454,7 @@ static int s10_remove(struct platform_device *pdev)
 	struct s10_priv *priv = mgr->priv;
 
 	fpga_mgr_unregister(mgr);
+	fpga_mgr_free(mgr);
 	stratix10_svc_free_channel(priv->chan);
 

Quoted 4 lines

drivers/fpga/stratix10-soc.c +0 -0

 	return 0;

Signature · 1 line

2.31.1

Does this fix a specific commit? Does it need a Fixes: and cc: stable line too?

thanks,

greg k-h

Quoted 25 lines · Greg KH

Quoted 24 lines · Moritz Fischer

From: Russ Weight <[email protected]>

The stratix10-soc driver uses fpga_mgr_create() function and is therefore responsible to call fpga_mgr_free() to release the class driver resources. Add a missing call to fpga_mgr_free in the s10_remove() function.

Signed-off-by Russ Weight <[email protected]>

Reviewed-by Xu Yilun <[email protected]>

Signed-off-by Moritz Fischer <[email protected]>

---
 drivers/fpga/stratix10-soc.c | 1 +
 1 file changed, 1 insertion(+)

drivers/fpga/stratix10-soc.c +1 -0

index 657a70c5fc99..9e34bbbce26e 100644
@@ -454,6 +454,7 @@ static int s10_remove(struct platform_device *pdev)
 	struct s10_priv *priv = mgr->priv;
 
 	fpga_mgr_unregister(mgr);
+	fpga_mgr_free(mgr);
 	stratix10_svc_free_channel(priv->chan);
 

Quoted 6 lines

Quoted 4 lines

drivers/fpga/stratix10-soc.c +0 -0

 	return 0;

Signature · 1 line

2.31.1

Does this fix a specific commit? Does it need a Fixes: and cc: stable line too?

It fixes:

e7eef1d7633a fpga: add intel stratix10 soc fpga manager driver

And yes, I think it needs the Fixes: and cc: stable lines

Moritz: Let me know if you want me to add the tags

Greg: Patch 1 in this series could be viewed as a fix for a particular commit as well, but the code being deleted is harmless/unused. I'm assuming it does NOT need the Fixes: and cc: stable lines?

Quoted 24 lines · Moritz Fischer

From: Russ Weight <[email protected]>

The stratix10-soc driver uses fpga_mgr_create() function and is therefore responsible to call fpga_mgr_free() to release the class driver resources. Add a missing call to fpga_mgr_free in the s10_remove() function.

Signed-off-by Russ Weight <[email protected]>

Reviewed-by Xu Yilun <[email protected]>

Signed-off-by Moritz Fischer <[email protected]>

---
 drivers/fpga/stratix10-soc.c | 1 +
 1 file changed, 1 insertion(+)

drivers/fpga/stratix10-soc.c +1 -0

index 657a70c5fc99..9e34bbbce26e 100644
@@ -454,6 +454,7 @@ static int s10_remove(struct platform_device *pdev)
 	struct s10_priv *priv = mgr->priv;
 
 	fpga_mgr_unregister(mgr);
+	fpga_mgr_free(mgr);
 	stratix10_svc_free_channel(priv->chan);
 

Quoted 4 lines

drivers/fpga/stratix10-soc.c +0 -0

 	return 0;

Signature · 1 line

2.31.1

Does this fix a specific commit? Does it need a Fixes: and cc: stable line too?

Quoted 25 lines · Greg KH

Quoted 24 lines · Moritz Fischer

From: Russ Weight <[email protected]>

The stratix10-soc driver uses fpga_mgr_create() function and is therefore responsible to call fpga_mgr_free() to release the class driver resources. Add a missing call to fpga_mgr_free in the s10_remove() function.

Signed-off-by Russ Weight <[email protected]>

Reviewed-by Xu Yilun <[email protected]>

Signed-off-by Moritz Fischer <[email protected]>

---
 drivers/fpga/stratix10-soc.c | 1 +
 1 file changed, 1 insertion(+)

drivers/fpga/stratix10-soc.c +1 -0

index 657a70c5fc99..9e34bbbce26e 100644
@@ -454,6 +454,7 @@ static int s10_remove(struct platform_device *pdev)
 	struct s10_priv *priv = mgr->priv;
 
 	fpga_mgr_unregister(mgr);
+	fpga_mgr_free(mgr);
 	stratix10_svc_free_channel(priv->chan);
 

Quoted 7 lines

Quoted 4 lines

drivers/fpga/stratix10-soc.c +0 -0

 	return 0;

Signature · 1 line

2.31.1

Does this fix a specific commit? Does it need a Fixes: and cc: stable line too?

Yes, I missed this. I think this should be:

Fixes e7eef1d7633a ("fpga: add intel stratix10 soc fpga manager

driver")

I can resend with Cc: stable

From: Russ Weight <[email protected]>

Rename variable "dev" to "parent" in cases where it represents the parent device.

Signed-off-by Russ Weight <[email protected]>

Reviewed-by Xu Yilun <[email protected]>

Signed-off-by Moritz Fischer <[email protected]>

---
  drivers/fpga/fpga-mgr.c | 20 ++++++++++----------
  1 file changed, 10 insertions(+), 10 deletions(-)

drivers/fpga/fpga-mgr.c +0 -0

index b85bc47c91a9..42ddc0844781 100644

drivers/fpga/fpga-mgr.c +9 -9

   * @name:	fpga manager name
   * @mops:	pointer to structure of fpga manager ops
   * @priv:	fpga manager private data

@@ -561,7 +561,7 @@ EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
   *
   * Return: pointer to struct fpga_manager or NULL
   */
-struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
+struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
  				     const struct fpga_manager_ops *mops,
  				     void *priv)
  {

@@ -571,12 +571,12 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
  	if (!mops || !mops->write_complete || !mops->state ||
  	    !mops->write_init || (!mops->write && !mops->write_sg) ||
  	    (mops->write && mops->write_sg)) {
-		dev_err(dev, "Attempt to register without fpga_manager_ops\n");
+		dev_err(parent, "Attempt to register without fpga_manager_ops\n");
  		return NULL;
  	}
 
  	if (!name || !strlen(name)) {
-		dev_err(dev, "Attempt to register with no name!\n");
+		dev_err(parent, "Attempt to register with no name!\n");
  		return NULL;
  	}
 

@@ -597,8 +597,8 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
  	device_initialize(&mgr->dev);
  	mgr->dev.class = fpga_mgr_class;
  	mgr->dev.groups = mops->groups;
-	mgr->dev.parent = dev;
-	mgr->dev.of_node = dev->of_node;
+	mgr->dev.parent = parent;
+	mgr->dev.of_node = parent->of_node;
  	mgr->dev.id = id;
 
  	ret = dev_set_name(&mgr->dev, "fpga%d", id);

@@ -636,7 +636,7 @@ static void devm_fpga_mgr_release(struct device *dev, void *res)
 
  /**
   * devm_fpga_mgr_create - create and initialize a managed FPGA manager struct
- * @dev:	fpga manager device from pdev
+ * @parent:	fpga manager device from pdev
   * @name:	fpga manager name
   * @mops:	pointer to structure of fpga manager ops
   * @priv:	fpga manager private data

@@ -651,7 +651,7 @@ static void devm_fpga_mgr_release(struct device *dev, void *res)
   *
   * Return: pointer to struct fpga_manager or NULL
   */
-struct fpga_manager *devm_fpga_mgr_create(struct device *dev, const char *name,
+struct fpga_manager *devm_fpga_mgr_create(struct device *parent, const char *name,
  					  const struct fpga_manager_ops *mops,
  					  void *priv)
  {

@@ -661,13 +661,13 @@ struct fpga_manager *devm_fpga_mgr_create(struct device *dev, const char *name,
  	if (!dr)
  		return NULL;
 
-	dr->mgr = fpga_mgr_create(dev, name, mops, priv);
+	dr->mgr = fpga_mgr_create(parent, name, mops, priv);
  	if (!dr->mgr) {
  		devres_free(dr);
  		return NULL;
  	}
 
-	devres_add(dev, dr);
+	devres_add(parent, dr);
 
  	return dr->mgr;
  }

From: Russ Weight <[email protected]>

The FPGA manager class driver data structure is being treated as a managed resource instead of using the class.dev_release call-back function to release the class data structure. This change populates the class.dev_release function, changes the fpga_mgr_free() function to call put_device() and changes the fpga_mgr_unregister() function to call device_del() instead of device_unregister().

Signed-off-by Russ Weight <[email protected]>

Reviewed-by Xu Yilun <[email protected]>

Signed-off-by Moritz Fischer <[email protected]>

---
 drivers/fpga/fpga-mgr.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

drivers/fpga/fpga-mgr.c +9 -13

index 42ddc0844781..9f6c3760b6ff 100644
@@ -585,8 +585,10 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
 		return NULL;
 
 	id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
-	if (id < 0)
-		goto error_kfree;
+	if (id < 0) {
+		kfree(mgr);
+		return NULL;
+	}
 
 	mutex_init(&mgr->ref_mutex);
 

@@ -602,17 +604,12 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
 	mgr->dev.id = id;
 
 	ret = dev_set_name(&mgr->dev, "fpga%d", id);
-	if (ret)
-		goto error_device;
+	if (ret) {
+		put_device(&mgr->dev);
+		return NULL;
+	}
 
 	return mgr;
-
-error_device:
-	ida_simple_remove(&fpga_mgr_ida, id);
-error_kfree:
-	kfree(mgr);
-
-	return NULL;
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_create);
 

@@ -622,8 +619,7 @@ EXPORT_SYMBOL_GPL(fpga_mgr_create);
  */
 void fpga_mgr_free(struct fpga_manager *mgr)
 {
-	ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
-	kfree(mgr);
+	put_device(&mgr->dev);
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_free);
 

Quoted 66 lines · Moritz Fischer

From: Russ Weight <[email protected]>

The FPGA manager class driver data structure is being treated as a managed resource instead of using the class.dev_release call-back function to release the class data structure. This change populates the class.dev_release function, changes the fpga_mgr_free() function to call put_device() and changes the fpga_mgr_unregister() function to call device_del() instead of device_unregister().

Signed-off-by Russ Weight <[email protected]>

Reviewed-by Xu Yilun <[email protected]>

Signed-off-by Moritz Fischer <[email protected]>

---
 drivers/fpga/fpga-mgr.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

drivers/fpga/fpga-mgr.c +9 -13

index 42ddc0844781..9f6c3760b6ff 100644
@@ -585,8 +585,10 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
 		return NULL;
 
 	id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
-	if (id < 0)
-		goto error_kfree;
+	if (id < 0) {
+		kfree(mgr);
+		return NULL;
+	}
 
 	mutex_init(&mgr->ref_mutex);
 

@@ -602,17 +604,12 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
 	mgr->dev.id = id;
 
 	ret = dev_set_name(&mgr->dev, "fpga%d", id);
-	if (ret)
-		goto error_device;
+	if (ret) {
+		put_device(&mgr->dev);
+		return NULL;
+	}
 
 	return mgr;
-
-error_device:
-	ida_simple_remove(&fpga_mgr_ida, id);
-error_kfree:
-	kfree(mgr);
-
-	return NULL;
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_create);
 

@@ -622,8 +619,7 @@ EXPORT_SYMBOL_GPL(fpga_mgr_create);
  */
 void fpga_mgr_free(struct fpga_manager *mgr)
 {
-	ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
-	kfree(mgr);
+	put_device(&mgr->dev);
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_free);
 

drivers/fpga/fpga-mgr.c int fpga_mgr_register(struct fpga_manager *mgr) · line 691

Quoted 5 lines

drivers/fpga/fpga-mgr.c +1 -1

@@ -692,16 +688,11 @@ int fpga_mgr_register(struct fpga_manager *mgr)
 
 	ret = device_add(&mgr->dev);
 	if (ret)
-		goto error_device;
+		return ret;

If this fails, are you sure you want to just return the error number?

You can not call device_del() afterward if this fails, you have to call put_device(). See the documentation for device_add() for details.

This is messy as you are doing a "two step" initialization of your fpga_manager device for some odd reason. Why do you need to do that?

When you call this you seem to be forced to do:
fpga_mgr_create()
fpga_mgr_register()
in each individual driver.

Why force drivers to do this and not just do a simple:
fpga_mgr_register()
that internally does the create/add process?

Why the two steps? That's normally reserved for when you need to do something complex in the "core" for the subsystem, and shouldn't be pushed out to each individual driver like it currently is for the fpga core as you will run into the problem you have here.

Namely when you want to clean up from a failure, you don't know if you really did register that device properly or not.

And no, don't add another flag, just make this simple and hard to get wrong. As it is it feels like each fpga driver has to do extra work to be sure to get this all correct each time.

So I can not take this as-is, sorry.

And sorry I never noticed these problems when the code when in originally.

thanks,

greg k-h

From: Russ Weight <[email protected]>

The FPGA bridge class driver data structure is being treated as a managed resource instead of using standard dev_release call-back to release the class data structure. This change populates the class.dev_release function and changes the fpga_bridge_free() function to call put_device(). It also changes fpga_bridge_unregister() to call device_del() instead of device_unregister().

From: Russ Weight <[email protected]>

The FPGA region class driver data structure is being treated as a managed resource instead of using standard dev_release call-back to release the class data structure. This change populates the class.dev_release function and changes the fpga_region_free() function to call put_device(). It also changes fpga_region_unregister() to call device_del() instead of device_unregister().

Hi Greg,

please consider this series for inclusion into 5.14-rc1.

It attempts to clean up the issues pointed out by you earlier in https://lore.kernel.org/linux-fpga/[email protected]

If you prefer I can provide a signed tag instead.