[PATCH v3 0/4] staging: bcm2835-audio: fix coding style issues

[PATCH v3 0/4] staging: bcm2835-audio: fix coding style issues

[Aishwarya Pant]

This patchset makes the following changes:
        - Replace kmalloc and memset with kzalloc
	- Replace null return value with PTR_ERR values
	- Propagate the PTR_ERR values forward instead of a hardcoded 
	  value for easier debugging
	- Replace if (success) else { } after kmalloc with if(error) 
	  to fail fast. Fix memory leak when queue_work fails.  
	  
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 (4):
  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

 .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 98 +++++++++++-----------
 1 file changed, 50 insertions(+), 48 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] 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 v3 2/4] 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>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 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..d2686b4 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -287,12 +287,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;
 
@@ -341,7 +341,7 @@ vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
 	kfree(instance);
 	LOG_ERR("%s: error\n", __func__);
 
-	return NULL;
+	return ERR_PTR(-EPERM);
 }
 
 static int vc_vchi_audio_deinit(struct bcm2835_audio_instance *instance)
@@ -432,7 +432,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 v3 3/4] 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 d2686b4..c5f3be8 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -435,7 +435,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 v3 4/4] staging: bcm2835-audio: use conditional only for error case

[Aishwarya Pant]

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

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

* Add __func__ to debug logs

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

---
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    | 85 +++++++++++-----------
 1 file changed, 44 insertions(+), 41 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index c5f3be8..f5d8a47 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -131,77 +131,80 @@ 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");
+	LOG_DBG("%s: .. IN\n", __func__);
 	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 {
-			LOG_ERR(" .. Error: NULL work kmalloc\n");
+		if (!work) {
+			LOG_ERR("%s: Error: NULL work kmalloc\n", __func__);
+			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)) {
+			LOG_ERR("%s: Error: Unable to start audio\n", __func__);
+			kfree(work);
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG("%s: .. OUT\n", __func__);
+	return 0;
 }
 
 int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
 {
-	int ret = -1;
-
-	LOG_DBG(" .. IN\n");
+	LOG_DBG("%s: .. IN\n", __func__);
 	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 {
-			LOG_ERR(" .. Error: NULL work kmalloc\n");
+		if (!work) {
+			LOG_ERR("%s: Error: NULL work kmalloc\n", __func__);
+			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)) {
+			LOG_ERR("%s: Error: Unable to stop audio\n", __func__);
+			kfree(work);
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG("%s: .. OUT\n", __func__);
+	return 0;
 }
 
 int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 			unsigned int count, void *src)
 {
-	int ret = -1;
-
-	LOG_DBG(" .. IN\n");
+	LOG_DBG("%s: .. IN\n", __func__);
 	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 {
-			LOG_ERR(" .. Error: NULL work kmalloc\n");
+		if (!work) {
+			LOG_ERR("%s: Error: NULL work kmalloc\n", __func__);
+			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)) {
+			LOG_ERR("%s: Error: Unable to write\n", __func__);
+			kfree(work);
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG("%s: .. OUT\n", __func__);
+	return 0;
 }
 
 static void my_workqueue_init(struct bcm2835_alsa_stream *alsa_stream)
-- 
2.7.4

Re: [Outreachy kernel] [PATCH v3 4/4] staging: bcm2835-audio: use conditional only for error case

[Julia Lawall]

On Tue, 7 Mar 2017, Aishwarya Pant wrote:

> * Refactor conditional to check if memory allocation has failed and
> immediately returns (-ENOMEM); the redundant if block for success case
> is removed.
>
> * Return the error value -EBUSY and kfree(work) when queue_work() fails.
>
> * Add __func__ to debug logs

This is too much for one patch.

julia

>
> Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
>
> ---
> 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    | 85 +++++++++++-----------
>  1 file changed, 44 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> index c5f3be8..f5d8a47 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> @@ -131,77 +131,80 @@ 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");
> +	LOG_DBG("%s: .. IN\n", __func__);
>  	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 {
> -			LOG_ERR(" .. Error: NULL work kmalloc\n");
> +		if (!work) {
> +			LOG_ERR("%s: Error: NULL work kmalloc\n", __func__);
> +			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)) {
> +			LOG_ERR("%s: Error: Unable to start audio\n", __func__);
> +			kfree(work);
> +			return -EBUSY;
>  		}
>  	}
> -	LOG_DBG(" .. OUT %d\n", ret);
> -	return ret;
> +	LOG_DBG("%s: .. OUT\n", __func__);
> +	return 0;
>  }
>
>  int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
>  {
> -	int ret = -1;
> -
> -	LOG_DBG(" .. IN\n");
> +	LOG_DBG("%s: .. IN\n", __func__);
>  	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 {
> -			LOG_ERR(" .. Error: NULL work kmalloc\n");
> +		if (!work) {
> +			LOG_ERR("%s: Error: NULL work kmalloc\n", __func__);
> +			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)) {
> +			LOG_ERR("%s: Error: Unable to stop audio\n", __func__);
> +			kfree(work);
> +			return -EBUSY;
>  		}
>  	}
> -	LOG_DBG(" .. OUT %d\n", ret);
> -	return ret;
> +	LOG_DBG("%s: .. OUT\n", __func__);
> +	return 0;
>  }
>
>  int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
>  			unsigned int count, void *src)
>  {
> -	int ret = -1;
> -
> -	LOG_DBG(" .. IN\n");
> +	LOG_DBG("%s: .. IN\n", __func__);
>  	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 {
> -			LOG_ERR(" .. Error: NULL work kmalloc\n");
> +		if (!work) {
> +			LOG_ERR("%s: Error: NULL work kmalloc\n", __func__);
> +			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)) {
> +			LOG_ERR("%s: Error: Unable to write\n", __func__);
> +			kfree(work);
> +			return -EBUSY;
>  		}
>  	}
> -	LOG_DBG(" .. OUT %d\n", ret);
> -	return ret;
> +	LOG_DBG("%s: .. OUT\n", __func__);
> +	return 0;
>  }
>
>  static void my_workqueue_init(struct bcm2835_alsa_stream *alsa_stream)
> --
> 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/b0229b2efb92a235bfb3dd1f1e889f61904e3c9f.1488906373.git.aishpant%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH v3 0/4] staging: bcm2835-audio: fix coding style issues

[Julia Lawall]

On Tue, 7 Mar 2017, Aishwarya Pant wrote:

> This patchset makes the following changes:
>         - Replace kmalloc and memset with kzalloc
> 	- Replace null return value with PTR_ERR values
> 	- Propagate the PTR_ERR values forward instead of a hardcoded
> 	  value for easier debugging
> 	- Replace if (success) else { } after kmalloc with if(error)
> 	  to fail fast. Fix memory leak when queue_work fails.
>
> Changes in v3:
> 	- Fix memory leak when queue_work fails
> 	- Add __func__ to debug logs

It could be helpful to indicate in which patch of the series the changes
have occurred.

julia

> Changes in v2:
>         - Return error value -EBUSY instead of -1 when queue_work()
> 	  fails
>
> Aishwarya Pant (4):
>   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
>
>  .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 98 +++++++++++-----------
>  1 file changed, 50 insertions(+), 48 deletions(-)
>
> --
> 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/cover.1488906373.git.aishpant%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>