[PATCH V2 4/9] staging: vchiq_arm: Make index variable more self-explaining

[Stefan Wahren]

The chance to mixup i and j is very high. So rename variable j to a more
explaining one.

Signed-off-by: Stefan Wahren <***@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 01fcd15..2b0d764 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -3426,7 +3426,7 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
{
VCHIQ_ARM_STATE_T *arm_state = vchiq_platform_get_arm_state(state);
struct service_data_struct *service_data;
- int i, j = 0;
+ int i, found = 0;
/* If there's more than 64 services, only dump ones with
* non-zero counts */
int only_nonzero = 0;
@@ -3467,11 +3467,11 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
if (service_ptr->srvstate == VCHIQ_SRVSTATE_FREE)
continue;

- service_data[j].fourcc = service_ptr->base.fourcc;
- service_data[j].clientid = service_ptr->client_id;
- service_data[j].use_count = service_ptr->service_use_count;
- j++;
- if (j >= MAX_SERVICES)
+ service_data[found].fourcc = service_ptr->base.fourcc;
+ service_data[found].clientid = service_ptr->client_id;
+ service_data[found].use_count = service_ptr->service_use_count;
+ found++;
+ if (found >= MAX_SERVICES)
break;
}

@@ -3487,9 +3487,9 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
if (only_nonzero)
vchiq_log_warning(vchiq_susp_log_level, "Too many active "
"services (%d). Only dumping up to first %d services "
- "with non-zero use-count", active_services, j);
+ "with non-zero use-count", active_services, found);

- for (i = 0; i < j; i++) {
+ for (i = 0; i < found; i++) {
vchiq_log_warning(vchiq_susp_log_level,
"----- %c%c%c%c:%d service count %d %s",
VCHIQ_FOURCC_AS_4CHARS(service_data[i].fourcc),

[Stefan Wahren]

This fixes a whitespace issue reported by checkpatch:

ERROR: code indent should use tabs where possible

Signed-off-by: Stefan Wahren <***@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 2b214a3..e767209 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -475,7 +475,7 @@ create_pagelist(char __user *buf, size_t count, unsigned short type)
/* do not try and release vmalloc pages */
} else {
actual_pages = get_user_pages_fast(
- (unsigned long)buf & PAGE_MASK,
+ (unsigned long)buf & PAGE_MASK,
num_pages,
type == PAGELIST_READ,
pages);

[Stefan Wahren]

We better use the real loop limit in the warning about
"too many active services". Another benefit is than we don't have to care
about the format string in the following change.

Signed-off-by: Stefan Wahren <***@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 4b16aa4..8575bd8 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -3479,8 +3479,7 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
if (only_nonzero)
vchiq_log_warning(vchiq_susp_log_level, "Too many active "
"services (%d). Only dumping up to first %d services "
- "with non-zero use-count", active_services,
- local_max_services);
+ "with non-zero use-count", active_services, j);

for (i = 0; i < j; i++) {
vchiq_log_warning(vchiq_susp_log_level,

[Stefan Wahren]

The definition of USE_VCHIQ_ARM and VCOS_VERIFY_BKPTS isn't really necessary
so remove them.

Signed-off-by: Stefan Wahren <***@i2se.com>
---
drivers/staging/vc04_services/Makefile | 2 +-
drivers/staging/vc04_services/interface/vchi/vchi_common.h | 2 --
2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/vc04_services/Makefile b/drivers/staging/vc04_services/Makefile
index fb26b82..afe43fa 100644
--- a/drivers/staging/vc04_services/Makefile
+++ b/drivers/staging/vc04_services/Makefile
@@ -13,5 +13,5 @@ vchiq-objs := \
obj-$(CONFIG_SND_BCM2835) += bcm2835-audio/
obj-$(CONFIG_VIDEO_BCM2835) += bcm2835-camera/

-ccflags-y += -DVCOS_VERIFY_BKPTS=1 -Idrivers/staging/vc04_services -DUSE_VCHIQ_ARM -D__VCCOREVER__=0x04000000
+ccflags-y += -Idrivers/staging/vc04_services -D__VCCOREVER__=0x04000000

diff --git a/drivers/staging/vc04_services/interface/vchi/vchi_common.h b/drivers/staging/vc04_services/interface/vchi/vchi_common.h
index 76e10fe..8eb2bb9 100644
--- a/drivers/staging/vc04_services/interface/vchi/vchi_common.h
+++ b/drivers/staging/vc04_services/interface/vchi/vchi_common.h
@@ -97,12 +97,10 @@ typedef enum {
VCHI_CALLBACK_PEER_RESUMED,
VCHI_CALLBACK_FORCED_POWER_OFF,

-#ifdef USE_VCHIQ_ARM
// some extra notifications provided by vchiq_arm
VCHI_CALLBACK_SERVICE_OPENED,
VCHI_CALLBACK_BULK_RECEIVE_ABORTED,
VCHI_CALLBACK_BULK_TRANSMIT_ABORTED,
-#endif

VCHI_CALLBACK_REASON_MAX
} VCHI_CALLBACK_REASON_T;

[Stefan Wahren]

This removes an unnecessary break which has been reported by checkpatch.pl.

Signed-off-by: Stefan Wahren <***@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
index 29984f9..dddc828 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
@@ -590,7 +590,6 @@ static VCHIQ_STATUS_T shim_callback(VCHIQ_REASON_T reason,
VCHI_CALLBACK_MSG_AVAILABLE, NULL);

goto done;
- break;

case VCHIQ_BULK_TRANSMIT_DONE:
service->callback(service->callback_param,

[Stefan Wahren]

Since there is no vchiq function listed by "make checkstack" anymore,
we can remove the stack hog issue from the TODO.

Signed-off-by: Stefan Wahren <***@i2se.com>
---
drivers/staging/vc04_services/interface/vchi/TODO | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchi/TODO b/drivers/staging/vc04_services/interface/vchi/TODO
index 86708c7..0b3ec75 100644
--- a/drivers/staging/vc04_services/interface/vchi/TODO
+++ b/drivers/staging/vc04_services/interface/vchi/TODO
@@ -40,17 +40,12 @@ should properly handle a module unload. This also includes that all
resouces must be freed (kthreads, debugfs entries, ...) and global
variables avoided.

-5) Fix stack hog
-
-Running make checkstack shows that vchiq_dump_service_use_state() has
-an extensive stack usage. Maybe other functions are also affected.
-
-6) Cleanup logging mechanism
+5) Cleanup logging mechanism

The driver should probably be using the standard kernel logging mechanisms
such as dev_info, dev_dbg, and friends.

-7) Documentation
+6) Documentation

A short top-down description of this driver's architecture (function of
kthreads, userspace, limitations) could be very helpful for reviewers.

[Stefan Wahren]

The kernel would like to have all stack VLA usage removed[1]. The array
here is fixed (declared with a const variable) but it appears like a VLA
to the compiler. Also, currently we are putting 768 bytes on the
stack. This function is only called on the error path so performance is
not critical, let's just allocate the memory instead of using the
stack. This saves stack space and removes the VLA build warning.

kmalloc a buffer for dumping state instead of using the stack.

[1]: https://lkml.org/lkml/2018/3/7/621

CC: Kees Cook <***@google.com>
Signed-off-by: Tobin C. Harding <***@tobin.cc>
Signed-off-by: Stefan Wahren <***@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 25 ++++++++++++++--------
1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 8575bd8..d46a5b8 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -3415,13 +3415,18 @@ vchiq_release_service(VCHIQ_SERVICE_HANDLE_T handle)
return ret;
}

+struct service_data_struct {
+ int fourcc;
+ int clientid;
+ int use_count;
+};
+
void
vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
{
VCHIQ_ARM_STATE_T *arm_state = vchiq_platform_get_arm_state(state);
+ struct service_data_struct *service_data;
int i, j = 0;
- /* Only dump 64 services */
- static const int local_max_services = 64;
/* If there's more than 64 services, only dump ones with
* non-zero counts */
int only_nonzero = 0;
@@ -3432,25 +3437,25 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
int peer_count;
int vc_use_count;
int active_services;
- struct service_data_struct {
- int fourcc;
- int clientid;
- int use_count;
- } service_data[local_max_services];

if (!arm_state)
return;

+ service_data = kmalloc_array(MAX_SERVICES, sizeof(*service_data),
+ GFP_KERNEL);
+ if (!service_data)
+ return;
+
read_lock_bh(&arm_state->susp_res_lock);
vc_suspend_state = arm_state->vc_suspend_state;
vc_resume_state = arm_state->vc_resume_state;
peer_count = arm_state->peer_use_count;
vc_use_count = arm_state->videocore_use_count;
active_services = state->unused_service;
- if (active_services > local_max_services)
+ if (active_services > MAX_SERVICES)
only_nonzero = 1;

- for (i = 0; (i < active_services) && (j < local_max_services); i++) {
+ for (i = 0; (i < active_services) && (j < MAX_SERVICES); i++) {
VCHIQ_SERVICE_T *service_ptr = state->services[i];

if (!service_ptr)
@@ -3494,6 +3499,8 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
vchiq_log_warning(vchiq_susp_log_level,
"--- Overall vchiq instance use count %d", vc_use_count);

+ kfree(service_data);
+
vchiq_dump_platform_use_state(state);
}

[Kees Cook]

Post by Stefan Wahren
The kernel would like to have all stack VLA usage removed[1]. The array
here is fixed (declared with a const variable) but it appears like a VLA
to the compiler. Also, currently we are putting 768 bytes on the
stack. This function is only called on the error path so performance is
not critical, let's just allocate the memory instead of using the
stack. This saves stack space and removes the VLA build warning.
kmalloc a buffer for dumping state instead of using the stack.
[1]: https://lkml.org/lkml/2018/3/7/621

Reviewed-by: Kees Cook <***@chromium.org>

-Kees

Post by Stefan Wahren
---
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 25 ++++++++++++++--------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 8575bd8..d46a5b8 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -3415,13 +3415,18 @@ vchiq_release_service(VCHIQ_SERVICE_HANDLE_T handle)
return ret;
}
+struct service_data_struct {
+ int fourcc;
+ int clientid;
+ int use_count;
+};
+
void
vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
{
VCHIQ_ARM_STATE_T *arm_state = vchiq_platform_get_arm_state(state);
+ struct service_data_struct *service_data;
int i, j = 0;
- /* Only dump 64 services */
- static const int local_max_services = 64;
/* If there's more than 64 services, only dump ones with
* non-zero counts */
int only_nonzero = 0;
@@ -3432,25 +3437,25 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
int peer_count;
int vc_use_count;
int active_services;
- struct service_data_struct {
- int fourcc;
- int clientid;
- int use_count;
- } service_data[local_max_services];
if (!arm_state)
return;
+ service_data = kmalloc_array(MAX_SERVICES, sizeof(*service_data),
+ GFP_KERNEL);
+ if (!service_data)
+ return;
+
read_lock_bh(&arm_state->susp_res_lock);
vc_suspend_state = arm_state->vc_suspend_state;
vc_resume_state = arm_state->vc_resume_state;
peer_count = arm_state->peer_use_count;
vc_use_count = arm_state->videocore_use_count;
active_services = state->unused_service;
- if (active_services > local_max_services)
+ if (active_services > MAX_SERVICES)
only_nonzero = 1;
- for (i = 0; (i < active_services) && (j < local_max_services); i++) {
+ for (i = 0; (i < active_services) && (j < MAX_SERVICES); i++) {
VCHIQ_SERVICE_T *service_ptr = state->services[i];
if (!service_ptr)
@@ -3494,6 +3499,8 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
vchiq_log_warning(vchiq_susp_log_level,
"--- Overall vchiq instance use count %d", vc_use_count);
+ kfree(service_data);
+
vchiq_dump_platform_use_state(state);
}
--
2.7.4

[Stefan Wahren]

In order to make the code easier to review, move the second
abort criterion into the loop and the incrementation into
a separate line.

Signed-off-by: Stefan Wahren <***@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index d46a5b8..01fcd15 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -3455,7 +3455,7 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
if (active_services > MAX_SERVICES)
only_nonzero = 1;

- for (i = 0; (i < active_services) && (j < MAX_SERVICES); i++) {
+ for (i = 0; i < active_services; i++) {
VCHIQ_SERVICE_T *service_ptr = state->services[i];

if (!service_ptr)
@@ -3469,7 +3469,10 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)

service_data[j].fourcc = service_ptr->base.fourcc;
service_data[j].clientid = service_ptr->client_id;
- service_data[j++].use_count = service_ptr->service_use_count;
+ service_data[j].use_count = service_ptr->service_use_count;
+ j++;
+ if (j >= MAX_SERVICES)
+ break;
}

read_unlock_bh(&arm_state->susp_res_lock);

[Stefan Wahren]

This removing an unnecessary return which has been reported by checkpatch.pl.

Signed-off-by: Stefan Wahren <***@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 2b0d764..3cd6177 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -3020,7 +3020,6 @@ vchiq_check_suspend(VCHIQ_STATE_T *state)

out:
vchiq_log_trace(vchiq_susp_log_level, "%s exit", __func__);
- return;
}

int