staging: bcm2835-audio: clean-up coding style issues

PATCH Superseded Aishwarya Pant 2 participants 12 messages Mar 9, 2017

Subsystems (3)

Staging Broadcom BCM2711/BCM2835 Arm architecture Broadcom BCM281XX/BCM11XXX/BCM216XX Arm architecture

lore

Share & tools

lore

Aishwarya Pant

author

2017-03-09 10:37 UTC #0 lore raw

v5 cover staging: bcm2835-audio: clean-up coding style issues

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

Signature · 1 line

2.7.4

Aishwarya Pant

author

2017-03-09 10:38 UTC #1 lore raw

PATCH v5 1/7 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

Julia Lawall

Acked-by on v5 1/7

2017-03-09 13:12 UTC #2 lore raw

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

Quoted 3 lines · Aishwarya Pant

Replace kmalloc and memset with kzalloc.
Signed-off-by Aishwarya Pant <[email protected]>

Acked-by Julia Lawall <[email protected]>

Quoted 31 lines

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

Aishwarya Pant

author

2017-03-09 10:38 UTC #3 lore raw

PATCH v5 2/7 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

Julia Lawall

2017-03-09 13:24 UTC #4 lore raw

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

Quoted 35 lines · 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 +2 -2
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;
 

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance, · line 344

Quoted 5 lines

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -1

@@ -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

Quoted 22 lines

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -2
 }
 
 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 [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/cc9cc8c7a3576a9d556815fd9f57ca58338abb01.1489055580.git.aishpant%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Aishwarya Pant

author

2017-03-09 18:35 UTC #5 lore raw

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

Quoted 51 lines · Julia Lawall

Quoted 35 lines · 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 +2 -2
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;
 
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance, · line 344
Quoted 5 lines
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -1
@@ -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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 16 lines

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

Quoted 3 lines

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.

Quoted 24 lines

julia
Quoted 22 lines
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -2
 }
 
 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 [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/cc9cc8c7a3576a9d556815fd9f57ca58338abb01.1489055580.git.aishpant%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Julia Lawall

2017-03-09 20:40 UTC #6 lore raw

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

Quoted 82 lines · Aishwarya Pant

Quoted 51 lines · Julia Lawall
Quoted 35 lines · 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 +2 -2
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;
 
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance, · line 344
Quoted 5 lines
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -1
@@ -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.
Quoted 2 lines
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.
Quoted 4 lines
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.
Quoted 16 lines
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
};

Quoted 6 lines

Quoted 3 lines
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

Quoted 33 lines

Quoted 18 lines
julia
Quoted 15 lines
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -2
 }
 
 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
Quoted 7 lines
Quoted 7 lines
--
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/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 [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/20170309183432.GA15683%40aishwarya.
For more options, visit https://groups.google.com/d/optout.

Aishwarya Pant

author

2017-03-09 10:38 UTC #7 lore raw

PATCH v5 3/7 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

Aishwarya Pant

author

2017-03-09 10:38 UTC #8 lore raw

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

In reply to 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 <[email protected]>

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

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +32 ~~-35~~

index c5f3be8..9545d35 100644
@@ -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)

Signature · 1 line

2.7.4

Aishwarya Pant

author

2017-03-09 10:38 UTC #9 lore raw

PATCH v5 5/7 staging: bcm2835-audio: deallocate work when queue_work(...) fails

In reply to Aishwarya Pant

This patch de-allocates work when queue_work(..) fails in the bcm2835-audio work functions

Signed-off-by Aishwarya Pant <[email protected]>

---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +3 -0

index 9545d35..026582e 100644
@@ -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;
 		}
 	}

Signature · 1 line

2.7.4

Aishwarya Pant

author

2017-03-09 10:39 UTC #10 lore raw

PATCH v5 6/7 staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()

In reply to 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 <[email protected]>

---
 .../staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +8 -6

index 026582e..c9afad0c 100644
@@ -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;
 }

Signature · 1 line

2.7.4

Aishwarya Pant

author

2017-03-09 10:39 UTC #11 lore raw

PATCH v5 7/7 staging: bcm2835-audio: remove BUG_ON() in bcm2835_audio_open_connection()

In reply to 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 <[email protected]>

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

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -2

index c9afad0c..df3267e 100644
@@ -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;
 	}

Signature · 1 line

2.7.4