[PATCH 1/2] staging: bcm2835-audio: Check if workqueue allocation failed

[PATCH 1/2] staging: bcm2835-audio: Check if workqueue allocation failed

[Tuomas Tynkkynen]

Currently, if allocating a workqueue fails, the driver will probe
successfully but it will silently do nothing, which is rather silly.
So instead bail out with -ENOMEM in bcm2835_audio_open() if
alloc_workqueue() fails, and remove the now pointless checks for a NULL
workqueue.

While at it, get rid of the rather pointless one-line function
my_workqueue_init().

Signed-off-by: Tuomas Tynkkynen <tuomas@tuxera.com>
---
 .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 111 ++++++++++-----------
 1 file changed, 50 insertions(+), 61 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index a4a48f31f1a3..85ed807bb873 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -118,44 +118,40 @@ static void my_wq_function(struct work_struct *work)
 
 int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream)
 {
-	if (alsa_stream->my_wq) {
-		struct bcm2835_audio_work *work;
-
-		work = kmalloc(sizeof(*work), GFP_ATOMIC);
-		/*--- Queue some work (item 1) ---*/
-		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)) {
-			kfree(work);
-			return -EBUSY;
-		}
+	struct bcm2835_audio_work *work;
+
+	work = kmalloc(sizeof(*work), GFP_ATOMIC);
+	/*--- Queue some work (item 1) ---*/
+	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)) {
+		kfree(work);
+		return -EBUSY;
 	}
 	return 0;
 }
 
 int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
 {
-	if (alsa_stream->my_wq) {
-		struct bcm2835_audio_work *work;
-
-		work = kmalloc(sizeof(*work), GFP_ATOMIC);
-		/*--- Queue some work (item 1) ---*/
-		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)) {
-			kfree(work);
-			return -EBUSY;
-		}
+	struct bcm2835_audio_work *work;
+
+	work = kmalloc(sizeof(*work), GFP_ATOMIC);
+	/*--- Queue some work (item 1) ---*/
+	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)) {
+		kfree(work);
+		return -EBUSY;
 	}
 	return 0;
 }
@@ -163,40 +159,31 @@ int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
 int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 			unsigned int count, void *src)
 {
-	if (alsa_stream->my_wq) {
-		struct bcm2835_audio_work *work;
-
-		work = kmalloc(sizeof(*work), GFP_ATOMIC);
-		/*--- Queue some work (item 1) ---*/
-		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)) {
-			kfree(work);
-			return -EBUSY;
-		}
+	struct bcm2835_audio_work *work;
+
+	work = kmalloc(sizeof(*work), GFP_ATOMIC);
+	/*--- Queue some work (item 1) ---*/
+	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)) {
+		kfree(work);
+		return -EBUSY;
 	}
 	return 0;
 }
 
-static void my_workqueue_init(struct bcm2835_alsa_stream *alsa_stream)
-{
-	alsa_stream->my_wq = alloc_workqueue("my_queue", WQ_HIGHPRI, 1);
-}
-
 static void my_workqueue_quit(struct bcm2835_alsa_stream *alsa_stream)
 {
-	if (alsa_stream->my_wq) {
-		flush_workqueue(alsa_stream->my_wq);
-		destroy_workqueue(alsa_stream->my_wq);
-		alsa_stream->my_wq = NULL;
-	}
+	flush_workqueue(alsa_stream->my_wq);
+	destroy_workqueue(alsa_stream->my_wq);
+	alsa_stream->my_wq = NULL;
 }
 
 static void audio_vchi_callback(void *param,
@@ -424,7 +411,9 @@ int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
 	int status;
 	int ret;
 
-	my_workqueue_init(alsa_stream);
+	alsa_stream->my_wq = alloc_workqueue("my_queue", WQ_HIGHPRI, 1);
+	if (!alsa_stream->my_wq)
+		return -ENOMEM;
 
 	ret = bcm2835_audio_open_connection(alsa_stream);
 	if (ret) {
-- 
2.16.3

[PATCH 2/2] staging: bcm2835-audio: Don't leak workqueue if open fails

[Tuomas Tynkkynen]

Currently, if bcm2835_audio_open() fails partway, the allocated
workqueue is leaked. Avoid that.

While at it, propagate the return value of
bcm2835_audio_open_connection() on failure instead of returning -1.

Signed-off-by: Tuomas Tynkkynen <tuomas@tuxera.com>
---
 .../staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c  | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 85ed807bb873..779c1e993b55 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -416,16 +416,16 @@ int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
 		return -ENOMEM;
 
 	ret = bcm2835_audio_open_connection(alsa_stream);
-	if (ret) {
-		ret = -1;
-		goto exit;
-	}
+	if (ret)
+		goto free_wq;
+
 	instance = alsa_stream->instance;
 	LOG_DBG(" instance (%p)\n", instance);
 
 	if (mutex_lock_interruptible(&instance->vchi_mutex)) {
 		LOG_DBG("Interrupted whilst waiting for lock on (%d)\n", instance->num_connections);
-		return -EINTR;
+		ret = -EINTR;
+		goto free_wq;
 	}
 	vchi_service_use(instance->vchi_handle[0]);
 
@@ -448,7 +448,11 @@ int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
 unlock:
 	vchi_service_release(instance->vchi_handle[0]);
 	mutex_unlock(&instance->vchi_mutex);
-exit:
+
+free_wq:
+	if (ret)
+		destroy_workqueue(alsa_stream->my_wq);
+
 	return ret;
 }
 
-- 
2.16.3

Re: [PATCH 1/2] staging: bcm2835-audio: Check if workqueue allocation failed

[Dan Carpenter]

On Fri, Jul 13, 2018 at 12:54:16AM +0300, Tuomas Tynkkynen wrote:
> @@ -424,7 +411,9 @@ int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
>  	int status;
>  	int ret;
>  
> -	my_workqueue_init(alsa_stream);
> +	alsa_stream->my_wq = alloc_workqueue("my_queue", WQ_HIGHPRI, 1);
> +	if (!alsa_stream->my_wq)
> +		return -ENOMEM;
>  
>  	ret = bcm2835_audio_open_connection(alsa_stream);
>  	if (ret) {

This patch is good but if bcm2835_audio_open_connection() fails then
we need to release alsa_stream->my_wq.

regards,
dan carpenter

Re: [PATCH 1/2] staging: bcm2835-audio: Check if workqueue allocation failed

[Dan Carpenter]

On Fri, Jul 13, 2018 at 09:48:16AM +0300, Dan Carpenter wrote:
> On Fri, Jul 13, 2018 at 12:54:16AM +0300, Tuomas Tynkkynen wrote:
> > @@ -424,7 +411,9 @@ int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
> >  	int status;
> >  	int ret;
> >  
> > -	my_workqueue_init(alsa_stream);
> > +	alsa_stream->my_wq = alloc_workqueue("my_queue", WQ_HIGHPRI, 1);
> > +	if (!alsa_stream->my_wq)
> > +		return -ENOMEM;
> >  
> >  	ret = bcm2835_audio_open_connection(alsa_stream);
> >  	if (ret) {
> 
> This patch is good but if bcm2835_audio_open_connection() fails then
> we need to release alsa_stream->my_wq.

Never mind, you handle it in the next patch.  The bug *was* there in the
original code as well, so that's a legit way to split the patches.

regards,
dan carpenter