[PATCH 07/11] staging: vc04_services: Use __func_

Discussion:

[PATCH 07/11] staging: vc04_services: Use __func__ macro

Stefan Wahren

2018-03-31 20:09:43 UTC

It's better to use the __func__ macro instead of open-code the function
name. This fixes the following checkpatch warning:

WARNING: Prefer using '"%s...", __func__' to using 'x',
this function's name, in a string

Signed-off-by: Stefan Wahren <***@i2se.com>
---
.../interface/vchiq_arm/vchiq_2835_arm.c | 4 ++--
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 22 +++++++++++-----------
.../vc04_services/interface/vchiq_arm/vchiq_core.c | 22 +++++++++++-----------
3 files changed, 24 insertions(+), 24 deletions(-)

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 afdd3e9..2b214a3 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
@@ -582,8 +582,8 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
struct page **pages = pagelistinfo->pages;
unsigned int num_pages = pagelistinfo->num_pages;

- vchiq_log_trace(vchiq_arm_log_level, "free_pagelist - %pK, %d",
- pagelistinfo->pagelist, actual);
+ vchiq_log_trace(vchiq_arm_log_level, "%s - %pK, %d",
+ __func__, pagelistinfo->pagelist, actual);

/*
* NOTE: dma_unmap_sg must be called before the
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 1259c26..0985fd4 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -563,7 +563,7 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T reason,
/* Out of space - wait for the client */
DEBUG_TRACE(SERVICE_CALLBACK_LINE);
vchiq_log_trace(vchiq_arm_log_level,
- "add_completion - completion queue full");
+ "%s - completion queue full", __func__);
DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT);
if (down_interruptible(&instance->remove_event) != 0) {
vchiq_log_info(vchiq_arm_log_level,
@@ -641,9 +641,9 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T *header,
return VCHIQ_SUCCESS;

vchiq_log_trace(vchiq_arm_log_level,
- "service_callback - service %lx(%d,%p), reason %d, header %lx, "
+ "%s - service %lx(%d,%p), reason %d, header %lx, "
"instance %lx, bulk_userdata %lx",
- (unsigned long)user_service,
+ __func__, (unsigned long)user_service,
service->localport, user_service->userdata,
reason, (unsigned long)header,
(unsigned long)instance, (unsigned long)bulk_userdata);
@@ -679,12 +679,12 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T *header,
if (down_interruptible(&user_service->remove_event)
!= 0) {
vchiq_log_info(vchiq_arm_log_level,
- "service_callback interrupted");
+ "%s interrupted", __func__);
DEBUG_TRACE(SERVICE_CALLBACK_LINE);
return VCHIQ_RETRY;
} else if (instance->closing) {
vchiq_log_info(vchiq_arm_log_level,
- "service_callback closing");
+ "%s closing", __func__);
DEBUG_TRACE(SERVICE_CALLBACK_LINE);
return VCHIQ_ERROR;
}
@@ -740,8 +740,8 @@ user_service_free(void *userdata)
static void close_delivered(USER_SERVICE_T *user_service)
{
vchiq_log_info(vchiq_arm_log_level,
- "close_delivered(handle=%x)",
- user_service->service->handle);
+ "%s(handle=%x)",
+ __func__, user_service->service->handle);

if (user_service->close_pending) {
/* Allow the underlying service to be culled */
@@ -872,8 +872,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
DEBUG_INITIALISE(g_state.local)

vchiq_log_trace(vchiq_arm_log_level,
- "vchiq_ioctl - instance %pK, cmd %s, arg %lx",
- instance,
+ "%s - instance %pK, cmd %s, arg %lx",
+ __func__, instance,
((_IOC_TYPE(cmd) == VCHIQ_IOC_MAGIC) &&
(_IOC_NR(cmd) <= VCHIQ_IOC_MAX)) ?
ioctl_names[_IOC_NR(cmd)] : "<invalid>", arg);
@@ -2078,8 +2078,8 @@ vchiq_release(struct inode *inode, struct file *file)
int i;

vchiq_log_info(vchiq_arm_log_level,
- "vchiq_release: instance=%lx",
- (unsigned long)instance);
+ "%s: instance=%lx",
+ __func__, (unsigned long)instance);

if (!state) {
ret = -EPERM;
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index cc5b72f..6915904 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1197,8 +1197,8 @@ release_slot(VCHIQ_STATE_T *state, VCHIQ_SLOT_INFO_T *slot_info,
SLOT_INDEX_FROM_INFO(state, slot_info);
state->remote->slot_queue_recycle = slot_queue_recycle + 1;
vchiq_log_info(vchiq_core_log_level,
- "%d: release_slot %d - recycle->%x",
- state->id, SLOT_INDEX_FROM_INFO(state, slot_info),
+ "%d: %s %d - recycle->%x", state->id, __func__,
+ SLOT_INDEX_FROM_INFO(state, slot_info),
state->remote->slot_queue_recycle);

/* A write barrier is necessary, but remote_event_signal
@@ -2338,8 +2338,8 @@ vchiq_init_slots(void *mem_base, int mem_size)

if (num_slots < 4) {
vchiq_log_error(vchiq_core_log_level,
- "vchiq_init_slots - insufficient memory %x bytes",
- mem_size);
+ "%s - insufficient memory %x bytes",
+ __func__, mem_size);
return NULL;
}

@@ -2870,9 +2870,9 @@ close_service_complete(VCHIQ_SERVICE_T *service, int failstate)
break;
default:
vchiq_log_error(vchiq_core_log_level,
- "close_service_complete(%x) called in state %s",
+ "%s(%x) called in state %s", __func__,
service->handle, srvstate_names[service->srvstate]);
- WARN(1, "close_service_complete in unexpected state\n");
+ WARN(1, "%s in unexpected state\n", __func__);
return VCHIQ_ERROR;
}

@@ -2924,9 +2924,9 @@ vchiq_close_service_internal(VCHIQ_SERVICE_T *service, int close_recvd)
case VCHIQ_SRVSTATE_CLOSEWAIT:
if (close_recvd)
vchiq_log_error(vchiq_core_log_level,
- "vchiq_close_service_internal(1) called "
+ "%s(1) called "
"in state %s",
- srvstate_names[service->srvstate]);
+ __func__, srvstate_names[service->srvstate]);
else if (is_server) {
if (service->srvstate == VCHIQ_SRVSTATE_LISTENING) {
status = VCHIQ_ERROR;
@@ -3033,7 +3033,7 @@ vchiq_close_service_internal(VCHIQ_SERVICE_T *service, int close_recvd)

default:
vchiq_log_error(vchiq_core_log_level,
- "vchiq_close_service_internal(%d) called in state %s",
+ "%s(%d) called in state %s", __func__,
close_recvd, srvstate_names[service->srvstate]);
break;
}
@@ -3154,8 +3154,8 @@ vchiq_pause_internal(VCHIQ_STATE_T *state)
break;
default:
vchiq_log_error(vchiq_core_log_level,
- "vchiq_pause_internal in state %s\n",
- conn_state_names[state->conn_state]);
+ "%s in state %s\n",
+ __func__, conn_state_names[state->conn_state]);
status = VCHIQ_ERROR;
VCHIQ_STATS_INC(state, error_count);
break;

--
2.7.4

Stefan Wahren

2018-03-31 20:09:39 UTC

Permalink

Hiding memset behind a macro isn't the best, because it relies on that
the parameter is not a pointer. Luckily all user has been removed, so
we can remove BITSET_ZERO too.

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

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index afc1d81..10deb57 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -147,7 +147,6 @@ vchiq_static_assert((sizeof(BITSET_T) * 8) == 32);
#define BITSET_SIZE(b) ((b + 31) >> 5)
#define BITSET_WORD(b) (b >> 5)
#define BITSET_BIT(b) (1 << (b & 31))
-#define BITSET_ZERO(bs) memset(bs, 0, sizeof(bs))
#define BITSET_IS_SET(bs, b) (bs[BITSET_WORD(b)] & BITSET_BIT(b))
#define BITSET_SET(bs, b) (bs[BITSET_WORD(b)] |= BITSET_BIT(b))
#define BITSET_CLR(bs, b) (bs[BITSET_WORD(b)] &= ~BITSET_BIT(b))

--
2.7.4

Stefan Wahren

2018-03-31 20:09:44 UTC

Permalink

Multiline dereferences aren't nice to review. So fix this checkpatch
warning.

Signed-off-by: Stefan Wahren <***@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 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 0985fd4..00cfc50 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -2128,9 +2128,11 @@ vchiq_release(struct inode *inode, struct file *file)

while (user_service->msg_remove !=
user_service->msg_insert) {
- VCHIQ_HEADER_T *header = user_service->
- msg_queue[user_service->msg_remove &
- (MSG_QUEUE_SIZE - 1)];
+ VCHIQ_HEADER_T *header;
+ int m = user_service->msg_remove &
+ (MSG_QUEUE_SIZE - 1);
+
+ header = user_service->msg_queue[m];
user_service->msg_remove++;
spin_unlock(&msg_queue_spinlock);

@@ -2666,8 +2668,7 @@ start_suspend_timer(VCHIQ_ARM_STATE_T *arm_state)
{
del_timer(&arm_state->suspend_timer);
arm_state->suspend_timer.expires = jiffies +
- msecs_to_jiffies(arm_state->
- suspend_timer_timeout);
+ msecs_to_jiffies(arm_state->suspend_timer_timeout);
add_timer(&arm_state->suspend_timer);
arm_state->suspend_timer_running = 1;
}

--
2.7.4

Stefan Wahren

2018-03-31 20:09:42 UTC

Permalink

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>
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 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 aee721e..1259c26 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -3425,7 +3425,7 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
{
VCHIQ_ARM_STATE_T *arm_state = vchiq_platform_get_arm_state(state);
static struct service_data_struct service_data[64];
- 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;
@@ -3461,11 +3461,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 >= ARRAY_SIZE(service_data))
+ 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 >= ARRAY_SIZE(service_data))
break;
}

@@ -3484,7 +3484,7 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
"with non-zero use-count", active_services,
ARRAY_SIZE(service_data));

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

--
2.7.4

Stefan Wahren

2018-03-31 20:09:40 UTC

Permalink

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. So save stack space and removes the VLA build warning by
making it static.

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

CC: Tobin C. Harding <***@tobin.cc>
Signed-off-by: Stefan Wahren <***@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 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 24d456b..f276437 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -3414,13 +3414,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);
+ static struct service_data_struct service_data[64];
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;
@@ -3431,11 +3436,6 @@ 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;
@@ -3446,10 +3446,10 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *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 > ARRAY_SIZE(service_data))
only_nonzero = 1;

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

if (!service_ptr)
@@ -3479,7 +3479,7 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
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);
+ ARRAY_SIZE(service_data));

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

--
2.7.4

kbuild test robot

2018-03-31 23:36:13 UTC

Permalink

Hi Stefan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]
[also build test WARNING on v4.16-rc7 next-20180329]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Stefan-Wahren/staging-vchiq_core-Fix-missing-semaphore-release-in-error-case/20180401-055855
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64

All warnings (new ones prefixed by >>):

In file included from include/linux/printk.h:7:0,
from include/linux/kernel.h:14,
from drivers/staging//vc04_services/interface/vchiq_arm/vchiq_arm.c:35:
drivers/staging//vc04_services/interface/vchiq_arm/vchiq_arm.c: In function 'vchiq_dump_service_use_state':
include/linux/kern_levels.h:5:18: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long unsigned int' [-Wformat=]
#define KERN_SOH "\001" /* ASCII Start Of Header */
^
include/linux/kern_levels.h:14:19: note: in expansion of macro 'KERN_SOH'
#define KERN_INFO KERN_SOH "6" /* informational */
^~~~~~~~
drivers/staging//vc04_services/interface/vchiq_arm/vchiq_core.h:52:28: note: in expansion of macro 'KERN_INFO'
#define VCHIQ_LOG_PREFIX KERN_INFO "vchiq: "
^~~~~~~~~
drivers/staging//vc04_services/interface/vchiq_arm/vchiq_core.h:62:11: note: in expansion of macro 'VCHIQ_LOG_PREFIX'
printk(VCHIQ_LOG_PREFIX fmt "\n", ##__VA_ARGS__); } while (0)
^~~~~~~~~~~~~~~~

drivers/staging//vc04_services/interface/vchiq_arm/vchiq_arm.c:3479:3: note: in expansion of macro 'vchiq_log_warning'

vchiq_log_warning(vchiq_susp_log_level, "Too many active "
^~~~~~~~~~~~~~~~~
drivers/staging//vc04_services/interface/vchiq_arm/vchiq_arm.c:3480:47: note: format string is defined here
"services (%d). Only dumping up to first %d services "
~^
%ld

vim +/vchiq_log_warning +3479 drivers/staging//vc04_services/interface/vchiq_arm/vchiq_arm.c

8dbc30957 Stefan Wahren 2018-03-31 3422
71bad7f08 popcornmix 2013-07-02 3423 void
71bad7f08 popcornmix 2013-07-02 3424 vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
71bad7f08 popcornmix 2013-07-02 3425 {
71bad7f08 popcornmix 2013-07-02 3426 VCHIQ_ARM_STATE_T *arm_state = vchiq_platform_get_arm_state(state);
8dbc30957 Stefan Wahren 2018-03-31 3427 static struct service_data_struct service_data[64];
71bad7f08 popcornmix 2013-07-02 3428 int i, j = 0;
71bad7f08 popcornmix 2013-07-02 3429 /* If there's more than 64 services, only dump ones with
71bad7f08 popcornmix 2013-07-02 3430 * non-zero counts */
71bad7f08 popcornmix 2013-07-02 3431 int only_nonzero = 0;
71bad7f08 popcornmix 2013-07-02 3432 static const char *nz = "<-- preventing suspend";
71bad7f08 popcornmix 2013-07-02 3433
71bad7f08 popcornmix 2013-07-02 3434 enum vc_suspend_status vc_suspend_state;
71bad7f08 popcornmix 2013-07-02 3435 enum vc_resume_status vc_resume_state;
71bad7f08 popcornmix 2013-07-02 3436 int peer_count;
71bad7f08 popcornmix 2013-07-02 3437 int vc_use_count;
71bad7f08 popcornmix 2013-07-02 3438 int active_services;
71bad7f08 popcornmix 2013-07-02 3439
71bad7f08 popcornmix 2013-07-02 3440 if (!arm_state)
71bad7f08 popcornmix 2013-07-02 3441 return;
71bad7f08 popcornmix 2013-07-02 3442
71bad7f08 popcornmix 2013-07-02 3443 read_lock_bh(&arm_state->susp_res_lock);
71bad7f08 popcornmix 2013-07-02 3444 vc_suspend_state = arm_state->vc_suspend_state;
71bad7f08 popcornmix 2013-07-02 3445 vc_resume_state = arm_state->vc_resume_state;
71bad7f08 popcornmix 2013-07-02 3446 peer_count = arm_state->peer_use_count;
71bad7f08 popcornmix 2013-07-02 3447 vc_use_count = arm_state->videocore_use_count;
71bad7f08 popcornmix 2013-07-02 3448 active_services = state->unused_service;
8dbc30957 Stefan Wahren 2018-03-31 3449 if (active_services > ARRAY_SIZE(service_data))
71bad7f08 popcornmix 2013-07-02 3450 only_nonzero = 1;
71bad7f08 popcornmix 2013-07-02 3451
8dbc30957 Stefan Wahren 2018-03-31 3452 for (i = 0; (i < active_services) && (j < ARRAY_SIZE(service_data)); i++) {
71bad7f08 popcornmix 2013-07-02 3453 VCHIQ_SERVICE_T *service_ptr = state->services[i];
6e475350a Yamanappagouda Patil 2017-02-21 3454
71bad7f08 popcornmix 2013-07-02 3455 if (!service_ptr)
71bad7f08 popcornmix 2013-07-02 3456 continue;
71bad7f08 popcornmix 2013-07-02 3457
71bad7f08 popcornmix 2013-07-02 3458 if (only_nonzero && !service_ptr->service_use_count)
71bad7f08 popcornmix 2013-07-02 3459 continue;
71bad7f08 popcornmix 2013-07-02 3460
b322396ce Stefan Wahren 2017-05-26 3461 if (service_ptr->srvstate == VCHIQ_SRVSTATE_FREE)
b322396ce Stefan Wahren 2017-05-26 3462 continue;
b322396ce Stefan Wahren 2017-05-26 3463
71bad7f08 popcornmix 2013-07-02 3464 service_data[j].fourcc = service_ptr->base.fourcc;
71bad7f08 popcornmix 2013-07-02 3465 service_data[j].clientid = service_ptr->client_id;
b322396ce Stefan Wahren 2017-05-26 3466 service_data[j++].use_count = service_ptr->service_use_count;
71bad7f08 popcornmix 2013-07-02 3467 }
71bad7f08 popcornmix 2013-07-02 3468
71bad7f08 popcornmix 2013-07-02 3469 read_unlock_bh(&arm_state->susp_res_lock);
71bad7f08 popcornmix 2013-07-02 3470
71bad7f08 popcornmix 2013-07-02 3471 vchiq_log_warning(vchiq_susp_log_level,
71bad7f08 popcornmix 2013-07-02 3472 "-- Videcore suspend state: %s --",
71bad7f08 popcornmix 2013-07-02 3473 suspend_state_names[vc_suspend_state + VC_SUSPEND_NUM_OFFSET]);
71bad7f08 popcornmix 2013-07-02 3474 vchiq_log_warning(vchiq_susp_log_level,
71bad7f08 popcornmix 2013-07-02 3475 "-- Videcore resume state: %s --",
71bad7f08 popcornmix 2013-07-02 3476 resume_state_names[vc_resume_state + VC_RESUME_NUM_OFFSET]);
71bad7f08 popcornmix 2013-07-02 3477
71bad7f08 popcornmix 2013-07-02 3478 if (only_nonzero)
71bad7f08 popcornmix 2013-07-02 @3479 vchiq_log_warning(vchiq_susp_log_level, "Too many active "
71bad7f08 popcornmix 2013-07-02 3480 "services (%d). Only dumping up to first %d services "
71bad7f08 popcornmix 2013-07-02 3481 "with non-zero use-count", active_services,
8dbc30957 Stefan Wahren 2018-03-31 3482 ARRAY_SIZE(service_data));
71bad7f08 popcornmix 2013-07-02 3483
71bad7f08 popcornmix 2013-07-02 3484 for (i = 0; i < j; i++) {
71bad7f08 popcornmix 2013-07-02 3485 vchiq_log_warning(vchiq_susp_log_level,
71bad7f08 popcornmix 2013-07-02 3486 "----- %c%c%c%c:%d service count %d %s",
71bad7f08 popcornmix 2013-07-02 3487 VCHIQ_FOURCC_AS_4CHARS(service_data[i].fourcc),
71bad7f08 popcornmix 2013-07-02 3488 service_data[i].clientid,
71bad7f08 popcornmix 2013-07-02 3489 service_data[i].use_count,
71bad7f08 popcornmix 2013-07-02 3490 service_data[i].use_count ? nz : "");
71bad7f08 popcornmix 2013-07-02 3491 }
71bad7f08 popcornmix 2013-07-02 3492 vchiq_log_warning(vchiq_susp_log_level,
71bad7f08 popcornmix 2013-07-02 3493 "----- VCHIQ use count count %d", peer_count);
71bad7f08 popcornmix 2013-07-02 3494 vchiq_log_warning(vchiq_susp_log_level,
71bad7f08 popcornmix 2013-07-02 3495 "--- Overall vchiq instance use count %d", vc_use_count);
71bad7f08 popcornmix 2013-07-02 3496
71bad7f08 popcornmix 2013-07-02 3497 vchiq_dump_platform_use_state(state);
71bad7f08 popcornmix 2013-07-02 3498 }
71bad7f08 popcornmix 2013-07-02 3499

:::::: The code at line 3479 was first introduced by commit
:::::: 71bad7f086419dc674244b91ca35a12bfa4cb597 staging: add bcm2708 vchiq driver

:::::: TO: popcornmix <***@gmail.com>
:::::: CC: Greg Kroah-Hartman <***@linuxfoundation.org>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Stefan Wahren

2018-04-04 19:25:54 UTC

Permalink

Post by kbuild test robot
Hi Stefan,
[auto build test WARNING on staging/staging-testing]
[also build test WARNING on v4.16-rc7 next-20180329]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Stefan-Wahren/staging-vchiq_core-Fix-missing-semaphore-release-in-error-case/20180401-055855
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.2.0
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64
In file included from include/linux/printk.h:7:0,
from include/linux/kernel.h:14,
include/linux/kern_levels.h:5:18: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long unsigned int' [-Wformat=]
#define KERN_SOH "\001" /* ASCII Start Of Header */
^
include/linux/kern_levels.h:14:19: note: in expansion of macro 'KERN_SOH'
#define KERN_INFO KERN_SOH "6" /* informational */
^~~~~~~~
drivers/staging//vc04_services/interface/vchiq_arm/vchiq_core.h:52:28: note: in expansion of macro 'KERN_INFO'
#define VCHIQ_LOG_PREFIX KERN_INFO "vchiq: "
^~~~~~~~~
drivers/staging//vc04_services/interface/vchiq_arm/vchiq_core.h:62:11: note: in expansion of macro 'VCHIQ_LOG_PREFIX'
printk(VCHIQ_LOG_PREFIX fmt "\n", ##__VA_ARGS__); } while (0)
^~~~~~~~~~~~~~~~

drivers/staging//vc04_services/interface/vchiq_arm/vchiq_arm.c:3479:3: note: in expansion of macro 'vchiq_log_warning'

Sorry, i will fix that in the next round.

Stefan

Greg Kroah-Hartman

2018-04-23 13:48:53 UTC

Permalink

Post by Stefan Wahren

drivers/staging//vc04_services/interface/vchiq_arm/vchiq_arm.c:3479:3: note: in expansion of macro 'vchiq_log_warning'

Sorry, i will fix that in the next round.

Ok, I've applied some patches in this series, please fix up and rebase
and resend the remaining ones.

thanks,

greg k-h

Tobin C . Harding

2018-04-03 22:32:25 UTC

Permalink

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. So save stack space and removes the VLA build warning by
making it static.
[1]: https://lkml.org/lkml/2018/3/7/621
---
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 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 24d456b..f276437 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -3414,13 +3414,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);
+ static struct service_data_struct service_data[64];
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;
@@ -3431,11 +3436,6 @@ 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;
@@ -3446,10 +3446,10 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *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 > ARRAY_SIZE(service_data))
only_nonzero = 1;
- for (i = 0; (i < active_services) && (j < local_max_services); i++) {
+ for (i = 0; (i < active_services) && (j < ARRAY_SIZE(service_data)); i++) {
VCHIQ_SERVICE_T *service_ptr = state->services[i];
if (!service_ptr)
@@ -3479,7 +3479,7 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
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);
+ ARRAY_SIZE(service_data));
for (i = 0; i < j; i++) {
vchiq_log_warning(vchiq_susp_log_level,
--
2.7.4

On a previous attempt to fix this VLA Linus commented that 64 was too
big (it equates to 768 bytes on the stack). Here is that email for your
reference

https://lkml.org/lkml/2018/3/9/744

I didn't understand what he meant by 'just do the stack size limitation'

Hope this helps,
Tobin.

Stefan Wahren

2018-03-31 20:09:45 UTC

Permalink

vchiq_initialise() is used in non-interrupt context, so we can
replace udelay with usleep_range as suggested by timers-howto.txt.

Signed-off-by: Stefan Wahren <***@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 2 +-
1 file changed, 1 insertion(+), 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 00cfc50..285849b 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -215,7 +215,7 @@ VCHIQ_STATUS_T vchiq_initialise(VCHIQ_INSTANCE_T *instance_out)
state = vchiq_get_state();
if (state)
break;
- udelay(500);
+ usleep_range(500, 600);
}
if (i == VCHIQ_INIT_RETRIES) {
vchiq_log_error(vchiq_core_log_level,

--
2.7.4

Stefan Wahren

2018-03-31 20:09:47 UTC

Permalink

We need to free the kthreads in error case before leaving
vchiq_init_state() otherwise we leak resources.

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

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index d4f2458..7642ced 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2562,7 +2562,7 @@ vchiq_init_state(VCHIQ_STATE_T *state, VCHIQ_SLOT_ZERO_T *slot_zero,
vchiq_loud_error_header();
vchiq_loud_error("couldn't create thread %s", threadname);
vchiq_loud_error_footer();
- return VCHIQ_ERROR;
+ goto fail_free_handler_thread;
}
set_user_nice(state->recycle_thread, -19);

@@ -2574,7 +2574,7 @@ vchiq_init_state(VCHIQ_STATE_T *state, VCHIQ_SLOT_ZERO_T *slot_zero,
vchiq_loud_error_header();
vchiq_loud_error("couldn't create thread %s", threadname);
vchiq_loud_error_footer();
- return VCHIQ_ERROR;
+ goto fail_free_recycle_thread;
}
set_user_nice(state->sync_thread, -20);

@@ -2588,6 +2588,13 @@ vchiq_init_state(VCHIQ_STATE_T *state, VCHIQ_SLOT_ZERO_T *slot_zero,
local->initialised = 1;

return status;
+
+fail_free_recycle_thread:
+ kthread_stop(state->recycle_thread);
+fail_free_handler_thread:
+ kthread_stop(state->slot_handler_thread);
+
+ return VCHIQ_ERROR;
}

/* Called from application thread when a client or server service is created. */

--
2.7.4

Stefan Wahren

2018-03-31 20:09:38 UTC

Permalink

This removes the stackhog in process_free_queue by allocating the
necessary memory within the recycle thread main function instead
of the stack.

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

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 80f6168..cc5b72f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -620,10 +620,9 @@ reserve_space(VCHIQ_STATE_T *state, size_t space, int is_blocking)

/* Called by the recycle thread. */
static void
-process_free_queue(VCHIQ_STATE_T *state)
+process_free_queue(VCHIQ_STATE_T *state, BITSET_T *service_found, size_t length)
{
VCHIQ_SHARED_STATE_T *local = state->local;
- BITSET_T service_found[BITSET_SIZE(VCHIQ_MAX_SERVICES)];
int slot_queue_available;

/* Find slots which have been freed by the other side, and return them
@@ -656,7 +655,7 @@ process_free_queue(VCHIQ_STATE_T *state)

/* Initialise the bitmask for services which have used this
** slot */
- BITSET_ZERO(service_found);
+ memset(service_found, 0, length);

pos = 0;

@@ -2183,11 +2182,20 @@ recycle_func(void *v)
{
VCHIQ_STATE_T *state = (VCHIQ_STATE_T *) v;
VCHIQ_SHARED_STATE_T *local = state->local;
+ BITSET_T *found;
+ size_t length;
+
+ length = sizeof(*found) * BITSET_SIZE(VCHIQ_MAX_SERVICES);
+
+ found = kmalloc_array(BITSET_SIZE(VCHIQ_MAX_SERVICES), sizeof(*found),
+ GFP_KERNEL);
+ if (!found)
+ return -ENOMEM;

while (1) {
remote_event_wait(state, &local->recycle);

- process_free_queue(state);
+ process_free_queue(state, found, length);
}
return 0;
}

--
2.7.4

Stefan Wahren

2018-03-31 20:09:46 UTC

Permalink

Move all calls of wake_up_process to one point, whichs makes the
following implementation of clean-up code easier.

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

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 6915904..d4f2458 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2553,7 +2553,6 @@ vchiq_init_state(VCHIQ_STATE_T *state, VCHIQ_SLOT_ZERO_T *slot_zero,
return VCHIQ_ERROR;
}
set_user_nice(state->slot_handler_thread, -19);
- wake_up_process(state->slot_handler_thread);

snprintf(threadname, sizeof(threadname), "vchiq-recy/%d", state->id);
state->recycle_thread = kthread_create(&recycle_func,
@@ -2566,7 +2565,6 @@ vchiq_init_state(VCHIQ_STATE_T *state, VCHIQ_SLOT_ZERO_T *slot_zero,
return VCHIQ_ERROR;
}
set_user_nice(state->recycle_thread, -19);
- wake_up_process(state->recycle_thread);

snprintf(threadname, sizeof(threadname), "vchiq-sync/%d", state->id);
state->sync_thread = kthread_create(&sync_func,
@@ -2579,6 +2577,9 @@ vchiq_init_state(VCHIQ_STATE_T *state, VCHIQ_SLOT_ZERO_T *slot_zero,
return VCHIQ_ERROR;
}
set_user_nice(state->sync_thread, -20);
+
+ wake_up_process(state->slot_handler_thread);
+ wake_up_process(state->recycle_thread);
wake_up_process(state->sync_thread);

vchiq_states[0] = state;

--
2.7.4

Stefan Wahren

2018-03-31 20:09:41 UTC

Permalink

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 f276437..aee721e 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -3449,7 +3449,7 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
if (active_services > ARRAY_SIZE(service_data))
only_nonzero = 1;

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

if (!service_ptr)
@@ -3463,7 +3463,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 >= ARRAY_SIZE(service_data))
+ break;
}

read_unlock_bh(&arm_state->susp_res_lock);

--
2.7.4