Skip to contentSkip to conversation

staging: bcm2835-audio: fix coding style issues

PATCH In review Aishwarya Pant 2 participants 6 messages Mar 6, 2017

Subsystems (3)
Staging Broadcom BCM2711/BCM2835 Arm architecture Broadcom BCM281XX/BCM11XXX/BCM216XX Arm architecture
lore
Share & tools
lore

Aishwarya Pant

author
#0 lore raw

cover staging: bcm2835-audio: fix coding style issues

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.

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    | 59 +++++++++++-----------
 1 file changed, 29 insertions(+), 30 deletions(-)
Signature · 1 line
2.7.4
author
#1 lore raw

PATCH 1/4 staging: bcm2835-audio: Replace kmalloc with kzalloc

Replace kmalloc and memset with kzalloc.

Signed-off-by Aishwarya Pant <[email protected]>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -2
index c54bef3..005e287 100644
@@ -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 */
Signature · 1 line
2.7.4
author
#2 lore raw

PATCH 2/4 staging: bcm2835-audio: replace null with error pointer value

In reply to 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 <[email protected]>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +4 -4
index 005e287..d2686b4 100644
@@ -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;
Signature · 1 line
2.7.4
author
#3 lore raw

PATCH 3/4 staging: bcm2835-audio: propagate PTR_ERR value instead of -EPERM

In reply to Aishwarya Pant

It is better to propagate PTR_ERR value instead of a hardcoded value (-EPERM here)

Signed-off-by Aishwarya Pant <[email protected]>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -1
index d2686b4..c5f3be8 100644
@@ -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;
 	}
Signature · 1 line
2.7.4
author
#4 lore raw

PATCH 4/4 staging: bcm2835-audio: use conditional only for error case

In reply to 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.

Signed-off-by Aishwarya Pant <[email protected]>
---
 .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 46 +++++++++++-----------
 1 file changed, 23 insertions(+), 23 deletions(-)
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +23 -23
index c5f3be8..75a43a5 100644
@@ -139,15 +139,15 @@ int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream)
 
 		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))
+			ret = 0;
 	}
 	LOG_DBG(" .. OUT %d\n", ret);
 	return ret;

@@ -163,15 +163,15 @@ int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
 
 		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))
+			ret = 0;
 	}
 	LOG_DBG(" .. OUT %d\n", ret);
 	return ret;

@@ -188,17 +188,17 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 
 		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))
+			ret = 0;
 	}
 	LOG_DBG(" .. OUT %d\n", ret);
 	return ret;
Signature · 1 line
2.7.4
#5 lore raw

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

Quoted 23 lines · 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.

Signed-off-by Aishwarya Pant <[email protected]>
---
 .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 46 +++++++++++-----------
 1 file changed, 23 insertions(+), 23 deletions(-)
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +0 -6
index c5f3be8..75a43a5 100644
@@ -139,15 +139,15 @@ int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream)
 
 		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;

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream) · line 150

Quoted 10 lines
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +7 -1
-		} 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))
+			ret = 0;

These functions are returning -1 if queue_work fails. Maybe you can find a more appropriate value by looking at the defintion.

julia

Quoted 63 lines
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +16 -17
 	}
 	LOG_DBG(" .. OUT %d\n", ret);
 	return ret;

@@ -163,15 +163,15 @@ int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
 
 		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))
+			ret = 0;
 	}
 	LOG_DBG(" .. OUT %d\n", ret);
 	return ret;

@@ -188,17 +188,17 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 
 		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))
+			ret = 0;
 	}
 	LOG_DBG(" .. OUT %d\n", ret);
 	return ret;
--

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 [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1e7304c424c82b9f5a03d32aa9dab343b4b70dc4.1488811728.git.aishpant%40gmail.com.
For more options, visit https://groups.google.com/d/optout.