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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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 
---
 .../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(>my_work, my_wq_function);
-   work->alsa_stream = alsa_stream;
-   work->cmd = BCM2835_AUDIO_START;
-   if (!queue_work(alsa_stream->my_wq, >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(>my_work, my_wq_function);
+   work->alsa_stream = alsa_stream;
+   work->cmd = BCM2835_AUDIO_START;
+   if (!queue_work(alsa_stream->my_wq, >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(>my_work, my_wq_function);
-   work->alsa_stream = alsa_stream;
-   work->cmd = BCM2835_AUDIO_STOP;
-   if (!queue_work(alsa_stream->my_wq, >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(>my_work, my_wq_function);
+   work->alsa_stream = alsa_stream;
+   work->cmd = BCM2835_AUDIO_STOP;
+   if (!queue_work(alsa_stream->my_wq, >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(>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, >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(>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, >my_work)) {
+   kfree(work);
+   return -EBUSY;
}
return 0;
 }
 
-static void