[PATCH 0/8] FPGA Manager devres cleanup

[PATCH 0/8] FPGA Manager devres cleanup

[Moritz Fischer]

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/YKKuBSLp5Fe0Zh0v@kroah.com

If you prefer I can provide a signed tag instead.

Thanks,

Moritz

Russ Weight (8):
  fpga: altera-pr-ip: Remove function alt_pr_unregister
  fpga: stratix10-soc: Add missing fpga_mgr_free() call
  fpga: mgr: Rename dev to parent for parent device
  fpga: bridge: Rename dev to parent for parent device
  fpga: region: Rename dev to parent for parent device
  fpga: mgr: Use standard dev_release for class driver
  fpga: bridge: Use standard dev_release for class driver
  fpga: region: Use standard dev_release for class driver

 drivers/fpga/altera-pr-ip-core.c       | 10 -----
 drivers/fpga/fpga-bridge.c             | 46 ++++++++++-----------
 drivers/fpga/fpga-mgr.c                | 55 ++++++++++++--------------
 drivers/fpga/fpga-region.c             | 44 ++++++++++-----------
 drivers/fpga/stratix10-soc.c           |  1 +
 include/linux/fpga/altera-pr-ip-core.h |  1 -
 6 files changed, 71 insertions(+), 86 deletions(-)

-- 
2.31.1

[PATCH 1/8] fpga: altera-pr-ip: Remove function alt_pr_unregister

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

Remove the alt_pr_unregister() function; it is no longer used.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/altera-pr-ip-core.c       | 10 ----------
 include/linux/fpga/altera-pr-ip-core.h |  1 -
 2 files changed, 11 deletions(-)

diff --git a/drivers/fpga/altera-pr-ip-core.c b/drivers/fpga/altera-pr-ip-core.c
index 5b130c4d9882..dfdf21ed34c4 100644
--- a/drivers/fpga/altera-pr-ip-core.c
+++ b/drivers/fpga/altera-pr-ip-core.c
@@ -199,16 +199,6 @@ int alt_pr_register(struct device *dev, void __iomem *reg_base)
 }
 EXPORT_SYMBOL_GPL(alt_pr_register);
 
-void alt_pr_unregister(struct device *dev)
-{
-	struct fpga_manager *mgr = dev_get_drvdata(dev);
-
-	dev_dbg(dev, "%s\n", __func__);
-
-	fpga_mgr_unregister(mgr);
-}
-EXPORT_SYMBOL_GPL(alt_pr_unregister);
-
 MODULE_AUTHOR("Matthew Gerlach <matthew.gerlach@linux.intel.com>");
 MODULE_DESCRIPTION("Altera Partial Reconfiguration IP Core");
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/fpga/altera-pr-ip-core.h b/include/linux/fpga/altera-pr-ip-core.h
index 0b08ac20ab16..a6b4c07858cc 100644
--- a/include/linux/fpga/altera-pr-ip-core.h
+++ b/include/linux/fpga/altera-pr-ip-core.h
@@ -13,6 +13,5 @@
 #include <linux/io.h>
 
 int alt_pr_register(struct device *dev, void __iomem *reg_base);
-void alt_pr_unregister(struct device *dev);
 
 #endif /* _ALT_PR_IP_CORE_H */
-- 
2.31.1

[PATCH 2/8] fpga: stratix10-soc: Add missing fpga_mgr_free() call

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

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 <russell.h.weight@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/stratix10-soc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
index 657a70c5fc99..9e34bbbce26e 100644
--- a/drivers/fpga/stratix10-soc.c
+++ b/drivers/fpga/stratix10-soc.c
@@ -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;
-- 
2.31.1

[PATCH 3/8] fpga: mgr: Rename dev to parent for parent device

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

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

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/fpga-mgr.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index b85bc47c91a9..42ddc0844781 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -551,7 +551,7 @@ EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
 
 /**
  * fpga_mgr_create - create and initialize a 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
@@ -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;
 }
-- 
2.31.1

[PATCH 4/8] fpga: bridge: Rename dev to parent for parent device

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

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

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/fpga-bridge.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
index e9266b2a357f..d4aca0f724f5 100644
--- a/drivers/fpga/fpga-bridge.c
+++ b/drivers/fpga/fpga-bridge.c
@@ -313,7 +313,7 @@ ATTRIBUTE_GROUPS(fpga_bridge);
 
 /**
  * fpga_bridge_create - create and initialize a struct fpga_bridge
- * @dev:	FPGA bridge device from pdev
+ * @parent:	FPGA bridge device from pdev
  * @name:	FPGA bridge name
  * @br_ops:	pointer to structure of fpga bridge ops
  * @priv:	FPGA bridge private data
@@ -323,7 +323,7 @@ ATTRIBUTE_GROUPS(fpga_bridge);
  *
  * Return: struct fpga_bridge or NULL
  */
-struct fpga_bridge *fpga_bridge_create(struct device *dev, const char *name,
+struct fpga_bridge *fpga_bridge_create(struct device *parent, const char *name,
 				       const struct fpga_bridge_ops *br_ops,
 				       void *priv)
 {
@@ -331,7 +331,7 @@ struct fpga_bridge *fpga_bridge_create(struct device *dev, const char *name,
 	int id, ret;
 
 	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;
 	}
 
@@ -353,8 +353,8 @@ struct fpga_bridge *fpga_bridge_create(struct device *dev, const char *name,
 	device_initialize(&bridge->dev);
 	bridge->dev.groups = br_ops->groups;
 	bridge->dev.class = fpga_bridge_class;
-	bridge->dev.parent = dev;
-	bridge->dev.of_node = dev->of_node;
+	bridge->dev.parent = parent;
+	bridge->dev.of_node = parent->of_node;
 	bridge->dev.id = id;
 
 	ret = dev_set_name(&bridge->dev, "br%d", id);
@@ -392,7 +392,7 @@ static void devm_fpga_bridge_release(struct device *dev, void *res)
 
 /**
  * devm_fpga_bridge_create - create and init a managed struct fpga_bridge
- * @dev:	FPGA bridge device from pdev
+ * @parent:	FPGA bridge device from pdev
  * @name:	FPGA bridge name
  * @br_ops:	pointer to structure of fpga bridge ops
  * @priv:	FPGA bridge private data
@@ -408,7 +408,7 @@ static void devm_fpga_bridge_release(struct device *dev, void *res)
  *  Return: struct fpga_bridge or NULL
  */
 struct fpga_bridge
-*devm_fpga_bridge_create(struct device *dev, const char *name,
+*devm_fpga_bridge_create(struct device *parent, const char *name,
 			 const struct fpga_bridge_ops *br_ops, void *priv)
 {
 	struct fpga_bridge **ptr, *bridge;
@@ -417,12 +417,12 @@ struct fpga_bridge
 	if (!ptr)
 		return NULL;
 
-	bridge = fpga_bridge_create(dev, name, br_ops, priv);
+	bridge = fpga_bridge_create(parent, name, br_ops, priv);
 	if (!bridge) {
 		devres_free(ptr);
 	} else {
 		*ptr = bridge;
-		devres_add(dev, ptr);
+		devres_add(parent, ptr);
 	}
 
 	return bridge;
-- 
2.31.1

[PATCH 5/8] fpga: region: Rename dev to parent for parent device

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

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

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/fpga-region.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index c3134b89c3fe..4d60d643cada 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -181,7 +181,7 @@ ATTRIBUTE_GROUPS(fpga_region);
 
 /**
  * fpga_region_create - alloc and init a struct fpga_region
- * @dev: device parent
+ * @parent: device parent
  * @mgr: manager that programs this region
  * @get_bridges: optional function to get bridges to a list
  *
@@ -192,7 +192,7 @@ ATTRIBUTE_GROUPS(fpga_region);
  * Return: struct fpga_region or NULL
  */
 struct fpga_region
-*fpga_region_create(struct device *dev,
+*fpga_region_create(struct device *parent,
 		    struct fpga_manager *mgr,
 		    int (*get_bridges)(struct fpga_region *))
 {
@@ -214,8 +214,8 @@ struct fpga_region
 
 	device_initialize(&region->dev);
 	region->dev.class = fpga_region_class;
-	region->dev.parent = dev;
-	region->dev.of_node = dev->of_node;
+	region->dev.parent = parent;
+	region->dev.of_node = parent->of_node;
 	region->dev.id = id;
 
 	ret = dev_set_name(&region->dev, "region%d", id);
@@ -253,7 +253,7 @@ static void devm_fpga_region_release(struct device *dev, void *res)
 
 /**
  * devm_fpga_region_create - create and initialize a managed FPGA region struct
- * @dev: device parent
+ * @parent: device parent
  * @mgr: manager that programs this region
  * @get_bridges: optional function to get bridges to a list
  *
@@ -268,7 +268,7 @@ static void devm_fpga_region_release(struct device *dev, void *res)
  * Return: struct fpga_region or NULL
  */
 struct fpga_region
-*devm_fpga_region_create(struct device *dev,
+*devm_fpga_region_create(struct device *parent,
 			 struct fpga_manager *mgr,
 			 int (*get_bridges)(struct fpga_region *))
 {
@@ -278,12 +278,12 @@ struct fpga_region
 	if (!ptr)
 		return NULL;
 
-	region = fpga_region_create(dev, mgr, get_bridges);
+	region = fpga_region_create(parent, mgr, get_bridges);
 	if (!region) {
 		devres_free(ptr);
 	} else {
 		*ptr = region;
-		devres_add(dev, ptr);
+		devres_add(parent, ptr);
 	}
 
 	return region;
-- 
2.31.1

[PATCH 6/8] fpga: mgr: Use standard dev_release for class driver

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

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 <russell.h.weight@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/fpga-mgr.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 42ddc0844781..9f6c3760b6ff 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -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);
 
@@ -692,16 +688,11 @@ int fpga_mgr_register(struct fpga_manager *mgr)
 
 	ret = device_add(&mgr->dev);
 	if (ret)
-		goto error_device;
+		return ret;
 
 	dev_info(&mgr->dev, "%s registered\n", mgr->name);
 
 	return 0;
-
-error_device:
-	ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
-
-	return ret;
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_register);
 
@@ -722,7 +713,7 @@ void fpga_mgr_unregister(struct fpga_manager *mgr)
 	if (mgr->mops->fpga_remove)
 		mgr->mops->fpga_remove(mgr);
 
-	device_unregister(&mgr->dev);
+	device_del(&mgr->dev);
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_unregister);
 
@@ -781,6 +772,10 @@ EXPORT_SYMBOL_GPL(devm_fpga_mgr_register);
 
 static void fpga_mgr_dev_release(struct device *dev)
 {
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+
+	ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
+	kfree(mgr);
 }
 
 static int __init fpga_mgr_class_init(void)
-- 
2.31.1

[PATCH 7/8] fpga: bridge: Use standard dev_release for class driver

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

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().

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/fpga-bridge.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
index d4aca0f724f5..cf06c887593a 100644
--- a/drivers/fpga/fpga-bridge.c
+++ b/drivers/fpga/fpga-bridge.c
@@ -340,8 +340,10 @@ struct fpga_bridge *fpga_bridge_create(struct device *parent, const char *name,
 		return NULL;
 
 	id = ida_simple_get(&fpga_bridge_ida, 0, 0, GFP_KERNEL);
-	if (id < 0)
-		goto error_kfree;
+	if (id < 0) {
+		kfree(bridge);
+		return NULL;
+	}
 
 	mutex_init(&bridge->mutex);
 	INIT_LIST_HEAD(&bridge->node);
@@ -358,17 +360,12 @@ struct fpga_bridge *fpga_bridge_create(struct device *parent, const char *name,
 	bridge->dev.id = id;
 
 	ret = dev_set_name(&bridge->dev, "br%d", id);
-	if (ret)
-		goto error_device;
+	if (ret) {
+		put_device(&bridge->dev);
+		return NULL;
+	}
 
 	return bridge;
-
-error_device:
-	ida_simple_remove(&fpga_bridge_ida, id);
-error_kfree:
-	kfree(bridge);
-
-	return NULL;
 }
 EXPORT_SYMBOL_GPL(fpga_bridge_create);
 
@@ -378,8 +375,7 @@ EXPORT_SYMBOL_GPL(fpga_bridge_create);
  */
 void fpga_bridge_free(struct fpga_bridge *bridge)
 {
-	ida_simple_remove(&fpga_bridge_ida, bridge->dev.id);
-	kfree(bridge);
+	put_device(&bridge->dev);
 }
 EXPORT_SYMBOL_GPL(fpga_bridge_free);
 
@@ -469,12 +465,16 @@ void fpga_bridge_unregister(struct fpga_bridge *bridge)
 	if (bridge->br_ops && bridge->br_ops->fpga_bridge_remove)
 		bridge->br_ops->fpga_bridge_remove(bridge);
 
-	device_unregister(&bridge->dev);
+	device_del(&bridge->dev);
 }
 EXPORT_SYMBOL_GPL(fpga_bridge_unregister);
 
 static void fpga_bridge_dev_release(struct device *dev)
 {
+	struct fpga_bridge *bridge = to_fpga_bridge(dev);
+
+	ida_simple_remove(&fpga_bridge_ida, bridge->dev.id);
+	kfree(bridge);
 }
 
 static int __init fpga_bridge_dev_init(void)
-- 
2.31.1

[PATCH 8/8] fpga: region: Use standard dev_release for class driver

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

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().

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/fpga-region.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 4d60d643cada..bdc15fab60c0 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -204,8 +204,10 @@ struct fpga_region
 		return NULL;
 
 	id = ida_simple_get(&fpga_region_ida, 0, 0, GFP_KERNEL);
-	if (id < 0)
-		goto err_free;
+	if (id < 0) {
+		kfree(region);
+		return NULL;
+	}
 
 	region->mgr = mgr;
 	region->get_bridges = get_bridges;
@@ -219,17 +221,12 @@ struct fpga_region
 	region->dev.id = id;
 
 	ret = dev_set_name(&region->dev, "region%d", id);
-	if (ret)
-		goto err_remove;
+	if (ret) {
+		put_device(&region->dev);
+		return NULL;
+	}
 
 	return region;
-
-err_remove:
-	ida_simple_remove(&fpga_region_ida, id);
-err_free:
-	kfree(region);
-
-	return NULL;
 }
 EXPORT_SYMBOL_GPL(fpga_region_create);
 
@@ -239,8 +236,7 @@ EXPORT_SYMBOL_GPL(fpga_region_create);
  */
 void fpga_region_free(struct fpga_region *region)
 {
-	ida_simple_remove(&fpga_region_ida, region->dev.id);
-	kfree(region);
+	put_device(&region->dev);
 }
 EXPORT_SYMBOL_GPL(fpga_region_free);
 
@@ -310,12 +306,16 @@ EXPORT_SYMBOL_GPL(fpga_region_register);
  */
 void fpga_region_unregister(struct fpga_region *region)
 {
-	device_unregister(&region->dev);
+	device_del(&region->dev);
 }
 EXPORT_SYMBOL_GPL(fpga_region_unregister);
 
 static void fpga_region_dev_release(struct device *dev)
 {
+	struct fpga_region *region = to_fpga_region(dev);
+
+	ida_simple_remove(&fpga_region_ida, region->dev.id);
+	kfree(region);
 }
 
 /**
-- 
2.31.1

Re: [PATCH 2/8] fpga: stratix10-soc: Add missing fpga_mgr_free() call

[Greg KH]

On Mon, Jun 14, 2021 at 10:09:03AM -0700, Moritz Fischer wrote:
> From: Russ Weight <russell.h.weight@intel.com>
> 
> 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 <russell.h.weight@intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
>  drivers/fpga/stratix10-soc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
> index 657a70c5fc99..9e34bbbce26e 100644
> --- a/drivers/fpga/stratix10-soc.c
> +++ b/drivers/fpga/stratix10-soc.c
> @@ -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;
> -- 
> 2.31.1
> 

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

thanks,

greg k-h

Re: [PATCH 2/8] fpga: stratix10-soc: Add missing fpga_mgr_free() call

[Russ Weight]

On 6/14/21 10:30 AM, Greg KH wrote:
> On Mon, Jun 14, 2021 at 10:09:03AM -0700, Moritz Fischer wrote:
>> From: Russ Weight <russell.h.weight@intel.com>
>>
>> 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 <russell.h.weight@intel.com>
>> Reviewed-by: Xu Yilun <yilun.xu@intel.com>
>> Signed-off-by: Moritz Fischer <mdf@kernel.org>
>> ---
>>  drivers/fpga/stratix10-soc.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
>> index 657a70c5fc99..9e34bbbce26e 100644
>> --- a/drivers/fpga/stratix10-soc.c
>> +++ b/drivers/fpga/stratix10-soc.c
>> @@ -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;
>> -- 
>> 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?

- Russ

>
> thanks,
>
> greg k-h

Re: [PATCH 2/8] fpga: stratix10-soc: Add missing fpga_mgr_free() call

[Moritz Fischer]

On Mon, Jun 14, 2021 at 07:30:03PM +0200, Greg KH wrote:
> On Mon, Jun 14, 2021 at 10:09:03AM -0700, Moritz Fischer wrote:
> > From: Russ Weight <russell.h.weight@intel.com>
> > 
> > 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 <russell.h.weight@intel.com>
> > Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> >  drivers/fpga/stratix10-soc.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
> > index 657a70c5fc99..9e34bbbce26e 100644
> > --- a/drivers/fpga/stratix10-soc.c
> > +++ b/drivers/fpga/stratix10-soc.c
> > @@ -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;
> > -- 
> > 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

Thanks,
Moritz

Re: [PATCH 3/8] fpga: mgr: Rename dev to parent for parent device

[Tom Rix]

On 6/14/21 10:09 AM, Moritz Fischer wrote:
> From: Russ Weight <russell.h.weight@intel.com>
>
> Rename variable "dev" to "parent" in cases where it represents the parent
> device.
>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
>   drivers/fpga/fpga-mgr.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index b85bc47c91a9..42ddc0844781 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -551,7 +551,7 @@ EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
>   
>   /**
>    * fpga_mgr_create - create and initialize a FPGA manager struct
> - * @dev:	fpga manager device from pdev
> + * @parent:	fpga manager device from pdev

This line has a conflict with -next because of my recent 'a FPGA -> an 
FPGA' change in the preceding line

Tom

>    * @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;
>   }

Re: [PATCH 2/8] fpga: stratix10-soc: Add missing fpga_mgr_free() call

[Greg KH]

On Mon, Jun 14, 2021 at 10:38:01AM -0700, Russ Weight wrote:
> 
> 
> On 6/14/21 10:30 AM, Greg KH wrote:
> > On Mon, Jun 14, 2021 at 10:09:03AM -0700, Moritz Fischer wrote:
> >> From: Russ Weight <russell.h.weight@intel.com>
> >>
> >> 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 <russell.h.weight@intel.com>
> >> Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> >> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> >> ---
> >>  drivers/fpga/stratix10-soc.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
> >> index 657a70c5fc99..9e34bbbce26e 100644
> >> --- a/drivers/fpga/stratix10-soc.c
> >> +++ b/drivers/fpga/stratix10-soc.c
> >> @@ -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;
> >> -- 
> >> 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?

All patch 1 did is delete unused code, that doesn't really "fix"
anything that could be seen as a bug :)

thanks,

greg k-h

Re: [PATCH 6/8] fpga: mgr: Use standard dev_release for class driver

[Greg KH]

On Mon, Jun 14, 2021 at 10:09:07AM -0700, Moritz Fischer wrote:
> From: Russ Weight <russell.h.weight@intel.com>
> 
> 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 <russell.h.weight@intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
>  drivers/fpga/fpga-mgr.c | 35 +++++++++++++++--------------------
>  1 file changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 42ddc0844781..9f6c3760b6ff 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -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);
>  
> @@ -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

Re: [PATCH 7/8] fpga: bridge: Use standard dev_release for class driver

[Greg KH]

On Mon, Jun 14, 2021 at 10:09:08AM -0700, Moritz Fischer wrote:
> From: Russ Weight <russell.h.weight@intel.com>
> 
> 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().

Same "two step" problem here as the previous patch shows.

And again, why are you having both a create/register process?  Why not
just do it all internally?

thanks,

greg k-h

Re: [PATCH 8/8] fpga: region: Use standard dev_release for class driver

[Greg KH]

On Mon, Jun 14, 2021 at 10:09:09AM -0700, Moritz Fischer wrote:
> From: Russ Weight <russell.h.weight@intel.com>
> 
> 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().

Same problem as the other ones :(

Re: [PATCH 2/8] fpga: stratix10-soc: Add missing fpga_mgr_free() call

[Greg KH]

On Mon, Jun 14, 2021 at 10:40:39AM -0700, Moritz Fischer wrote:
> On Mon, Jun 14, 2021 at 07:30:03PM +0200, Greg KH wrote:
> > On Mon, Jun 14, 2021 at 10:09:03AM -0700, Moritz Fischer wrote:
> > > From: Russ Weight <russell.h.weight@intel.com>
> > > 
> > > 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 <russell.h.weight@intel.com>
> > > Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> > > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > > ---
> > >  drivers/fpga/stratix10-soc.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
> > > index 657a70c5fc99..9e34bbbce26e 100644
> > > --- a/drivers/fpga/stratix10-soc.c
> > > +++ b/drivers/fpga/stratix10-soc.c
> > > @@ -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;
> > > -- 
> > > 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

No need, I fixed this up by hand when I applied it, thanks.

greg k-h

Re: [PATCH 0/8] FPGA Manager devres cleanup

[Greg KH]

On Mon, Jun 14, 2021 at 10:09:01AM -0700, Moritz Fischer wrote:
> 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/YKKuBSLp5Fe0Zh0v@kroah.com
> 
> If you prefer I can provide a signed tag instead.

No need, I took the first 5 patches here, the last 3 need more work.

thanks,

greg k-h

Re: [PATCH 6/8] fpga: mgr: Use standard dev_release for class driver

[Russ Weight]

On 6/15/21 12:25 AM, Greg KH wrote:
> On Mon, Jun 14, 2021 at 10:09:07AM -0700, Moritz Fischer wrote:
>> From: Russ Weight <russell.h.weight@intel.com>
>>
>> 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 <russell.h.weight@intel.com>
>> Reviewed-by: Xu Yilun <yilun.xu@intel.com>
>> Signed-off-by: Moritz Fischer <mdf@kernel.org>
>> ---
>>  drivers/fpga/fpga-mgr.c | 35 +++++++++++++++--------------------
>>  1 file changed, 15 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>> index 42ddc0844781..9f6c3760b6ff 100644
>> --- a/drivers/fpga/fpga-mgr.c
>> +++ b/drivers/fpga/fpga-mgr.c
>> @@ -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);
>>  
>> @@ -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.

I started on this patchset by moving to a single fpga_mgr_register call. There were some subtle issues that made it more complex, and it was suggested that there was value in simplifying the fix and maintaining the current API.

Based on your comments, I'll go back resume my original fix, address the remaining issues, and go back through the review process.

Thanks,
- Russ

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