* [PATCH 00/10] staging: vchiq_arm: address TODO list
@ 2021-04-22 20:07 Stefan Wahren
2021-04-22 20:07 ` [PATCH 01/10] staging: vchiq_arm: avoid crashing the kernel Stefan Wahren
` (9 more replies)
0 siblings, 10 replies; 25+ messages in thread
From: Stefan Wahren @ 2021-04-22 20:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, Nicolas Saenz Julienne; +Cc: linux-staging, Stefan Wahren
This series mostly addresses the following points from the TODO list:
- Get rid of custom function return values
- Reformat core code with more sane indentations
Additionally this addresses some issues reported by checkpatch.pl
Stefan Wahren (10):
staging: vchiq_arm: avoid crashing the kernel
staging: vchiq_core: break early in vchiq_close_service_internal
staging: vchiq_core: introduce get_bulk_reason
staging: vchiq_core: Drop unnecessary check in notify_bulks
staging: vchiq_arm: drop return value of vchiq_arm_init_state
staging: vchiq_2835_arm: drop enum vchiq_status
staging: vchiq_arm: drop enum vchiq_status from vchiq_*_internal
staging: vchiq_core: drop vchiq_status from vchiq_set_service_option
staging: vchiq_core: drop vchiq_status from vchiq_initialise
staging: vchiq_core: drop vchiq_status from vchiq_init_state
.../interface/vchiq_arm/vchiq_2835_arm.c | 23 ++-
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 75 ++++----
.../vc04_services/interface/vchiq_arm/vchiq_arm.h | 2 +-
.../vc04_services/interface/vchiq_arm/vchiq_core.c | 191 +++++++++++----------
.../vc04_services/interface/vchiq_arm/vchiq_core.h | 12 +-
5 files changed, 158 insertions(+), 145 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 01/10] staging: vchiq_arm: avoid crashing the kernel
2021-04-22 20:07 [PATCH 00/10] staging: vchiq_arm: address TODO list Stefan Wahren
@ 2021-04-22 20:07 ` Stefan Wahren
2021-04-24 9:49 ` nicolas saenz julienne
2021-04-22 20:07 ` [PATCH 02/10] staging: vchiq_core: break early in vchiq_close_service_internal Stefan Wahren
` (8 subsequent siblings)
9 siblings, 1 reply; 25+ messages in thread
From: Stefan Wahren @ 2021-04-22 20:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, Nicolas Saenz Julienne; +Cc: linux-staging, Stefan Wahren
Using BUG_ON in a non-essential driver isn't recommend. So better
trigger a stacktrace and bailout.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 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 8b2b477..c51840a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -602,7 +602,9 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
DEBUG_TRACE(SERVICE_CALLBACK_LINE);
service = handle_to_service(handle);
- BUG_ON(!service);
+ if (WARN_ON(!service))
+ return VCHIQ_SUCCESS;
+
user_service = (struct user_service *)service->base.userdata;
instance = user_service->instance;
@@ -918,8 +920,12 @@ static int vchiq_ioc_dequeue_message(struct vchiq_instance *instance,
goto out;
}
- BUG_ON((int)(user_service->msg_insert -
- user_service->msg_remove) < 0);
+ if (WARN_ON_ONCE((int)(user_service->msg_insert -
+ user_service->msg_remove) < 0)) {
+ spin_unlock(&msg_queue_spinlock);
+ ret = -EINVAL;
+ goto out;
+ }
header = user_service->msg_queue[user_service->msg_remove &
(MSG_QUEUE_SIZE - 1)];
@@ -1937,7 +1943,10 @@ static int vchiq_release(struct inode *inode, struct file *file)
wait_for_completion(&service->remove_event);
- BUG_ON(service->srvstate != VCHIQ_SRVSTATE_FREE);
+ if (WARN_ON(service->srvstate != VCHIQ_SRVSTATE_FREE)) {
+ unlock_service(service);
+ break;
+ }
spin_lock(&msg_queue_spinlock);
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 02/10] staging: vchiq_core: break early in vchiq_close_service_internal
2021-04-22 20:07 [PATCH 00/10] staging: vchiq_arm: address TODO list Stefan Wahren
2021-04-22 20:07 ` [PATCH 01/10] staging: vchiq_arm: avoid crashing the kernel Stefan Wahren
@ 2021-04-22 20:07 ` Stefan Wahren
2021-04-23 9:10 ` nicolas saenz julienne
2021-04-22 20:07 ` [PATCH 03/10] staging: vchiq_core: introduce get_bulk_reason Stefan Wahren
` (7 subsequent siblings)
9 siblings, 1 reply; 25+ messages in thread
From: Stefan Wahren @ 2021-04-22 20:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, Nicolas Saenz Julienne; +Cc: linux-staging, Stefan Wahren
The logic in VCHIQ_SRVSTATE_OPEN* is unnecessary complex. Handle the error
case of queue_message() first makes it easier to read.
Btw we get the rid of the checkpatch warning:
WARNING: else is not generally useful after a break or return
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_core.c | 26 +++++++++++-----------
1 file changed, 13 insertions(+), 13 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 517a8c9..8a1e6f5 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2763,21 +2763,21 @@ vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
VCHIQ_MSG_DSTPORT(service->remoteport)),
NULL, NULL, 0, QMFLAGS_NO_MUTEX_UNLOCK);
- if (status == VCHIQ_SUCCESS) {
- if (!close_recvd) {
- /* Change the state while the mutex is still held */
- vchiq_set_service_state(service,
- VCHIQ_SRVSTATE_CLOSESENT);
- mutex_unlock(&state->slot_mutex);
- if (service->sync)
- mutex_unlock(&state->sync_mutex);
- break;
- }
- } else if (service->srvstate == VCHIQ_SRVSTATE_OPENSYNC) {
- mutex_unlock(&state->sync_mutex);
+ if (status != VCHIQ_SUCCESS) {
+ if (service->srvstate == VCHIQ_SRVSTATE_OPENSYNC)
+ mutex_unlock(&state->sync_mutex);
break;
- } else
+ }
+
+ if (!close_recvd) {
+ /* Change the state while the mutex is still held */
+ vchiq_set_service_state(service,
+ VCHIQ_SRVSTATE_CLOSESENT);
+ mutex_unlock(&state->slot_mutex);
+ if (service->sync)
+ mutex_unlock(&state->sync_mutex);
break;
+ }
/* Change the state while the mutex is still held */
vchiq_set_service_state(service, VCHIQ_SRVSTATE_CLOSERECVD);
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 03/10] staging: vchiq_core: introduce get_bulk_reason
2021-04-22 20:07 [PATCH 00/10] staging: vchiq_arm: address TODO list Stefan Wahren
2021-04-22 20:07 ` [PATCH 01/10] staging: vchiq_arm: avoid crashing the kernel Stefan Wahren
2021-04-22 20:07 ` [PATCH 02/10] staging: vchiq_core: break early in vchiq_close_service_internal Stefan Wahren
@ 2021-04-22 20:07 ` Stefan Wahren
2021-04-23 7:23 ` Fabio Aiuto
2021-04-23 9:17 ` nicolassaenzj
2021-04-22 20:07 ` [PATCH 04/10] staging: vchiq_core: Drop unnecessary check in notify_bulks Stefan Wahren
` (6 subsequent siblings)
9 siblings, 2 replies; 25+ messages in thread
From: Stefan Wahren @ 2021-04-22 20:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, Nicolas Saenz Julienne; +Cc: linux-staging, Stefan Wahren
Nesting multiple ternary operators over multiple lines isn't easy to
read. Move this logic into a separate inline function.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_core.c | 35 ++++++++++++++--------
1 file changed, 22 insertions(+), 13 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 8a1e6f5..3a72c36 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1226,6 +1226,22 @@ release_slot(struct vchiq_state *state, struct vchiq_slot_info *slot_info,
mutex_unlock(&state->recycle_mutex);
}
+static inline enum vchiq_reason
+get_bulk_reason(struct vchiq_bulk *bulk)
+{
+ if (bulk->dir == VCHIQ_BULK_TRANSMIT) {
+ if (bulk->actual == VCHIQ_BULK_ACTUAL_ABORTED)
+ return VCHIQ_BULK_TRANSMIT_ABORTED;
+
+ return VCHIQ_BULK_TRANSMIT_DONE;
+ }
+
+ if (bulk->actual == VCHIQ_BULK_ACTUAL_ABORTED)
+ return VCHIQ_BULK_RECEIVE_ABORTED;
+
+ return VCHIQ_BULK_RECEIVE_DONE;
+}
+
/* Called by the slot handler - don't hold the bulk mutex */
static enum vchiq_status
notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
@@ -1281,16 +1297,8 @@ notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
spin_unlock(&bulk_waiter_spinlock);
} else if (bulk->mode ==
VCHIQ_BULK_MODE_CALLBACK) {
- enum vchiq_reason reason = (bulk->dir ==
- VCHIQ_BULK_TRANSMIT) ?
- ((bulk->actual ==
- VCHIQ_BULK_ACTUAL_ABORTED) ?
- VCHIQ_BULK_TRANSMIT_ABORTED :
- VCHIQ_BULK_TRANSMIT_DONE) :
- ((bulk->actual ==
- VCHIQ_BULK_ACTUAL_ABORTED) ?
- VCHIQ_BULK_RECEIVE_ABORTED :
- VCHIQ_BULK_RECEIVE_DONE);
+ enum vchiq_reason reason =
+ get_bulk_reason(bulk);
status = make_service_callback(service,
reason, NULL, bulk->userdata);
if (status == VCHIQ_RETRY)
@@ -2622,9 +2630,10 @@ do_abort_bulks(struct vchiq_service *service)
mutex_unlock(&service->bulk_mutex);
status = notify_bulks(service, &service->bulk_tx, 0/*!retry_poll*/);
- if (status == VCHIQ_SUCCESS)
- status = notify_bulks(service, &service->bulk_rx,
- 0/*!retry_poll*/);
+ if (status != VCHIQ_SUCCESS)
+ return 0;
+
+ status = notify_bulks(service, &service->bulk_rx, 0/*!retry_poll*/);
return (status == VCHIQ_SUCCESS);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 04/10] staging: vchiq_core: Drop unnecessary check in notify_bulks
2021-04-22 20:07 [PATCH 00/10] staging: vchiq_arm: address TODO list Stefan Wahren
` (2 preceding siblings ...)
2021-04-22 20:07 ` [PATCH 03/10] staging: vchiq_core: introduce get_bulk_reason Stefan Wahren
@ 2021-04-22 20:07 ` Stefan Wahren
2021-04-23 9:22 ` nicolassaenzj
2021-04-22 20:07 ` [PATCH 05/10] staging: vchiq_arm: drop return value of vchiq_arm_init_state Stefan Wahren
` (5 subsequent siblings)
9 siblings, 1 reply; 25+ messages in thread
From: Stefan Wahren @ 2021-04-22 20:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, Nicolas Saenz Julienne; +Cc: linux-staging, Stefan Wahren
There is no modification to the vchiq_status before the first if, so drop
this unnecessary check.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_core.c | 92 +++++++++++-----------
1 file changed, 45 insertions(+), 47 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 3a72c36..9dc9a6a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1257,61 +1257,59 @@ notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
queue->remote_notify = queue->process;
- if (status == VCHIQ_SUCCESS) {
- while (queue->remove != queue->remote_notify) {
- struct vchiq_bulk *bulk =
- &queue->bulks[BULK_INDEX(queue->remove)];
+ while (queue->remove != queue->remote_notify) {
+ struct vchiq_bulk *bulk =
+ &queue->bulks[BULK_INDEX(queue->remove)];
- /*
- * Only generate callbacks for non-dummy bulk
- * requests, and non-terminated services
- */
- if (bulk->data && service->instance) {
- if (bulk->actual != VCHIQ_BULK_ACTUAL_ABORTED) {
- if (bulk->dir == VCHIQ_BULK_TRANSMIT) {
- VCHIQ_SERVICE_STATS_INC(service,
- bulk_tx_count);
- VCHIQ_SERVICE_STATS_ADD(service,
- bulk_tx_bytes,
- bulk->actual);
- } else {
- VCHIQ_SERVICE_STATS_INC(service,
- bulk_rx_count);
- VCHIQ_SERVICE_STATS_ADD(service,
- bulk_rx_bytes,
- bulk->actual);
- }
+ /*
+ * Only generate callbacks for non-dummy bulk
+ * requests, and non-terminated services
+ */
+ if (bulk->data && service->instance) {
+ if (bulk->actual != VCHIQ_BULK_ACTUAL_ABORTED) {
+ if (bulk->dir == VCHIQ_BULK_TRANSMIT) {
+ VCHIQ_SERVICE_STATS_INC(service,
+ bulk_tx_count);
+ VCHIQ_SERVICE_STATS_ADD(service,
+ bulk_tx_bytes,
+ bulk->actual);
} else {
VCHIQ_SERVICE_STATS_INC(service,
- bulk_aborted_count);
+ bulk_rx_count);
+ VCHIQ_SERVICE_STATS_ADD(service,
+ bulk_rx_bytes,
+ bulk->actual);
}
- if (bulk->mode == VCHIQ_BULK_MODE_BLOCKING) {
- struct bulk_waiter *waiter;
-
- spin_lock(&bulk_waiter_spinlock);
- waiter = bulk->userdata;
- if (waiter) {
- waiter->actual = bulk->actual;
- complete(&waiter->event);
- }
- spin_unlock(&bulk_waiter_spinlock);
- } else if (bulk->mode ==
- VCHIQ_BULK_MODE_CALLBACK) {
- enum vchiq_reason reason =
- get_bulk_reason(bulk);
- status = make_service_callback(service,
- reason, NULL, bulk->userdata);
- if (status == VCHIQ_RETRY)
- break;
+ } else {
+ VCHIQ_SERVICE_STATS_INC(service,
+ bulk_aborted_count);
+ }
+ if (bulk->mode == VCHIQ_BULK_MODE_BLOCKING) {
+ struct bulk_waiter *waiter;
+
+ spin_lock(&bulk_waiter_spinlock);
+ waiter = bulk->userdata;
+ if (waiter) {
+ waiter->actual = bulk->actual;
+ complete(&waiter->event);
}
+ spin_unlock(&bulk_waiter_spinlock);
+ } else if (bulk->mode ==
+ VCHIQ_BULK_MODE_CALLBACK) {
+ enum vchiq_reason reason =
+ get_bulk_reason(bulk);
+ status = make_service_callback(service,
+ reason, NULL, bulk->userdata);
+ if (status == VCHIQ_RETRY)
+ break;
}
-
- queue->remove++;
- complete(&service->bulk_remove_event);
}
- if (!retry_poll)
- status = VCHIQ_SUCCESS;
+
+ queue->remove++;
+ complete(&service->bulk_remove_event);
}
+ if (!retry_poll)
+ status = VCHIQ_SUCCESS;
if (status == VCHIQ_RETRY)
request_poll(service->state, service,
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 05/10] staging: vchiq_arm: drop return value of vchiq_arm_init_state
2021-04-22 20:07 [PATCH 00/10] staging: vchiq_arm: address TODO list Stefan Wahren
` (3 preceding siblings ...)
2021-04-22 20:07 ` [PATCH 04/10] staging: vchiq_core: Drop unnecessary check in notify_bulks Stefan Wahren
@ 2021-04-22 20:07 ` Stefan Wahren
2021-04-23 9:28 ` nicolas saenz julienne
2021-04-22 20:07 ` [PATCH 06/10] staging: vchiq_2835_arm: drop enum vchiq_status Stefan Wahren
` (4 subsequent siblings)
9 siblings, 1 reply; 25+ messages in thread
From: Stefan Wahren @ 2021-04-22 20:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, Nicolas Saenz Julienne; +Cc: linux-staging, Stefan Wahren
The function vchiq_arm_init_state() cannot fail. So drop the return
value and the unnecessary code.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 8 ++------
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 +--
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h | 2 +-
3 files changed, 4 insertions(+), 9 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 8782ebe..7c336d0 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
@@ -172,7 +172,6 @@ int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
enum vchiq_status
vchiq_platform_init_state(struct vchiq_state *state)
{
- enum vchiq_status status = VCHIQ_SUCCESS;
struct vchiq_2835_state *platform_state;
state->platform_state = kzalloc(sizeof(*platform_state), GFP_KERNEL);
@@ -182,12 +181,9 @@ vchiq_platform_init_state(struct vchiq_state *state)
platform_state = (struct vchiq_2835_state *)state->platform_state;
platform_state->inited = 1;
- status = vchiq_arm_init_state(state, &platform_state->arm_state);
+ vchiq_arm_init_state(state, &platform_state->arm_state);
- if (status != VCHIQ_SUCCESS)
- platform_state->inited = 0;
-
- return status;
+ return VCHIQ_SUCCESS;
}
struct vchiq_arm_state*
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 c51840a..db10d83 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -2312,7 +2312,7 @@ vchiq_keepalive_thread_func(void *v)
return 0;
}
-enum vchiq_status
+void
vchiq_arm_init_state(struct vchiq_state *state,
struct vchiq_arm_state *arm_state)
{
@@ -2328,7 +2328,6 @@ vchiq_arm_init_state(struct vchiq_state *state,
arm_state->first_connect = 0;
}
- return VCHIQ_SUCCESS;
}
enum vchiq_status
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index 24c331c..c7d2cf1 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -67,7 +67,7 @@ int vchiq_platform_init(struct platform_device *pdev,
extern struct vchiq_state *
vchiq_get_state(void);
-extern enum vchiq_status
+extern void
vchiq_arm_init_state(struct vchiq_state *state,
struct vchiq_arm_state *arm_state);
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 06/10] staging: vchiq_2835_arm: drop enum vchiq_status
2021-04-22 20:07 [PATCH 00/10] staging: vchiq_arm: address TODO list Stefan Wahren
` (4 preceding siblings ...)
2021-04-22 20:07 ` [PATCH 05/10] staging: vchiq_arm: drop return value of vchiq_arm_init_state Stefan Wahren
@ 2021-04-22 20:07 ` Stefan Wahren
2021-04-23 9:30 ` nicolas saenz julienne
2021-04-22 20:07 ` [PATCH 07/10] staging: vchiq_arm: drop enum vchiq_status from vchiq_*_internal Stefan Wahren
` (3 subsequent siblings)
9 siblings, 1 reply; 25+ messages in thread
From: Stefan Wahren @ 2021-04-22 20:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, Nicolas Saenz Julienne; +Cc: linux-staging, Stefan Wahren
Replace the custom set of return values with proper Linux error codes.
As a result we need to initialize vchiq_status in vchiq_init_state.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 12 ++++++------
.../staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 11 +++++------
.../staging/vc04_services/interface/vchiq_arm/vchiq_core.h | 4 ++--
3 files changed, 13 insertions(+), 14 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 7c336d0..a644fe6 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
@@ -169,21 +169,21 @@ int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
return 0;
}
-enum vchiq_status
+int
vchiq_platform_init_state(struct vchiq_state *state)
{
struct vchiq_2835_state *platform_state;
state->platform_state = kzalloc(sizeof(*platform_state), GFP_KERNEL);
if (!state->platform_state)
- return VCHIQ_ERROR;
+ return -ENOMEM;
platform_state = (struct vchiq_2835_state *)state->platform_state;
platform_state->inited = 1;
vchiq_arm_init_state(state, &platform_state->arm_state);
- return VCHIQ_SUCCESS;
+ return 0;
}
struct vchiq_arm_state*
@@ -211,7 +211,7 @@ remote_event_signal(struct remote_event *event)
writel(0, g_regs + BELL2); /* trigger vc interrupt */
}
-enum vchiq_status
+int
vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset,
void __user *uoffset, int size, int dir)
{
@@ -223,7 +223,7 @@ vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset,
: PAGELIST_WRITE);
if (!pagelistinfo)
- return VCHIQ_ERROR;
+ return -ENOMEM;
bulk->data = pagelistinfo->dma_addr;
@@ -233,7 +233,7 @@ vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset,
*/
bulk->remote_data = pagelistinfo;
- return VCHIQ_SUCCESS;
+ return 0;
}
void
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 9dc9a6a..9f9677a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2162,9 +2162,9 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
{
struct vchiq_shared_state *local;
struct vchiq_shared_state *remote;
- enum vchiq_status status;
+ enum vchiq_status status = VCHIQ_SUCCESS;
char threadname[16];
- int i;
+ int i, ret;
if (vchiq_states[0]) {
pr_err("%s: VCHIQ state already initialized\n", __func__);
@@ -2246,8 +2246,8 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
local->debug[DEBUG_ENTRIES] = DEBUG_MAX;
- status = vchiq_platform_init_state(state);
- if (status != VCHIQ_SUCCESS)
+ ret = vchiq_platform_init_state(state);
+ if (ret)
return VCHIQ_ERROR;
/*
@@ -3141,8 +3141,7 @@ enum vchiq_status vchiq_bulk_transfer(unsigned int handle,
bulk->size = size;
bulk->actual = VCHIQ_BULK_ACTUAL_ABORTED;
- if (vchiq_prepare_bulk_data(bulk, offset, uoffset, size, dir)
- != VCHIQ_SUCCESS)
+ if (vchiq_prepare_bulk_data(bulk, offset, uoffset, size, dir))
goto unlock_error_exit;
wmb();
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 b817097..6b3a907 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -644,7 +644,7 @@ vchiq_queue_message(unsigned int handle,
* implementations must be provided.
*/
-extern enum vchiq_status
+extern int
vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset,
void __user *uoffset, int size, int dir);
@@ -679,7 +679,7 @@ vchiq_on_remote_use(struct vchiq_state *state);
extern void
vchiq_on_remote_release(struct vchiq_state *state);
-extern enum vchiq_status
+extern int
vchiq_platform_init_state(struct vchiq_state *state);
extern enum vchiq_status
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 07/10] staging: vchiq_arm: drop enum vchiq_status from vchiq_*_internal
2021-04-22 20:07 [PATCH 00/10] staging: vchiq_arm: address TODO list Stefan Wahren
` (5 preceding siblings ...)
2021-04-22 20:07 ` [PATCH 06/10] staging: vchiq_2835_arm: drop enum vchiq_status Stefan Wahren
@ 2021-04-22 20:07 ` Stefan Wahren
2021-04-23 9:34 ` nicolas saenz julienne
2021-04-22 20:07 ` [PATCH 08/10] staging: vchiq_core: drop vchiq_status from vchiq_set_service_option Stefan Wahren
` (2 subsequent siblings)
9 siblings, 1 reply; 25+ messages in thread
From: Stefan Wahren @ 2021-04-22 20:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, Nicolas Saenz Julienne; +Cc: linux-staging, Stefan Wahren
Replace the custom set of return values with proper Linux error codes
for the following functions:
vchiq_use_internal()
vchiq_release_internal()
vchiq_use_service_internal()
vchiq_release_service_internal()
Now we can use the result directly as return value for vchiq_ioctl().
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 31 +++++++++++-----------
.../vc04_services/interface/vchiq_arm/vchiq_core.h | 4 +--
2 files changed, 17 insertions(+), 18 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 db10d83..3395201 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1385,21 +1385,20 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
service = find_service_for_instance(instance, handle);
if (service) {
- status = (cmd == VCHIQ_IOC_USE_SERVICE) ?
+ ret = (cmd == VCHIQ_IOC_USE_SERVICE) ?
vchiq_use_service_internal(service) :
vchiq_release_service_internal(service);
- if (status != VCHIQ_SUCCESS) {
+ if (ret) {
vchiq_log_error(vchiq_susp_log_level,
- "%s: cmd %s returned error %d for service %c%c%c%c:%03d",
+ "%s: cmd %s returned error %ld for service %c%c%c%c:%03d",
__func__,
(cmd == VCHIQ_IOC_USE_SERVICE) ?
"VCHIQ_IOC_USE_SERVICE" :
"VCHIQ_IOC_RELEASE_SERVICE",
- status,
+ ret,
VCHIQ_FOURCC_AS_4CHARS(
service->base.fourcc),
service->client_id);
- ret = -EINVAL;
}
} else
ret = -EINVAL;
@@ -2330,18 +2329,18 @@ vchiq_arm_init_state(struct vchiq_state *state,
}
}
-enum vchiq_status
+int
vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
enum USE_TYPE_E use_type)
{
struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
- enum vchiq_status ret = VCHIQ_SUCCESS;
+ int ret = 0;
char entity[16];
int *entity_uc;
int local_uc;
if (!arm_state) {
- ret = VCHIQ_ERROR;
+ ret = -EINVAL;
goto out;
}
@@ -2357,7 +2356,7 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
entity_uc = &service->service_use_count;
} else {
vchiq_log_error(vchiq_susp_log_level, "%s null service ptr", __func__);
- ret = VCHIQ_ERROR;
+ ret = -EINVAL;
goto out;
}
@@ -2371,7 +2370,7 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
write_unlock_bh(&arm_state->susp_res_lock);
- if (ret == VCHIQ_SUCCESS) {
+ if (!ret) {
enum vchiq_status status = VCHIQ_SUCCESS;
long ack_cnt = atomic_xchg(&arm_state->ka_use_ack_count, 0);
@@ -2391,16 +2390,16 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
return ret;
}
-enum vchiq_status
+int
vchiq_release_internal(struct vchiq_state *state, struct vchiq_service *service)
{
struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
- enum vchiq_status ret = VCHIQ_SUCCESS;
+ int ret = 0;
char entity[16];
int *entity_uc;
if (!arm_state) {
- ret = VCHIQ_ERROR;
+ ret = -EINVAL;
goto out;
}
@@ -2421,7 +2420,7 @@ vchiq_release_internal(struct vchiq_state *state, struct vchiq_service *service)
/* Don't use BUG_ON - don't allow user thread to crash kernel */
WARN_ON(!arm_state->videocore_use_count);
WARN_ON(!(*entity_uc));
- ret = VCHIQ_ERROR;
+ ret = -EINVAL;
goto unlock;
}
--arm_state->videocore_use_count;
@@ -2460,13 +2459,13 @@ vchiq_on_remote_release(struct vchiq_state *state)
complete(&arm_state->ka_evt);
}
-enum vchiq_status
+int
vchiq_use_service_internal(struct vchiq_service *service)
{
return vchiq_use_internal(service->state, service, USE_TYPE_SERVICE);
}
-enum vchiq_status
+int
vchiq_release_service_internal(struct vchiq_service *service)
{
return vchiq_release_internal(service->state, service);
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 6b3a907..aad1c5c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -667,10 +667,10 @@ extern int
vchiq_dump_platform_service_state(void *dump_context,
struct vchiq_service *service);
-extern enum vchiq_status
+extern int
vchiq_use_service_internal(struct vchiq_service *service);
-extern enum vchiq_status
+extern int
vchiq_release_service_internal(struct vchiq_service *service);
extern void
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 08/10] staging: vchiq_core: drop vchiq_status from vchiq_set_service_option
2021-04-22 20:07 [PATCH 00/10] staging: vchiq_arm: address TODO list Stefan Wahren
` (6 preceding siblings ...)
2021-04-22 20:07 ` [PATCH 07/10] staging: vchiq_arm: drop enum vchiq_status from vchiq_*_internal Stefan Wahren
@ 2021-04-22 20:07 ` Stefan Wahren
2021-04-23 9:59 ` nicolas saenz julienne
2021-04-22 20:07 ` [PATCH 09/10] staging: vchiq_core: drop vchiq_status from vchiq_initialise Stefan Wahren
2021-04-22 20:07 ` [PATCH 10/10] staging: vchiq_core: drop vchiq_status from vchiq_init_state Stefan Wahren
9 siblings, 1 reply; 25+ messages in thread
From: Stefan Wahren @ 2021-04-22 20:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, Nicolas Saenz Julienne; +Cc: linux-staging, Stefan Wahren
Replace the custom set of return values with proper Linux error codes for
vchiq_set_service_option(). Now we can use the result directly as return
value for vchiq_ioctl().
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
.../vc04_services/interface/vchiq_arm/vchiq_core.c | 18 +++++++++---------
.../vc04_services/interface/vchiq_arm/vchiq_core.h | 2 +-
3 files changed, 12 insertions(+), 12 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 3395201..ab2ce07 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1517,8 +1517,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
break;
}
- status = vchiq_set_service_option(
- args.handle, args.option, args.value);
+ ret = vchiq_set_service_option(args.handle, args.option,
+ args.value);
} break;
case VCHIQ_IOC_LIB_VERSION: {
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 9f9677a..31c1c1a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3371,21 +3371,21 @@ void vchiq_get_config(struct vchiq_config *config)
config->version_min = VCHIQ_VERSION_MIN;
}
-enum vchiq_status
+int
vchiq_set_service_option(unsigned int handle,
enum vchiq_service_option option, int value)
{
struct vchiq_service *service = find_service_by_handle(handle);
- enum vchiq_status status = VCHIQ_ERROR;
struct vchiq_service_quota *quota;
+ int ret = -EIO;
if (!service)
- return VCHIQ_ERROR;
+ return -EINVAL;
switch (option) {
case VCHIQ_SERVICE_OPTION_AUTOCLOSE:
service->auto_close = value;
- status = VCHIQ_SUCCESS;
+ ret = 0;
break;
case VCHIQ_SERVICE_OPTION_SLOT_QUOTA:
@@ -3402,7 +3402,7 @@ vchiq_set_service_option(unsigned int handle,
* dropped below its quota
*/
complete("a->quota_event);
- status = VCHIQ_SUCCESS;
+ ret = 0;
}
break;
@@ -3420,7 +3420,7 @@ vchiq_set_service_option(unsigned int handle,
* dropped below its quota
*/
complete("a->quota_event);
- status = VCHIQ_SUCCESS;
+ ret = 0;
}
break;
@@ -3428,13 +3428,13 @@ vchiq_set_service_option(unsigned int handle,
if ((service->srvstate == VCHIQ_SRVSTATE_HIDDEN) ||
(service->srvstate == VCHIQ_SRVSTATE_LISTENING)) {
service->sync = value;
- status = VCHIQ_SUCCESS;
+ ret = 0;
}
break;
case VCHIQ_SERVICE_OPTION_TRACE:
service->trace = value;
- status = VCHIQ_SUCCESS;
+ ret = 0;
break;
default:
@@ -3442,7 +3442,7 @@ vchiq_set_service_option(unsigned int handle,
}
unlock_service(service);
- return status;
+ return ret;
}
static int
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 aad1c5c..bd1f590 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -712,7 +712,7 @@ extern int vchiq_get_client_id(unsigned int service);
extern void vchiq_get_config(struct vchiq_config *config);
-extern enum vchiq_status
+extern int
vchiq_set_service_option(unsigned int service, enum vchiq_service_option option,
int value);
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 09/10] staging: vchiq_core: drop vchiq_status from vchiq_initialise
2021-04-22 20:07 [PATCH 00/10] staging: vchiq_arm: address TODO list Stefan Wahren
` (7 preceding siblings ...)
2021-04-22 20:07 ` [PATCH 08/10] staging: vchiq_core: drop vchiq_status from vchiq_set_service_option Stefan Wahren
@ 2021-04-22 20:07 ` Stefan Wahren
2021-04-23 10:02 ` nicolas saenz julienne
2021-04-22 20:07 ` [PATCH 10/10] staging: vchiq_core: drop vchiq_status from vchiq_init_state Stefan Wahren
9 siblings, 1 reply; 25+ messages in thread
From: Stefan Wahren @ 2021-04-22 20:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, Nicolas Saenz Julienne; +Cc: linux-staging, Stefan Wahren
Replace the custom set of return values with proper Linux error codes for
vchiq_initialise().
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 20 +++++++++++---------
1 file changed, 11 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 ab2ce07..04a72d4 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -147,12 +147,11 @@ vchiq_blocking_bulk_transfer(unsigned int handle, void *data,
unsigned int size, enum vchiq_bulk_dir dir);
#define VCHIQ_INIT_RETRIES 10
-enum vchiq_status vchiq_initialise(struct vchiq_instance **instance_out)
+int vchiq_initialise(struct vchiq_instance **instance_out)
{
- enum vchiq_status status = VCHIQ_ERROR;
struct vchiq_state *state;
struct vchiq_instance *instance = NULL;
- int i;
+ int i, ret;
vchiq_log_trace(vchiq_core_log_level, "%s called", __func__);
@@ -170,6 +169,7 @@ enum vchiq_status vchiq_initialise(struct vchiq_instance **instance_out)
if (i == VCHIQ_INIT_RETRIES) {
vchiq_log_error(vchiq_core_log_level,
"%s: videocore not initialized\n", __func__);
+ ret = -ENOTCONN;
goto failed;
} else if (i > 0) {
vchiq_log_warning(vchiq_core_log_level,
@@ -181,6 +181,7 @@ enum vchiq_status vchiq_initialise(struct vchiq_instance **instance_out)
if (!instance) {
vchiq_log_error(vchiq_core_log_level,
"%s: error allocating vchiq instance\n", __func__);
+ ret = -ENOMEM;
goto failed;
}
@@ -191,13 +192,13 @@ enum vchiq_status vchiq_initialise(struct vchiq_instance **instance_out)
*instance_out = instance;
- status = VCHIQ_SUCCESS;
+ ret = 0;
failed:
vchiq_log_trace(vchiq_core_log_level,
- "%s(%p): returning %d", __func__, instance, status);
+ "%s(%p): returning %d", __func__, instance, ret);
- return status;
+ return ret;
}
EXPORT_SYMBOL(vchiq_initialise);
@@ -2236,6 +2237,7 @@ vchiq_keepalive_thread_func(void *v)
enum vchiq_status status;
struct vchiq_instance *instance;
unsigned int ka_handle;
+ int ret;
struct vchiq_service_params_kernel params = {
.fourcc = VCHIQ_MAKE_FOURCC('K', 'E', 'E', 'P'),
@@ -2244,10 +2246,10 @@ vchiq_keepalive_thread_func(void *v)
.version_min = KEEPALIVE_VER_MIN
};
- status = vchiq_initialise(&instance);
- if (status != VCHIQ_SUCCESS) {
+ ret = vchiq_initialise(&instance);
+ if (ret) {
vchiq_log_error(vchiq_susp_log_level,
- "%s vchiq_initialise failed %d", __func__, status);
+ "%s vchiq_initialise failed %d", __func__, ret);
goto exit;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 10/10] staging: vchiq_core: drop vchiq_status from vchiq_init_state
2021-04-22 20:07 [PATCH 00/10] staging: vchiq_arm: address TODO list Stefan Wahren
` (8 preceding siblings ...)
2021-04-22 20:07 ` [PATCH 09/10] staging: vchiq_core: drop vchiq_status from vchiq_initialise Stefan Wahren
@ 2021-04-22 20:07 ` Stefan Wahren
2021-04-23 10:04 ` nicolas saenz julienne
9 siblings, 1 reply; 25+ messages in thread
From: Stefan Wahren @ 2021-04-22 20:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, Nicolas Saenz Julienne; +Cc: linux-staging, Stefan Wahren
Replace the custom set of return values with proper Linux error codes for
vchiq_init_state().
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 5 +++--
.../vc04_services/interface/vchiq_arm/vchiq_core.c | 15 ++++++++-------
.../vc04_services/interface/vchiq_arm/vchiq_core.h | 2 +-
3 files changed, 12 insertions(+), 10 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 a644fe6..c3fbb29 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
@@ -132,8 +132,9 @@ int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
*(char **)&g_fragments_base[i * g_fragments_size] = NULL;
sema_init(&g_free_fragments_sema, MAX_FRAGMENTS);
- if (vchiq_init_state(state, vchiq_slot_zero) != VCHIQ_SUCCESS)
- return -EINVAL;
+ err = vchiq_init_state(state, vchiq_slot_zero);
+ if (err)
+ return err;
g_regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(g_regs))
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 31c1c1a..4a4ce3c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2162,13 +2162,12 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
{
struct vchiq_shared_state *local;
struct vchiq_shared_state *remote;
- enum vchiq_status status = VCHIQ_SUCCESS;
char threadname[16];
int i, ret;
if (vchiq_states[0]) {
pr_err("%s: VCHIQ state already initialized\n", __func__);
- return VCHIQ_ERROR;
+ return -EINVAL;
}
local = &slot_zero->slave;
@@ -2181,7 +2180,7 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
else
vchiq_loud_error("master/slave mismatch two slaves");
vchiq_loud_error_footer();
- return VCHIQ_ERROR;
+ return -EINVAL;
}
memset(state, 0, sizeof(struct vchiq_state));
@@ -2248,7 +2247,7 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
ret = vchiq_platform_init_state(state);
if (ret)
- return VCHIQ_ERROR;
+ return ret;
/*
* bring up slot handler thread
@@ -2262,7 +2261,7 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
vchiq_loud_error_header();
vchiq_loud_error("couldn't create thread %s", threadname);
vchiq_loud_error_footer();
- return VCHIQ_ERROR;
+ return PTR_ERR(state->slot_handler_thread);
}
set_user_nice(state->slot_handler_thread, -19);
@@ -2274,6 +2273,7 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
vchiq_loud_error_header();
vchiq_loud_error("couldn't create thread %s", threadname);
vchiq_loud_error_footer();
+ ret = PTR_ERR(state->recycle_thread);
goto fail_free_handler_thread;
}
set_user_nice(state->recycle_thread, -19);
@@ -2286,6 +2286,7 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
vchiq_loud_error_header();
vchiq_loud_error("couldn't create thread %s", threadname);
vchiq_loud_error_footer();
+ ret = PTR_ERR(state->sync_thread);
goto fail_free_recycle_thread;
}
set_user_nice(state->sync_thread, -20);
@@ -2299,14 +2300,14 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
/* Indicate readiness to the other side */
local->initialised = 1;
- return status;
+ return 0;
fail_free_recycle_thread:
kthread_stop(state->recycle_thread);
fail_free_handler_thread:
kthread_stop(state->slot_handler_thread);
- return VCHIQ_ERROR;
+ return ret;
}
void vchiq_msg_queue_push(unsigned int handle, struct vchiq_header *header)
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 bd1f590..89f898ce 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -539,7 +539,7 @@ get_conn_state_name(enum vchiq_connstate conn_state);
extern struct vchiq_slot_zero *
vchiq_init_slots(void *mem_base, int mem_size);
-extern enum vchiq_status
+extern int
vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero);
extern enum vchiq_status
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 03/10] staging: vchiq_core: introduce get_bulk_reason
2021-04-22 20:07 ` [PATCH 03/10] staging: vchiq_core: introduce get_bulk_reason Stefan Wahren
@ 2021-04-23 7:23 ` Fabio Aiuto
2021-04-23 10:54 ` Stefan Wahren
2021-04-23 9:17 ` nicolassaenzj
1 sibling, 1 reply; 25+ messages in thread
From: Fabio Aiuto @ 2021-04-23 7:23 UTC (permalink / raw)
To: Stefan Wahren; +Cc: Greg Kroah-Hartman, Nicolas Saenz Julienne, linux-staging
On Thu, Apr 22, 2021 at 10:07:49PM +0200, Stefan Wahren wrote:
> Nesting multiple ternary operators over multiple lines isn't easy to
> read. Move this logic into a separate inline function.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
> .../vc04_services/interface/vchiq_arm/vchiq_core.c | 35 ++++++++++++++--------
> 1 file changed, 22 insertions(+), 13 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 8a1e6f5..3a72c36 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -1226,6 +1226,22 @@ release_slot(struct vchiq_state *state, struct vchiq_slot_info *slot_info,
> mutex_unlock(&state->recycle_mutex);
> }
>
> +static inline enum vchiq_reason
> +get_bulk_reason(struct vchiq_bulk *bulk)
> +{
> + if (bulk->dir == VCHIQ_BULK_TRANSMIT) {
> + if (bulk->actual == VCHIQ_BULK_ACTUAL_ABORTED)
> + return VCHIQ_BULK_TRANSMIT_ABORTED;
> +
> + return VCHIQ_BULK_TRANSMIT_DONE;
> + }
> +
> + if (bulk->actual == VCHIQ_BULK_ACTUAL_ABORTED)
> + return VCHIQ_BULK_RECEIVE_ABORTED;
> +
> + return VCHIQ_BULK_RECEIVE_DONE;
> +}
> +
> /* Called by the slot handler - don't hold the bulk mutex */
> static enum vchiq_status
> notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
> @@ -1281,16 +1297,8 @@ notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
> spin_unlock(&bulk_waiter_spinlock);
> } else if (bulk->mode ==
> VCHIQ_BULK_MODE_CALLBACK) {
> - enum vchiq_reason reason = (bulk->dir ==
> - VCHIQ_BULK_TRANSMIT) ?
> - ((bulk->actual ==
> - VCHIQ_BULK_ACTUAL_ABORTED) ?
> - VCHIQ_BULK_TRANSMIT_ABORTED :
> - VCHIQ_BULK_TRANSMIT_DONE) :
> - ((bulk->actual ==
> - VCHIQ_BULK_ACTUAL_ABORTED) ?
> - VCHIQ_BULK_RECEIVE_ABORTED :
> - VCHIQ_BULK_RECEIVE_DONE);
> + enum vchiq_reason reason =
> + get_bulk_reason(bulk);
> status = make_service_callback(service,
> reason, NULL, bulk->userdata);
> if (status == VCHIQ_RETRY)
> @@ -2622,9 +2630,10 @@ do_abort_bulks(struct vchiq_service *service)
> mutex_unlock(&service->bulk_mutex);
>
> status = notify_bulks(service, &service->bulk_tx, 0/*!retry_poll*/);
> - if (status == VCHIQ_SUCCESS)
> - status = notify_bulks(service, &service->bulk_rx,
> - 0/*!retry_poll*/);
> + if (status != VCHIQ_SUCCESS)
> + return 0;
> +
> + status = notify_bulks(service, &service->bulk_rx, 0/*!retry_poll*/);
> return (status == VCHIQ_SUCCESS);
> }
Hi Stefan,
this change seem to be unrelated ti what's pointed out in changelog.
Should it be put in a separate patch?
>
> --
> 2.7.4
>
>
thank you,
fabio
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 02/10] staging: vchiq_core: break early in vchiq_close_service_internal
2021-04-22 20:07 ` [PATCH 02/10] staging: vchiq_core: break early in vchiq_close_service_internal Stefan Wahren
@ 2021-04-23 9:10 ` nicolas saenz julienne
0 siblings, 0 replies; 25+ messages in thread
From: nicolas saenz julienne @ 2021-04-23 9:10 UTC (permalink / raw)
To: Stefan Wahren, Greg Kroah-Hartman; +Cc: linux-staging
[-- Attachment #1: Type: text/plain, Size: 456 bytes --]
On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> The logic in VCHIQ_SRVSTATE_OPEN* is unnecessary complex. Handle the error
> case of queue_message() first makes it easier to read.
>
> Btw we get the rid of the checkpatch warning:
> WARNING: else is not generally useful after a break or return
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Thanks!
Nicolas
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 03/10] staging: vchiq_core: introduce get_bulk_reason
2021-04-22 20:07 ` [PATCH 03/10] staging: vchiq_core: introduce get_bulk_reason Stefan Wahren
2021-04-23 7:23 ` Fabio Aiuto
@ 2021-04-23 9:17 ` nicolassaenzj
1 sibling, 0 replies; 25+ messages in thread
From: nicolassaenzj @ 2021-04-23 9:17 UTC (permalink / raw)
To: Stefan Wahren, Greg Kroah-Hartman; +Cc: linux-staging
[-- Attachment #1: Type: text/plain, Size: 310 bytes --]
On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> Nesting multiple ternary operators over multiple lines isn't easy to
> read. Move this logic into a separate inline function.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
Looks good to me modulo Fabio's comment.
Nicolas
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 04/10] staging: vchiq_core: Drop unnecessary check in notify_bulks
2021-04-22 20:07 ` [PATCH 04/10] staging: vchiq_core: Drop unnecessary check in notify_bulks Stefan Wahren
@ 2021-04-23 9:22 ` nicolassaenzj
0 siblings, 0 replies; 25+ messages in thread
From: nicolassaenzj @ 2021-04-23 9:22 UTC (permalink / raw)
To: Stefan Wahren, Greg Kroah-Hartman; +Cc: linux-staging
[-- Attachment #1: Type: text/plain, Size: 308 bytes --]
On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> There is no modification to the vchiq_status before the first if, so drop
> this unnecessary check.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Thanks,
Nicolas
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 05/10] staging: vchiq_arm: drop return value of vchiq_arm_init_state
2021-04-22 20:07 ` [PATCH 05/10] staging: vchiq_arm: drop return value of vchiq_arm_init_state Stefan Wahren
@ 2021-04-23 9:28 ` nicolas saenz julienne
0 siblings, 0 replies; 25+ messages in thread
From: nicolas saenz julienne @ 2021-04-23 9:28 UTC (permalink / raw)
To: Stefan Wahren, Greg Kroah-Hartman; +Cc: linux-staging
On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> The function vchiq_arm_init_state() cannot fail. So drop the return
> value and the unnecessary code.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Thanks,
Nicolas
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 06/10] staging: vchiq_2835_arm: drop enum vchiq_status
2021-04-22 20:07 ` [PATCH 06/10] staging: vchiq_2835_arm: drop enum vchiq_status Stefan Wahren
@ 2021-04-23 9:30 ` nicolas saenz julienne
0 siblings, 0 replies; 25+ messages in thread
From: nicolas saenz julienne @ 2021-04-23 9:30 UTC (permalink / raw)
To: Stefan Wahren, Greg Kroah-Hartman; +Cc: linux-staging
On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> Replace the custom set of return values with proper Linux error codes.
> As a result we need to initialize vchiq_status in vchiq_init_state.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Thanks,
Nicolas
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 07/10] staging: vchiq_arm: drop enum vchiq_status from vchiq_*_internal
2021-04-22 20:07 ` [PATCH 07/10] staging: vchiq_arm: drop enum vchiq_status from vchiq_*_internal Stefan Wahren
@ 2021-04-23 9:34 ` nicolas saenz julienne
0 siblings, 0 replies; 25+ messages in thread
From: nicolas saenz julienne @ 2021-04-23 9:34 UTC (permalink / raw)
To: Stefan Wahren, Greg Kroah-Hartman; +Cc: linux-staging
On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> Replace the custom set of return values with proper Linux error codes
> for the following functions:
>
> vchiq_use_internal()
> vchiq_release_internal()
> vchiq_use_service_internal()
> vchiq_release_service_internal()
>
> Now we can use the result directly as return value for vchiq_ioctl().
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Thanks,
Nicolas
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 08/10] staging: vchiq_core: drop vchiq_status from vchiq_set_service_option
2021-04-22 20:07 ` [PATCH 08/10] staging: vchiq_core: drop vchiq_status from vchiq_set_service_option Stefan Wahren
@ 2021-04-23 9:59 ` nicolas saenz julienne
2021-04-23 11:03 ` Stefan Wahren
0 siblings, 1 reply; 25+ messages in thread
From: nicolas saenz julienne @ 2021-04-23 9:59 UTC (permalink / raw)
To: Stefan Wahren, Greg Kroah-Hartman; +Cc: linux-staging
On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> Replace the custom set of return values with proper Linux error codes for
> vchiq_set_service_option(). Now we can use the result directly as return
> value for vchiq_ioctl().
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
> .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
> .../vc04_services/interface/vchiq_arm/vchiq_core.c | 18 +++++++++---------
> .../vc04_services/interface/vchiq_arm/vchiq_core.h | 2 +-
> 3 files changed, 12 insertions(+), 12 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 3395201..ab2ce07 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1517,8 +1517,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> break;
> }
>
>
> - status = vchiq_set_service_option(
> - args.handle, args.option, args.value);
> + ret = vchiq_set_service_option(args.handle, args.option,
> + args.value);
> } break;
>
>
> case VCHIQ_IOC_LIB_VERSION: {
> 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 9f9677a..31c1c1a 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -3371,21 +3371,21 @@ void vchiq_get_config(struct vchiq_config *config)
> config->version_min = VCHIQ_VERSION_MIN;
> }
>
>
> -enum vchiq_status
> +int
> vchiq_set_service_option(unsigned int handle,
> enum vchiq_service_option option, int value)
> {
> struct vchiq_service *service = find_service_by_handle(handle);
> - enum vchiq_status status = VCHIQ_ERROR;
> struct vchiq_service_quota *quota;
> + int ret = -EIO;
Due to the nature of the driver it's hard to make a clear distinction, but is
-EIO really the best error for the function? I don't see error paths that
really depend on communication with VC4. I see it more aligned with -EINVAL.
Regards,
Nicolas
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 09/10] staging: vchiq_core: drop vchiq_status from vchiq_initialise
2021-04-22 20:07 ` [PATCH 09/10] staging: vchiq_core: drop vchiq_status from vchiq_initialise Stefan Wahren
@ 2021-04-23 10:02 ` nicolas saenz julienne
0 siblings, 0 replies; 25+ messages in thread
From: nicolas saenz julienne @ 2021-04-23 10:02 UTC (permalink / raw)
To: Stefan Wahren, Greg Kroah-Hartman; +Cc: linux-staging
On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> Replace the custom set of return values with proper Linux error codes for
> vchiq_initialise().
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Thanks,
Nicolas
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 10/10] staging: vchiq_core: drop vchiq_status from vchiq_init_state
2021-04-22 20:07 ` [PATCH 10/10] staging: vchiq_core: drop vchiq_status from vchiq_init_state Stefan Wahren
@ 2021-04-23 10:04 ` nicolas saenz julienne
0 siblings, 0 replies; 25+ messages in thread
From: nicolas saenz julienne @ 2021-04-23 10:04 UTC (permalink / raw)
To: Stefan Wahren, Greg Kroah-Hartman; +Cc: linux-staging
On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> Replace the custom set of return values with proper Linux error codes for
> vchiq_init_state().
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Thanks,
Nicolas
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 03/10] staging: vchiq_core: introduce get_bulk_reason
2021-04-23 7:23 ` Fabio Aiuto
@ 2021-04-23 10:54 ` Stefan Wahren
0 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2021-04-23 10:54 UTC (permalink / raw)
To: Fabio Aiuto; +Cc: Greg Kroah-Hartman, Nicolas Saenz Julienne, linux-staging
Am 23.04.21 um 09:23 schrieb Fabio Aiuto:
> On Thu, Apr 22, 2021 at 10:07:49PM +0200, Stefan Wahren wrote:
>> Nesting multiple ternary operators over multiple lines isn't easy to
>> read. Move this logic into a separate inline function.
>>
>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>> ---
>> .../vc04_services/interface/vchiq_arm/vchiq_core.c | 35 ++++++++++++++--------
>> 1 file changed, 22 insertions(+), 13 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 8a1e6f5..3a72c36 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> @@ -2622,9 +2630,10 @@ do_abort_bulks(struct vchiq_service *service)
>> mutex_unlock(&service->bulk_mutex);
>>
>> status = notify_bulks(service, &service->bulk_tx, 0/*!retry_poll*/);
>> - if (status == VCHIQ_SUCCESS)
>> - status = notify_bulks(service, &service->bulk_rx,
>> - 0/*!retry_poll*/);
>> + if (status != VCHIQ_SUCCESS)
>> + return 0;
>> +
>> + status = notify_bulks(service, &service->bulk_rx, 0/*!retry_poll*/);
>> return (status == VCHIQ_SUCCESS);
>> }
> Hi Stefan,
>
> this change seem to be unrelated ti what's pointed out in changelog.
> Should it be put in a separate patch?
Oops, yes this should be a separate patch.
>
>>
>> --
>> 2.7.4
>>
>>
> thank you,
>
> fabio
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 08/10] staging: vchiq_core: drop vchiq_status from vchiq_set_service_option
2021-04-23 9:59 ` nicolas saenz julienne
@ 2021-04-23 11:03 ` Stefan Wahren
2021-04-24 9:34 ` nicolas saenz julienne
0 siblings, 1 reply; 25+ messages in thread
From: Stefan Wahren @ 2021-04-23 11:03 UTC (permalink / raw)
To: nicolas saenz julienne, Greg Kroah-Hartman; +Cc: linux-staging
Am 23.04.21 um 11:59 schrieb nicolas saenz julienne:
> On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
>> Replace the custom set of return values with proper Linux error codes for
>> vchiq_set_service_option(). Now we can use the result directly as return
>> value for vchiq_ioctl().
>>
>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>> ---
>> .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
>> .../vc04_services/interface/vchiq_arm/vchiq_core.c | 18 +++++++++---------
>> .../vc04_services/interface/vchiq_arm/vchiq_core.h | 2 +-
>> 3 files changed, 12 insertions(+), 12 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 3395201..ab2ce07 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -1517,8 +1517,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>> break;
>> }
>>
>>
>> - status = vchiq_set_service_option(
>> - args.handle, args.option, args.value);
>> + ret = vchiq_set_service_option(args.handle, args.option,
>> + args.value);
>> } break;
>>
>>
>> case VCHIQ_IOC_LIB_VERSION: {
>> 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 9f9677a..31c1c1a 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> @@ -3371,21 +3371,21 @@ void vchiq_get_config(struct vchiq_config *config)
>> config->version_min = VCHIQ_VERSION_MIN;
>> }
>>
>>
>> -enum vchiq_status
>> +int
>> vchiq_set_service_option(unsigned int handle,
>> enum vchiq_service_option option, int value)
>> {
>> struct vchiq_service *service = find_service_by_handle(handle);
>> - enum vchiq_status status = VCHIQ_ERROR;
>> struct vchiq_service_quota *quota;
>> + int ret = -EIO;
> Due to the nature of the driver it's hard to make a clear distinction, but is
> -EIO really the best error for the function? I don't see error paths that
> really depend on communication with VC4. I see it more aligned with -EINVAL.
In general i totally agree with you. I just wanted to the keep the
o(l)dd behavior, because vchiq_ioctl() has a catch all for VCHIQ_ERROR
-> -EIO
I'm not sure if any user application relies on the error code of
vchiq_ioctl(). Otherwise this isn't argument for all the other patches ;-)
I will change it to -EINVAL.
>
> Regards,
> Nicolas
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 08/10] staging: vchiq_core: drop vchiq_status from vchiq_set_service_option
2021-04-23 11:03 ` Stefan Wahren
@ 2021-04-24 9:34 ` nicolas saenz julienne
0 siblings, 0 replies; 25+ messages in thread
From: nicolas saenz julienne @ 2021-04-24 9:34 UTC (permalink / raw)
To: Stefan Wahren, Greg Kroah-Hartman; +Cc: linux-staging
On Fri, 2021-04-23 at 13:03 +0200, Stefan Wahren wrote:
> Am 23.04.21 um 11:59 schrieb nicolas saenz julienne:
> > On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> > > Replace the custom set of return values with proper Linux error codes for
> > > vchiq_set_service_option(). Now we can use the result directly as return
> > > value for vchiq_ioctl().
> > >
> > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > > ---
> > > .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
> > > .../vc04_services/interface/vchiq_arm/vchiq_core.c | 18 +++++++++---------
> > > .../vc04_services/interface/vchiq_arm/vchiq_core.h | 2 +-
> > > 3 files changed, 12 insertions(+), 12 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 3395201..ab2ce07 100644
> > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > @@ -1517,8 +1517,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > > break;
> > > }
> > >
> > >
> > > - status = vchiq_set_service_option(
> > > - args.handle, args.option, args.value);
> > > + ret = vchiq_set_service_option(args.handle, args.option,
> > > + args.value);
> > > } break;
> > >
> > >
> > > case VCHIQ_IOC_LIB_VERSION: {
> > > 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 9f9677a..31c1c1a 100644
> > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> > > @@ -3371,21 +3371,21 @@ void vchiq_get_config(struct vchiq_config *config)
> > > config->version_min = VCHIQ_VERSION_MIN;
> > > }
> > >
> > >
> > > -enum vchiq_status
> > > +int
> > > vchiq_set_service_option(unsigned int handle,
> > > enum vchiq_service_option option, int value)
> > > {
> > > struct vchiq_service *service = find_service_by_handle(handle);
> > > - enum vchiq_status status = VCHIQ_ERROR;
> > > struct vchiq_service_quota *quota;
> > > + int ret = -EIO;
> > Due to the nature of the driver it's hard to make a clear distinction, but is
> > -EIO really the best error for the function? I don't see error paths that
> > really depend on communication with VC4. I see it more aligned with -EINVAL.
>
> In general i totally agree with you. I just wanted to the keep the
> o(l)dd behavior, because vchiq_ioctl() has a catch all for VCHIQ_ERROR
> -> -EIO
>
> I'm not sure if any user application relies on the error code of
> vchiq_ioctl(). Otherwise this isn't argument for all the other patches ;-)
This is something that bugged me the last time I tried to do a vchiq cleanup.
At some point we'll start affecting the IOCTL interface in subtle ways. It's a
PITA as we don't really know if there are users out there that wrote their own
VCHIQ applications. RPi ones we could fix.
I remember thinking that the best course of action would be to completely
decouple the core part from the char device. As in, for it to live under a
completely different directory, and registered the way we do it with the camera
interface. We'd get a way more concise VCHIQ core. But IIRC there are lots of
quirks and hacks to accomodate to the IOCTl behavior we'd have to iron out.
Regards,
Nicolas
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 01/10] staging: vchiq_arm: avoid crashing the kernel
2021-04-22 20:07 ` [PATCH 01/10] staging: vchiq_arm: avoid crashing the kernel Stefan Wahren
@ 2021-04-24 9:49 ` nicolas saenz julienne
0 siblings, 0 replies; 25+ messages in thread
From: nicolas saenz julienne @ 2021-04-24 9:49 UTC (permalink / raw)
To: Stefan Wahren, Greg Kroah-Hartman; +Cc: linux-staging
On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> Using BUG_ON in a non-essential driver isn't recommend. So better
> trigger a stacktrace and bailout.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Thanks,
Nicolas
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2021-04-24 9:49 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-22 20:07 [PATCH 00/10] staging: vchiq_arm: address TODO list Stefan Wahren
2021-04-22 20:07 ` [PATCH 01/10] staging: vchiq_arm: avoid crashing the kernel Stefan Wahren
2021-04-24 9:49 ` nicolas saenz julienne
2021-04-22 20:07 ` [PATCH 02/10] staging: vchiq_core: break early in vchiq_close_service_internal Stefan Wahren
2021-04-23 9:10 ` nicolas saenz julienne
2021-04-22 20:07 ` [PATCH 03/10] staging: vchiq_core: introduce get_bulk_reason Stefan Wahren
2021-04-23 7:23 ` Fabio Aiuto
2021-04-23 10:54 ` Stefan Wahren
2021-04-23 9:17 ` nicolassaenzj
2021-04-22 20:07 ` [PATCH 04/10] staging: vchiq_core: Drop unnecessary check in notify_bulks Stefan Wahren
2021-04-23 9:22 ` nicolassaenzj
2021-04-22 20:07 ` [PATCH 05/10] staging: vchiq_arm: drop return value of vchiq_arm_init_state Stefan Wahren
2021-04-23 9:28 ` nicolas saenz julienne
2021-04-22 20:07 ` [PATCH 06/10] staging: vchiq_2835_arm: drop enum vchiq_status Stefan Wahren
2021-04-23 9:30 ` nicolas saenz julienne
2021-04-22 20:07 ` [PATCH 07/10] staging: vchiq_arm: drop enum vchiq_status from vchiq_*_internal Stefan Wahren
2021-04-23 9:34 ` nicolas saenz julienne
2021-04-22 20:07 ` [PATCH 08/10] staging: vchiq_core: drop vchiq_status from vchiq_set_service_option Stefan Wahren
2021-04-23 9:59 ` nicolas saenz julienne
2021-04-23 11:03 ` Stefan Wahren
2021-04-24 9:34 ` nicolas saenz julienne
2021-04-22 20:07 ` [PATCH 09/10] staging: vchiq_core: drop vchiq_status from vchiq_initialise Stefan Wahren
2021-04-23 10:02 ` nicolas saenz julienne
2021-04-22 20:07 ` [PATCH 10/10] staging: vchiq_core: drop vchiq_status from vchiq_init_state Stefan Wahren
2021-04-23 10:04 ` nicolas saenz julienne
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).