[PATCH 00/11] staging: vc04_services: Random cleanups

[PATCH 00/11] staging: vc04_services: Random cleanups

[Stefan Wahren]

This patch series contains a wild mix of cleanups. Most of them
remove unused parts. But there are also improvements to better
understand the code. Finally the testing instructions are
updated.

The series has been tested with a Raspberry Pi 3 B Plus.

Phil Elwell (1):
  staging: vchiq_core: Add comments to mutex/spinlocks

Stefan Wahren (10):
  staging: vchiq_arm: Replace variable ret with status
  staging: vchiq_arm: Drop obsolete comment
  staging: vchiq_core: Drop non-functional struct members
  staging: vchiq_arm: Drop unnecessary declarations
  staging: vchiq_arm: Get the rid off struct vchiq_2835_state
  staging: vchiq_arm: Drop vchiq_arm_init_state
  staging: vchiq_arm: Reduce indentation of service_callback
  staging: vchiq: Move struct vchiq_config to vchiq.h
  staging: vchiq_core: Add hex prefix to debugfs output
  staging: vc04_services: Update testing instructions

 .../include/linux/raspberrypi/vchiq.h         |  12 ++
 .../staging/vc04_services/interface/TESTING   |  45 ++++-
 .../interface/vchiq_arm/vchiq_arm.c           | 168 ++++++++----------
 .../interface/vchiq_arm/vchiq_arm.h           |   3 -
 .../interface/vchiq_arm/vchiq_core.c          |  10 +-
 .../interface/vchiq_arm/vchiq_core.h          |  28 +--
 6 files changed, 143 insertions(+), 123 deletions(-)

--
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 01/11] staging: vchiq_arm: Replace variable ret with status

[Stefan Wahren]

The variable ret is just storing the result of last function
call like the variable status. So replace ret with status
and make a consistent use of this variable.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++----
 1 file changed, 4 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 69daeba974f2..e2f4aa00cb70 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1298,7 +1298,6 @@ vchiq_keepalive_thread_func(void *v)
 	int 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'),
@@ -1307,9 +1306,10 @@ vchiq_keepalive_thread_func(void *v)
 		.version_min = KEEPALIVE_VER_MIN
 	};

-	ret = vchiq_initialise(state, &instance);
-	if (ret) {
-		dev_err(state->dev, "suspend: %s: vchiq_initialise failed %d\n", __func__, ret);
+	status = vchiq_initialise(state, &instance);
+	if (status) {
+		dev_err(state->dev, "suspend: %s: vchiq_initialise failed %d\n",
+			__func__, status);
 		goto exit;
 	}

--
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 02/11] staging: vchiq_arm: Drop obsolete comment

[Stefan Wahren]

The BUG_ON has been replaced with WARN_ON. So we can drop the
comment.

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

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index e2f4aa00cb70..515cdcba043d 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1451,7 +1451,6 @@ vchiq_release_internal(struct vchiq_state *state, struct vchiq_service *service)

 	write_lock_bh(&arm_state->susp_res_lock);
 	if (!arm_state->videocore_use_count || !(*entity_uc)) {
-		/* 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 = -EINVAL;
--
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 03/11] staging: vchiq_core: Drop non-functional struct members

[Stefan Wahren]

There are some struct members, which don't have a real
function. So it's safe to drop them.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c    | 2 --
 .../staging/vc04_services/interface/vchiq_arm/vchiq_core.h    | 4 ----
 2 files changed, 6 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 df3af821f218..51cfc366519b 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2163,14 +2163,12 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero, s
 	mutex_init(&state->slot_mutex);
 	mutex_init(&state->recycle_mutex);
 	mutex_init(&state->sync_mutex);
-	mutex_init(&state->bulk_transfer_mutex);

 	spin_lock_init(&state->msg_queue_spinlock);
 	spin_lock_init(&state->bulk_waiter_spinlock);
 	spin_lock_init(&state->quota_spinlock);

 	init_completion(&state->slot_available_event);
-	init_completion(&state->slot_remove_event);
 	init_completion(&state->data_quota_event);

 	state->slot_queue_available = 0;
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 8af209e34fb2..be20abcfad75 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -347,8 +347,6 @@ struct vchiq_state {

 	struct mutex sync_mutex;

-	struct mutex bulk_transfer_mutex;
-
 	spinlock_t msg_queue_spinlock;

 	spinlock_t bulk_waiter_spinlock;
@@ -393,8 +391,6 @@ struct vchiq_state {
 	/* Signalled when a free slot becomes available. */
 	struct completion slot_available_event;

-	struct completion slot_remove_event;
-
 	/* Signalled when a free data slot becomes available. */
 	struct completion data_quota_event;

--
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 04/11] staging: vchiq_arm: Drop unnecessary declarations

[Stefan Wahren]

These declarations are left from cleanups and not necessary
anymore. So we can drop them.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h  | 3 ---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h | 2 --
 2 files changed, 5 deletions(-)

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 fd1b9d3555ce..b402aac333d9 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -109,9 +109,6 @@ vchiq_release_service(struct vchiq_instance *instance, unsigned int handle);
 extern int
 vchiq_check_service(struct vchiq_service *service);

-extern void
-vchiq_dump_platform_use_state(struct vchiq_state *state);
-
 extern void
 vchiq_dump_service_use_state(struct vchiq_state *state);

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 be20abcfad75..3c7a6838ddba 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -539,8 +539,6 @@ int vchiq_platform_init_state(struct vchiq_state *state);

 int vchiq_check_service(struct vchiq_service *service);

-void vchiq_on_remote_use_active(struct vchiq_state *state);
-
 int vchiq_send_remote_use(struct vchiq_state *state);

 int vchiq_send_remote_use_active(struct vchiq_state *state);
--
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 05/11] staging: vchiq_arm: Get the rid off struct vchiq_2835_state

[Stefan Wahren]

The whole benefit of this encapsulating struct is questionable.
It just stores a flag to signalize the init state of vchiq_arm_state.
Beside the fact this flag is set too soon, the access to uninitialized
members should be avoided per design. So initialize vchiq_arm_state
properly before assign it directly to vchiq_state.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 25 +++++--------------
 1 file changed, 6 insertions(+), 19 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 515cdcba043d..98a0b2d52af5 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -109,11 +109,6 @@ struct vchiq_arm_state {
 	int first_connect;
 };

-struct vchiq_2835_state {
-	int inited;
-	struct vchiq_arm_state arm_state;
-};
-
 struct vchiq_pagelist_info {
 	struct pagelist *pagelist;
 	size_t pagelist_buffer_size;
@@ -613,29 +608,21 @@ vchiq_arm_init_state(struct vchiq_state *state,
 int
 vchiq_platform_init_state(struct vchiq_state *state)
 {
-	struct vchiq_2835_state *platform_state;
+	struct vchiq_arm_state *platform_state;

-	state->platform_state = kzalloc(sizeof(*platform_state), GFP_KERNEL);
-	if (!state->platform_state)
+	platform_state = kzalloc(sizeof(*platform_state), GFP_KERNEL);
+	if (!platform_state)
 		return -ENOMEM;

-	platform_state = (struct vchiq_2835_state *)state->platform_state;
-
-	platform_state->inited = 1;
-	vchiq_arm_init_state(state, &platform_state->arm_state);
+	vchiq_arm_init_state(state, platform_state);
+	state->platform_state = (struct opaque_platform_state *)platform_state;

 	return 0;
 }

 static struct vchiq_arm_state *vchiq_platform_get_arm_state(struct vchiq_state *state)
 {
-	struct vchiq_2835_state *platform_state;
-
-	platform_state   = (struct vchiq_2835_state *)state->platform_state;
-
-	WARN_ON_ONCE(!platform_state->inited);
-
-	return &platform_state->arm_state;
+	return (struct vchiq_arm_state *)state->platform_state;
 }

 void
--
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 06/11] staging: vchiq_arm: Drop vchiq_arm_init_state

[Stefan Wahren]

After removal of struct vchiq_2835_state, the init of vchiq_arm_state
can be simplified by doing it directly within vchiq_platform_init_state.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 27 +++++++------------
 1 file changed, 9 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 98a0b2d52af5..45acca670bbd 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -588,23 +588,6 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 	return 0;
 }

-static void
-vchiq_arm_init_state(struct vchiq_state *state,
-		     struct vchiq_arm_state *arm_state)
-{
-	if (arm_state) {
-		rwlock_init(&arm_state->susp_res_lock);
-
-		init_completion(&arm_state->ka_evt);
-		atomic_set(&arm_state->ka_use_count, 0);
-		atomic_set(&arm_state->ka_use_ack_count, 0);
-		atomic_set(&arm_state->ka_release_count, 0);
-
-		arm_state->state = state;
-		arm_state->first_connect = 0;
-	}
-}
-
 int
 vchiq_platform_init_state(struct vchiq_state *state)
 {
@@ -614,7 +597,15 @@ vchiq_platform_init_state(struct vchiq_state *state)
 	if (!platform_state)
 		return -ENOMEM;

-	vchiq_arm_init_state(state, platform_state);
+	rwlock_init(&platform_state->susp_res_lock);
+
+	init_completion(&platform_state->ka_evt);
+	atomic_set(&platform_state->ka_use_count, 0);
+	atomic_set(&platform_state->ka_use_ack_count, 0);
+	atomic_set(&platform_state->ka_release_count, 0);
+
+	platform_state->state = state;
+
 	state->platform_state = (struct opaque_platform_state *)platform_state;

 	return 0;
--
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 07/11] staging: vchiq_arm: Reduce indentation of service_callback

[Stefan Wahren]

The service_callback has 5 levels of indentation, which makes it
hard to read. Reduce this by using a goto for the corner cases
(no header or VCHI service).

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 111 +++++++++---------
 1 file changed, 57 insertions(+), 54 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 45acca670bbd..0055c7d7e617 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1101,71 +1101,74 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
 		user_service, service->localport, user_service->userdata,
 		reason, header, instance, bulk_userdata);

-	if (header && user_service->is_vchi) {
-		spin_lock(&service->state->msg_queue_spinlock);
-		while (user_service->msg_insert ==
-			(user_service->msg_remove + MSG_QUEUE_SIZE)) {
-			spin_unlock(&service->state->msg_queue_spinlock);
-			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
-			DEBUG_COUNT(MSG_QUEUE_FULL_COUNT);
-			dev_dbg(service->state->dev, "arm: msg queue full\n");
-			/*
-			 * If there is no MESSAGE_AVAILABLE in the completion
-			 * queue, add one
-			 */
-			if ((user_service->message_available_pos -
-				instance->completion_remove) < 0) {
-				int status;
-
-				dev_dbg(instance->state->dev,
-					"arm: Inserting extra MESSAGE_AVAILABLE\n");
-				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
-				status = add_completion(instance, reason, NULL, user_service,
-							bulk_userdata);
-				if (status) {
-					DEBUG_TRACE(SERVICE_CALLBACK_LINE);
-					vchiq_service_put(service);
-					return status;
-				}
-			}
+	if (!header || !user_service->is_vchi)
+		goto service_put;
+
+	spin_lock(&service->state->msg_queue_spinlock);
+	while (user_service->msg_insert ==
+		(user_service->msg_remove + MSG_QUEUE_SIZE)) {
+		spin_unlock(&service->state->msg_queue_spinlock);
+		DEBUG_TRACE(SERVICE_CALLBACK_LINE);
+		DEBUG_COUNT(MSG_QUEUE_FULL_COUNT);
+		dev_dbg(service->state->dev, "arm: msg queue full\n");
+		/*
+		 * If there is no MESSAGE_AVAILABLE in the completion
+		 * queue, add one
+		 */
+		if ((user_service->message_available_pos -
+			instance->completion_remove) < 0) {
+			int status;

+			dev_dbg(instance->state->dev,
+				"arm: Inserting extra MESSAGE_AVAILABLE\n");
 			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
-			if (wait_for_completion_interruptible(&user_service->remove_event)) {
-				dev_dbg(instance->state->dev, "arm: interrupted\n");
-				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
-				vchiq_service_put(service);
-				return -EAGAIN;
-			} else if (instance->closing) {
-				dev_dbg(instance->state->dev, "arm: closing\n");
+			status = add_completion(instance, reason, NULL, user_service,
+						bulk_userdata);
+			if (status) {
 				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 				vchiq_service_put(service);
-				return -EINVAL;
+				return status;
 			}
-			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
-			spin_lock(&service->state->msg_queue_spinlock);
 		}

-		user_service->msg_queue[user_service->msg_insert &
-			(MSG_QUEUE_SIZE - 1)] = header;
-		user_service->msg_insert++;
-
-		/*
-		 * If there is a thread waiting in DEQUEUE_MESSAGE, or if
-		 * there is a MESSAGE_AVAILABLE in the completion queue then
-		 * bypass the completion queue.
-		 */
-		if (((user_service->message_available_pos -
-			instance->completion_remove) >= 0) ||
-			user_service->dequeue_pending) {
-			user_service->dequeue_pending = 0;
-			skip_completion = true;
+		DEBUG_TRACE(SERVICE_CALLBACK_LINE);
+		if (wait_for_completion_interruptible(&user_service->remove_event)) {
+			dev_dbg(instance->state->dev, "arm: interrupted\n");
+			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
+			vchiq_service_put(service);
+			return -EAGAIN;
+		} else if (instance->closing) {
+			dev_dbg(instance->state->dev, "arm: closing\n");
+			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
+			vchiq_service_put(service);
+			return -EINVAL;
 		}
+		DEBUG_TRACE(SERVICE_CALLBACK_LINE);
+		spin_lock(&service->state->msg_queue_spinlock);
+	}

-		spin_unlock(&service->state->msg_queue_spinlock);
-		complete(&user_service->insert_event);
+	user_service->msg_queue[user_service->msg_insert &
+		(MSG_QUEUE_SIZE - 1)] = header;
+	user_service->msg_insert++;

-		header = NULL;
+	/*
+	 * If there is a thread waiting in DEQUEUE_MESSAGE, or if
+	 * there is a MESSAGE_AVAILABLE in the completion queue then
+	 * bypass the completion queue.
+	 */
+	if (((user_service->message_available_pos -
+		instance->completion_remove) >= 0) ||
+		user_service->dequeue_pending) {
+		user_service->dequeue_pending = 0;
+		skip_completion = true;
 	}
+
+	spin_unlock(&service->state->msg_queue_spinlock);
+	complete(&user_service->insert_event);
+
+	header = NULL;
+
+service_put:
 	DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 	vchiq_service_put(service);

--
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 08/11] staging: vchiq_core: Add comments to mutex/spinlocks

[Stefan Wahren]

From: Phil Elwell <phil@raspberrypi.com>

checkpatch.pl complains about missing comments at
mutex/spinlock definitions. So add them accordingly.

Link: https://github.com/raspberrypi/linux/pull/6139/
Signed-off-by: Phil Elwell <phil@raspberrypi.com>
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../vc04_services/interface/vchiq_arm/vchiq_core.h     | 10 +++++++++-
 1 file changed, 9 insertions(+), 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 3c7a6838ddba..3abcd6910f25 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -196,6 +196,8 @@ struct vchiq_service {

 	struct completion remove_event;
 	struct completion bulk_remove_event;
+
+	/* Serialise access to the bulk transfer queues */
 	struct mutex bulk_mutex;

 	struct service_stats_struct {
@@ -312,7 +314,7 @@ struct vchiq_state {
 	/* Event indicating connect message received */
 	struct completion connect;

-	/* Mutex protecting services */
+	/* Mutex protecting service creation */
 	struct mutex mutex;
 	struct vchiq_instance **instance;

@@ -341,16 +343,22 @@ struct vchiq_state {
 	char *rx_data;
 	struct vchiq_slot_info *rx_info;

+	/* Serialise access to the main message slots */
 	struct mutex slot_mutex;

+	/* Serialise slot refcount updates */
 	struct mutex recycle_mutex;

+	/* Serialise access to the single synchronous message slot */
 	struct mutex sync_mutex;

+	/* Serialise access to the message queues to userspace */
 	spinlock_t msg_queue_spinlock;

+	/* Serialise completion of blocking transfers */
 	spinlock_t bulk_waiter_spinlock;

+	/* Serialise updates to slot quota data */
 	spinlock_t quota_spinlock;

 	/*
--
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 09/11] staging: vchiq: Move struct vchiq_config to vchiq.h

[Stefan Wahren]

This struct is part of the VCHIQ userspace API, which we
don't want to break. So move the struct definition to
vchiq.h, which contains the rest of the userspace API.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../vc04_services/include/linux/raspberrypi/vchiq.h  | 12 ++++++++++++
 .../vc04_services/interface/vchiq_arm/vchiq_core.h   | 12 ------------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h b/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
index 6c40d8c1dde6..2e34c67966c6 100644
--- a/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
+++ b/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
@@ -79,6 +79,18 @@ struct vchiq_service_params_kernel {
 	short version_min;   /* Update for incompatible changes */
 };

+struct vchiq_config {
+	unsigned int max_msg_size;
+	unsigned int bulk_threshold;	/* The message size above which it
+					 * is better to use a bulk transfer
+					 * (<= max_msg_size)
+					 */
+	unsigned int max_outstanding_bulks;
+	unsigned int max_services;
+	short version;      /* The version of VCHIQ */
+	short version_min;  /* The minimum compatible version of VCHIQ */
+};
+
 extern int vchiq_initialise(struct vchiq_state *state,
 			    struct vchiq_instance **pinstance);
 extern int vchiq_shutdown(struct vchiq_instance *instance);
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 3abcd6910f25..a83f9a5d478f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -423,18 +423,6 @@ struct bulk_waiter {
 	int actual;
 };

-struct vchiq_config {
-	unsigned int max_msg_size;
-	unsigned int bulk_threshold;	/* The message size above which it
-					 * is better to use a bulk transfer
-					 * (<= max_msg_size)
-					 */
-	unsigned int max_outstanding_bulks;
-	unsigned int max_services;
-	short version;      /* The version of VCHIQ */
-	short version_min;  /* The minimum compatible version of VCHIQ */
-};
-
 extern spinlock_t bulk_waiter_spinlock;

 extern const char *
--
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 10/11] staging: vchiq_core: Add hex prefix to debugfs output

[Stefan Wahren]

The number format of VCHIQ debugfs isn't always clear. So let's
add a prefix for all hex values, in order to make things clear.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../vc04_services/interface/vchiq_arm/vchiq_core.c        | 8 ++++----
 1 file changed, 4 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 51cfc366519b..4f65e4021c4d 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3370,7 +3370,7 @@ vchiq_dump_shared_state(struct seq_file *f, struct vchiq_state *state,
 	};
 	int i;

-	seq_printf(f, "  %s: slots %d-%d tx_pos=%x recycle=%x\n",
+	seq_printf(f, "  %s: slots %d-%d tx_pos=0x%x recycle=0x%x\n",
 		   label, shared->slot_first, shared->slot_last,
 		   shared->tx_pos, shared->slot_queue_recycle);

@@ -3386,7 +3386,7 @@ vchiq_dump_shared_state(struct seq_file *f, struct vchiq_state *state,
 	}

 	for (i = 1; i < shared->debug[DEBUG_ENTRIES]; i++) {
-		seq_printf(f, "    DEBUG: %s = %d(%x)\n",
+		seq_printf(f, "    DEBUG: %s = %d(0x%x)\n",
 			   debug_names[i], shared->debug[i], shared->debug[i]);
 	}
 }
@@ -3414,7 +3414,7 @@ vchiq_dump_service_state(struct seq_file *f, struct vchiq_service *service)

 			if (service->public_fourcc != VCHIQ_FOURCC_INVALID)
 				scnprintf(remoteport + len2, sizeof(remoteport) - len2,
-					  " (client %x)", service->client_id);
+					  " (client 0x%x)", service->client_id);
 		} else {
 			strscpy(remoteport, "n/a", sizeof(remoteport));
 		}
@@ -3475,7 +3475,7 @@ void vchiq_dump_state(struct seq_file *f, struct vchiq_state *state)
 	seq_printf(f, "State %d: %s\n", state->id,
 		   conn_state_names[state->conn_state]);

-	seq_printf(f, "  tx_pos=%x(@%pK), rx_pos=%x(@%pK)\n",
+	seq_printf(f, "  tx_pos=0x%x(@%pK), rx_pos=0x%x(@%pK)\n",
 		   state->local->tx_pos,
 		   state->tx_data + (state->local_tx_pos & VCHIQ_SLOT_MASK),
 		   state->rx_pos,
--
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 11/11] staging: vc04_services: Update testing instructions

[Stefan Wahren]

Since the initial version of the testing instructions a few
things has changed. So consider the latest arm64/defconfig changes
and the new debugfs entry.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../staging/vc04_services/interface/TESTING   | 45 ++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/vc04_services/interface/TESTING b/drivers/staging/vc04_services/interface/TESTING
index a6d63efcbcb9..273952dc9d85 100644
--- a/drivers/staging/vc04_services/interface/TESTING
+++ b/drivers/staging/vc04_services/interface/TESTING
@@ -31,7 +31,7 @@ Here are the most common kernel configurations:

  3. BCM2837 target SoC (ARM 64 bit)

-    Use the defconfig as a base and then enable all VCHIQ options.
+    Use the defconfig which has most of the VCHIQ options enabled.

 * Scenarios

@@ -80,3 +80,46 @@ Here are the most common kernel configurations:
    vchi ping (size 0, 100 async, 100 oneway) -> infus
    vchi ping (size 0, 200 async, 0 oneway) -> infus
    ...
+
+ * Debugfs test
+
+   Command: cat /sys/kernel/debug/vchiq/state
+
+   Example output:
+   State 0: CONNECTED
+     tx_pos=0x1e8(@43b0acda), rx_pos=0x170(@05493af8)
+     Version: 8 (min 3)
+     Stats: ctrl_tx_count=7, ctrl_rx_count=7, error_count=0
+     Slots: 30 available (29 data), 0 recyclable, 0 stalls (0 data)
+     Platform: 2835 (VC master)
+     Local: slots 34-64 tx_pos=0x1e8 recycle=0x1f
+       Slots claimed:
+       DEBUG: SLOT_HANDLER_COUNT = 20(0x14)
+       DEBUG: SLOT_HANDLER_LINE = 1937(0x791)
+       DEBUG: PARSE_LINE = 1864(0x748)
+       DEBUG: PARSE_HEADER = -249155224(0xf1263168)
+       DEBUG: PARSE_MSGID = 67362817(0x403e001)
+       DEBUG: AWAIT_COMPLETION_LINE = 0(0x0)
+       DEBUG: DEQUEUE_MESSAGE_LINE = 0(0x0)
+       DEBUG: SERVICE_CALLBACK_LINE = 0(0x0)
+       DEBUG: MSG_QUEUE_FULL_COUNT = 0(0x0)
+       DEBUG: COMPLETION_QUEUE_FULL_COUNT = 0(0x0)
+     Remote: slots 2-32 tx_pos=0x170 recycle=0x1f
+       Slots claimed:
+         2: 10/9
+       DEBUG: SLOT_HANDLER_COUNT = 20(0x14)
+       DEBUG: SLOT_HANDLER_LINE = 1851(0x73b)
+       DEBUG: PARSE_LINE = 1827(0x723)
+       DEBUG: PARSE_HEADER = -150330912(0xf70a21e0)
+       DEBUG: PARSE_MSGID = 67113022(0x400103e)
+       DEBUG: AWAIT_COMPLETION_LINE = 0(0x0)
+       DEBUG: DEQUEUE_MESSAGE_LINE = 0(0x0)
+       DEBUG: SERVICE_CALLBACK_LINE = 0(0x0)
+       DEBUG: MSG_QUEUE_FULL_COUNT = 0(0x0)
+       DEBUG: COMPLETION_QUEUE_FULL_COUNT = 0(0x0)
+   Service 0: LISTENING (ref 1) 'PEEK little-endian (0x4b454550)' remote n/a (msg use 0/3840, slot use 0/15)
+     Bulk: tx_pending=0 (size 0), rx_pending=0 (size 0)
+     Ctrl: tx_count=0, tx_bytes=0, rx_count=0, rx_bytes=0
+     Bulk: tx_count=0, tx_bytes=0, rx_count=0, rx_bytes=0
+     0 quota stalls, 0 slot stalls, 0 bulk stalls, 0 aborted, 0 errors
+     instance b511f60b
--
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/11] staging: vchiq_arm: Replace variable ret with status

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Tue, Jun 04, 2024 at 07:28:54PM +0200, Stefan Wahren wrote:
> The variable ret is just storing the result of last function
> call like the variable status. So replace ret with status
> and make a consistent use of this variable.

I would have gone the other way around, and replaced 'status' with 'ret'
as the latter is more commonly used for this purpose in kernel code. I
know it will generate more churn, but I think the result will be more
maintainable.

> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>  .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++----
>  1 file changed, 4 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 69daeba974f2..e2f4aa00cb70 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1298,7 +1298,6 @@ vchiq_keepalive_thread_func(void *v)
>  	int 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'),
> @@ -1307,9 +1306,10 @@ vchiq_keepalive_thread_func(void *v)
>  		.version_min = KEEPALIVE_VER_MIN
>  	};
> 
> -	ret = vchiq_initialise(state, &instance);
> -	if (ret) {
> -		dev_err(state->dev, "suspend: %s: vchiq_initialise failed %d\n", __func__, ret);
> +	status = vchiq_initialise(state, &instance);
> +	if (status) {
> +		dev_err(state->dev, "suspend: %s: vchiq_initialise failed %d\n",
> +			__func__, status);
>  		goto exit;
>  	}
> 

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/11] staging: vchiq_arm: Drop obsolete comment

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Tue, Jun 04, 2024 at 07:28:55PM +0200, Stefan Wahren wrote:
> The BUG_ON has been replaced with WARN_ON. So we can drop the
> comment.

Note https://lwn.net/Articles/969923/ :-)

This being said, I think the comment can be dropped, regardless of
whether or not we drop the WARN_ON() later.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index e2f4aa00cb70..515cdcba043d 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1451,7 +1451,6 @@ vchiq_release_internal(struct vchiq_state *state, struct vchiq_service *service)
> 
>  	write_lock_bh(&arm_state->susp_res_lock);
>  	if (!arm_state->videocore_use_count || !(*entity_uc)) {
> -		/* 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 = -EINVAL;

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/11] staging: vchiq_core: Drop non-functional struct members

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Tue, Jun 04, 2024 at 07:28:56PM +0200, Stefan Wahren wrote:
> There are some struct members, which don't have a real
> function. So it's safe to drop them.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

Both fields are unused in the RPi kernel as well, so this looks fine to
me.

The last user of bulk_transfer_mutex was removed in commit
14f4d72fb799a9b3170a45ab80d4a3ddad541960 which you could reference if
desired. slot_remove_event has never been used.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c    | 2 --
>  .../staging/vc04_services/interface/vchiq_arm/vchiq_core.h    | 4 ----
>  2 files changed, 6 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 df3af821f218..51cfc366519b 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -2163,14 +2163,12 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero, s
>  	mutex_init(&state->slot_mutex);
>  	mutex_init(&state->recycle_mutex);
>  	mutex_init(&state->sync_mutex);
> -	mutex_init(&state->bulk_transfer_mutex);
> 
>  	spin_lock_init(&state->msg_queue_spinlock);
>  	spin_lock_init(&state->bulk_waiter_spinlock);
>  	spin_lock_init(&state->quota_spinlock);
> 
>  	init_completion(&state->slot_available_event);
> -	init_completion(&state->slot_remove_event);
>  	init_completion(&state->data_quota_event);
> 
>  	state->slot_queue_available = 0;
> 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 8af209e34fb2..be20abcfad75 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> @@ -347,8 +347,6 @@ struct vchiq_state {
> 
>  	struct mutex sync_mutex;
> 
> -	struct mutex bulk_transfer_mutex;
> -
>  	spinlock_t msg_queue_spinlock;
> 
>  	spinlock_t bulk_waiter_spinlock;
> @@ -393,8 +391,6 @@ struct vchiq_state {
>  	/* Signalled when a free slot becomes available. */
>  	struct completion slot_available_event;
> 
> -	struct completion slot_remove_event;
> -
>  	/* Signalled when a free data slot becomes available. */
>  	struct completion data_quota_event;
> 

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/11] staging: vchiq_arm: Drop unnecessary declarations

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Tue, Jun 04, 2024 at 07:28:57PM +0200, Stefan Wahren wrote:
> These declarations are left from cleanups and not necessary
> anymore. So we can drop them.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h  | 3 ---
>  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h | 2 --
>  2 files changed, 5 deletions(-)
> 
> 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 fd1b9d3555ce..b402aac333d9 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> @@ -109,9 +109,6 @@ vchiq_release_service(struct vchiq_instance *instance, unsigned int handle);
>  extern int
>  vchiq_check_service(struct vchiq_service *service);
> 
> -extern void
> -vchiq_dump_platform_use_state(struct vchiq_state *state);
> -
>  extern void
>  vchiq_dump_service_use_state(struct vchiq_state *state);
> 
> 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 be20abcfad75..3c7a6838ddba 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> @@ -539,8 +539,6 @@ int vchiq_platform_init_state(struct vchiq_state *state);
> 
>  int vchiq_check_service(struct vchiq_service *service);
> 
> -void vchiq_on_remote_use_active(struct vchiq_state *state);
> -
>  int vchiq_send_remote_use(struct vchiq_state *state);
> 
>  int vchiq_send_remote_use_active(struct vchiq_state *state);

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/11] staging: vchiq_arm: Reduce indentation of service_callback

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Tue, Jun 04, 2024 at 07:29:00PM +0200, Stefan Wahren wrote:
> The service_callback has 5 levels of indentation, which makes it
> hard to read. Reduce this by using a goto for the corner cases
> (no header or VCHI service).

I think the goto isn't very nice. Could you instead try to split code to
a separate function ? That should do a better job at improve
readability.

> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>  .../interface/vchiq_arm/vchiq_arm.c           | 111 +++++++++---------
>  1 file changed, 57 insertions(+), 54 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 45acca670bbd..0055c7d7e617 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1101,71 +1101,74 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
>  		user_service, service->localport, user_service->userdata,
>  		reason, header, instance, bulk_userdata);
> 
> -	if (header && user_service->is_vchi) {
> -		spin_lock(&service->state->msg_queue_spinlock);
> -		while (user_service->msg_insert ==
> -			(user_service->msg_remove + MSG_QUEUE_SIZE)) {
> -			spin_unlock(&service->state->msg_queue_spinlock);
> -			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> -			DEBUG_COUNT(MSG_QUEUE_FULL_COUNT);
> -			dev_dbg(service->state->dev, "arm: msg queue full\n");
> -			/*
> -			 * If there is no MESSAGE_AVAILABLE in the completion
> -			 * queue, add one
> -			 */
> -			if ((user_service->message_available_pos -
> -				instance->completion_remove) < 0) {
> -				int status;
> -
> -				dev_dbg(instance->state->dev,
> -					"arm: Inserting extra MESSAGE_AVAILABLE\n");
> -				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> -				status = add_completion(instance, reason, NULL, user_service,
> -							bulk_userdata);
> -				if (status) {
> -					DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> -					vchiq_service_put(service);
> -					return status;
> -				}
> -			}
> +	if (!header || !user_service->is_vchi)
> +		goto service_put;
> +
> +	spin_lock(&service->state->msg_queue_spinlock);
> +	while (user_service->msg_insert ==
> +		(user_service->msg_remove + MSG_QUEUE_SIZE)) {
> +		spin_unlock(&service->state->msg_queue_spinlock);
> +		DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> +		DEBUG_COUNT(MSG_QUEUE_FULL_COUNT);
> +		dev_dbg(service->state->dev, "arm: msg queue full\n");
> +		/*
> +		 * If there is no MESSAGE_AVAILABLE in the completion
> +		 * queue, add one
> +		 */
> +		if ((user_service->message_available_pos -
> +			instance->completion_remove) < 0) {
> +			int status;
> 
> +			dev_dbg(instance->state->dev,
> +				"arm: Inserting extra MESSAGE_AVAILABLE\n");
>  			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> -			if (wait_for_completion_interruptible(&user_service->remove_event)) {
> -				dev_dbg(instance->state->dev, "arm: interrupted\n");
> -				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> -				vchiq_service_put(service);
> -				return -EAGAIN;
> -			} else if (instance->closing) {
> -				dev_dbg(instance->state->dev, "arm: closing\n");
> +			status = add_completion(instance, reason, NULL, user_service,
> +						bulk_userdata);
> +			if (status) {
>  				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
>  				vchiq_service_put(service);
> -				return -EINVAL;
> +				return status;
>  			}
> -			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> -			spin_lock(&service->state->msg_queue_spinlock);
>  		}
> 
> -		user_service->msg_queue[user_service->msg_insert &
> -			(MSG_QUEUE_SIZE - 1)] = header;
> -		user_service->msg_insert++;
> -
> -		/*
> -		 * If there is a thread waiting in DEQUEUE_MESSAGE, or if
> -		 * there is a MESSAGE_AVAILABLE in the completion queue then
> -		 * bypass the completion queue.
> -		 */
> -		if (((user_service->message_available_pos -
> -			instance->completion_remove) >= 0) ||
> -			user_service->dequeue_pending) {
> -			user_service->dequeue_pending = 0;
> -			skip_completion = true;
> +		DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> +		if (wait_for_completion_interruptible(&user_service->remove_event)) {
> +			dev_dbg(instance->state->dev, "arm: interrupted\n");
> +			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> +			vchiq_service_put(service);
> +			return -EAGAIN;
> +		} else if (instance->closing) {
> +			dev_dbg(instance->state->dev, "arm: closing\n");
> +			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> +			vchiq_service_put(service);
> +			return -EINVAL;
>  		}
> +		DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> +		spin_lock(&service->state->msg_queue_spinlock);
> +	}
> 
> -		spin_unlock(&service->state->msg_queue_spinlock);
> -		complete(&user_service->insert_event);
> +	user_service->msg_queue[user_service->msg_insert &
> +		(MSG_QUEUE_SIZE - 1)] = header;
> +	user_service->msg_insert++;
> 
> -		header = NULL;
> +	/*
> +	 * If there is a thread waiting in DEQUEUE_MESSAGE, or if
> +	 * there is a MESSAGE_AVAILABLE in the completion queue then
> +	 * bypass the completion queue.
> +	 */
> +	if (((user_service->message_available_pos -
> +		instance->completion_remove) >= 0) ||
> +		user_service->dequeue_pending) {
> +		user_service->dequeue_pending = 0;
> +		skip_completion = true;
>  	}
> +
> +	spin_unlock(&service->state->msg_queue_spinlock);
> +	complete(&user_service->insert_event);
> +
> +	header = NULL;
> +
> +service_put:
>  	DEBUG_TRACE(SERVICE_CALLBACK_LINE);
>  	vchiq_service_put(service);
> 

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 06/11] staging: vchiq_arm: Drop vchiq_arm_init_state

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Tue, Jun 04, 2024 at 07:28:59PM +0200, Stefan Wahren wrote:
> After removal of struct vchiq_2835_state, the init of vchiq_arm_state
> can be simplified by doing it directly within vchiq_platform_init_state.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../interface/vchiq_arm/vchiq_arm.c           | 27 +++++++------------
>  1 file changed, 9 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 98a0b2d52af5..45acca670bbd 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -588,23 +588,6 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>  	return 0;
>  }
> 
> -static void
> -vchiq_arm_init_state(struct vchiq_state *state,
> -		     struct vchiq_arm_state *arm_state)
> -{
> -	if (arm_state) {
> -		rwlock_init(&arm_state->susp_res_lock);
> -
> -		init_completion(&arm_state->ka_evt);
> -		atomic_set(&arm_state->ka_use_count, 0);
> -		atomic_set(&arm_state->ka_use_ack_count, 0);
> -		atomic_set(&arm_state->ka_release_count, 0);
> -
> -		arm_state->state = state;
> -		arm_state->first_connect = 0;
> -	}
> -}
> -
>  int
>  vchiq_platform_init_state(struct vchiq_state *state)
>  {
> @@ -614,7 +597,15 @@ vchiq_platform_init_state(struct vchiq_state *state)
>  	if (!platform_state)
>  		return -ENOMEM;
> 
> -	vchiq_arm_init_state(state, platform_state);
> +	rwlock_init(&platform_state->susp_res_lock);
> +
> +	init_completion(&platform_state->ka_evt);
> +	atomic_set(&platform_state->ka_use_count, 0);
> +	atomic_set(&platform_state->ka_use_ack_count, 0);
> +	atomic_set(&platform_state->ka_release_count, 0);
> +
> +	platform_state->state = state;
> +
>  	state->platform_state = (struct opaque_platform_state *)platform_state;
> 
>  	return 0;

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/11] staging: vchiq_core: Add comments to mutex/spinlocks

[Laurent Pinchart]

Hi Stefan and Phil,

Thank you for the patch.

On Tue, Jun 04, 2024 at 07:29:01PM +0200, Stefan Wahren wrote:
> From: Phil Elwell <phil@raspberrypi.com>
> 
> checkpatch.pl complains about missing comments at
> mutex/spinlock definitions. So add them accordingly.

Less warnings is good, but we should address the problem they outline,
not just silence them for the sake of it. It would be better, for each
lock, to explicitly list the data fields the lock protects.

> Link: https://github.com/raspberrypi/linux/pull/6139/
> Signed-off-by: Phil Elwell <phil@raspberrypi.com>
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>  .../vc04_services/interface/vchiq_arm/vchiq_core.h     | 10 +++++++++-
>  1 file changed, 9 insertions(+), 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 3c7a6838ddba..3abcd6910f25 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> @@ -196,6 +196,8 @@ struct vchiq_service {
> 
>  	struct completion remove_event;
>  	struct completion bulk_remove_event;
> +
> +	/* Serialise access to the bulk transfer queues */
>  	struct mutex bulk_mutex;
> 
>  	struct service_stats_struct {
> @@ -312,7 +314,7 @@ struct vchiq_state {
>  	/* Event indicating connect message received */
>  	struct completion connect;
> 
> -	/* Mutex protecting services */
> +	/* Mutex protecting service creation */
>  	struct mutex mutex;
>  	struct vchiq_instance **instance;
> 
> @@ -341,16 +343,22 @@ struct vchiq_state {
>  	char *rx_data;
>  	struct vchiq_slot_info *rx_info;
> 
> +	/* Serialise access to the main message slots */
>  	struct mutex slot_mutex;
> 
> +	/* Serialise slot refcount updates */
>  	struct mutex recycle_mutex;
> 
> +	/* Serialise access to the single synchronous message slot */
>  	struct mutex sync_mutex;
> 
> +	/* Serialise access to the message queues to userspace */
>  	spinlock_t msg_queue_spinlock;
> 
> +	/* Serialise completion of blocking transfers */
>  	spinlock_t bulk_waiter_spinlock;
> 
> +	/* Serialise updates to slot quota data */
>  	spinlock_t quota_spinlock;
> 
>  	/*

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 05/11] staging: vchiq_arm: Get the rid off struct vchiq_2835_state

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Tue, Jun 04, 2024 at 07:28:58PM +0200, Stefan Wahren wrote:
> The whole benefit of this encapsulating struct is questionable.
> It just stores a flag to signalize the init state of vchiq_arm_state.
> Beside the fact this flag is set too soon, the access to uninitialized
> members should be avoided per design.

Do you have plans to address the design ?

> So initialize vchiq_arm_state
> properly before assign it directly to vchiq_state.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>  .../interface/vchiq_arm/vchiq_arm.c           | 25 +++++--------------
>  1 file changed, 6 insertions(+), 19 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 515cdcba043d..98a0b2d52af5 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -109,11 +109,6 @@ struct vchiq_arm_state {
>  	int first_connect;
>  };
> 
> -struct vchiq_2835_state {
> -	int inited;
> -	struct vchiq_arm_state arm_state;
> -};
> -
>  struct vchiq_pagelist_info {
>  	struct pagelist *pagelist;
>  	size_t pagelist_buffer_size;
> @@ -613,29 +608,21 @@ vchiq_arm_init_state(struct vchiq_state *state,
>  int
>  vchiq_platform_init_state(struct vchiq_state *state)
>  {
> -	struct vchiq_2835_state *platform_state;
> +	struct vchiq_arm_state *platform_state;
> 
> -	state->platform_state = kzalloc(sizeof(*platform_state), GFP_KERNEL);
> -	if (!state->platform_state)
> +	platform_state = kzalloc(sizeof(*platform_state), GFP_KERNEL);
> +	if (!platform_state)
>  		return -ENOMEM;
> 
> -	platform_state = (struct vchiq_2835_state *)state->platform_state;
> -
> -	platform_state->inited = 1;
> -	vchiq_arm_init_state(state, &platform_state->arm_state);
> +	vchiq_arm_init_state(state, platform_state);
> +	state->platform_state = (struct opaque_platform_state *)platform_state;
> 
>  	return 0;
>  }
> 
>  static struct vchiq_arm_state *vchiq_platform_get_arm_state(struct vchiq_state *state)
>  {
> -	struct vchiq_2835_state *platform_state;
> -
> -	platform_state   = (struct vchiq_2835_state *)state->platform_state;
> -
> -	WARN_ON_ONCE(!platform_state->inited);
> -
> -	return &platform_state->arm_state;
> +	return (struct vchiq_arm_state *)state->platform_state;
>  }
> 
>  void

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 10/11] staging: vchiq_core: Add hex prefix to debugfs output

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Tue, Jun 04, 2024 at 07:29:03PM +0200, Stefan Wahren wrote:
> The number format of VCHIQ debugfs isn't always clear. So let's
> add a prefix for all hex values, in order to make things clear.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../vc04_services/interface/vchiq_arm/vchiq_core.c        | 8 ++++----
>  1 file changed, 4 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 51cfc366519b..4f65e4021c4d 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -3370,7 +3370,7 @@ vchiq_dump_shared_state(struct seq_file *f, struct vchiq_state *state,
>  	};
>  	int i;
> 
> -	seq_printf(f, "  %s: slots %d-%d tx_pos=%x recycle=%x\n",
> +	seq_printf(f, "  %s: slots %d-%d tx_pos=0x%x recycle=0x%x\n",
>  		   label, shared->slot_first, shared->slot_last,
>  		   shared->tx_pos, shared->slot_queue_recycle);
> 
> @@ -3386,7 +3386,7 @@ vchiq_dump_shared_state(struct seq_file *f, struct vchiq_state *state,
>  	}
> 
>  	for (i = 1; i < shared->debug[DEBUG_ENTRIES]; i++) {
> -		seq_printf(f, "    DEBUG: %s = %d(%x)\n",
> +		seq_printf(f, "    DEBUG: %s = %d(0x%x)\n",
>  			   debug_names[i], shared->debug[i], shared->debug[i]);
>  	}
>  }
> @@ -3414,7 +3414,7 @@ vchiq_dump_service_state(struct seq_file *f, struct vchiq_service *service)
> 
>  			if (service->public_fourcc != VCHIQ_FOURCC_INVALID)
>  				scnprintf(remoteport + len2, sizeof(remoteport) - len2,
> -					  " (client %x)", service->client_id);
> +					  " (client 0x%x)", service->client_id);
>  		} else {
>  			strscpy(remoteport, "n/a", sizeof(remoteport));
>  		}
> @@ -3475,7 +3475,7 @@ void vchiq_dump_state(struct seq_file *f, struct vchiq_state *state)
>  	seq_printf(f, "State %d: %s\n", state->id,
>  		   conn_state_names[state->conn_state]);
> 
> -	seq_printf(f, "  tx_pos=%x(@%pK), rx_pos=%x(@%pK)\n",
> +	seq_printf(f, "  tx_pos=0x%x(@%pK), rx_pos=0x%x(@%pK)\n",
>  		   state->local->tx_pos,
>  		   state->tx_data + (state->local_tx_pos & VCHIQ_SLOT_MASK),
>  		   state->rx_pos,

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/11] staging: vchiq: Move struct vchiq_config to vchiq.h

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Tue, Jun 04, 2024 at 07:29:02PM +0200, Stefan Wahren wrote:
> This struct is part of the VCHIQ userspace API, which we
> don't want to break. So move the struct definition to
> vchiq.h, which contains the rest of the userspace API.

Dies it ? vchiq.h contains lots of kernel API elements. Beside, we have
headers such as vc04_services/interface/vchiq_arm/vchiq_ioctl.h that
contain UAPI elements.

Splitting the UAPI to separate headers is a good idea. Could you address
that task as a whole, and create a
drivers/staging/vc04_services/include/uapi/linux/raspberrypi/vchiq.h
file for the whole UAPI ?

> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>  .../vc04_services/include/linux/raspberrypi/vchiq.h  | 12 ++++++++++++
>  .../vc04_services/interface/vchiq_arm/vchiq_core.h   | 12 ------------
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h b/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
> index 6c40d8c1dde6..2e34c67966c6 100644
> --- a/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
> +++ b/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
> @@ -79,6 +79,18 @@ struct vchiq_service_params_kernel {
>  	short version_min;   /* Update for incompatible changes */
>  };
> 
> +struct vchiq_config {
> +	unsigned int max_msg_size;
> +	unsigned int bulk_threshold;	/* The message size above which it
> +					 * is better to use a bulk transfer
> +					 * (<= max_msg_size)
> +					 */
> +	unsigned int max_outstanding_bulks;
> +	unsigned int max_services;
> +	short version;      /* The version of VCHIQ */
> +	short version_min;  /* The minimum compatible version of VCHIQ */
> +};
> +
>  extern int vchiq_initialise(struct vchiq_state *state,
>  			    struct vchiq_instance **pinstance);
>  extern int vchiq_shutdown(struct vchiq_instance *instance);
> 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 3abcd6910f25..a83f9a5d478f 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> @@ -423,18 +423,6 @@ struct bulk_waiter {
>  	int actual;
>  };
> 
> -struct vchiq_config {
> -	unsigned int max_msg_size;
> -	unsigned int bulk_threshold;	/* The message size above which it
> -					 * is better to use a bulk transfer
> -					 * (<= max_msg_size)
> -					 */
> -	unsigned int max_outstanding_bulks;
> -	unsigned int max_services;
> -	short version;      /* The version of VCHIQ */
> -	short version_min;  /* The minimum compatible version of VCHIQ */
> -};
> -
>  extern spinlock_t bulk_waiter_spinlock;
> 
>  extern const char *

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/11] staging: vchiq_arm: Drop obsolete comment

[Stefan Wahren]

Hi Laurent,

Am 05.06.24 um 08:52 schrieb Laurent Pinchart:
> Hi Stefan,
>
> Thank you for the patch.
>
> On Tue, Jun 04, 2024 at 07:28:55PM +0200, Stefan Wahren wrote:
>> The BUG_ON has been replaced with WARN_ON. So we can drop the
>> comment.
> Note https://lwn.net/Articles/969923/ :-)
>
> This being said, I think the comment can be dropped, regardless of
> whether or not we drop the WARN_ON() later.
thanks i wasn't aware of that article. I think, we can address this in a
separate series and try to replace WARN_ON() with dev_warn() in cases it
makes sense.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>> ---
>>   drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> index e2f4aa00cb70..515cdcba043d 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -1451,7 +1451,6 @@ vchiq_release_internal(struct vchiq_state *state, struct vchiq_service *service)
>>
>>   	write_lock_bh(&arm_state->susp_res_lock);
>>   	if (!arm_state->videocore_use_count || !(*entity_uc)) {
>> -		/* 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 = -EINVAL;


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/11] staging: vchiq_arm: Replace variable ret with status

[Stefan Wahren]

Hi Laurent,

Am 05.06.24 um 08:51 schrieb Laurent Pinchart:
> Hi Stefan,
>
> Thank you for the patch.
>
> On Tue, Jun 04, 2024 at 07:28:54PM +0200, Stefan Wahren wrote:
>> The variable ret is just storing the result of last function
>> call like the variable status. So replace ret with status
>> and make a consistent use of this variable.
> I would have gone the other way around, and replaced 'status' with 'ret'
> as the latter is more commonly used for this purpose in kernel code. I
> know it will generate more churn, but I think the result will be more
> maintainable.
i will look how many of the existing code is affect, because this is not
the only function.
>
>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>> ---
>>   .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++----
>>   1 file changed, 4 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 69daeba974f2..e2f4aa00cb70 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -1298,7 +1298,6 @@ vchiq_keepalive_thread_func(void *v)
>>   	int 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'),
>> @@ -1307,9 +1306,10 @@ vchiq_keepalive_thread_func(void *v)
>>   		.version_min = KEEPALIVE_VER_MIN
>>   	};
>>
>> -	ret = vchiq_initialise(state, &instance);
>> -	if (ret) {
>> -		dev_err(state->dev, "suspend: %s: vchiq_initialise failed %d\n", __func__, ret);
>> +	status = vchiq_initialise(state, &instance);
>> +	if (status) {
>> +		dev_err(state->dev, "suspend: %s: vchiq_initialise failed %d\n",
>> +			__func__, status);
>>   		goto exit;
>>   	}
>>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 05/11] staging: vchiq_arm: Get the rid off struct vchiq_2835_state

[Stefan Wahren]

Hi Laurent,

Am 05.06.24 um 09:11 schrieb Laurent Pinchart:
> Hi Stefan,
>
> Thank you for the patch.
>
> On Tue, Jun 04, 2024 at 07:28:58PM +0200, Stefan Wahren wrote:
>> The whole benefit of this encapsulating struct is questionable.
>> It just stores a flag to signalize the init state of vchiq_arm_state.
>> Beside the fact this flag is set too soon, the access to uninitialized
>> members should be avoided per design.
> Do you have plans to address the design ?
by using kzalloc and assigning platform_state at the end of
vchiq_platform_init_state, i would consider this as fulfilled. Or do you
care about the possible platform_state NULL pointer?
>
>> So initialize vchiq_arm_state
>> properly before assign it directly to vchiq_state.
>>
>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>> ---
>>   .../interface/vchiq_arm/vchiq_arm.c           | 25 +++++--------------
>>   1 file changed, 6 insertions(+), 19 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 515cdcba043d..98a0b2d52af5 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -109,11 +109,6 @@ struct vchiq_arm_state {
>>   	int first_connect;
>>   };
>>
>> -struct vchiq_2835_state {
>> -	int inited;
>> -	struct vchiq_arm_state arm_state;
>> -};
>> -
>>   struct vchiq_pagelist_info {
>>   	struct pagelist *pagelist;
>>   	size_t pagelist_buffer_size;
>> @@ -613,29 +608,21 @@ vchiq_arm_init_state(struct vchiq_state *state,
>>   int
>>   vchiq_platform_init_state(struct vchiq_state *state)
>>   {
>> -	struct vchiq_2835_state *platform_state;
>> +	struct vchiq_arm_state *platform_state;
>>
>> -	state->platform_state = kzalloc(sizeof(*platform_state), GFP_KERNEL);
>> -	if (!state->platform_state)
>> +	platform_state = kzalloc(sizeof(*platform_state), GFP_KERNEL);
>> +	if (!platform_state)
>>   		return -ENOMEM;
>>
>> -	platform_state = (struct vchiq_2835_state *)state->platform_state;
>> -
>> -	platform_state->inited = 1;
>> -	vchiq_arm_init_state(state, &platform_state->arm_state);
>> +	vchiq_arm_init_state(state, platform_state);
>> +	state->platform_state = (struct opaque_platform_state *)platform_state;
>>
>>   	return 0;
>>   }
>>
>>   static struct vchiq_arm_state *vchiq_platform_get_arm_state(struct vchiq_state *state)
>>   {
>> -	struct vchiq_2835_state *platform_state;
>> -
>> -	platform_state   = (struct vchiq_2835_state *)state->platform_state;
>> -
>> -	WARN_ON_ONCE(!platform_state->inited);
>> -
>> -	return &platform_state->arm_state;
>> +	return (struct vchiq_arm_state *)state->platform_state;
>>   }
>>
>>   void


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/11] staging: vchiq: Move struct vchiq_config to vchiq.h

[Stefan Wahren]

Hi Laurent,

Am 05.06.24 um 09:16 schrieb Laurent Pinchart:
> Hi Stefan,
>
> Thank you for the patch.
>
> On Tue, Jun 04, 2024 at 07:29:02PM +0200, Stefan Wahren wrote:
>> This struct is part of the VCHIQ userspace API, which we
>> don't want to break. So move the struct definition to
>> vchiq.h, which contains the rest of the userspace API.
> Dies it ? vchiq.h contains lots of kernel API elements. Beside, we have
> headers such as vc04_services/interface/vchiq_arm/vchiq_ioctl.h that
> contain UAPI elements.
>
> Splitting the UAPI to separate headers is a good idea. Could you address
> that task as a whole, and create a
> drivers/staging/vc04_services/include/uapi/linux/raspberrypi/vchiq.h
> file for the whole UAPI ?
yes, but not in this series. So i will drop it.
>
>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>> ---
>>   .../vc04_services/include/linux/raspberrypi/vchiq.h  | 12 ++++++++++++
>>   .../vc04_services/interface/vchiq_arm/vchiq_core.h   | 12 ------------
>>   2 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h b/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
>> index 6c40d8c1dde6..2e34c67966c6 100644
>> --- a/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
>> +++ b/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
>> @@ -79,6 +79,18 @@ struct vchiq_service_params_kernel {
>>   	short version_min;   /* Update for incompatible changes */
>>   };
>>
>> +struct vchiq_config {
>> +	unsigned int max_msg_size;
>> +	unsigned int bulk_threshold;	/* The message size above which it
>> +					 * is better to use a bulk transfer
>> +					 * (<= max_msg_size)
>> +					 */
>> +	unsigned int max_outstanding_bulks;
>> +	unsigned int max_services;
>> +	short version;      /* The version of VCHIQ */
>> +	short version_min;  /* The minimum compatible version of VCHIQ */
>> +};
>> +
>>   extern int vchiq_initialise(struct vchiq_state *state,
>>   			    struct vchiq_instance **pinstance);
>>   extern int vchiq_shutdown(struct vchiq_instance *instance);
>> 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 3abcd6910f25..a83f9a5d478f 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
>> @@ -423,18 +423,6 @@ struct bulk_waiter {
>>   	int actual;
>>   };
>>
>> -struct vchiq_config {
>> -	unsigned int max_msg_size;
>> -	unsigned int bulk_threshold;	/* The message size above which it
>> -					 * is better to use a bulk transfer
>> -					 * (<= max_msg_size)
>> -					 */
>> -	unsigned int max_outstanding_bulks;
>> -	unsigned int max_services;
>> -	short version;      /* The version of VCHIQ */
>> -	short version_min;  /* The minimum compatible version of VCHIQ */
>> -};
>> -
>>   extern spinlock_t bulk_waiter_spinlock;
>>
>>   extern const char *


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/11] staging: vchiq_arm: Drop obsolete comment

[Dan Carpenter]

On Wed, Jun 05, 2024 at 12:02:40PM +0200, Stefan Wahren wrote:
> Hi Laurent,
> 
> Am 05.06.24 um 08:52 schrieb Laurent Pinchart:
> > Hi Stefan,
> > 
> > Thank you for the patch.
> > 
> > On Tue, Jun 04, 2024 at 07:28:55PM +0200, Stefan Wahren wrote:
> > > The BUG_ON has been replaced with WARN_ON. So we can drop the
> > > comment.
> > Note https://lwn.net/Articles/969923/ :-)
> > 
> > This being said, I think the comment can be dropped, regardless of
> > whether or not we drop the WARN_ON() later.
> thanks i wasn't aware of that article. I think, we can address this in a
> separate series and try to replace WARN_ON() with dev_warn() in cases it
> makes sense.

I looked at the callers and it does look like a user could trigger these
via the VCHIQ_IOC_RELEASE_SERVICE ioctl so it should be dev_warn(), yes.
It's not necessarily in use at the time.


regards,
dan carpenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 05/11] staging: vchiq_arm: Get the rid off struct vchiq_2835_state

[Laurent Pinchart]

Hi Stefan,

On Wed, Jun 05, 2024 at 12:11:41PM +0200, Stefan Wahren wrote:
> Am 05.06.24 um 09:11 schrieb Laurent Pinchart:
> > On Tue, Jun 04, 2024 at 07:28:58PM +0200, Stefan Wahren wrote:
> >> The whole benefit of this encapsulating struct is questionable.
> >> It just stores a flag to signalize the init state of vchiq_arm_state.
> >> Beside the fact this flag is set too soon, the access to uninitialized
> >> members should be avoided per design.
> >
> > Do you have plans to address the design ?
>
> by using kzalloc and assigning platform_state at the end of
> vchiq_platform_init_state, i would consider this as fulfilled. Or do you
> care about the possible platform_state NULL pointer?

Reading the commit message, I thought you meant further changes were
need to fix the design. A clarification in the commit message could be
useful.

> >> So initialize vchiq_arm_state
> >> properly before assign it directly to vchiq_state.
> >>
> >> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> >> ---
> >>   .../interface/vchiq_arm/vchiq_arm.c           | 25 +++++--------------
> >>   1 file changed, 6 insertions(+), 19 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 515cdcba043d..98a0b2d52af5 100644
> >> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >> @@ -109,11 +109,6 @@ struct vchiq_arm_state {
> >>   	int first_connect;
> >>   };
> >>
> >> -struct vchiq_2835_state {
> >> -	int inited;
> >> -	struct vchiq_arm_state arm_state;
> >> -};
> >> -
> >>   struct vchiq_pagelist_info {
> >>   	struct pagelist *pagelist;
> >>   	size_t pagelist_buffer_size;
> >> @@ -613,29 +608,21 @@ vchiq_arm_init_state(struct vchiq_state *state,
> >>   int
> >>   vchiq_platform_init_state(struct vchiq_state *state)
> >>   {
> >> -	struct vchiq_2835_state *platform_state;
> >> +	struct vchiq_arm_state *platform_state;
> >>
> >> -	state->platform_state = kzalloc(sizeof(*platform_state), GFP_KERNEL);
> >> -	if (!state->platform_state)
> >> +	platform_state = kzalloc(sizeof(*platform_state), GFP_KERNEL);
> >> +	if (!platform_state)
> >>   		return -ENOMEM;
> >>
> >> -	platform_state = (struct vchiq_2835_state *)state->platform_state;
> >> -
> >> -	platform_state->inited = 1;
> >> -	vchiq_arm_init_state(state, &platform_state->arm_state);
> >> +	vchiq_arm_init_state(state, platform_state);
> >> +	state->platform_state = (struct opaque_platform_state *)platform_state;
> >>
> >>   	return 0;
> >>   }
> >>
> >>   static struct vchiq_arm_state *vchiq_platform_get_arm_state(struct vchiq_state *state)
> >>   {
> >> -	struct vchiq_2835_state *platform_state;
> >> -
> >> -	platform_state   = (struct vchiq_2835_state *)state->platform_state;
> >> -
> >> -	WARN_ON_ONCE(!platform_state->inited);
> >> -
> >> -	return &platform_state->arm_state;
> >> +	return (struct vchiq_arm_state *)state->platform_state;
> >>   }
> >>
> >>   void

-- 
Regards,

Laurent Pinchart

Re: [PATCH 05/11] staging: vchiq_arm: Get the rid off struct vchiq_2835_state

[Stefan Wahren]

Hi Laurent,

Am 11.06.24 um 23:22 schrieb Laurent Pinchart:
> Hi Stefan,
>
> On Wed, Jun 05, 2024 at 12:11:41PM +0200, Stefan Wahren wrote:
>> Am 05.06.24 um 09:11 schrieb Laurent Pinchart:
>>> On Tue, Jun 04, 2024 at 07:28:58PM +0200, Stefan Wahren wrote:
>>>> The whole benefit of this encapsulating struct is questionable.
>>>> It just stores a flag to signalize the init state of vchiq_arm_state.
>>>> Beside the fact this flag is set too soon, the access to uninitialized
>>>> members should be avoided per design.
>>> Do you have plans to address the design ?
>> by using kzalloc and assigning platform_state at the end of
>> vchiq_platform_init_state, i would consider this as fulfilled. Or do you
>> care about the possible platform_state NULL pointer?
> Reading the commit message, I thought you meant further changes were
> need to fix the design. A clarification in the commit message could be
> useful.
i missed to mention it in the V2 changelog, but i shorten the sentence
there to avoid the confusion. Is it better in V2 or does it still need
adjustment?