[PATCH V2 00/10] staging: vc04_services: Random cleanups

[PATCH V2 00/10] 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.

Changes in V2:
- Add Laurent's Reviewed-bys
- Drop "staging: vchiq_core: Add comments to mutex/spinlocks" and
  "staging: vchiq: Move struct vchiq_config to vchiq.h" from the series
  this will be addressed in a different series
- Rework patch #1 to use a common return code variable
- Rework patch #7 to avoid goto
- Add patch #10 to fix scatter-gather casting

Stefan Wahren (10):
  staging: vchiq_arm: Unify return code variable
  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_core: Add hex prefix to debugfs output
  staging: vchiq_arm: Don't cast scatter-gather elements
  staging: vc04_services: Update testing instructions

 .../staging/vc04_services/interface/TESTING   |  45 +++-
 .../interface/vchiq_arm/vchiq_arm.c           | 254 ++++++++----------
 .../interface/vchiq_arm/vchiq_arm.h           |   3 -
 .../interface/vchiq_arm/vchiq_core.c          |  10 +-
 .../interface/vchiq_arm/vchiq_core.h          |   6 -
 5 files changed, 167 insertions(+), 151 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 V2 01/10] staging: vchiq_arm: Unify return code variable

[Stefan Wahren]

The file vchiq_arm uses a wild mixture of variable names for
return codes. Unify them by using the common name "ret".

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 141 +++++++++---------
 1 file changed, 69 insertions(+), 72 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..f3815101e1c4 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -758,8 +758,8 @@ void free_bulk_waiter(struct vchiq_instance *instance)

 int vchiq_shutdown(struct vchiq_instance *instance)
 {
-	int status = 0;
 	struct vchiq_state *state = instance->state;
+	int ret = 0;

 	if (mutex_lock_killable(&state->mutex))
 		return -EAGAIN;
@@ -769,12 +769,12 @@ int vchiq_shutdown(struct vchiq_instance *instance)

 	mutex_unlock(&state->mutex);

-	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, status);
+	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, ret);

 	free_bulk_waiter(instance);
 	kfree(instance);

-	return status;
+	return ret;
 }
 EXPORT_SYMBOL(vchiq_shutdown);

@@ -785,26 +785,26 @@ static int vchiq_is_connected(struct vchiq_instance *instance)

 int vchiq_connect(struct vchiq_instance *instance)
 {
-	int status;
 	struct vchiq_state *state = instance->state;
+	int ret;

 	if (mutex_lock_killable(&state->mutex)) {
 		dev_dbg(state->dev,
 			"core: call to mutex_lock failed\n");
-		status = -EAGAIN;
+		ret = -EAGAIN;
 		goto failed;
 	}
-	status = vchiq_connect_internal(state, instance);
+	ret = vchiq_connect_internal(state, instance);

-	if (!status)
+	if (!ret)
 		instance->connected = 1;

 	mutex_unlock(&state->mutex);

 failed:
-	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, status);
+	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, ret);

-	return status;
+	return ret;
 }
 EXPORT_SYMBOL(vchiq_connect);

@@ -813,10 +813,9 @@ vchiq_add_service(struct vchiq_instance *instance,
 		  const struct vchiq_service_params_kernel *params,
 		  unsigned int *phandle)
 {
-	int status;
 	struct vchiq_state *state = instance->state;
 	struct vchiq_service *service = NULL;
-	int srvstate;
+	int srvstate, ret;

 	*phandle = VCHIQ_SERVICE_HANDLE_INVALID;

@@ -828,14 +827,14 @@ vchiq_add_service(struct vchiq_instance *instance,

 	if (service) {
 		*phandle = service->handle;
-		status = 0;
+		ret = 0;
 	} else {
-		status = -EINVAL;
+		ret = -EINVAL;
 	}

-	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, status);
+	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, ret);

-	return status;
+	return ret;
 }

 int
@@ -843,9 +842,9 @@ vchiq_open_service(struct vchiq_instance *instance,
 		   const struct vchiq_service_params_kernel *params,
 		   unsigned int *phandle)
 {
-	int status = -EINVAL;
 	struct vchiq_state   *state = instance->state;
 	struct vchiq_service *service = NULL;
+	int ret = -EINVAL;

 	*phandle = VCHIQ_SERVICE_HANDLE_INVALID;

@@ -856,17 +855,17 @@ vchiq_open_service(struct vchiq_instance *instance,

 	if (service) {
 		*phandle = service->handle;
-		status = vchiq_open_service_internal(service, current->pid);
-		if (status) {
+		ret = vchiq_open_service_internal(service, current->pid);
+		if (ret) {
 			vchiq_remove_service(instance, service->handle);
 			*phandle = VCHIQ_SERVICE_HANDLE_INVALID;
 		}
 	}

 failed:
-	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, status);
+	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, ret);

-	return status;
+	return ret;
 }
 EXPORT_SYMBOL(vchiq_open_service);

@@ -874,20 +873,20 @@ int
 vchiq_bulk_transmit(struct vchiq_instance *instance, unsigned int handle, const void *data,
 		    unsigned int size, void *userdata, enum vchiq_bulk_mode mode)
 {
-	int status;
+	int ret;

 	while (1) {
 		switch (mode) {
 		case VCHIQ_BULK_MODE_NOCALLBACK:
 		case VCHIQ_BULK_MODE_CALLBACK:
-			status = vchiq_bulk_transfer(instance, handle,
-						     (void *)data, NULL,
-						     size, userdata, mode,
-						     VCHIQ_BULK_TRANSMIT);
+			ret = vchiq_bulk_transfer(instance, handle,
+						  (void *)data, NULL,
+						  size, userdata, mode,
+						  VCHIQ_BULK_TRANSMIT);
 			break;
 		case VCHIQ_BULK_MODE_BLOCKING:
-			status = vchiq_blocking_bulk_transfer(instance, handle, (void *)data, size,
-							      VCHIQ_BULK_TRANSMIT);
+			ret = vchiq_blocking_bulk_transfer(instance, handle, (void *)data, size,
+							   VCHIQ_BULK_TRANSMIT);
 			break;
 		default:
 			return -EINVAL;
@@ -898,13 +897,13 @@ vchiq_bulk_transmit(struct vchiq_instance *instance, unsigned int handle, const
 		 * to implement a retry mechanism since this function is
 		 * supposed to block until queued
 		 */
-		if (status != -EAGAIN)
+		if (ret != -EAGAIN)
 			break;

 		msleep(1);
 	}

-	return status;
+	return ret;
 }
 EXPORT_SYMBOL(vchiq_bulk_transmit);

@@ -912,19 +911,19 @@ int vchiq_bulk_receive(struct vchiq_instance *instance, unsigned int handle,
 		       void *data, unsigned int size, void *userdata,
 		       enum vchiq_bulk_mode mode)
 {
-	int status;
+	int ret;

 	while (1) {
 		switch (mode) {
 		case VCHIQ_BULK_MODE_NOCALLBACK:
 		case VCHIQ_BULK_MODE_CALLBACK:
-			status = vchiq_bulk_transfer(instance, handle, data, NULL,
-						     size, userdata,
-						     mode, VCHIQ_BULK_RECEIVE);
+			ret = vchiq_bulk_transfer(instance, handle, data, NULL,
+						  size, userdata,
+						  mode, VCHIQ_BULK_RECEIVE);
 			break;
 		case VCHIQ_BULK_MODE_BLOCKING:
-			status = vchiq_blocking_bulk_transfer(instance, handle, (void *)data, size,
-							      VCHIQ_BULK_RECEIVE);
+			ret = vchiq_blocking_bulk_transfer(instance, handle, (void *)data, size,
+							   VCHIQ_BULK_RECEIVE);
 			break;
 		default:
 			return -EINVAL;
@@ -935,13 +934,13 @@ int vchiq_bulk_receive(struct vchiq_instance *instance, unsigned int handle,
 		 * to implement a retry mechanism since this function is
 		 * supposed to block until queued
 		 */
-		if (status != -EAGAIN)
+		if (ret != -EAGAIN)
 			break;

 		msleep(1);
 	}

-	return status;
+	return ret;
 }
 EXPORT_SYMBOL(vchiq_bulk_receive);

@@ -950,8 +949,8 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
 			     unsigned int size, enum vchiq_bulk_dir dir)
 {
 	struct vchiq_service *service;
-	int status;
 	struct bulk_waiter_node *waiter = NULL, *iter;
+	int ret;

 	service = find_service_by_handle(instance, handle);
 	if (!service)
@@ -991,10 +990,10 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
 			return -ENOMEM;
 	}

-	status = vchiq_bulk_transfer(instance, handle, data, NULL, size,
-				     &waiter->bulk_waiter,
-				     VCHIQ_BULK_MODE_BLOCKING, dir);
-	if ((status != -EAGAIN) || fatal_signal_pending(current) || !waiter->bulk_waiter.bulk) {
+	ret = vchiq_bulk_transfer(instance, handle, data, NULL, size,
+				  &waiter->bulk_waiter,
+				  VCHIQ_BULK_MODE_BLOCKING, dir);
+	if ((ret != -EAGAIN) || fatal_signal_pending(current) || !waiter->bulk_waiter.bulk) {
 		struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;

 		if (bulk) {
@@ -1013,7 +1012,7 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
 			waiter, current->pid);
 	}

-	return status;
+	return ret;
 }

 static int
@@ -1137,17 +1136,17 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
 			 */
 			if ((user_service->message_available_pos -
 				instance->completion_remove) < 0) {
-				int status;
+				int ret;

 				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) {
+				ret = add_completion(instance, reason, NULL, user_service,
+						     bulk_userdata);
+				if (ret) {
 					DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 					vchiq_service_put(service);
-					return status;
+					return ret;
 				}
 			}

@@ -1294,8 +1293,6 @@ vchiq_keepalive_thread_func(void *v)
 {
 	struct vchiq_state *state = (struct vchiq_state *)v;
 	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
-
-	int status;
 	struct vchiq_instance *instance;
 	unsigned int ka_handle;
 	int ret;
@@ -1313,16 +1310,16 @@ vchiq_keepalive_thread_func(void *v)
 		goto exit;
 	}

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

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

@@ -1348,17 +1345,17 @@ vchiq_keepalive_thread_func(void *v)
 		 */
 		while (uc--) {
 			atomic_inc(&arm_state->ka_use_ack_count);
-			status = vchiq_use_service(instance, ka_handle);
-			if (status) {
+			ret = vchiq_use_service(instance, ka_handle);
+			if (ret) {
 				dev_err(state->dev, "suspend: %s: vchiq_use_service error %d\n",
-					__func__, status);
+					__func__, ret);
 			}
 		}
 		while (rc--) {
-			status = vchiq_release_service(instance, ka_handle);
-			if (status) {
+			ret = vchiq_release_service(instance, ka_handle);
+			if (ret) {
 				dev_err(state->dev, "suspend: %s: vchiq_release_service error %d\n",
-					__func__, status);
+					__func__, ret);
 			}
 		}
 	}
@@ -1408,13 +1405,13 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
 	write_unlock_bh(&arm_state->susp_res_lock);

 	if (!ret) {
-		int status = 0;
+		int ret = 0;
 		long ack_cnt = atomic_xchg(&arm_state->ka_use_ack_count, 0);

-		while (ack_cnt && !status) {
+		while (ack_cnt && !ret) {
 			/* Send the use notify to videocore */
-			status = vchiq_send_remote_use_active(state);
-			if (!status)
+			ret = vchiq_send_remote_use_active(state);
+			if (!ret)
 				ack_cnt--;
 			else
 				atomic_add(ack_cnt, &arm_state->ka_use_ack_count);
@@ -1730,7 +1727,7 @@ static int vchiq_probe(struct platform_device *pdev)
 	struct device_node *fw_node;
 	const struct vchiq_platform_info *info;
 	struct vchiq_drv_mgmt *mgmt;
-	int err;
+	int ret;

 	info = of_device_get_match_data(&pdev->dev);
 	if (!info)
@@ -1755,8 +1752,8 @@ static int vchiq_probe(struct platform_device *pdev)
 	mgmt->info = info;
 	platform_set_drvdata(pdev, mgmt);

-	err = vchiq_platform_init(pdev, &mgmt->state);
-	if (err)
+	ret = vchiq_platform_init(pdev, &mgmt->state);
+	if (ret)
 		goto failed_platform_init;

 	vchiq_debugfs_init(&mgmt->state);
@@ -1768,8 +1765,8 @@ static int vchiq_probe(struct platform_device *pdev)
 	 * Simply exit on error since the function handles cleanup in
 	 * cases of failure.
 	 */
-	err = vchiq_register_chrdev(&pdev->dev);
-	if (err) {
+	ret = vchiq_register_chrdev(&pdev->dev);
+	if (ret) {
 		dev_err(&pdev->dev, "arm: Failed to initialize vchiq cdev\n");
 		goto error_exit;
 	}
@@ -1782,7 +1779,7 @@ static int vchiq_probe(struct platform_device *pdev)
 failed_platform_init:
 	dev_err(&pdev->dev, "arm: Could not initialize vchiq platform\n");
 error_exit:
-	return err;
+	return ret;
 }

 static void vchiq_remove(struct platform_device *pdev)
--
2.34.1


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

[PATCH V2 02/10] 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index f3815101e1c4..094ba1aea8f6 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1448,7 +1448,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 V2 03/10] 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>
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;

--
2.34.1


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

[PATCH V2 04/10] 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>
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);
--
2.34.1


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

[PATCH V2 05/10] 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. 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 094ba1aea8f6..6ef4cf702c78 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 V2 06/10] 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>
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 6ef4cf702c78..0ba52c9d8bc3 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 V2 07/10] 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 splitting the code in a new function
service_single_message() as suggested by Laurent Pinchart.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 68 +++++++++++--------
 1 file changed, 39 insertions(+), 29 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 0ba52c9d8bc3..706bfc7a0b90 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1055,6 +1055,39 @@ add_completion(struct vchiq_instance *instance, enum vchiq_reason reason,
 	return 0;
 }

+static int
+service_single_message(struct vchiq_instance *instance,
+		       enum vchiq_reason reason,
+		       struct vchiq_service *service, void *bulk_userdata)
+{
+	struct user_service *user_service;
+
+	user_service = (struct user_service *)service->base.userdata;
+
+	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) {
+		dev_dbg(instance->state->dev,
+			"arm: Inserting extra MESSAGE_AVAILABLE\n");
+		return add_completion(instance, reason, NULL, user_service,
+				      bulk_userdata);
+	}
+
+	if (wait_for_completion_interruptible(&user_service->remove_event)) {
+		dev_dbg(instance->state->dev, "arm: interrupted\n");
+		return -EAGAIN;
+	} else if (instance->closing) {
+		dev_dbg(instance->state->dev, "arm: closing\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 int
 service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
 		 struct vchiq_header *header, unsigned int handle, void *bulk_userdata)
@@ -1104,41 +1137,18 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
 		spin_lock(&service->state->msg_queue_spinlock);
 		while (user_service->msg_insert ==
 			(user_service->msg_remove + MSG_QUEUE_SIZE)) {
+			int ret;
+
 			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 ret;
-
-				dev_dbg(instance->state->dev,
-					"arm: Inserting extra MESSAGE_AVAILABLE\n");
-				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
-				ret = add_completion(instance, reason, NULL, user_service,
-						     bulk_userdata);
-				if (ret) {
-					DEBUG_TRACE(SERVICE_CALLBACK_LINE);
-					vchiq_service_put(service);
-					return ret;
-				}
-			}

-			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");
+			ret = service_single_message(instance, reason,
+						     service, bulk_userdata);
+			if (ret) {
 				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 				vchiq_service_put(service);
-				return -EINVAL;
+				return ret;
 			}
 			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 			spin_lock(&service->state->msg_queue_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 V2 08/10] 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>
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,
--
2.34.1


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

[PATCH V2 09/10] staging: vchiq_arm: Don't cast scatter-gather elements

[Stefan Wahren]

The kernel uses different types for DMA length & address,
so better use them.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c   | 6 +++---
 1 file changed, 3 insertions(+), 3 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 706bfc7a0b90..02392073c4aa 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -162,7 +162,7 @@ cleanup_pagelistinfo(struct vchiq_instance *instance, struct vchiq_pagelist_info
 }

 static inline bool
-is_adjacent_block(u32 *addrs, u32 addr, unsigned int k)
+is_adjacent_block(u32 *addrs, dma_addr_t addr, unsigned int k)
 {
 	u32 tmp;

@@ -377,8 +377,8 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
 	/* Combine adjacent blocks for performance */
 	k = 0;
 	for_each_sg(scatterlist, sg, dma_buffers, i) {
-		u32 len = sg_dma_len(sg);
-		u32 addr = sg_dma_address(sg);
+		unsigned int len = sg_dma_len(sg);
+		dma_addr_t addr = sg_dma_address(sg);

 		/* Note: addrs is the address + page_count - 1
 		 * The firmware expects blocks after the first to be page-
--
2.34.1


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

[PATCH V2 10/10] 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 V2 07/10] staging: vchiq_arm: Reduce indentation of service_callback

[Dan Carpenter]

On Mon, Jun 10, 2024 at 11:02:17PM +0200, Stefan Wahren wrote:
> The service_callback has 5 levels of indentation, which makes it
> hard to read. Reduce this by splitting the code in a new function
> service_single_message() as suggested by Laurent Pinchart.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>  .../interface/vchiq_arm/vchiq_arm.c           | 68 +++++++++++--------
>  1 file changed, 39 insertions(+), 29 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 0ba52c9d8bc3..706bfc7a0b90 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1055,6 +1055,39 @@ add_completion(struct vchiq_instance *instance, enum vchiq_reason reason,
>  	return 0;
>  }
> 
> +static int
> +service_single_message(struct vchiq_instance *instance,
> +		       enum vchiq_reason reason,
> +		       struct vchiq_service *service, void *bulk_userdata)
> +{
> +	struct user_service *user_service;
> +
> +	user_service = (struct user_service *)service->base.userdata;
> +
> +	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) {
> +		dev_dbg(instance->state->dev,
> +			"arm: Inserting extra MESSAGE_AVAILABLE\n");
> +		return add_completion(instance, reason, NULL, user_service,
> +				      bulk_userdata);

In the original code we added a completion and

> +	}
> +
> +	if (wait_for_completion_interruptible(&user_service->remove_event)) {

then waited for it here...  Why did this change?

regards,
dan carpenter

> +		dev_dbg(instance->state->dev, "arm: interrupted\n");
> +		return -EAGAIN;
> +	} else if (instance->closing) {
> +		dev_dbg(instance->state->dev, "arm: closing\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  int
>  service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
>  		 struct vchiq_header *header, unsigned int handle, void *bulk_userdata)
> @@ -1104,41 +1137,18 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
>  		spin_lock(&service->state->msg_queue_spinlock);
>  		while (user_service->msg_insert ==
>  			(user_service->msg_remove + MSG_QUEUE_SIZE)) {
> +			int ret;
> +
>  			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 ret;
> -
> -				dev_dbg(instance->state->dev,
> -					"arm: Inserting extra MESSAGE_AVAILABLE\n");
> -				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> -				ret = add_completion(instance, reason, NULL, user_service,
> -						     bulk_userdata);
> -				if (ret) {
> -					DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> -					vchiq_service_put(service);
> -					return ret;
> -				}
> -			}
> 
> -			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");
> +			ret = service_single_message(instance, reason,
> +						     service, bulk_userdata);
> +			if (ret) {
>  				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
>  				vchiq_service_put(service);
> -				return -EINVAL;
> +				return ret;
>  			}
>  			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
>  			spin_lock(&service->state->msg_queue_spinlock);
> --
> 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 V2 07/10] staging: vchiq_arm: Reduce indentation of service_callback

[Dan Carpenter]

On Mon, Jun 10, 2024 at 11:02:17PM +0200, Stefan Wahren wrote:
> The service_callback has 5 levels of indentation, which makes it
> hard to read. Reduce this by splitting the code in a new function
> service_single_message() as suggested by Laurent Pinchart.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

To be honest, I liked the version of this patch which Laurent didn't
like.  Even after pulling this code into a separate function, I'd still
support flipping the conditions around and adding the goto...

Laurent, you really didn't like the goto?

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 V2 07/10] staging: vchiq_arm: Reduce indentation of service_callback

[Stefan Wahren]

Hi Dan,

Am 11.06.24 um 08:01 schrieb Dan Carpenter:
> On Mon, Jun 10, 2024 at 11:02:17PM +0200, Stefan Wahren wrote:
>> The service_callback has 5 levels of indentation, which makes it
>> hard to read. Reduce this by splitting the code in a new function
>> service_single_message() as suggested by Laurent Pinchart.
>>
>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>> ---
>>   .../interface/vchiq_arm/vchiq_arm.c           | 68 +++++++++++--------
>>   1 file changed, 39 insertions(+), 29 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 0ba52c9d8bc3..706bfc7a0b90 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -1055,6 +1055,39 @@ add_completion(struct vchiq_instance *instance, enum vchiq_reason reason,
>>   	return 0;
>>   }
>>
>> +static int
>> +service_single_message(struct vchiq_instance *instance,
>> +		       enum vchiq_reason reason,
>> +		       struct vchiq_service *service, void *bulk_userdata)
>> +{
>> +	struct user_service *user_service;
>> +
>> +	user_service = (struct user_service *)service->base.userdata;
>> +
>> +	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) {
>> +		dev_dbg(instance->state->dev,
>> +			"arm: Inserting extra MESSAGE_AVAILABLE\n");
>> +		return add_completion(instance, reason, NULL, user_service,
>> +				      bulk_userdata);
> In the original code we added a completion and
>
>> +	}
>> +
>> +	if (wait_for_completion_interruptible(&user_service->remove_event)) {
> then waited for it here...  Why did this change?
Oops, this wasn't intended. Thanks for catching this.
>
> regards,
> dan carpenter
>
>> +		dev_dbg(instance->state->dev, "arm: interrupted\n");
>> +		return -EAGAIN;
>> +	} else if (instance->closing) {
>> +		dev_dbg(instance->state->dev, "arm: closing\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   int
>>   service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
>>   		 struct vchiq_header *header, unsigned int handle, void *bulk_userdata)
>> @@ -1104,41 +1137,18 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
>>   		spin_lock(&service->state->msg_queue_spinlock);
>>   		while (user_service->msg_insert ==
>>   			(user_service->msg_remove + MSG_QUEUE_SIZE)) {
>> +			int ret;
>> +
>>   			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 ret;
>> -
>> -				dev_dbg(instance->state->dev,
>> -					"arm: Inserting extra MESSAGE_AVAILABLE\n");
>> -				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
>> -				ret = add_completion(instance, reason, NULL, user_service,
>> -						     bulk_userdata);
>> -				if (ret) {
>> -					DEBUG_TRACE(SERVICE_CALLBACK_LINE);
>> -					vchiq_service_put(service);
>> -					return ret;
>> -				}
>> -			}
>>
>> -			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");
>> +			ret = service_single_message(instance, reason,
>> +						     service, bulk_userdata);
>> +			if (ret) {
>>   				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
>>   				vchiq_service_put(service);
>> -				return -EINVAL;
>> +				return ret;
>>   			}
>>   			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
>>   			spin_lock(&service->state->msg_queue_spinlock);
>> --
>> 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 V2 01/10] staging: vchiq_arm: Unify return code variable

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Mon, Jun 10, 2024 at 11:02:11PM +0200, Stefan Wahren wrote:
> The file vchiq_arm uses a wild mixture of variable names for
> return codes. Unify them by using the common name "ret".
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

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

> ---
>  .../interface/vchiq_arm/vchiq_arm.c           | 141 +++++++++---------
>  1 file changed, 69 insertions(+), 72 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..f3815101e1c4 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -758,8 +758,8 @@ void free_bulk_waiter(struct vchiq_instance *instance)
> 
>  int vchiq_shutdown(struct vchiq_instance *instance)
>  {
> -	int status = 0;
>  	struct vchiq_state *state = instance->state;
> +	int ret = 0;
> 
>  	if (mutex_lock_killable(&state->mutex))
>  		return -EAGAIN;
> @@ -769,12 +769,12 @@ int vchiq_shutdown(struct vchiq_instance *instance)
> 
>  	mutex_unlock(&state->mutex);
> 
> -	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, status);
> +	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, ret);
> 
>  	free_bulk_waiter(instance);
>  	kfree(instance);
> 
> -	return status;
> +	return ret;
>  }
>  EXPORT_SYMBOL(vchiq_shutdown);
> 
> @@ -785,26 +785,26 @@ static int vchiq_is_connected(struct vchiq_instance *instance)
> 
>  int vchiq_connect(struct vchiq_instance *instance)
>  {
> -	int status;
>  	struct vchiq_state *state = instance->state;
> +	int ret;
> 
>  	if (mutex_lock_killable(&state->mutex)) {
>  		dev_dbg(state->dev,
>  			"core: call to mutex_lock failed\n");
> -		status = -EAGAIN;
> +		ret = -EAGAIN;
>  		goto failed;
>  	}
> -	status = vchiq_connect_internal(state, instance);
> +	ret = vchiq_connect_internal(state, instance);
> 
> -	if (!status)
> +	if (!ret)
>  		instance->connected = 1;
> 
>  	mutex_unlock(&state->mutex);
> 
>  failed:
> -	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, status);
> +	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, ret);
> 
> -	return status;
> +	return ret;
>  }
>  EXPORT_SYMBOL(vchiq_connect);
> 
> @@ -813,10 +813,9 @@ vchiq_add_service(struct vchiq_instance *instance,
>  		  const struct vchiq_service_params_kernel *params,
>  		  unsigned int *phandle)
>  {
> -	int status;
>  	struct vchiq_state *state = instance->state;
>  	struct vchiq_service *service = NULL;
> -	int srvstate;
> +	int srvstate, ret;
> 
>  	*phandle = VCHIQ_SERVICE_HANDLE_INVALID;
> 
> @@ -828,14 +827,14 @@ vchiq_add_service(struct vchiq_instance *instance,
> 
>  	if (service) {
>  		*phandle = service->handle;
> -		status = 0;
> +		ret = 0;
>  	} else {
> -		status = -EINVAL;
> +		ret = -EINVAL;
>  	}
> 
> -	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, status);
> +	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, ret);
> 
> -	return status;
> +	return ret;
>  }
> 
>  int
> @@ -843,9 +842,9 @@ vchiq_open_service(struct vchiq_instance *instance,
>  		   const struct vchiq_service_params_kernel *params,
>  		   unsigned int *phandle)
>  {
> -	int status = -EINVAL;
>  	struct vchiq_state   *state = instance->state;
>  	struct vchiq_service *service = NULL;
> +	int ret = -EINVAL;
> 
>  	*phandle = VCHIQ_SERVICE_HANDLE_INVALID;
> 
> @@ -856,17 +855,17 @@ vchiq_open_service(struct vchiq_instance *instance,
> 
>  	if (service) {
>  		*phandle = service->handle;
> -		status = vchiq_open_service_internal(service, current->pid);
> -		if (status) {
> +		ret = vchiq_open_service_internal(service, current->pid);
> +		if (ret) {
>  			vchiq_remove_service(instance, service->handle);
>  			*phandle = VCHIQ_SERVICE_HANDLE_INVALID;
>  		}
>  	}
> 
>  failed:
> -	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, status);
> +	dev_dbg(state->dev, "core: (%p): returning %d\n", instance, ret);
> 
> -	return status;
> +	return ret;
>  }
>  EXPORT_SYMBOL(vchiq_open_service);
> 
> @@ -874,20 +873,20 @@ int
>  vchiq_bulk_transmit(struct vchiq_instance *instance, unsigned int handle, const void *data,
>  		    unsigned int size, void *userdata, enum vchiq_bulk_mode mode)
>  {
> -	int status;
> +	int ret;
> 
>  	while (1) {
>  		switch (mode) {
>  		case VCHIQ_BULK_MODE_NOCALLBACK:
>  		case VCHIQ_BULK_MODE_CALLBACK:
> -			status = vchiq_bulk_transfer(instance, handle,
> -						     (void *)data, NULL,
> -						     size, userdata, mode,
> -						     VCHIQ_BULK_TRANSMIT);
> +			ret = vchiq_bulk_transfer(instance, handle,
> +						  (void *)data, NULL,
> +						  size, userdata, mode,
> +						  VCHIQ_BULK_TRANSMIT);
>  			break;
>  		case VCHIQ_BULK_MODE_BLOCKING:
> -			status = vchiq_blocking_bulk_transfer(instance, handle, (void *)data, size,
> -							      VCHIQ_BULK_TRANSMIT);
> +			ret = vchiq_blocking_bulk_transfer(instance, handle, (void *)data, size,
> +							   VCHIQ_BULK_TRANSMIT);
>  			break;
>  		default:
>  			return -EINVAL;
> @@ -898,13 +897,13 @@ vchiq_bulk_transmit(struct vchiq_instance *instance, unsigned int handle, const
>  		 * to implement a retry mechanism since this function is
>  		 * supposed to block until queued
>  		 */
> -		if (status != -EAGAIN)
> +		if (ret != -EAGAIN)
>  			break;
> 
>  		msleep(1);
>  	}
> 
> -	return status;
> +	return ret;
>  }
>  EXPORT_SYMBOL(vchiq_bulk_transmit);
> 
> @@ -912,19 +911,19 @@ int vchiq_bulk_receive(struct vchiq_instance *instance, unsigned int handle,
>  		       void *data, unsigned int size, void *userdata,
>  		       enum vchiq_bulk_mode mode)
>  {
> -	int status;
> +	int ret;
> 
>  	while (1) {
>  		switch (mode) {
>  		case VCHIQ_BULK_MODE_NOCALLBACK:
>  		case VCHIQ_BULK_MODE_CALLBACK:
> -			status = vchiq_bulk_transfer(instance, handle, data, NULL,
> -						     size, userdata,
> -						     mode, VCHIQ_BULK_RECEIVE);
> +			ret = vchiq_bulk_transfer(instance, handle, data, NULL,
> +						  size, userdata,
> +						  mode, VCHIQ_BULK_RECEIVE);
>  			break;
>  		case VCHIQ_BULK_MODE_BLOCKING:
> -			status = vchiq_blocking_bulk_transfer(instance, handle, (void *)data, size,
> -							      VCHIQ_BULK_RECEIVE);
> +			ret = vchiq_blocking_bulk_transfer(instance, handle, (void *)data, size,
> +							   VCHIQ_BULK_RECEIVE);
>  			break;
>  		default:
>  			return -EINVAL;
> @@ -935,13 +934,13 @@ int vchiq_bulk_receive(struct vchiq_instance *instance, unsigned int handle,
>  		 * to implement a retry mechanism since this function is
>  		 * supposed to block until queued
>  		 */
> -		if (status != -EAGAIN)
> +		if (ret != -EAGAIN)
>  			break;
> 
>  		msleep(1);
>  	}
> 
> -	return status;
> +	return ret;
>  }
>  EXPORT_SYMBOL(vchiq_bulk_receive);
> 
> @@ -950,8 +949,8 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
>  			     unsigned int size, enum vchiq_bulk_dir dir)
>  {
>  	struct vchiq_service *service;
> -	int status;
>  	struct bulk_waiter_node *waiter = NULL, *iter;
> +	int ret;
> 
>  	service = find_service_by_handle(instance, handle);
>  	if (!service)
> @@ -991,10 +990,10 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
>  			return -ENOMEM;
>  	}
> 
> -	status = vchiq_bulk_transfer(instance, handle, data, NULL, size,
> -				     &waiter->bulk_waiter,
> -				     VCHIQ_BULK_MODE_BLOCKING, dir);
> -	if ((status != -EAGAIN) || fatal_signal_pending(current) || !waiter->bulk_waiter.bulk) {
> +	ret = vchiq_bulk_transfer(instance, handle, data, NULL, size,
> +				  &waiter->bulk_waiter,
> +				  VCHIQ_BULK_MODE_BLOCKING, dir);
> +	if ((ret != -EAGAIN) || fatal_signal_pending(current) || !waiter->bulk_waiter.bulk) {
>  		struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;
> 
>  		if (bulk) {
> @@ -1013,7 +1012,7 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
>  			waiter, current->pid);
>  	}
> 
> -	return status;
> +	return ret;
>  }
> 
>  static int
> @@ -1137,17 +1136,17 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
>  			 */
>  			if ((user_service->message_available_pos -
>  				instance->completion_remove) < 0) {
> -				int status;
> +				int ret;
> 
>  				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) {
> +				ret = add_completion(instance, reason, NULL, user_service,
> +						     bulk_userdata);
> +				if (ret) {
>  					DEBUG_TRACE(SERVICE_CALLBACK_LINE);
>  					vchiq_service_put(service);
> -					return status;
> +					return ret;
>  				}
>  			}
> 
> @@ -1294,8 +1293,6 @@ vchiq_keepalive_thread_func(void *v)
>  {
>  	struct vchiq_state *state = (struct vchiq_state *)v;
>  	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
> -
> -	int status;
>  	struct vchiq_instance *instance;
>  	unsigned int ka_handle;
>  	int ret;
> @@ -1313,16 +1310,16 @@ vchiq_keepalive_thread_func(void *v)
>  		goto exit;
>  	}
> 
> -	status = vchiq_connect(instance);
> -	if (status) {
> -		dev_err(state->dev, "suspend: %s: vchiq_connect failed %d\n", __func__, status);
> +	ret = vchiq_connect(instance);
> +	if (ret) {
> +		dev_err(state->dev, "suspend: %s: vchiq_connect failed %d\n", __func__, ret);
>  		goto shutdown;
>  	}
> 
> -	status = vchiq_add_service(instance, &params, &ka_handle);
> -	if (status) {
> +	ret = vchiq_add_service(instance, &params, &ka_handle);
> +	if (ret) {
>  		dev_err(state->dev, "suspend: %s: vchiq_open_service failed %d\n",
> -			__func__, status);
> +			__func__, ret);
>  		goto shutdown;
>  	}
> 
> @@ -1348,17 +1345,17 @@ vchiq_keepalive_thread_func(void *v)
>  		 */
>  		while (uc--) {
>  			atomic_inc(&arm_state->ka_use_ack_count);
> -			status = vchiq_use_service(instance, ka_handle);
> -			if (status) {
> +			ret = vchiq_use_service(instance, ka_handle);
> +			if (ret) {
>  				dev_err(state->dev, "suspend: %s: vchiq_use_service error %d\n",
> -					__func__, status);
> +					__func__, ret);
>  			}
>  		}
>  		while (rc--) {
> -			status = vchiq_release_service(instance, ka_handle);
> -			if (status) {
> +			ret = vchiq_release_service(instance, ka_handle);
> +			if (ret) {
>  				dev_err(state->dev, "suspend: %s: vchiq_release_service error %d\n",
> -					__func__, status);
> +					__func__, ret);
>  			}
>  		}
>  	}
> @@ -1408,13 +1405,13 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
>  	write_unlock_bh(&arm_state->susp_res_lock);
> 
>  	if (!ret) {
> -		int status = 0;
> +		int ret = 0;
>  		long ack_cnt = atomic_xchg(&arm_state->ka_use_ack_count, 0);
> 
> -		while (ack_cnt && !status) {
> +		while (ack_cnt && !ret) {
>  			/* Send the use notify to videocore */
> -			status = vchiq_send_remote_use_active(state);
> -			if (!status)
> +			ret = vchiq_send_remote_use_active(state);
> +			if (!ret)
>  				ack_cnt--;
>  			else
>  				atomic_add(ack_cnt, &arm_state->ka_use_ack_count);
> @@ -1730,7 +1727,7 @@ static int vchiq_probe(struct platform_device *pdev)
>  	struct device_node *fw_node;
>  	const struct vchiq_platform_info *info;
>  	struct vchiq_drv_mgmt *mgmt;
> -	int err;
> +	int ret;
> 
>  	info = of_device_get_match_data(&pdev->dev);
>  	if (!info)
> @@ -1755,8 +1752,8 @@ static int vchiq_probe(struct platform_device *pdev)
>  	mgmt->info = info;
>  	platform_set_drvdata(pdev, mgmt);
> 
> -	err = vchiq_platform_init(pdev, &mgmt->state);
> -	if (err)
> +	ret = vchiq_platform_init(pdev, &mgmt->state);
> +	if (ret)
>  		goto failed_platform_init;
> 
>  	vchiq_debugfs_init(&mgmt->state);
> @@ -1768,8 +1765,8 @@ static int vchiq_probe(struct platform_device *pdev)
>  	 * Simply exit on error since the function handles cleanup in
>  	 * cases of failure.
>  	 */
> -	err = vchiq_register_chrdev(&pdev->dev);
> -	if (err) {
> +	ret = vchiq_register_chrdev(&pdev->dev);
> +	if (ret) {
>  		dev_err(&pdev->dev, "arm: Failed to initialize vchiq cdev\n");
>  		goto error_exit;
>  	}
> @@ -1782,7 +1779,7 @@ static int vchiq_probe(struct platform_device *pdev)
>  failed_platform_init:
>  	dev_err(&pdev->dev, "arm: Could not initialize vchiq platform\n");
>  error_exit:
> -	return err;
> +	return ret;
>  }
> 
>  static void vchiq_remove(struct platform_device *pdev)

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Tue, Jun 11, 2024 at 09:04:28AM +0300, Dan Carpenter wrote:
> On Mon, Jun 10, 2024 at 11:02:17PM +0200, Stefan Wahren wrote:
> > The service_callback has 5 levels of indentation, which makes it
> > hard to read. Reduce this by splitting the code in a new function
> > service_single_message() as suggested by Laurent Pinchart.
> > 
> > Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> 
> To be honest, I liked the version of this patch which Laurent didn't
> like.  Even after pulling this code into a separate function, I'd still
> support flipping the conditions around and adding the goto...
> 
> Laurent, you really didn't like the goto?

I won't nack it, but I really think gotos seriously hinder readability,
with the exception of error paths.

-- 
Regards,

Laurent Pinchart

Re: [PATCH V2 09/10] staging: vchiq_arm: Don't cast scatter-gather elements

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Mon, Jun 10, 2024 at 11:02:19PM +0200, Stefan Wahren wrote:
> The kernel uses different types for DMA length & address,
> so better use them.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

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

> ---
>  .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c   | 6 +++---
>  1 file changed, 3 insertions(+), 3 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 706bfc7a0b90..02392073c4aa 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -162,7 +162,7 @@ cleanup_pagelistinfo(struct vchiq_instance *instance, struct vchiq_pagelist_info
>  }
> 
>  static inline bool
> -is_adjacent_block(u32 *addrs, u32 addr, unsigned int k)
> +is_adjacent_block(u32 *addrs, dma_addr_t addr, unsigned int k)
>  {
>  	u32 tmp;
> 
> @@ -377,8 +377,8 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
>  	/* Combine adjacent blocks for performance */
>  	k = 0;
>  	for_each_sg(scatterlist, sg, dma_buffers, i) {
> -		u32 len = sg_dma_len(sg);
> -		u32 addr = sg_dma_address(sg);
> +		unsigned int len = sg_dma_len(sg);
> +		dma_addr_t addr = sg_dma_address(sg);
> 
>  		/* Note: addrs is the address + page_count - 1
>  		 * The firmware expects blocks after the first to be page-

-- 
Regards,

Laurent Pinchart

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

[Stefan Wahren]

Hi,

Am 11.06.24 um 23:23 schrieb Laurent Pinchart:
> On Tue, Jun 11, 2024 at 09:04:28AM +0300, Dan Carpenter wrote:
>> On Mon, Jun 10, 2024 at 11:02:17PM +0200, Stefan Wahren wrote:
>>> The service_callback has 5 levels of indentation, which makes it
>>> hard to read. Reduce this by splitting the code in a new function
>>> service_single_message() as suggested by Laurent Pinchart.
>>>
>>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>> To be honest, I liked the version of this patch which Laurent didn't
>> like.  Even after pulling this code into a separate function, I'd still
>> support flipping the conditions around and adding the goto...
>>
>> Laurent, you really didn't like the goto?
> I won't nack it, but I really think gotos seriously hinder readability,
> with the exception of error paths.
>
so at least you both are fine with the split approach here (assuming the
Dan's comment will be fixed in V3)?

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

[Dan Carpenter]

On Wed, Jun 12, 2024 at 12:23:47AM +0300, Laurent Pinchart wrote:
> On Tue, Jun 11, 2024 at 09:04:28AM +0300, Dan Carpenter wrote:
> > On Mon, Jun 10, 2024 at 11:02:17PM +0200, Stefan Wahren wrote:
> > > The service_callback has 5 levels of indentation, which makes it
> > > hard to read. Reduce this by splitting the code in a new function
> > > service_single_message() as suggested by Laurent Pinchart.
> > > 
> > > Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> > 
> > To be honest, I liked the version of this patch which Laurent didn't
> > like.  Even after pulling this code into a separate function, I'd still
> > support flipping the conditions around and adding the goto...
> > 
> > Laurent, you really didn't like the goto?
> 
> I won't nack it, but I really think gotos seriously hinder readability,
> with the exception of error paths.

Hm...  I guess I only hate backwards gotos (ones that go up instead of
down).  To me this one is kind of like a goto unlock.  It's slightly
more complicated than just an unlock because we add a completion as
well...  I don't have strong feelings about it, I was just interested.

regards,
dan carpenter

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

[Laurent Pinchart]

On Wed, Jun 12, 2024 at 07:16:16AM +0200, Stefan Wahren wrote:
> Hi,
> 
> Am 11.06.24 um 23:23 schrieb Laurent Pinchart:
> > On Tue, Jun 11, 2024 at 09:04:28AM +0300, Dan Carpenter wrote:
> >> On Mon, Jun 10, 2024 at 11:02:17PM +0200, Stefan Wahren wrote:
> >>> The service_callback has 5 levels of indentation, which makes it
> >>> hard to read. Reduce this by splitting the code in a new function
> >>> service_single_message() as suggested by Laurent Pinchart.
> >>>
> >>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> >> To be honest, I liked the version of this patch which Laurent didn't
> >> like.  Even after pulling this code into a separate function, I'd still
> >> support flipping the conditions around and adding the goto...
> >>
> >> Laurent, you really didn't like the goto?
> > I won't nack it, but I really think gotos seriously hinder readability,
> > with the exception of error paths.
> >
> so at least you both are fine with the split approach here (assuming the
> Dan's comment will be fixed in V3)?

Fine with me.

-- 
Regards,

Laurent Pinchart