[PATCH 0/8] staging: vchiq: Lower indentation at various places

[PATCH 0/8] staging: vchiq: Lower indentation at various places

[Umang Jain]

Series is attempted to fix few alignments issues.
Also, it aims to lower indentation of various nested `if` conditional
blocks without introducing any functional changes.

Umang Jain (8):
  staging: vchiq_core: Locally cache cache_line_size information
  staging: vchiq_core: Properly log dev_err()
  staging: vchiq_core: Do not log debug in a separate scope
  staging: vchiq_core: Lower indentation of a conditional block
  staging: vchiq_core: Indent copy_message_data() on a single line
  staging: vchiq_arm: Lower indentation of a conditional block
  staging: vchiq_core: Lower indentation in parse_open()
  staging: vchiq_core: Lower indentation in vchiq_close_service_internal

 .../interface/vchiq_arm/vchiq_arm.c           |  34 ++--
 .../interface/vchiq_arm/vchiq_core.c          | 185 +++++++++---------
 2 files changed, 111 insertions(+), 108 deletions(-)

-- 
2.45.2

[PATCH 1/8] staging: vchiq_core: Locally cache cache_line_size information

[Umang Jain]

Locally cache 'cache_line_size' information in a variable instead of
repeatedly accessing it from drv_mgmt->info. This helps to reflow lines
under 80 columns.

No functional change intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../interface/vchiq_arm/vchiq_core.c          | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 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 1e4b2978c186..e9b60dd8d419 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1490,6 +1490,7 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
 	size_t pagelist_size;
 	struct scatterlist *scatterlist, *sg;
 	int dma_buffers;
+	unsigned int cache_line_size;
 	dma_addr_t dma_addr;
 
 	if (count >= INT_MAX - PAGE_SIZE)
@@ -1638,10 +1639,10 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
 	}
 
 	/* Partial cache lines (fragments) require special measures */
+	cache_line_size = drv_mgmt->info->cache_line_size;
 	if ((type == PAGELIST_READ) &&
-	    ((pagelist->offset & (drv_mgmt->info->cache_line_size - 1)) ||
-	    ((pagelist->offset + pagelist->length) &
-	    (drv_mgmt->info->cache_line_size - 1)))) {
+	    ((pagelist->offset & (cache_line_size - 1)) ||
+	    ((pagelist->offset + pagelist->length) & (cache_line_size - 1)))) {
 		char *fragments;
 
 		if (down_interruptible(&drv_mgmt->free_fragments_sema)) {
@@ -1671,6 +1672,7 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
 	struct pagelist *pagelist = pagelistinfo->pagelist;
 	struct page **pages = pagelistinfo->pages;
 	unsigned int num_pages = pagelistinfo->num_pages;
+	unsigned int cache_line_size;
 
 	dev_dbg(instance->state->dev, "arm: %pK, %d\n", pagelistinfo->pagelist, actual);
 
@@ -1685,16 +1687,17 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
 	pagelistinfo->scatterlist_mapped = 0;
 
 	/* Deal with any partial cache lines (fragments) */
+	cache_line_size = drv_mgmt->info->cache_line_size;
 	if (pagelist->type >= PAGELIST_READ_WITH_FRAGMENTS && drv_mgmt->fragments_base) {
 		char *fragments = drv_mgmt->fragments_base +
 			(pagelist->type - PAGELIST_READ_WITH_FRAGMENTS) *
 			drv_mgmt->fragments_size;
 		int head_bytes, tail_bytes;
 
-		head_bytes = (drv_mgmt->info->cache_line_size - pagelist->offset) &
-			(drv_mgmt->info->cache_line_size - 1);
+		head_bytes = (cache_line_size - pagelist->offset) &
+			     (cache_line_size - 1);
 		tail_bytes = (pagelist->offset + actual) &
-			(drv_mgmt->info->cache_line_size - 1);
+			     (cache_line_size - 1);
 
 		if ((actual >= 0) && (head_bytes != 0)) {
 			if (head_bytes > actual)
@@ -1707,8 +1710,8 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
 		    (tail_bytes != 0))
 			memcpy_to_page(pages[num_pages - 1],
 				       (pagelist->offset + actual) &
-				       (PAGE_SIZE - 1) & ~(drv_mgmt->info->cache_line_size - 1),
-				       fragments + drv_mgmt->info->cache_line_size,
+				       (PAGE_SIZE - 1) & ~(cache_line_size - 1),
+				       fragments + cache_line_size,
 				       tail_bytes);
 
 		down(&drv_mgmt->free_fragments_mutex);
-- 
2.45.2

[PATCH 2/8] staging: vchiq_core: Properly log dev_err()

[Umang Jain]

Properly log a dev_err() message when the msgid is not of
VCHIQ_MSG_PADDING type. Drop 'oldmsgid' scoped variable and improve
on the error string as well.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_core.c    | 11 ++++-------
 1 file changed, 4 insertions(+), 7 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 e9b60dd8d419..1dca676186b6 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1188,13 +1188,10 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
 	header = (struct vchiq_header *)SLOT_DATA_FROM_INDEX(state,
 		local->slot_sync);
 
-	{
-		int oldmsgid = header->msgid;
-
-		if (oldmsgid != VCHIQ_MSGID_PADDING)
-			dev_err(state->dev, "core: %d: qms - msgid %x, not PADDING\n",
-				state->id, oldmsgid);
-	}
+	if (header->msgid != VCHIQ_MSGID_PADDING)
+		dev_err(state->dev,
+			"core: %d: qms - msgid %x, is not a PADDING message\n",
+			state->id, header->msgid);
 
 	dev_dbg(state->dev, "sync: %d: qms %s@%pK,%x (%d->%d)\n",
 		state->id, msg_type_str(VCHIQ_MSG_TYPE(msgid)), header, size,
-- 
2.45.2

[PATCH 3/8] staging: vchiq_core: Do not log debug in a separate scope

[Umang Jain]

Do not log a dev_dbg() with a separate scope. Drop the {..}
scope and align the dev_dbg() to make it more readable.

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_core.c            | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 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 1dca676186b6..15257cf66fa4 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -936,6 +936,7 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
 	struct vchiq_service_quota *quota = NULL;
 	struct vchiq_header *header;
 	int type = VCHIQ_MSG_TYPE(msgid);
+	int svc_fourcc;
 
 	size_t stride;
 
@@ -1128,17 +1129,13 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
 	header->msgid = msgid;
 	header->size = size;
 
-	{
-		int svc_fourcc;
-
-		svc_fourcc = service
-			? service->base.fourcc
-			: VCHIQ_MAKE_FOURCC('?', '?', '?', '?');
+	svc_fourcc = service ? service->base.fourcc
+			     : VCHIQ_MAKE_FOURCC('?', '?', '?', '?');
 
-		dev_dbg(state->dev, "core_msg: Sent Msg %s(%u) to %p4cc s:%u d:%d len:%zu\n",
-			msg_type_str(VCHIQ_MSG_TYPE(msgid)), VCHIQ_MSG_TYPE(msgid),
-			&svc_fourcc, VCHIQ_MSG_SRCPORT(msgid), VCHIQ_MSG_DSTPORT(msgid), size);
-	}
+	dev_dbg(state->dev, "core_msg: Sent Msg %s(%u) to %p4cc s:%u d:%d len:%zu\n",
+		msg_type_str(VCHIQ_MSG_TYPE(msgid)),
+		VCHIQ_MSG_TYPE(msgid), &svc_fourcc,
+		VCHIQ_MSG_SRCPORT(msgid), VCHIQ_MSG_DSTPORT(msgid), size);
 
 	/* Make sure the new header is visible to the peer. */
 	wmb();
-- 
2.45.2

[PATCH 4/8] staging: vchiq_core: Lower indentation of a conditional block

[Umang Jain]

Lower indentation of 'if (bulk->data && service->instance)'
conditional block. This is achieved introducing a early check for
(!bulk->data || !service->instance) and using a goto label 'complete'
if it evaluates to true.

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_core.c          | 61 ++++++++++---------
 1 file changed, 31 insertions(+), 30 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 15257cf66fa4..b95443043c27 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1326,44 +1326,45 @@ notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
 		struct vchiq_bulk *bulk =
 			&queue->bulks[BULK_INDEX(queue->remove)];
 
+		if (!bulk->data || !service->instance)
+			goto complete;
+
 		/*
 		 * Only generate callbacks for non-dummy bulk
 		 * requests, and non-terminated services
 		 */
-		if (bulk->data && service->instance) {
-			if (bulk->actual != VCHIQ_BULK_ACTUAL_ABORTED) {
-				if (bulk->dir == VCHIQ_BULK_TRANSMIT) {
-					VCHIQ_SERVICE_STATS_INC(service, bulk_tx_count);
-					VCHIQ_SERVICE_STATS_ADD(service, bulk_tx_bytes,
-								bulk->actual);
-				} else {
-					VCHIQ_SERVICE_STATS_INC(service, bulk_rx_count);
-					VCHIQ_SERVICE_STATS_ADD(service, bulk_rx_bytes,
-								bulk->actual);
-				}
+		if (bulk->actual != VCHIQ_BULK_ACTUAL_ABORTED) {
+			if (bulk->dir == VCHIQ_BULK_TRANSMIT) {
+				VCHIQ_SERVICE_STATS_INC(service, bulk_tx_count);
+				VCHIQ_SERVICE_STATS_ADD(service, bulk_tx_bytes,
+							bulk->actual);
 			} else {
-				VCHIQ_SERVICE_STATS_INC(service, bulk_aborted_count);
-			}
-			if (bulk->mode == VCHIQ_BULK_MODE_BLOCKING) {
-				struct bulk_waiter *waiter;
-
-				spin_lock(&service->state->bulk_waiter_spinlock);
-				waiter = bulk->userdata;
-				if (waiter) {
-					waiter->actual = bulk->actual;
-					complete(&waiter->event);
-				}
-				spin_unlock(&service->state->bulk_waiter_spinlock);
-			} else if (bulk->mode == VCHIQ_BULK_MODE_CALLBACK) {
-				enum vchiq_reason reason =
-						get_bulk_reason(bulk);
-				status = make_service_callback(service, reason,	NULL,
-							       bulk->userdata);
-				if (status == -EAGAIN)
-					break;
+				VCHIQ_SERVICE_STATS_INC(service, bulk_rx_count);
+				VCHIQ_SERVICE_STATS_ADD(service, bulk_rx_bytes,
+							bulk->actual);
 			}
+		} else {
+			VCHIQ_SERVICE_STATS_INC(service, bulk_aborted_count);
 		}
+		if (bulk->mode == VCHIQ_BULK_MODE_BLOCKING) {
+			struct bulk_waiter *waiter;
 
+			spin_lock(&service->state->bulk_waiter_spinlock);
+			waiter = bulk->userdata;
+			if (waiter) {
+				waiter->actual = bulk->actual;
+				complete(&waiter->event);
+			}
+			spin_unlock(&service->state->bulk_waiter_spinlock);
+		} else if (bulk->mode == VCHIQ_BULK_MODE_CALLBACK) {
+			enum vchiq_reason reason =
+					get_bulk_reason(bulk);
+			status = make_service_callback(service, reason,	NULL,
+						       bulk->userdata);
+			if (status == -EAGAIN)
+				break;
+		}
+complete:
 		queue->remove++;
 		complete(&service->bulk_remove_event);
 	}
-- 
2.45.2

[PATCH 5/8] staging: vchiq_core: Indent copy_message_data() on a single line

[Umang Jain]

Fix the copy_message_data() indentation in queue_message_sync().

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c   | 5 ++---
 1 file changed, 2 insertions(+), 3 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 b95443043c27..3ed949343608 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1194,9 +1194,8 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
 		state->id, msg_type_str(VCHIQ_MSG_TYPE(msgid)), header, size,
 		VCHIQ_MSG_SRCPORT(msgid), VCHIQ_MSG_DSTPORT(msgid));
 
-	callback_result =
-		copy_message_data(copy_callback, context,
-				  header->data, size);
+	callback_result = copy_message_data(copy_callback, context,
+					    header->data, size);
 
 	if (callback_result < 0) {
 		mutex_unlock(&state->slot_mutex);
-- 
2.45.2

[PATCH 6/8] staging: vchiq_arm: Lower indentation of a conditional block

[Umang Jain]

Check early if we need to allocate the bulk waiter. This helps to
improve readability and reduces the indentation of the 'if (waiter)'
conditional block.

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 34 +++++++++----------
 1 file changed, 17 insertions(+), 17 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 27ceaac8f6cc..a4a7f31b124a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -564,28 +564,28 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
 	}
 	mutex_unlock(&instance->bulk_waiter_list_mutex);
 
-	if (waiter) {
-		struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;
-
-		if (bulk) {
-			/* This thread has an outstanding bulk transfer. */
-			/* FIXME: why compare a dma address to a pointer? */
-			if ((bulk->data != (dma_addr_t)(uintptr_t)data) || (bulk->size != size)) {
-				/*
-				 * This is not a retry of the previous one.
-				 * Cancel the signal when the transfer completes.
-				 */
-				spin_lock(&service->state->bulk_waiter_spinlock);
-				bulk->userdata = NULL;
-				spin_unlock(&service->state->bulk_waiter_spinlock);
-			}
-		}
-	} else {
+	if (!waiter) {
 		waiter = kzalloc(sizeof(*waiter), GFP_KERNEL);
 		if (!waiter)
 			return -ENOMEM;
 	}
 
+	struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;
+
+	if (bulk) {
+		/* This thread has an outstanding bulk transfer. */
+		/* FIXME: why compare a dma address to a pointer? */
+		if ((bulk->data != (dma_addr_t)(uintptr_t)data) || (bulk->size != size)) {
+			/*
+			 * This is not a retry of the previous one.
+			 * Cancel the signal when the transfer completes.
+			 */
+			spin_lock(&service->state->bulk_waiter_spinlock);
+			bulk->userdata = NULL;
+			spin_unlock(&service->state->bulk_waiter_spinlock);
+		}
+	}
+
 	ret = vchiq_bulk_xfer_blocking(instance, handle, data, NULL, size,
 				       &waiter->bulk_waiter, dir);
 	if ((ret != -EAGAIN) || fatal_signal_pending(current) || !waiter->bulk_waiter.bulk) {
-- 
2.45.2

[PATCH 7/8] staging: vchiq_core: Lower indentation in parse_open()

[Umang Jain]

If the service is not in VCHIQ_SRVSTATE_LISTENING state, it is
implied that the message is dealt with and parse_open() should return.
If this is the case, simply jump the code flow to return site using
'goto done;' statement.

This helps to lower the indentation of
	if (service->srvstate == VCHIQ_SRVSTATE_LISTENING)
conditional branch.

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_core.c          | 48 ++++++++++---------
 1 file changed, 26 insertions(+), 22 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 3ed949343608..04401baee9a6 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1813,8 +1813,10 @@ static int
 parse_open(struct vchiq_state *state, struct vchiq_header *header)
 {
 	const struct vchiq_open_payload *payload;
+	struct vchiq_openack_payload ack_payload;
 	struct vchiq_service *service = NULL;
 	int msgid, size;
+	int openack_id;
 	unsigned int localport, remoteport, fourcc;
 	short version, version_min;
 
@@ -1849,34 +1851,36 @@ parse_open(struct vchiq_state *state, struct vchiq_header *header)
 	}
 	service->peer_version = version;
 
-	if (service->srvstate == VCHIQ_SRVSTATE_LISTENING) {
-		struct vchiq_openack_payload ack_payload = {
-			service->version
-		};
-		int openack_id = MAKE_OPENACK(service->localport, remoteport);
+	if (service->srvstate != VCHIQ_SRVSTATE_LISTENING)
+		goto done;
 
-		if (state->version_common <
-		    VCHIQ_VERSION_SYNCHRONOUS_MODE)
-			service->sync = 0;
+	ack_payload.version = service->version;
+	openack_id = MAKE_OPENACK(service->localport, remoteport);
 
-		/* Acknowledge the OPEN */
-		if (service->sync) {
-			if (queue_message_sync(state, NULL, openack_id, memcpy_copy_callback,
-					       &ack_payload, sizeof(ack_payload)) == -EAGAIN)
-				goto bail_not_ready;
+	if (state->version_common < VCHIQ_VERSION_SYNCHRONOUS_MODE)
+		service->sync = 0;
 
-			/* The service is now open */
-			set_service_state(service, VCHIQ_SRVSTATE_OPENSYNC);
-		} else {
-			if (queue_message(state, NULL, openack_id, memcpy_copy_callback,
-					  &ack_payload, sizeof(ack_payload), 0) == -EINTR)
-				goto bail_not_ready;
+	/* Acknowledge the OPEN */
+	if (service->sync) {
+		if (queue_message_sync(state, NULL, openack_id,
+				       memcpy_copy_callback,
+				       &ack_payload,
+				       sizeof(ack_payload)) == -EAGAIN)
+			goto bail_not_ready;
 
-			/* The service is now open */
-			set_service_state(service, VCHIQ_SRVSTATE_OPEN);
-		}
+		/* The service is now open */
+		set_service_state(service, VCHIQ_SRVSTATE_OPENSYNC);
+	} else {
+		if (queue_message(state, NULL, openack_id,
+				  memcpy_copy_callback, &ack_payload,
+				  sizeof(ack_payload), 0) == -EINTR)
+			goto bail_not_ready;
+
+		/* The service is now open */
+		set_service_state(service, VCHIQ_SRVSTATE_OPEN);
 	}
 
+done:
 	/* Success - the message has been dealt with */
 	vchiq_service_put(service);
 	return 1;
-- 
2.45.2

[PATCH 8/8] staging: vchiq_core: Lower indentation in vchiq_close_service_internal

[Umang Jain]

Reduce indentation of the conditional nesting in
vchiq_close_service_internal() switch case by checking the error paths
first and break early. This helps to reduce conditional branching and
reduce indentation levels.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_core.c          | 24 ++++++++++---------
 1 file changed, 13 insertions(+), 11 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 04401baee9a6..204d73f4e904 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3152,19 +3152,21 @@ vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
 		if (close_recvd) {
 			dev_err(state->dev, "core: (1) called in state %s\n",
 				srvstate_names[service->srvstate]);
-		} else if (is_server) {
-			if (service->srvstate == VCHIQ_SRVSTATE_LISTENING) {
-				status = -EINVAL;
-			} else {
-				service->client_id = 0;
-				service->remoteport = VCHIQ_PORT_FREE;
-				if (service->srvstate == VCHIQ_SRVSTATE_CLOSEWAIT)
-					set_service_state(service, VCHIQ_SRVSTATE_LISTENING);
-			}
-			complete(&service->remove_event);
-		} else {
+			break;
+		} else if (!is_server) {
 			vchiq_free_service_internal(service);
+			break;
+		}
+
+		if (service->srvstate == VCHIQ_SRVSTATE_LISTENING) {
+			status = -EINVAL;
+		} else {
+			service->client_id = 0;
+			service->remoteport = VCHIQ_PORT_FREE;
+			if (service->srvstate == VCHIQ_SRVSTATE_CLOSEWAIT)
+				set_service_state(service, VCHIQ_SRVSTATE_LISTENING);
 		}
+		complete(&service->remove_event);
 		break;
 	case VCHIQ_SRVSTATE_OPENING:
 		if (close_recvd) {
-- 
2.45.2

Re: [PATCH 2/8] staging: vchiq_core: Properly log dev_err()

[Stefan Wahren]

Hi Umang,

[add Raspberry Pi guys to the loop]

Am 11.10.24 um 09:22 schrieb Umang Jain:
> Properly log a dev_err() message when the msgid is not of
> VCHIQ_MSG_PADDING type. Drop 'oldmsgid' scoped variable and improve
> on the error string as well.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   .../vc04_services/interface/vchiq_arm/vchiq_core.c    | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 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 e9b60dd8d419..1dca676186b6 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -1188,13 +1188,10 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
>   	header = (struct vchiq_header *)SLOT_DATA_FROM_INDEX(state,
>   		local->slot_sync);
>
> -	{
> -		int oldmsgid = header->msgid;
> -
> -		if (oldmsgid != VCHIQ_MSGID_PADDING)
> -			dev_err(state->dev, "core: %d: qms - msgid %x, not PADDING\n",
> -				state->id, oldmsgid);
> -	}
> +	if (header->msgid != VCHIQ_MSGID_PADDING)
> +		dev_err(state->dev,
> +			"core: %d: qms - msgid %x, is not a PADDING message\n",
> +			state->id, header->msgid);
I'm fine with this change, but the behavior looks strange to me.

Either this is a real error, I would expect the function would return
with something like EINVAL here or this should be a warning?

Sorry, no idea what's correct here.

Best regards
>
>   	dev_dbg(state->dev, "sync: %d: qms %s@%pK,%x (%d->%d)\n",
>   		state->id, msg_type_str(VCHIQ_MSG_TYPE(msgid)), header, size,

Re: [PATCH 3/8] staging: vchiq_core: Do not log debug in a separate scope

[Stefan Wahren]

Am 11.10.24 um 09:22 schrieb Umang Jain:
> Do not log a dev_dbg() with a separate scope. Drop the {..}
> scope and align the dev_dbg() to make it more readable.
>
> No functional changes intended in this patch.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>

Re: [PATCH 2/8] staging: vchiq_core: Properly log dev_err()

[Umang Jain]

Hi Stefan

On 11/10/24 4:15 pm, Stefan Wahren wrote:
> Hi Umang,
>
> [add Raspberry Pi guys to the loop]
>
> Am 11.10.24 um 09:22 schrieb Umang Jain:
>> Properly log a dev_err() message when the msgid is not of
>> VCHIQ_MSG_PADDING type. Drop 'oldmsgid' scoped variable and improve
>> on the error string as well.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   .../vc04_services/interface/vchiq_arm/vchiq_core.c    | 11 ++++-------
>>   1 file changed, 4 insertions(+), 7 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 e9b60dd8d419..1dca676186b6 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> @@ -1188,13 +1188,10 @@ queue_message_sync(struct vchiq_state *state, 
>> struct vchiq_service *service,
>>       header = (struct vchiq_header *)SLOT_DATA_FROM_INDEX(state,
>>           local->slot_sync);
>>
>> -    {
>> -        int oldmsgid = header->msgid;
>> -
>> -        if (oldmsgid != VCHIQ_MSGID_PADDING)
>> -            dev_err(state->dev, "core: %d: qms - msgid %x, not 
>> PADDING\n",
>> -                state->id, oldmsgid);
>> -    }
>> +    if (header->msgid != VCHIQ_MSGID_PADDING)
>> +        dev_err(state->dev,
>> +            "core: %d: qms - msgid %x, is not a PADDING message\n",
>> +            state->id, header->msgid);
> I'm fine with this change, but the behavior looks strange to me.
>
> Either this is a real error, I would expect the function would return
> with something like EINVAL here or this should be a warning?
>
> Sorry, no idea what's correct here.

indeed, I'll check the code path and see how it is used.

However, I will propose the change (if any) on top of this series/patch. 
Since it would be functional change ... so need to document it 
appropriately.

>
> Best regards
>>
>>       dev_dbg(state->dev, "sync: %d: qms %s@%pK,%x (%d->%d)\n",
>>           state->id, msg_type_str(VCHIQ_MSG_TYPE(msgid)), header, size,
>

Re: [PATCH 4/8] staging: vchiq_core: Lower indentation of a conditional block

[Stefan Wahren]

Hi Umang,

Am 11.10.24 um 09:22 schrieb Umang Jain:
> Lower indentation of 'if (bulk->data && service->instance)'
> conditional block. This is achieved introducing a early check for
> (!bulk->data || !service->instance) and using a goto label 'complete'
> if it evaluates to true.
>
> No functional changes intended in this patch.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   .../interface/vchiq_arm/vchiq_core.c          | 61 ++++++++++---------
>   1 file changed, 31 insertions(+), 30 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 15257cf66fa4..b95443043c27 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -1326,44 +1326,45 @@ notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
>   		struct vchiq_bulk *bulk =
>   			&queue->bulks[BULK_INDEX(queue->remove)];
>
> +		if (!bulk->data || !service->instance)
> +			goto complete;
> +
>   		/*
>   		 * Only generate callbacks for non-dummy bulk
>   		 * requests, and non-terminated services
>   		 */
> -		if (bulk->data && service->instance) {
> -			if (bulk->actual != VCHIQ_BULK_ACTUAL_ABORTED) {
> -				if (bulk->dir == VCHIQ_BULK_TRANSMIT) {
> -					VCHIQ_SERVICE_STATS_INC(service, bulk_tx_count);
> -					VCHIQ_SERVICE_STATS_ADD(service, bulk_tx_bytes,
> -								bulk->actual);
> -				} else {
> -					VCHIQ_SERVICE_STATS_INC(service, bulk_rx_count);
> -					VCHIQ_SERVICE_STATS_ADD(service, bulk_rx_bytes,
> -								bulk->actual);
> -				}
> +		if (bulk->actual != VCHIQ_BULK_ACTUAL_ABORTED) {
> +			if (bulk->dir == VCHIQ_BULK_TRANSMIT) {
> +				VCHIQ_SERVICE_STATS_INC(service, bulk_tx_count);
> +				VCHIQ_SERVICE_STATS_ADD(service, bulk_tx_bytes,
> +							bulk->actual);
>   			} else {
> -				VCHIQ_SERVICE_STATS_INC(service, bulk_aborted_count);
> -			}
> -			if (bulk->mode == VCHIQ_BULK_MODE_BLOCKING) {
> -				struct bulk_waiter *waiter;
> -
> -				spin_lock(&service->state->bulk_waiter_spinlock);
> -				waiter = bulk->userdata;
> -				if (waiter) {
> -					waiter->actual = bulk->actual;
> -					complete(&waiter->event);
> -				}
> -				spin_unlock(&service->state->bulk_waiter_spinlock);
> -			} else if (bulk->mode == VCHIQ_BULK_MODE_CALLBACK) {
> -				enum vchiq_reason reason =
> -						get_bulk_reason(bulk);
> -				status = make_service_callback(service, reason,	NULL,
> -							       bulk->userdata);
> -				if (status == -EAGAIN)
> -					break;
> +				VCHIQ_SERVICE_STATS_INC(service, bulk_rx_count);
> +				VCHIQ_SERVICE_STATS_ADD(service, bulk_rx_bytes,
> +							bulk->actual);
>   			}
> +		} else {
> +			VCHIQ_SERVICE_STATS_INC(service, bulk_aborted_count);
>   		}
> +		if (bulk->mode == VCHIQ_BULK_MODE_BLOCKING) {
> +			struct bulk_waiter *waiter;
>
> +			spin_lock(&service->state->bulk_waiter_spinlock);
> +			waiter = bulk->userdata;
> +			if (waiter) {
> +				waiter->actual = bulk->actual;
> +				complete(&waiter->event);
> +			}
> +			spin_unlock(&service->state->bulk_waiter_spinlock);
> +		} else if (bulk->mode == VCHIQ_BULK_MODE_CALLBACK) {
> +			enum vchiq_reason reason =
> +					get_bulk_reason(bulk);
> +			status = make_service_callback(service, reason,	NULL,
> +						       bulk->userdata);
> +			if (status == -EAGAIN)
> +				break;
> +		}
> +complete:
I would consider goto labels within a while loop as error prone and
ugly. Maybe moving the enclosing code into a separate function would be
a nicer approach?

Regards
>   		queue->remove++;
>   		complete(&service->bulk_remove_event);
>   	}

Re: [PATCH 5/8] staging: vchiq_core: Indent copy_message_data() on a single line

[Stefan Wahren]

Am 11.10.24 um 09:22 schrieb Umang Jain:
> Fix the copy_message_data() indentation in queue_message_sync().
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>

Re: [PATCH 6/8] staging: vchiq_arm: Lower indentation of a conditional block

[Stefan Wahren]

Hi Umang,

Am 11.10.24 um 09:22 schrieb Umang Jain:
> Check early if we need to allocate the bulk waiter. This helps to
> improve readability and reduces the indentation of the 'if (waiter)'
> conditional block.
>
> No functional changes intended in this patch.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   .../interface/vchiq_arm/vchiq_arm.c           | 34 +++++++++----------
>   1 file changed, 17 insertions(+), 17 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 27ceaac8f6cc..a4a7f31b124a 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -564,28 +564,28 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
>   	}
>   	mutex_unlock(&instance->bulk_waiter_list_mutex);
>
> -	if (waiter) {
> -		struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;
> -
> -		if (bulk) {
> -			/* This thread has an outstanding bulk transfer. */
> -			/* FIXME: why compare a dma address to a pointer? */
> -			if ((bulk->data != (dma_addr_t)(uintptr_t)data) || (bulk->size != size)) {
> -				/*
> -				 * This is not a retry of the previous one.
> -				 * Cancel the signal when the transfer completes.
> -				 */
> -				spin_lock(&service->state->bulk_waiter_spinlock);
> -				bulk->userdata = NULL;
> -				spin_unlock(&service->state->bulk_waiter_spinlock);
> -			}
> -		}
> -	} else {
> +	if (!waiter) {
>   		waiter = kzalloc(sizeof(*waiter), GFP_KERNEL);
>   		if (!waiter)
>   			return -ENOMEM;
>   	}
>
> +	struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;
I think this is a behavior change, which might lead to a null pointer
dereference in case waiter is freshly allocated.

Tbh I don't think indentation prevent us from unstaging this driver.
There are more important issues (e.g. resource leaks in probe error
paths or excessive usage of WARN) in this driver.

Regards
> +
> +	if (bulk) {
> +		/* This thread has an outstanding bulk transfer. */
> +		/* FIXME: why compare a dma address to a pointer? */
> +		if ((bulk->data != (dma_addr_t)(uintptr_t)data) || (bulk->size != size)) {
> +			/*
> +			 * This is not a retry of the previous one.
> +			 * Cancel the signal when the transfer completes.
> +			 */
> +			spin_lock(&service->state->bulk_waiter_spinlock);
> +			bulk->userdata = NULL;
> +			spin_unlock(&service->state->bulk_waiter_spinlock);
> +		}
> +	}
> +
>   	ret = vchiq_bulk_xfer_blocking(instance, handle, data, NULL, size,
>   				       &waiter->bulk_waiter, dir);
>   	if ((ret != -EAGAIN) || fatal_signal_pending(current) || !waiter->bulk_waiter.bulk) {

Re: [PATCH 7/8] staging: vchiq_core: Lower indentation in parse_open()

[Stefan Wahren]

Am 11.10.24 um 09:22 schrieb Umang Jain:
> If the service is not in VCHIQ_SRVSTATE_LISTENING state, it is
> implied that the message is dealt with and parse_open() should return.
> If this is the case, simply jump the code flow to return site using
> 'goto done;' statement.
>
> This helps to lower the indentation of
> 	if (service->srvstate == VCHIQ_SRVSTATE_LISTENING)
> conditional branch.
>
> No functional changes intended in this patch.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>