* [PATCH v4 0/8] staging: bcm2835-audio: fix coding style issues
@ 2017-03-08 14:48 Aishwarya Pant
2017-03-08 14:48 ` [PATCH v4 1/8] staging: bcm2835-audio: Replace kmalloc with kzalloc Aishwarya Pant
` (7 more replies)
0 siblings, 8 replies; 10+ messages in thread
From: Aishwarya Pant @ 2017-03-08 14:48 UTC (permalink / raw)
To: outreachy-kernel
This patch has the following fixes:
-- Replace kmalloc and memset with kzalloc vc_vchi_audio_init(..)
-- Replace null return value with PTR_ERR(..) values in
vc_vchi_audio_init(..)
-- Propagate the PTR_ERR values forward instead of a hardcoded
value for easier debugging in bcm2835_audio_open_connection(..)
-- Replace if (success) else { } after kmalloc with if(error)
to fail fast in the work function
-- De-allocate 'work' when queue_work(..) fails in the work
functions
-- Add current function names __func__ to debug and error logs in the
work functions
-- De-allocate 'vchi_instance' when VCHI connection fails or VCHI audio
instance initialisation fails in bcm2835_audio_open_connection()
-- Remove BUG_ON() in bcm2835_audio_open_connection()
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 (8):
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: Add function name to debug and error logs
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 | 109 +++++++++++----------
1 file changed, 56 insertions(+), 53 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 1/8] staging: bcm2835-audio: Replace kmalloc with kzalloc
2017-03-08 14:48 [PATCH v4 0/8] staging: bcm2835-audio: fix coding style issues Aishwarya Pant
@ 2017-03-08 14:48 ` Aishwarya Pant
2017-03-08 14:49 ` [PATCH v4 2/8] staging: bcm2835-audio: replace null with error pointer value Aishwarya Pant
` (6 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Aishwarya Pant @ 2017-03-08 14:48 UTC (permalink / raw)
To: outreachy-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 2/8] staging: bcm2835-audio: replace null with error pointer value
2017-03-08 14:48 [PATCH v4 0/8] staging: bcm2835-audio: fix coding style issues Aishwarya Pant
2017-03-08 14:48 ` [PATCH v4 1/8] staging: bcm2835-audio: Replace kmalloc with kzalloc Aishwarya Pant
@ 2017-03-08 14:49 ` Aishwarya Pant
2017-03-08 14:49 ` [PATCH v4 3/8] staging: bcm2835-audio: propagate PTR_ERR value instead of -EPERM Aishwarya Pant
` (5 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Aishwarya Pant @ 2017-03-08 14:49 UTC (permalink / raw)
To: outreachy-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 3/8] staging: bcm2835-audio: propagate PTR_ERR value instead of -EPERM
2017-03-08 14:48 [PATCH v4 0/8] staging: bcm2835-audio: fix coding style issues Aishwarya Pant
2017-03-08 14:48 ` [PATCH v4 1/8] staging: bcm2835-audio: Replace kmalloc with kzalloc Aishwarya Pant
2017-03-08 14:49 ` [PATCH v4 2/8] staging: bcm2835-audio: replace null with error pointer value Aishwarya Pant
@ 2017-03-08 14:49 ` Aishwarya Pant
2017-03-08 14:49 ` [PATCH v4 4/8] staging: bcm2835-audio: use conditional only for error case Aishwarya Pant
` (4 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Aishwarya Pant @ 2017-03-08 14:49 UTC (permalink / raw)
To: outreachy-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 4/8] staging: bcm2835-audio: use conditional only for error case
2017-03-08 14:48 [PATCH v4 0/8] staging: bcm2835-audio: fix coding style issues Aishwarya Pant
` (2 preceding siblings ...)
2017-03-08 14:49 ` [PATCH v4 3/8] staging: bcm2835-audio: propagate PTR_ERR value instead of -EPERM Aishwarya Pant
@ 2017-03-08 14:49 ` Aishwarya Pant
2017-03-08 14:49 ` [PATCH v4 5/8] staging: bcm2835-audio: deallocate work when queue_work(...) fails Aishwarya Pant
` (3 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Aishwarya Pant @ 2017-03-08 14:49 UTC (permalink / raw)
To: outreachy-kernel
* 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 5/8] staging: bcm2835-audio: deallocate work when queue_work(...) fails
2017-03-08 14:48 [PATCH v4 0/8] staging: bcm2835-audio: fix coding style issues Aishwarya Pant
` (3 preceding siblings ...)
2017-03-08 14:49 ` [PATCH v4 4/8] staging: bcm2835-audio: use conditional only for error case Aishwarya Pant
@ 2017-03-08 14:49 ` Aishwarya Pant
2017-03-08 14:49 ` [PATCH v4 6/8] staging: bcm2835-audio: Add function name to debug and error logs Aishwarya Pant
` (2 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Aishwarya Pant @ 2017-03-08 14:49 UTC (permalink / raw)
To: outreachy-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 6/8] staging: bcm2835-audio: Add function name to debug and error logs
2017-03-08 14:48 [PATCH v4 0/8] staging: bcm2835-audio: fix coding style issues Aishwarya Pant
` (4 preceding siblings ...)
2017-03-08 14:49 ` [PATCH v4 5/8] staging: bcm2835-audio: deallocate work when queue_work(...) fails Aishwarya Pant
@ 2017-03-08 14:49 ` Aishwarya Pant
2017-03-09 10:17 ` Aishwarya Pant
2017-03-08 14:50 ` [PATCH v4 7/8] staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection() Aishwarya Pant
2017-03-08 14:50 ` [PATCH v4 8/8] staging: bcm2835-audio: remove BUG_ON() " Aishwarya Pant
7 siblings, 1 reply; 10+ messages in thread
From: Aishwarya Pant @ 2017-03-08 14:49 UTC (permalink / raw)
To: outreachy-kernel
This patch adds current function names __func__ to debug and error logs
in the work functions.
Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
---
.../staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 15 +++++++++------
1 file changed, 9 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..77657a0 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -138,7 +138,7 @@ int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream)
work = kmalloc(sizeof(*work), GFP_ATOMIC);
/*--- Queue some work (item 1) ---*/
if (!work) {
- LOG_ERR(" .. Error: NULL work kmalloc\n");
+ LOG_ERR("%s: Error: NULL work kmalloc\n", __func__);
return -ENOMEM;
}
INIT_WORK(&work->my_work, my_wq_function);
@@ -146,10 +146,11 @@ int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream)
work->cmd = BCM2835_AUDIO_START;
if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
kfree(work);
+ LOG_ERR("%s: Error: Unable to start audio\n", __func__);
return -EBUSY;
}
}
- LOG_DBG(" .. OUT\n");
+ LOG_DBG("%s: .. OUT\n", __func__);
return 0;
}
@@ -162,7 +163,7 @@ int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
work = kmalloc(sizeof(*work), GFP_ATOMIC);
/*--- Queue some work (item 1) ---*/
if (!work) {
- LOG_ERR(" .. Error: NULL work kmalloc\n");
+ LOG_ERR("%s: Error: NULL work kmalloc\n", __func__);
return -ENOMEM;
}
INIT_WORK(&work->my_work, my_wq_function);
@@ -170,10 +171,11 @@ int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
work->cmd = BCM2835_AUDIO_STOP;
if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
kfree(work);
+ LOG_ERR("%s: Error: Unable to stop audio\n", __func__);
return -EBUSY;
}
}
- LOG_DBG(" .. OUT\n");
+ LOG_DBG("%s: .. OUT\n", __func__);
return 0;
}
@@ -187,7 +189,7 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
work = kmalloc(sizeof(*work), GFP_ATOMIC);
/*--- Queue some work (item 1) ---*/
if (!work) {
- LOG_ERR(" .. Error: NULL work kmalloc\n");
+ LOG_ERR("%s: Error: NULL work kmalloc\n", __func__);
return -ENOMEM;
}
INIT_WORK(&work->my_work, my_wq_function);
@@ -197,10 +199,11 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
work->count = count;
if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
kfree(work);
+ LOG_ERR("%s: Error: Unable to write\n", __func__);
return -EBUSY;
}
}
- LOG_DBG(" .. OUT\n");
+ LOG_DBG("%s: .. OUT\n", __func__);
return 0;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 7/8] staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection()
2017-03-08 14:48 [PATCH v4 0/8] staging: bcm2835-audio: fix coding style issues Aishwarya Pant
` (5 preceding siblings ...)
2017-03-08 14:49 ` [PATCH v4 6/8] staging: bcm2835-audio: Add function name to debug and error logs Aishwarya Pant
@ 2017-03-08 14:50 ` Aishwarya Pant
2017-03-08 14:50 ` [PATCH v4 8/8] staging: bcm2835-audio: remove BUG_ON() " Aishwarya Pant
7 siblings, 0 replies; 10+ messages in thread
From: Aishwarya Pant @ 2017-03-08 14:50 UTC (permalink / raw)
To: outreachy-kernel
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 77657a0..560fa12 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -408,7 +408,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 */
@@ -419,7 +419,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) {
@@ -427,7 +427,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;
}
@@ -439,7 +439,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;
@@ -447,9 +447,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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 8/8] staging: bcm2835-audio: remove BUG_ON() in bcm2835_audio_open_connection()
2017-03-08 14:48 [PATCH v4 0/8] staging: bcm2835-audio: fix coding style issues Aishwarya Pant
` (6 preceding siblings ...)
2017-03-08 14:50 ` [PATCH v4 7/8] staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection() Aishwarya Pant
@ 2017-03-08 14:50 ` Aishwarya Pant
7 siblings, 0 replies; 10+ messages in thread
From: Aishwarya Pant @ 2017-03-08 14:50 UTC (permalink / raw)
To: outreachy-kernel
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 560fa12..19e0f44 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -401,13 +401,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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v4 6/8] staging: bcm2835-audio: Add function name to debug and error logs
2017-03-08 14:49 ` [PATCH v4 6/8] staging: bcm2835-audio: Add function name to debug and error logs Aishwarya Pant
@ 2017-03-09 10:17 ` Aishwarya Pant
0 siblings, 0 replies; 10+ messages in thread
From: Aishwarya Pant @ 2017-03-09 10:17 UTC (permalink / raw)
To: outreachy-kernel
On Wed, Mar 08, 2017 at 08:19:51PM +0530, Aishwarya Pant wrote:
> This patch adds current function names __func__ to debug and error logs
> in the work functions.
>
> Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
> ---
> .../staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 15 +++++++++------
> 1 file changed, 9 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..77657a0 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> @@ -138,7 +138,7 @@ int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream)
> work = kmalloc(sizeof(*work), GFP_ATOMIC);
> /*--- Queue some work (item 1) ---*/
> if (!work) {
> - LOG_ERR(" .. Error: NULL work kmalloc\n");
> + LOG_ERR("%s: Error: NULL work kmalloc\n", __func__);
> return -ENOMEM;
> }
> INIT_WORK(&work->my_work, my_wq_function);
> @@ -146,10 +146,11 @@ int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream)
> work->cmd = BCM2835_AUDIO_START;
> if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
> kfree(work);
> + LOG_ERR("%s: Error: Unable to start audio\n", __func__);
> return -EBUSY;
> }
> }
> - LOG_DBG(" .. OUT\n");
> + LOG_DBG("%s: .. OUT\n", __func__);
> return 0;
> }
>
> @@ -162,7 +163,7 @@ int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
> work = kmalloc(sizeof(*work), GFP_ATOMIC);
> /*--- Queue some work (item 1) ---*/
> if (!work) {
> - LOG_ERR(" .. Error: NULL work kmalloc\n");
> + LOG_ERR("%s: Error: NULL work kmalloc\n", __func__);
> return -ENOMEM;
> }
> INIT_WORK(&work->my_work, my_wq_function);
> @@ -170,10 +171,11 @@ int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
> work->cmd = BCM2835_AUDIO_STOP;
> if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
> kfree(work);
> + LOG_ERR("%s: Error: Unable to stop audio\n", __func__);
> return -EBUSY;
> }
> }
> - LOG_DBG(" .. OUT\n");
> + LOG_DBG("%s: .. OUT\n", __func__);
> return 0;
> }
>
> @@ -187,7 +189,7 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
> work = kmalloc(sizeof(*work), GFP_ATOMIC);
> /*--- Queue some work (item 1) ---*/
> if (!work) {
> - LOG_ERR(" .. Error: NULL work kmalloc\n");
> + LOG_ERR("%s: Error: NULL work kmalloc\n", __func__);
> return -ENOMEM;
> }
> INIT_WORK(&work->my_work, my_wq_function);
> @@ -197,10 +199,11 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
> work->count = count;
> if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
> kfree(work);
> + LOG_ERR("%s: Error: Unable to write\n", __func__);
> return -EBUSY;
> }
> }
> - LOG_DBG(" .. OUT\n");
> + LOG_DBG("%s: .. OUT\n", __func__);
Please ignore this patch.
LOG_DBG/ERR/INFO(..) are wrappers around pr_*(..) family with line
number and function name appended. I'll send a new patchset with
these changes reverted.
> return 0;
> }
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-09 10:17 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-08 14:48 [PATCH v4 0/8] staging: bcm2835-audio: fix coding style issues Aishwarya Pant
2017-03-08 14:48 ` [PATCH v4 1/8] staging: bcm2835-audio: Replace kmalloc with kzalloc Aishwarya Pant
2017-03-08 14:49 ` [PATCH v4 2/8] staging: bcm2835-audio: replace null with error pointer value Aishwarya Pant
2017-03-08 14:49 ` [PATCH v4 3/8] staging: bcm2835-audio: propagate PTR_ERR value instead of -EPERM Aishwarya Pant
2017-03-08 14:49 ` [PATCH v4 4/8] staging: bcm2835-audio: use conditional only for error case Aishwarya Pant
2017-03-08 14:49 ` [PATCH v4 5/8] staging: bcm2835-audio: deallocate work when queue_work(...) fails Aishwarya Pant
2017-03-08 14:49 ` [PATCH v4 6/8] staging: bcm2835-audio: Add function name to debug and error logs Aishwarya Pant
2017-03-09 10:17 ` Aishwarya Pant
2017-03-08 14:50 ` [PATCH v4 7/8] staging: bcm2835-audio: fix memory leak in bcm2835_audio_open_connection() Aishwarya Pant
2017-03-08 14:50 ` [PATCH v4 8/8] staging: bcm2835-audio: remove BUG_ON() " Aishwarya Pant
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.