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

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

[Aishwarya Pant]

This patchset has the following fixes:

[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 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    | 100 ++++++++++-----------
 1 file changed, 50 insertions(+), 50 deletions(-)

-- 
2.7.4

[PATCH v5 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 v5 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>
---
 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 v5 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 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 v5 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 c5f3be8..9545d35 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 v5 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 9545d35..026582e 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 v5 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>
---
 .../staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 14 ++++++++------
 1 file changed, 8 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 026582e..c9afad0c 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -405,7 +405,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 		instance->alsa_stream = alsa_stream;
 		alsa_stream->instance = instance;
 		ret = 0; // xxx todo -1;
-		goto err_free_mem;
+		goto err_init;
 	}
 
 	/* Initialize and create a VCHI connection */
@@ -416,7 +416,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 				__func__, ret);
 
 			ret = -EIO;
-			goto err_free_mem;
+			goto err_init;
 		}
 		ret = vchi_connect(NULL, 0, vchi_instance);
 		if (ret) {
@@ -424,7 +424,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;
 	}
@@ -436,7 +436,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;
@@ -444,9 +444,11 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 
 	LOG_DBG(" success !\n");
 	ret = 0;
-err_free_mem:
+err_init:
 	LOG_DBG(" .. OUT\n");
-
+	return ret;
+err_free_memory:
+	kfree(vchi_instance);
 	return ret;
 }
 
-- 
2.7.4

[PATCH v5 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 | 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 c9afad0c..df3267e 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -398,13 +398,12 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 	LOG_DBG(" .. IN\n");
 
 	LOG_INFO("%s: start\n", __func__);
-	BUG_ON(instance);
 	if (instance) {
 		LOG_ERR("%s: VCHI instance already open (%p)\n",
 			__func__, instance);
 		instance->alsa_stream = alsa_stream;
 		alsa_stream->instance = instance;
-		ret = 0; // xxx todo -1;
+		ret = -EPERM;
 		goto err_init;
 	}
 
-- 
2.7.4

Re: [Outreachy kernel] [PATCH v5 1/7] staging: bcm2835-audio: Replace kmalloc with kzalloc

[Julia Lawall]

On Thu, 9 Mar 2017, Aishwarya Pant wrote:

> Replace kmalloc and memset with kzalloc.
>
> Signed-off-by: Aishwarya Pant <aishpant@gmail.com>

Acked-by: Julia Lawall <julia.lawall@lip6.fr>

> ---
>  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
>
> --
> 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/43aa5452c7e19cdf5bb0c1046c4c11a82569083a.1489055580.git.aishpant%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH v5 2/7] staging: bcm2835-audio: replace null with error pointer value

[Julia Lawall]

On Thu, 9 Mar 2017, Aishwarya Pant wrote:

> 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);

I find it a bit odd to have -EPERM hard coded at the end of the function.
If the function becomes more complicated, this code could end up being
shared.  In that case, different exits would have different codes.  Also,
ultimately the code should come from the result of vchi_service_open,
although that is not suitable now.

How about adding status = -EPERM just before the goto, and the putting
return ERR_PTR(status) at the end of the function?

By the way, I found that having all the LOG_DBG lines made the code really
hard to follow.  Maybe it could be possible to just remove them.

Another suggestion on this particular function is to do something about
this declaration:

                SERVICE_CREATION_T params = {
                        VCHI_VERSION_EX(VC_AUDIOSERV_VER, VC_AUDIOSERV_MIN_VER),
                        VC_AUDIO_SERVER_NAME, // 4cc service code
                        vchi_connections[i], // passed in fn pointers
                        0, // rx fifo size (unused)
                        0, // tx fifo size (unused)
                        audio_vchi_callback, // service callback
                        instance, // service callback parameter
                        1, //TODO: remove VCOS_FALSE,   // unaligned bulk receives
                        1, //TODO: remove VCOS_FALSE,   // unaligned bulk transmits
                        0 // want crc check on bulk transfers
                };

The typedef needs to go, the fields should be initialied using their names
(.x = e) and the comments should be dropped.

julia

>  }
>
>  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
>
> --
> 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/cc9cc8c7a3576a9d556815fd9f57ca58338abb01.1489055580.git.aishpant%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH v5 2/7] staging: bcm2835-audio: replace null with error pointer value

[Aishwarya Pant]

On Thu, Mar 09, 2017 at 02:24:08PM +0100, Julia Lawall wrote:
> 
> 
> On Thu, 9 Mar 2017, Aishwarya Pant wrote:
> 
> > 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);
> 
> I find it a bit odd to have -EPERM hard coded at the end of the function.
> If the function becomes more complicated, this code could end up being
> shared.  In that case, different exits would have different codes.  Also,
> ultimately the code should come from the result of vchi_service_open,
> although that is not suitable now.
> 
> How about adding status = -EPERM just before the goto, and the putting
> return ERR_PTR(status) at the end of the function?

Yes that sounds fair.

> 
> By the way, I found that having all the LOG_DBG lines made the code really
> hard to follow.  Maybe it could be possible to just remove them.
> 

The LOG_DBG / LOG_ERR messages here are a wrapper of pr_err / pr_dbg
appended with the current function names and line numbers. So I am
taking a guess they are useful as long as the driver is in staging.

> Another suggestion on this particular function is to do something about
> this declaration:
> 
>                 SERVICE_CREATION_T params = {
>                         VCHI_VERSION_EX(VC_AUDIOSERV_VER, VC_AUDIOSERV_MIN_VER),
>                         VC_AUDIO_SERVER_NAME, // 4cc service code
>                         vchi_connections[i], // passed in fn pointers
>                         0, // rx fifo size (unused)
>                         0, // tx fifo size (unused)
>                         audio_vchi_callback, // service callback
>                         instance, // service callback parameter
>                         1, //TODO: remove VCOS_FALSE,   // unaligned bulk receives
>                         1, //TODO: remove VCOS_FALSE,   // unaligned bulk transmits
>                         0 // want crc check on bulk transfers
>                 };
> 
> The typedef needs to go, the fields should be initialied using their names
> (.x = e) and the comments should be dropped.
> 

All-right I'll give this a go. The vchi_services driver is sprinkled
with typedefs.

> julia
> 
> >  }
> >
> >  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
> >
> > --
> > 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/cc9cc8c7a3576a9d556815fd9f57ca58338abb01.1489055580.git.aishpant%40gmail.com.
> > For more options, visit https://groups.google.com/d/optout.
> >

Re: [Outreachy kernel] [PATCH v5 2/7] staging: bcm2835-audio: replace null with error pointer value

[Julia Lawall]

On Fri, 10 Mar 2017, Aishwarya Pant wrote:

> On Thu, Mar 09, 2017 at 02:24:08PM +0100, Julia Lawall wrote:
> >
> >
> > On Thu, 9 Mar 2017, Aishwarya Pant wrote:
> >
> > > 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);
> >
> > I find it a bit odd to have -EPERM hard coded at the end of the function.
> > If the function becomes more complicated, this code could end up being
> > shared.  In that case, different exits would have different codes.  Also,
> > ultimately the code should come from the result of vchi_service_open,
> > although that is not suitable now.
> >
> > How about adding status = -EPERM just before the goto, and the putting
> > return ERR_PTR(status) at the end of the function?
>
> Yes that sounds fair.
>
> >
> > By the way, I found that having all the LOG_DBG lines made the code really
> > hard to follow.  Maybe it could be possible to just remove them.
> >
>
> The LOG_DBG / LOG_ERR messages here are a wrapper of pr_err / pr_dbg
> appended with the current function names and line numbers. So I am
> taking a guess they are useful as long as the driver is in staging.
>
> > Another suggestion on this particular function is to do something about
> > this declaration:
> >
> >                 SERVICE_CREATION_T params = {
> >                         VCHI_VERSION_EX(VC_AUDIOSERV_VER, VC_AUDIOSERV_MIN_VER),
> >                         VC_AUDIO_SERVER_NAME, // 4cc service code
> >                         vchi_connections[i], // passed in fn pointers
> >                         0, // rx fifo size (unused)
> >                         0, // tx fifo size (unused)
> >                         audio_vchi_callback, // service callback
> >                         instance, // service callback parameter
> >                         1, //TODO: remove VCOS_FALSE,   // unaligned bulk receives
> >                         1, //TODO: remove VCOS_FALSE,   // unaligned bulk transmits
> >                         0 // want crc check on bulk transfers
> >                 };
> >
> > The typedef needs to go, the fields should be initialied using their names
> > (.x = e) and the comments should be dropped.
> >
>
> All-right I'll give this a go. The vchi_services driver is sprinkled
> with typedefs.

Sounds good.  It's the anonymous field declarations that are really error
prone, but dropping the typdefs would be great as well.

julia

>
> > julia
> >
> > >  }
> > >
> > >  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
> > >
> > > --
> > > 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/cc9cc8c7a3576a9d556815fd9f57ca58338abb01.1489055580.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/20170309183432.GA15683%40aishwarya.
> For more options, visit https://groups.google.com/d/optout.
>