[PATCH v6 0/7] staging: bcm2835-audio: clean-up coding style issues

[PATCH v6 0/7] staging: bcm2835-audio: clean-up coding style issues

[Aishwarya Pant]

[1] Replace kmalloc and memset with kzalloc in vc_vchi_audio_init(..)
[2] Replace null return value with PTR_ERR(..) values 
    in vc_vchi_audio_init(..)
[3] Propagate the PTR_ERR values forward instead of a hardcoded 
    value for easier debugging in bcm2835_audio_open_connection(..)
[4] Replace if (success) else { } after kmalloc with if(error)
    to fail fast in the work function
[5] De-allocate 'work' when queue_work(..) fails in the work 
    functions
[6] De-allocate 'vchi_instance' when VCHI connection fails or VCHI audio
    instance initialisation fails in bcm2835_audio_open_connection()
[7] Remove BUG_ON() in bcm2835_audio_open_connection()


Changes in v6:
-- Add a local variable ret to set error value instead of
  returning -EPERM when initialisation of vchi_audio_service fails
  in bcm2835_audio_open_connection()

Changes in v5:
-- Remove patch which added function names to log messages

Changes in v4:
-- Break-up patch 4 into four smaller patches
-- Add a patch proposing removal of BUG_ON()
-- Make the cover letter verbose

Changes in v3:
-- Fix memory leak when queue_work fails
-- Add __func__ to debug logs

Changes in v2:
-- Return error value -EBUSY instead of -1 when queue_work()
   fails


Aishwarya Pant (7):
  staging: bcm2835-audio: Replace kmalloc with kzalloc
  staging: bcm2835-audio: replace null with error pointer value
  staging: bcm2835-audio: propagate PTR_ERR value instead of -EPERM
  staging: bcm2835-audio: use conditional only for error case
  staging: bcm2835-audio: deallocate work when queue_work(...) fails
  staging: bcm2835-audio: fix memory leak in    
    bcm2835_audio_open_connection()
  staging: bcm2835-audio: remove BUG_ON() in
    bcm2835_audio_open_connection()

 .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 103 ++++++++++-----------
 1 file changed, 49 insertions(+), 54 deletions(-)

-- 
2.7.4

[PATCH v6 1/7] staging: bcm2835-audio: Replace kmalloc with kzalloc

[Aishwarya Pant]

Replace kmalloc and memset with kzalloc.

Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index c54bef3..005e287 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -290,11 +290,10 @@ vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
 		return NULL;
 	}
 	/* Allocate memory for this instance */
-	instance = kmalloc(sizeof(*instance), GFP_KERNEL);
+	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
 	if (!instance)
 		return NULL;
 
-	memset(instance, 0, sizeof(*instance));
 	instance->num_connections = num_connections;
 
 	/* Create a lock for exclusive, serialized VCHI connection access */
-- 
2.7.4

[PATCH v6 2/7] staging: bcm2835-audio: replace null with error pointer value

[Aishwarya Pant]

This patch replaces NULL values returned by vc_vchi_audio_init(...) with
error pointer values:
	- Return ERR_PTR(-EINVAL) when too many instances of audio
	  service are initialised
	- Return ERR_PTR(-ENOMEM) when kzalloc fails
	- RETURN ERR_PTR(-EPERM) when vchi connections fail to open

Similarly, a NULL check where vc_vchi_audio_init(...) is called is
replaced by IS_ERR(..)

Signed-off-by: Aishwarya Pant <aishpant@gmail.com>

---
Changes in v6:
	- Add a local variable ret to set error value instead of
	  returning -EPERM when initialisation of vchi_audio_service
          fails

---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 005e287..6de5209 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -280,6 +280,7 @@ vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
 	unsigned int i;
 	struct bcm2835_audio_instance *instance;
 	int status;
+	int ret;
 
 	LOG_DBG("%s: start", __func__);
 
@@ -287,12 +288,12 @@ vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
 		LOG_ERR("%s: unsupported number of connections %u (max=%u)\n",
 			__func__, num_connections, VCHI_MAX_NUM_CONNECTIONS);
 
-		return NULL;
+		return ERR_PTR(-EINVAL);
 	}
 	/* Allocate memory for this instance */
 	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
 	if (!instance)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	instance->num_connections = num_connections;
 
@@ -321,7 +322,7 @@ vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
 		if (status) {
 			LOG_ERR("%s: failed to open VCHI service connection (status=%d)\n",
 				__func__, status);
-
+			ret = -EPERM;
 			goto err_close_services;
 		}
 		/* Finished with the service for now */
@@ -341,7 +342,7 @@ vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
 	kfree(instance);
 	LOG_ERR("%s: error\n", __func__);
 
-	return NULL;
+	return ERR_PTR(ret);
 }
 
 static int vc_vchi_audio_deinit(struct bcm2835_audio_instance *instance)
@@ -432,7 +433,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 	/* Initialize an instance of the audio service */
 	instance = vc_vchi_audio_init(vchi_instance, &vchi_connection, 1);
 
-	if (!instance) {
+	if (IS_ERR(instance)) {
 		LOG_ERR("%s: failed to initialize audio service\n", __func__);
 
 		ret = -EPERM;
-- 
2.7.4

[PATCH v6 3/7] staging: bcm2835-audio: propagate PTR_ERR value instead of -EPERM

[Aishwarya Pant]

It is better to propagate PTR_ERR value instead of a hardcoded value
(-EPERM here)

Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 6de5209..7186c98 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -436,7 +436,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 	if (IS_ERR(instance)) {
 		LOG_ERR("%s: failed to initialize audio service\n", __func__);
 
-		ret = -EPERM;
+		ret = PTR_ERR(instance);
 		goto err_free_mem;
 	}
 
-- 
2.7.4

[PATCH v6 4/7] staging: bcm2835-audio: use conditional only for error case

[Aishwarya Pant]

* Refactor conditional to check if memory allocation has failed and
immediately return (-ENOMEM); if block for success case is removed.

* Return the error value -EBUSY when queue_work() fails.

Signed-off-by: Aishwarya Pant <aishpant@gmail.com>

---
Changes in v4:
	- Break up the patch into smaller changes
Changes in v3:
	- Deallocate work when queue_work fails
	- Add __func__ to debug logs
Changes in v2:
	- Return error value -EBUSY when queue_work fails
---
 .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 67 +++++++++++-----------
 1 file changed, 32 insertions(+), 35 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 7186c98..9b10027 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -131,77 +131,74 @@ static void my_wq_function(struct work_struct *work)
 
 int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream)
 {
-	int ret = -1;
-
 	LOG_DBG(" .. IN\n");
 	if (alsa_stream->my_wq) {
 		struct bcm2835_audio_work *work;
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 		/*--- Queue some work (item 1) ---*/
-		if (work) {
-			INIT_WORK(&work->my_work, my_wq_function);
-			work->alsa_stream = alsa_stream;
-			work->cmd = BCM2835_AUDIO_START;
-			if (queue_work(alsa_stream->my_wq, &work->my_work))
-				ret = 0;
-		} else {
+		if (!work) {
 			LOG_ERR(" .. Error: NULL work kmalloc\n");
+			return -ENOMEM;
+		}
+		INIT_WORK(&work->my_work, my_wq_function);
+		work->alsa_stream = alsa_stream;
+		work->cmd = BCM2835_AUDIO_START;
+		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG(" .. OUT\n");
+	return 0;
 }
 
 int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
 {
-	int ret = -1;
-
 	LOG_DBG(" .. IN\n");
 	if (alsa_stream->my_wq) {
 		struct bcm2835_audio_work *work;
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 		/*--- Queue some work (item 1) ---*/
-		if (work) {
-			INIT_WORK(&work->my_work, my_wq_function);
-			work->alsa_stream = alsa_stream;
-			work->cmd = BCM2835_AUDIO_STOP;
-			if (queue_work(alsa_stream->my_wq, &work->my_work))
-				ret = 0;
-		} else {
+		if (!work) {
 			LOG_ERR(" .. Error: NULL work kmalloc\n");
+			return -ENOMEM;
+		}
+		INIT_WORK(&work->my_work, my_wq_function);
+		work->alsa_stream = alsa_stream;
+		work->cmd = BCM2835_AUDIO_STOP;
+		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG(" .. OUT\n");
+	return 0;
 }
 
 int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 			unsigned int count, void *src)
 {
-	int ret = -1;
-
 	LOG_DBG(" .. IN\n");
 	if (alsa_stream->my_wq) {
 		struct bcm2835_audio_work *work;
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 		/*--- Queue some work (item 1) ---*/
-		if (work) {
-			INIT_WORK(&work->my_work, my_wq_function);
-			work->alsa_stream = alsa_stream;
-			work->cmd = BCM2835_AUDIO_WRITE;
-			work->src = src;
-			work->count = count;
-			if (queue_work(alsa_stream->my_wq, &work->my_work))
-				ret = 0;
-		} else {
+		if (!work) {
 			LOG_ERR(" .. Error: NULL work kmalloc\n");
+			return -ENOMEM;
+		}
+		INIT_WORK(&work->my_work, my_wq_function);
+		work->alsa_stream = alsa_stream;
+		work->cmd = BCM2835_AUDIO_WRITE;
+		work->src = src;
+		work->count = count;
+		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG(" .. OUT\n");
+	return 0;
 }
 
 static void my_workqueue_init(struct bcm2835_alsa_stream *alsa_stream)
-- 
2.7.4

[PATCH v6 5/7] staging: bcm2835-audio: deallocate work when queue_work(...) fails

[Aishwarya Pant]

This patch de-allocates work when queue_work(..) fails in the
bcm2835-audio work functions

Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 9b10027..449799f 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -145,6 +145,7 @@ int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream)
 		work->alsa_stream = alsa_stream;
 		work->cmd = BCM2835_AUDIO_START;
 		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			kfree(work);
 			return -EBUSY;
 		}
 	}
@@ -168,6 +169,7 @@ int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
 		work->alsa_stream = alsa_stream;
 		work->cmd = BCM2835_AUDIO_STOP;
 		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			kfree(work);
 			return -EBUSY;
 		}
 	}
@@ -194,6 +196,7 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 		work->src = src;
 		work->count = count;
 		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			kfree(work);
 			return -EBUSY;
 		}
 	}
-- 
2.7.4

[PATCH v6 6/7] staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()

[Aishwarya Pant]

In bcm2835_audio_open_connection(), if VCHI connection fails or
initialisation of VCHI audio instance fails vchi_instance needs to be
deallocated otherwise it will cause a memory leak.

Signed-off-by: Aishwarya Pant <aishpant@gmail.com>

---
Changes in v6:
	- Remove redundant debug logs
---
 .../staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c  | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 449799f..fa06bfd 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -396,8 +396,6 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 		(struct bcm2835_audio_instance *)alsa_stream->instance;
 	int ret;
 
-	LOG_DBG(" .. IN\n");
-
 	LOG_INFO("%s: start\n", __func__);
 	BUG_ON(instance);
 	if (instance) {
@@ -405,8 +403,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 			__func__, instance);
 		instance->alsa_stream = alsa_stream;
 		alsa_stream->instance = instance;
-		ret = 0; // xxx todo -1;
-		goto err_free_mem;
+		return 0; // xxx todo -1;
 	}
 
 	/* Initialize and create a VCHI connection */
@@ -416,8 +413,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 			LOG_ERR("%s: failed to initialise VCHI instance (ret=%d)\n",
 				__func__, ret);
 
-			ret = -EIO;
-			goto err_free_mem;
+			return -EIO;
 		}
 		ret = vchi_connect(NULL, 0, vchi_instance);
 		if (ret) {
@@ -425,7 +421,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 				__func__, ret);
 
 			ret = -EIO;
-			goto err_free_mem;
+			goto err_free_memory;
 		}
 		initted = 1;
 	}
@@ -437,7 +433,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 		LOG_ERR("%s: failed to initialize audio service\n", __func__);
 
 		ret = PTR_ERR(instance);
-		goto err_free_mem;
+		goto err_free_memory;
 	}
 
 	instance->alsa_stream = alsa_stream;
@@ -445,9 +441,9 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 
 	LOG_DBG(" success !\n");
 	ret = 0;
-err_free_mem:
-	LOG_DBG(" .. OUT\n");
 
+err_free_memory:
+	kfree(vchi_instance);
 	return ret;
 }
 
-- 
2.7.4

[PATCH v6 7/7] staging: bcm2835-audio: remove BUG_ON() in bcm2835_audio_open_connection()

[Aishwarya Pant]

The presence of BUG_ON(instance) in bcm2835_audio_open_connection()
makes the next if(instance) block unreachable. The desired behaviour is
that the function should exit gracefully returning an operation not
permitted error value (-EPERM).

Signed-off-by: Aishwarya Pant <aishpant@gmail.com>

---
Not sure of this change. This driver is still in staging so maybe then
BUG_ON() is required here for debugging. The kernel coding style document
suggests that BUG_ON() should never be used since it kills the current process.
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index fa06bfd..8ab01c3 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -397,7 +397,6 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 	int ret;
 
 	LOG_INFO("%s: start\n", __func__);
-	BUG_ON(instance);
 	if (instance) {
 		LOG_ERR("%s: VCHI instance already open (%p)\n",
 			__func__, instance);
-- 
2.7.4

Re: [Outreachy kernel] [PATCH v6 7/7] staging: bcm2835-audio: remove BUG_ON() in bcm2835_audio_open_connection()

[Julia Lawall]

On Fri, 10 Mar 2017, Aishwarya Pant wrote:

> The presence of BUG_ON(instance) in bcm2835_audio_open_connection()
> makes the next if(instance) block unreachable. The desired behaviour is
> that the function should exit gracefully returning an operation not
> permitted error value (-EPERM).
>
> Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
>
> ---
> Not sure of this change. This driver is still in staging so maybe then
> BUG_ON() is required here for debugging. The kernel coding style document
> suggests that BUG_ON() should never be used since it kills the current process.

It could be a good idea to check with the bcm people, since this changes
the behavior.

julia

> ---
>  drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> index fa06bfd..8ab01c3 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> @@ -397,7 +397,6 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
>  	int ret;
>
>  	LOG_INFO("%s: start\n", __func__);
> -	BUG_ON(instance);
>  	if (instance) {
>  		LOG_ERR("%s: VCHI instance already open (%p)\n",
>  			__func__, instance);
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/9a7fa27ac10ecc40160bbf8500b1d015d0988099.1489160830.git.aishpant%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH v6 7/7] staging: bcm2835-audio: remove BUG_ON() in bcm2835_audio_open_connection()

[Julia Lawall]

On Fri, 10 Mar 2017, Aishwarya Pant wrote:

> The presence of BUG_ON(instance) in bcm2835_audio_open_connection()
> makes the next if(instance) block unreachable. The desired behaviour is
> that the function should exit gracefully returning an operation not
> permitted error value (-EPERM).
>
> Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
>
> ---
> Not sure of this change. This driver is still in staging so maybe then
> BUG_ON() is required here for debugging. The kernel coding style document
> suggests that BUG_ON() should never be used since it kills the current process.

Actually, I'm not at all sure that this is a good idea.  It seems that the
error handling code below might not be correct.  The

ret = 0; // xxx todo -1;

seems suspicious.  If this code is incorrect, thenit might be better to
leave the BUGF_ON, and have the system crash in a more deterministic way.

julia

> ---
>  drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> index fa06bfd..8ab01c3 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> @@ -397,7 +397,6 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
>  	int ret;
>
>  	LOG_INFO("%s: start\n", __func__);
> -	BUG_ON(instance);
>  	if (instance) {
>  		LOG_ERR("%s: VCHI instance already open (%p)\n",
>  			__func__, instance);
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/9a7fa27ac10ecc40160bbf8500b1d015d0988099.1489160830.git.aishpant%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH v6 6/7] staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()

[Julia Lawall]

On Fri, 10 Mar 2017, Aishwarya Pant wrote:

> In bcm2835_audio_open_connection(), if VCHI connection fails or
> initialisation of VCHI audio instance fails vchi_instance needs to be
> deallocated otherwise it will cause a memory leak.
>
> Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
>
> ---
> Changes in v6:
> 	- Remove redundant debug logs

All the changes should be reflected in the log message above the line.
The patch furthermore also changes goto label names.  Actually, I think
the old label name was fine, but if you want to make the change it should
also be mentioned in the commit log.

By the way, this code is a really good example of why typedefs are bad.
Who would realize that VCHI_INSTANCE_T is a pointer type.

julia

> ---
>  .../staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c  | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> index 449799f..fa06bfd 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> @@ -396,8 +396,6 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
>  		(struct bcm2835_audio_instance *)alsa_stream->instance;
>  	int ret;
>
> -	LOG_DBG(" .. IN\n");
> -
>  	LOG_INFO("%s: start\n", __func__);
>  	BUG_ON(instance);
>  	if (instance) {
> @@ -405,8 +403,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
>  			__func__, instance);
>  		instance->alsa_stream = alsa_stream;
>  		alsa_stream->instance = instance;
> -		ret = 0; // xxx todo -1;
> -		goto err_free_mem;
> +		return 0; // xxx todo -1;
>  	}
>
>  	/* Initialize and create a VCHI connection */
> @@ -416,8 +413,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
>  			LOG_ERR("%s: failed to initialise VCHI instance (ret=%d)\n",
>  				__func__, ret);
>
> -			ret = -EIO;
> -			goto err_free_mem;
> +			return -EIO;
>  		}
>  		ret = vchi_connect(NULL, 0, vchi_instance);
>  		if (ret) {
> @@ -425,7 +421,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
>  				__func__, ret);
>
>  			ret = -EIO;
> -			goto err_free_mem;
> +			goto err_free_memory;
>  		}
>  		initted = 1;
>  	}
> @@ -437,7 +433,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
>  		LOG_ERR("%s: failed to initialize audio service\n", __func__);
>
>  		ret = PTR_ERR(instance);
> -		goto err_free_mem;
> +		goto err_free_memory;
>  	}
>
>  	instance->alsa_stream = alsa_stream;
> @@ -445,9 +441,9 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
>
>  	LOG_DBG(" success !\n");
>  	ret = 0;
> -err_free_mem:
> -	LOG_DBG(" .. OUT\n");
>
> +err_free_memory:
> +	kfree(vchi_instance);
>  	return ret;
>  }
>
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/97c79973a44f4695f3d0161b4300fd185363f7f1.1489160830.git.aishpant%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH v6 6/7] staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()

[Greg KH]

On Fri, Mar 10, 2017 at 09:31:05PM +0530, Aishwarya Pant wrote:
> In bcm2835_audio_open_connection(), if VCHI connection fails or
> initialisation of VCHI audio instance fails vchi_instance needs to be
> deallocated otherwise it will cause a memory leak.
> 
> Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
> 
> ---
> Changes in v6:
> 	- Remove redundant debug logs
> ---
>  .../staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c  | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> index 449799f..fa06bfd 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> @@ -396,8 +396,6 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
>  		(struct bcm2835_audio_instance *)alsa_stream->instance;
>  	int ret;
>  
> -	LOG_DBG(" .. IN\n");
> -

It's nice to remove these, but don't do so in the same patch you are
doing something else in!

Remember, only one logical thing per patch please.

thanks,

greg k-h