Skip to contentSkip to conversation

fpga manager fixes

PATCH In review [email protected] 2 participants 4 messages Oct 22, 2015 FPGA manager framework

lore
Share & tools
lore
#0 lore raw

cover fpga manager fixes

From: Alan Tull <[email protected]>

Two patches to fix some issues that were brought up in the review of "add FPGA manager core".

Alan

Alan Tull (2):
fpga manager: ensure lifetime with of_fpga_mgr_get fpga manager: remove unnecessary null pointer checks

 drivers/fpga/fpga-mgr.c |   44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)
Signature · 1 line
1.7.9.5
#1 lore raw

PATCH 1/2 fpga manager: ensure lifetime with of_fpga_mgr_get

From: Alan Tull <[email protected]>

Ensure device and driver lifetime from of_fpga_mgr_get() to fpga_mgr_put().

* Don't put_device() in of_fpga_mgr_get, do it in fpga_mgr_put(). (still do put_device if there is an error).
* Do module_get on the low level driver.
* Don't need to module_get(THIS_MODULE) since we won't be allowed to unload the fpga manager core without unloading low level driver first.
* Remove unnedessary null check for node pointer.

---
 drivers/fpga/fpga-mgr.c |   32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)
drivers/fpga/fpga-mgr.c +17 -15
index 2526163..68d7b41 100644
@@ -204,9 +204,7 @@ struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
 {
 	struct fpga_manager *mgr;
 	struct device *dev;
-
-	if (!node)
-		return ERR_PTR(-EINVAL);
+	int ret = -ENODEV;
 
 	dev = class_find_device(fpga_mgr_class, NULL, node,
 				fpga_mgr_of_node_match);
@@ -214,20 +212,25 @@ struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
 		return ERR_PTR(-ENODEV);
 
 	mgr = to_fpga_manager(dev);
-	put_device(dev);
 	if (!mgr)
-		return ERR_PTR(-ENODEV);
+		goto err_dev;
 
 	/* Get exclusive use of fpga manager */
-	if (!mutex_trylock(&mgr->ref_mutex))
-		return ERR_PTR(-EBUSY);
-
-	if (!try_module_get(THIS_MODULE)) {
-		mutex_unlock(&mgr->ref_mutex);
-		return ERR_PTR(-ENODEV);
+	if (!mutex_trylock(&mgr->ref_mutex)) {
+		ret = -EBUSY;
+		goto err_dev;
 	}
 
+	if (!try_module_get(dev->parent->driver->owner))
+		goto err_ll_mod;
+
 	return mgr;
+
+err_ll_mod:
+	mutex_unlock(&mgr->ref_mutex);
+err_dev:
+	put_device(dev);
+	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
 
@@ -237,10 +240,9 @@ EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
  */
 void fpga_mgr_put(struct fpga_manager *mgr)
 {
-	if (mgr) {
-		module_put(THIS_MODULE);
-		mutex_unlock(&mgr->ref_mutex);
-	}
+	module_put(mgr->dev.parent->driver->owner);
+	mutex_unlock(&mgr->ref_mutex);
+	put_device(&mgr->dev);
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_put);
 
Signature · 1 line
1.7.9.5
#2 lore raw

PATCH 2/2 fpga manager: remove unnecessary null pointer checks

In reply to [email protected]

From: Alan Tull <[email protected]>

Remove unnecessary null pointer checks. We want the caller of these functions to do their own pointer checks. Add some comments to document this.

---
 drivers/fpga/fpga-mgr.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)
drivers/fpga/fpga-mgr.c +4 -8
index 68d7b41..a24f5cb 100644
@@ -38,7 +38,8 @@ static struct class *fpga_mgr_class;
  *
  * Step the low level fpga manager through the device-specific steps of getting
  * an FPGA ready to be configured, writing the image to it, then doing whatever
- * post-configuration steps necessary.
+ * post-configuration steps necessary.  This code assumes the caller got the
+ * mgr pointer from of_fpga_mgr_get() and checked that it is not an error code.
  *
  * Return: 0 on success, negative error code otherwise.
  */
@@ -48,9 +49,6 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
 	struct device *dev = &mgr->dev;
 	int ret;
 
-	if (!mgr)
-		return -ENODEV;
-
 	/*
 	 * Call the low level driver's write_init function.  This will do the
 	 * device-specific things to get the FPGA into the state where it is
@@ -100,7 +98,8 @@ EXPORT_SYMBOL_GPL(fpga_mgr_buf_load);
  *
  * Request an FPGA image using the firmware class, then write out to the FPGA.
  * Update the state before each step to provide info on what step failed if
- * there is a failure.
+ * there is a failure.  This code assumes the caller got the mgr pointer
+ * from of_fpga_mgr_get() and checked that it is not an error code.
  *
  * Return: 0 on success, negative error code otherwise.
  */
@@ -111,9 +110,6 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
 	const struct firmware *fw;
 	int ret;
 
-	if (!mgr)
-		return -ENODEV;
-
 	dev_info(dev, "writing %s to %s\n", image_name, mgr->name);
 
 	mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
Signature · 1 line
1.7.9.5
Reviewed-by on v1 2/2
#3 lore raw

Re: [PATCH 2/2] fpga manager: remove unnecessary null pointer checks

Quoted 7 lines · On Thu, Oct 22, 2015 at 10:38 AM, <[email protected]> wrote:

From: Alan Tull <[email protected]>

Remove unnecessary null pointer checks. We want the caller of these functions to do their own pointer checks. Add some comments to document this.

Looks good!