diff mbox series

[v3] /driver/raw/ifpga_rawdev: fix a memory leak bug in ifpga

Message ID 1545746524-116580-1-git-send-email-andy.pei@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series [v3] /driver/raw/ifpga_rawdev: fix a memory leak bug in ifpga | expand

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Andy Pei Dec. 25, 2018, 2:02 p.m. UTC
When ifpga_rawdev_create() allocate memory for a new rawdev, the original code
allocate redundant memory for adapter, which is a member of the rawdev.
What is actually necessary is the adapter to be initialized, not memory allocated.

What is different in v3 from v2 is that the adapter is no longer need to be freed.

fixes:ef1e8ede3da5
cc: rosen.xu@intel.com
cc: tianfei.zhang@intel.com

Signed-off-by: AndyPei <andy.pei@intel.com>
---
 drivers/raw/ifpga_rawdev/base/opae_hw_api.c | 32 ++++++++++++++++++++++++-----
 drivers/raw/ifpga_rawdev/base/opae_hw_api.h |  3 ++-
 drivers/raw/ifpga_rawdev/ifpga_rawdev.c     | 12 ++++-------
 3 files changed, 33 insertions(+), 14 deletions(-)

Comments

Zhang, Tianfei Jan. 2, 2019, 2:47 a.m. UTC | #1
> -----Original Message-----
> From: Pei, Andy
> Sent: Tuesday, December 25, 2018 10:02 PM
> To: dev@dpdk.org
> Cc: Xu, Rosen <rosen.xu@intel.com>; Zhang, Tianfei
> <tianfei.zhang@intel.com>; Pei, Andy <andy.pei@intel.com>
> Subject: [PATCH v3]/driver/raw/ifpga_rawdev: fix a memory leak bug in ifpga
> 
> When ifpga_rawdev_create() allocate memory for a new rawdev, the
> original code allocate redundant memory for adapter, which is a member of
> the rawdev.
> What is actually necessary is the adapter to be initialized, not memory
> allocated.
> 
> What is different in v3 from v2 is that the adapter is no longer need to be
> freed.
> 
> fixes:ef1e8ede3da5
> cc: rosen.xu@intel.com
> cc: tianfei.zhang@intel.com

It looks good for me, Acked-by: Tianfei zhang <tianfei.zhang@intel.com>
Xu, Rosen Jan. 2, 2019, 6:52 a.m. UTC | #2
Hi,

> -----Original Message-----
> From: Pei, Andy
> Sent: Tuesday, December 25, 2018 22:02
> To: dev@dpdk.org
> Cc: Xu, Rosen <rosen.xu@intel.com>; Zhang, Tianfei
> <tianfei.zhang@intel.com>; Pei, Andy <andy.pei@intel.com>
> Subject: [PATCH v3]/driver/raw/ifpga_rawdev: fix a memory leak bug in ifpga
> 
> When ifpga_rawdev_create() allocate memory for a new rawdev, the original
> code allocate redundant memory for adapter, which is a member of the
> rawdev.
> What is actually necessary is the adapter to be initialized, not memory
> allocated.
> 
> What is different in v3 from v2 is that the adapter is no longer need to be
> freed.
> 
> fixes:ef1e8ede3da5
> cc: rosen.xu@intel.com
> cc: tianfei.zhang@intel.com

Acked-by: Rosen Xu <rosen.xu@intel.com>
Thomas Monjalon Jan. 15, 2019, 12:03 a.m. UTC | #3
Hi Andy,

I comment here for all your patches (4 are pending).
They are your first patches for DPDK, welcome!
You may want to read the guides explaining how to contribute:
	http://doc.dpdk.org/guides/contributing/patches.html

The title should be "raw/ifpga: fix memory leak in [area here]"

25/12/2018 15:02, AndyPei:
> When ifpga_rawdev_create() allocate memory for a new rawdev, the original code
> allocate redundant memory for adapter, which is a member of the rawdev.
> What is actually necessary is the adapter to be initialized, not memory allocated.
> 
> What is different in v3 from v2 is that the adapter is no longer need to be freed.

The changelog is better located after --- below.

> fixes:ef1e8ede3da5
> cc: rosen.xu@intel.com
> cc: tianfei.zhang@intel.com

The format should be a bit different.
Please use git log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'
and check with devtools/check-git-log.sh

> Signed-off-by: AndyPei <andy.pei@intel.com>

Should there be a space in your name as "Andy Pei"?

[...]
>  /**
> - * opae_adapter_data_alloc - alloc opae_adapter_data data structure
> + * opae_adapter_init - init opae_adapter data structure
> + * @adpdate: pointer of opae_adater data structure
> + * @name: adapter name.
> + * @data: private data of this adapter.

The doxygen format is @param param_name param_description.
It is also important to strictly match the name of the parameter.
@adpdate is a typo (should be @param adapter).

You can build the doc with "make doc-api-html"
and check the result in your browser.

> + *
> + * Return: 0 on success.
> + */
> +int opae_adapter_init(struct opae_adapter *adapter,
> +		const char *name, void *data)
diff mbox series

Patch

diff --git a/drivers/raw/ifpga_rawdev/base/opae_hw_api.c b/drivers/raw/ifpga_rawdev/base/opae_hw_api.c
index a533dfe..50f6438 100644
--- a/drivers/raw/ifpga_rawdev/base/opae_hw_api.c
+++ b/drivers/raw/ifpga_rawdev/base/opae_hw_api.c
@@ -303,12 +303,35 @@  static struct opae_adapter_ops *match_ops(struct opae_adapter *adapter)
 }
 
 /**
- * opae_adapter_data_alloc - alloc opae_adapter_data data structure
+ * opae_adapter_init - init opae_adapter data structure
+ * @adpdate: pointer of opae_adater data structure
+ * @name: adapter name.
+ * @data: private data of this adapter.
+ *
+ * Return: 0 on success.
+ */
+int opae_adapter_init(struct opae_adapter *adapter,
+		const char *name, void *data)
+{
+	if (!adapter)
+		return -ENOMEM;
+
+	TAILQ_INIT(&adapter->acc_list);
+	adapter->data = data;
+	adapter->name = name;
+	adapter->ops = match_ops(adapter);
+
+	return 0;
+}
+
+/**
+ * opae_adapter_alloc - alloc opae_adapter data structure
  * @name: adapter name.
  * @data: private data of this adapter.
  *
  * Return: opae_adapter on success, otherwise NULL.
  */
+ /**This function will no longer be called.
 struct opae_adapter *opae_adapter_alloc(const char *name, void *data)
 {
 	struct opae_adapter *adapter = opae_zmalloc(sizeof(*adapter));
@@ -316,13 +339,12 @@  struct opae_adapter *opae_adapter_alloc(const char *name, void *data)
 	if (!adapter)
 		return NULL;
 
-	TAILQ_INIT(&adapter->acc_list);
-	adapter->data = data;
-	adapter->name = name;
-	adapter->ops = match_ops(adapter);
+	if (opae_adapter_init(adapter, name, data))
+		return NULL;
 
 	return adapter;
 }
+**/
 
 /**
  * opae_adapter_enumerate - enumerate this adapter
diff --git a/drivers/raw/ifpga_rawdev/base/opae_hw_api.h b/drivers/raw/ifpga_rawdev/base/opae_hw_api.h
index 4bbc9df..332e0f3 100644
--- a/drivers/raw/ifpga_rawdev/base/opae_hw_api.h
+++ b/drivers/raw/ifpga_rawdev/base/opae_hw_api.h
@@ -225,7 +225,8 @@  struct opae_adapter {
 void *opae_adapter_data_alloc(enum opae_adapter_type type);
 #define opae_adapter_data_free(data) opae_free(data)
 
-struct opae_adapter *opae_adapter_alloc(const char *name, void *data);
+int opae_adapter_init(struct opae_adapter *adapter,
+		const char *name, void *data);
 #define opae_adapter_free(adapter) opae_free(adapter)
 
 int opae_adapter_enumerate(struct opae_adapter *adapter);
diff --git a/drivers/raw/ifpga_rawdev/ifpga_rawdev.c b/drivers/raw/ifpga_rawdev/ifpga_rawdev.c
index 32e318f..da772d0 100644
--- a/drivers/raw/ifpga_rawdev/ifpga_rawdev.c
+++ b/drivers/raw/ifpga_rawdev/ifpga_rawdev.c
@@ -409,9 +409,10 @@ 
 	data->device_id = pci_dev->id.device_id;
 	data->vendor_id = pci_dev->id.vendor_id;
 
+	adapter = rawdev->dev_private;
 	/* create a opae_adapter based on above device data */
-	adapter = opae_adapter_alloc(pci_dev->device.name, data);
-	if (!adapter) {
+	ret = opae_adapter_init(adapter, pci_dev->device.name, data);
+	if (ret) {
 		ret = -ENOMEM;
 		goto free_adapter_data;
 	}
@@ -420,12 +421,10 @@ 
 	rawdev->device = &pci_dev->device;
 	rawdev->driver_name = pci_dev->device.driver->name;
 
-	rawdev->dev_private = adapter;
-
 	/* must enumerate the adapter before use it */
 	ret = opae_adapter_enumerate(adapter);
 	if (ret)
-		goto free_adapter;
+		goto free_adapter_data;
 
 	/* get opae_manager to rawdev */
 	mgr = opae_adapter_get_mgr(adapter);
@@ -436,9 +435,6 @@ 
 
 	return ret;
 
-free_adapter:
-	if (adapter)
-		opae_adapter_free(adapter);
 free_adapter_data:
 	if (data)
 		opae_adapter_data_free(data);