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

[PATCH v2 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.


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

-- 
2.7.4

[PATCH v2 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 v2 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 v2 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 v2 4/4] staging: bcm2835-audio: use conditional only for error case

[Aishwarya Pant]

This patch refactors conditional to check if memory allocation has
failed and immediately returns (-ENOMEM); the redundant if block for
success case is removed. The work functions now return the error value
-EBUSY instead of -1 when queue_work() fails.

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

---
Changes in v2:
	- Return error value -EBUSY when queue_work fails

 .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 70 +++++++++++-----------
 1 file changed, 35 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 c5f3be8..08f200f 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -131,77 +131,77 @@ 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)) {
+			LOG_ERR(" .. Error: Unable to queue work\n");
+			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)) {
+			LOG_ERR(" .. Error: Unable to queue work\n");
+			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)) {
+			LOG_ERR(" .. Error: Unable to queue work\n");
+			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

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

[Julia Lawall]

On Tue, 7 Mar 2017, Aishwarya Pant wrote:

> This patch refactors conditional to check if memory allocation has
> failed and immediately returns (-ENOMEM); the redundant if block for
> success case is removed. The work functions now return the error value
> -EBUSY instead of -1 when queue_work() fails.
>
> Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
>
> ---
> Changes in v2:
> 	- Return error value -EBUSY when queue_work fails

This is definitely an improvement.  Some opportunities for further
improvement remain:

queue_work seems to fail in a very particular situation, which is
mentioned in the definition of the function.  It could be good to make the
error message more spcific.

The code causes a memory leak if queue_work returns false.

The IN and OUT debugging messages are probably not so useful.  The don't
even say what function one is in or out of, and if that information is
needed, it could be just added to the error messages in the failure cases
(__func__).

julia

>  .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 70 +++++++++++-----------
>  1 file changed, 35 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 c5f3be8..08f200f 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> @@ -131,77 +131,77 @@ 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)) {
> +			LOG_ERR(" .. Error: Unable to queue work\n");
> +			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)) {
> +			LOG_ERR(" .. Error: Unable to queue work\n");
> +			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)) {
> +			LOG_ERR(" .. Error: Unable to queue work\n");
> +			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
>
> --
> 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/b013f557b3bff6e4264bc9e6d1f7e502a8d1ac8c.1488829304.git.aishpant%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

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

[Aishwarya Pant]

On Mon, Mar 06, 2017 at 10:41:27PM +0100, Julia Lawall wrote:
> 
> 
> On Tue, 7 Mar 2017, Aishwarya Pant wrote:
> 
> > This patch refactors conditional to check if memory allocation has
> > failed and immediately returns (-ENOMEM); the redundant if block for
> > success case is removed. The work functions now return the error value
> > -EBUSY instead of -1 when queue_work() fails.
> >
> > Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
> >
> > ---
> > Changes in v2:
> > 	- Return error value -EBUSY when queue_work fails
> 
> This is definitely an improvement.  Some opportunities for further
> improvement remain:
> 
> queue_work seems to fail in a very particular situation, which is
> mentioned in the definition of the function.  It could be good to make the
> error message more spcific.

Sure, will add error messages like failed to start/stop audio, write audio
bytes etc.

> 
> The code causes a memory leak if queue_work returns false.

Yes. kfree(work) needs to be done when queue_work is busy. One thing 
I noticed is that in the same file kfree((void *)work) has been used.
Both kfree(work) and kfree((void *)work) are valid as long as work is
not defined as const. kfree looks up the address and the memory actually 
keeps track of the size of allocation done by kmalloc, and that is how  
kfree knows how much to deallocate.

> 
> The IN and OUT debugging messages are probably not so useful.  The don't
> even say what function one is in or out of, and if that information is
> needed, it could be just added to the error messages in the failure cases
> (__func__).

The IN and OUT debug messages are present in every function of the file.
I'll keep them and add the current function name __func__

> 
> julia
> 
> >  .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 70 +++++++++++-----------
> >  1 file changed, 35 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 c5f3be8..08f200f 100644
> > --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> > +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> > @@ -131,77 +131,77 @@ 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)) {
> > +			LOG_ERR(" .. Error: Unable to queue work\n");
> > +			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)) {
> > +			LOG_ERR(" .. Error: Unable to queue work\n");
> > +			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)) {
> > +			LOG_ERR(" .. Error: Unable to queue work\n");
> > +			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
> >
> > --
> > 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/b013f557b3bff6e4264bc9e6d1f7e502a8d1ac8c.1488829304.git.aishpant%40gmail.com.
> > For more options, visit https://groups.google.com/d/optout.
> >

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

[Julia Lawall]

On Tue, 7 Mar 2017, Aishwarya Pant wrote:

> On Mon, Mar 06, 2017 at 10:41:27PM +0100, Julia Lawall wrote:
> >
> >
> > On Tue, 7 Mar 2017, Aishwarya Pant wrote:
> >
> > > This patch refactors conditional to check if memory allocation has
> > > failed and immediately returns (-ENOMEM); the redundant if block for
> > > success case is removed. The work functions now return the error value
> > > -EBUSY instead of -1 when queue_work() fails.
> > >
> > > Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
> > >
> > > ---
> > > Changes in v2:
> > > 	- Return error value -EBUSY when queue_work fails
> >
> > This is definitely an improvement.  Some opportunities for further
> > improvement remain:
> >
> > queue_work seems to fail in a very particular situation, which is
> > mentioned in the definition of the function.  It could be good to make the
> > error message more spcific.
>
> Sure, will add error messages like failed to start/stop audio, write audio
> bytes etc.
>
> >
> > The code causes a memory leak if queue_work returns false.
>
> Yes. kfree(work) needs to be done when queue_work is busy. One thing
> I noticed is that in the same file kfree((void *)work) has been used.
> Both kfree(work) and kfree((void *)work) are valid as long as work is
> not defined as const. kfree looks up the address and the memory actually
> keeps track of the size of allocation done by kmalloc, and that is how
> kfree knows how much to deallocate.
> >
> > The IN and OUT debugging messages are probably not so useful.  The don't
> > even say what function one is in or out of, and if that information is
> > needed, it could be just added to the error messages in the failure cases
> > (__func__).
>
> The IN and OUT debug messages are present in every function of the file.
> I'll keep them and add the current function name __func__

Probably not worth it.  They should probably all go, but you may want to
leave that for someone else to do.

julia

>
> >
> > julia
> >
> > >  .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 70 +++++++++++-----------
> > >  1 file changed, 35 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 c5f3be8..08f200f 100644
> > > --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> > > +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> > > @@ -131,77 +131,77 @@ 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)) {
> > > +			LOG_ERR(" .. Error: Unable to queue work\n");
> > > +			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)) {
> > > +			LOG_ERR(" .. Error: Unable to queue work\n");
> > > +			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)) {
> > > +			LOG_ERR(" .. Error: Unable to queue work\n");
> > > +			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
> > >
> > > --
> > > 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/b013f557b3bff6e4264bc9e6d1f7e502a8d1ac8c.1488829304.git.aishpant%40gmail.com.
> > > For more options, visit https://groups.google.com/d/optout.
> > >
>
> --
> 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/20170307055034.GA30995%40aishwarya.
> For more options, visit https://groups.google.com/d/optout.
>