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

[PATCH v3 0/6] 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.                              
                                                                                
Changes in v3:
- fix status (return value) in 4/6

Changes in v2:                                                                  
- Assimilated fixes from v1 which can make independent progress                 
- drop following patches from v1:                                               
  - [PATCH 2/8] staging: vchiq_core: Properly log dev_err()                     
  - [PATCH 6/8] staging: vchiq_arm: Lower indentation of a conditional block    
  - Will be handled separately                                                  
- Rework 4/8 from v1                                                            
  - Now included as "staging: vchiq_core: Refactor notify_bulks()"              
                                                                                
Link to v1:                                                                     
https://lore.kernel.org/linux-staging/20241011072210.494672-1-umang.jain@ideasonboard.com/T/#t

Umang Jain (6):
  staging: vchiq_core: Locally cache cache_line_size information
  staging: vchiq_core: Do not log debug in a separate scope
  staging: vchiq_core: Indent copy_message_data() on a single line
  staging: vchiq_core: Refactor notify_bulks()
  staging: vchiq_core: Lower indentation in parse_open()
  staging: vchiq_core: Lower indentation in vchiq_close_service_internal

 .../interface/vchiq_arm/vchiq_core.c          | 190 ++++++++++--------
 1 file changed, 105 insertions(+), 85 deletions(-)

-- 
2.45.2

[PATCH v3 1/6] 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 v3 2/6] 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>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../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 e9b60dd8d419..0324dfe59dca 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 v3 3/6] 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>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../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 0324dfe59dca..e9cd012e2b5f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1197,9 +1197,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 v3 4/6] staging: vchiq_core: Refactor notify_bulks()

[Umang Jain]

Move the statistics and bulk completion events handling  to a separate
function. This helps to improve readability for notify_bulks().

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_core.c          | 77 +++++++++++--------
 1 file changed, 46 insertions(+), 31 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 e9cd012e2b5f..19dfcd98dcde 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1309,6 +1309,49 @@ get_bulk_reason(struct vchiq_bulk *bulk)
 	return VCHIQ_BULK_RECEIVE_DONE;
 }
 
+static int service_notify_bulk(struct vchiq_service *service,
+			       struct vchiq_bulk *bulk)
+{
+	int status = -EINVAL;
+
+	if (!service || !bulk)
+		return status;
+
+	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);
+				}
+	} 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);
+
+		status = 0;
+	} 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);
+	}
+
+	return status;
+}
+
 /* Called by the slot handler - don't hold the bulk mutex */
 static int
 notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
@@ -1333,37 +1376,9 @@ notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
 		 * 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);
-				}
-			} 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;
-			}
+			status = service_notify_bulk(service, bulk);
+			if (status == -EAGAIN)
+				break;
 		}
 
 		queue->remove++;
-- 
2.45.2

[PATCH v3 5/6] 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>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../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 19dfcd98dcde..e2183d55e5cf 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1830,8 +1830,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;
 
@@ -1866,34 +1868,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 v3 6/6] 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 e2183d55e5cf..a2bb368836e4 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3169,19 +3169,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 v3 4/6] staging: vchiq_core: Refactor notify_bulks()

[Kieran Bingham]

Quoting Umang Jain (2024-10-12 19:56:50)
> Move the statistics and bulk completion events handling  to a separate
> function. This helps to improve readability for notify_bulks().
> 
> No functional changes intended in this patch.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../interface/vchiq_arm/vchiq_core.c          | 77 +++++++++++--------
>  1 file changed, 46 insertions(+), 31 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 e9cd012e2b5f..19dfcd98dcde 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -1309,6 +1309,49 @@ get_bulk_reason(struct vchiq_bulk *bulk)
>         return VCHIQ_BULK_RECEIVE_DONE;
>  }
>  
> +static int service_notify_bulk(struct vchiq_service *service,
> +                              struct vchiq_bulk *bulk)
> +{
> +       int status = -EINVAL;
> +
> +       if (!service || !bulk)
> +               return status;

Both of these are guaranteed by the (only) caller so I'm not sure they're
needed ?

But maybe it would be used elsewhere later?

If these checks were kept, and the int status removed as mentioned below
this would just be ' return -EINVAL;' of course.

Or just drop them if it's easier and guaranteed.

> +
> +       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);
> +                               }

I think the indentation on this } has gone wrong here.

> +       } 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);
> +
> +               status = 0;

This just looks odd here. If it weren't for this I'd have probably been
fine with the initialisation of status

> +       } 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);

I think I would probably just drop the int status altogether and make this

		return make_service_callback(service, reason, NULL,
					     bulk->userdata);

> +       }
> +
> +       return status;

And return 0 here. Then we get rid of the awkward initialisation and
usages above.

> +}
> +
>  /* Called by the slot handler - don't hold the bulk mutex */
>  static int
>  notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
> @@ -1333,37 +1376,9 @@ notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
>                  * 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);
> -                               }
> -                       } 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;
> -                       }
> +                       status = service_notify_bulk(service, bulk);
> +                       if (status == -EAGAIN)
> +                               break;

This now reads as 
                 if (bulk->data && service->instance) {
                         status = service_notify_bulk(service, bulk);
                         if (status == -EAGAIN)
                                 break;
		}

which is much nicer.

With the updates above handled, then I think we're more accurately at no
functional changes:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>



>                 }
>  
>                 queue->remove++;
> -- 
> 2.45.2
>

Re: [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks()

[Umang Jain]

On 13/10/24 12:52 pm, Kieran Bingham wrote:
> Quoting Umang Jain (2024-10-12 19:56:50)
>> Move the statistics and bulk completion events handling  to a separate
>> function. This helps to improve readability for notify_bulks().
>>
>> No functional changes intended in this patch.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   .../interface/vchiq_arm/vchiq_core.c          | 77 +++++++++++--------
>>   1 file changed, 46 insertions(+), 31 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 e9cd012e2b5f..19dfcd98dcde 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> @@ -1309,6 +1309,49 @@ get_bulk_reason(struct vchiq_bulk *bulk)
>>          return VCHIQ_BULK_RECEIVE_DONE;
>>   }
>>   
>> +static int service_notify_bulk(struct vchiq_service *service,
>> +                              struct vchiq_bulk *bulk)
>> +{
>> +       int status = -EINVAL;
>> +
>> +       if (!service || !bulk)
>> +               return status;
> Both of these are guaranteed by the (only) caller so I'm not sure they're
> needed ?
>
> But maybe it would be used elsewhere later?
>
> If these checks were kept, and the int status removed as mentioned below
> this would just be ' return -EINVAL;' of course.
>
> Or just drop them if it's easier and guaranteed.
>
>> +
>> +       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);
>> +                               }
> I think the indentation on this } has gone wrong here.
>
>> +       } 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);
>> +
>> +               status = 0;
> This just looks odd here. If it weren't for this I'd have probably been
> fine with the initialisation of status
>
>> +       } 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);
> I think I would probably just drop the int status altogether and make this
>
> 		return make_service_callback(service, reason, NULL,
> 					     bulk->userdata);
>
>> +       }
>> +
>> +       return status;
> And return 0 here. Then we get rid of the awkward initialisation and
> usages above.

I usually have the tendency to minimise return  statements in a routine 
and ideally target for single return statement at the end.

  But I do agree on the awkward initialisation of status = 0

>
>> +}
>> +
>>   /* Called by the slot handler - don't hold the bulk mutex */
>>   static int
>>   notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
>> @@ -1333,37 +1376,9 @@ notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
>>                   * 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);
>> -                               }
>> -                       } 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;
>> -                       }
>> +                       status = service_notify_bulk(service, bulk);
>> +                       if (status == -EAGAIN)
>> +                               break;
> This now reads as
>                   if (bulk->data && service->instance) {
>                           status = service_notify_bulk(service, bulk);
>                           if (status == -EAGAIN)
>                                   break;
> 		}
>
> which is much nicer.

agreed, will address this
>
> With the updates above handled, then I think we're more accurately at no
> functional changes:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>
>
>>                  }
>>   
>>                  queue->remove++;
>> -- 
>> 2.45.2
>>

Re: [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks()

[Dan Carpenter]

On Sun, Oct 13, 2024 at 12:26:50AM +0530, Umang Jain wrote:
> 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 e9cd012e2b5f..19dfcd98dcde 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -1309,6 +1309,49 @@ get_bulk_reason(struct vchiq_bulk *bulk)
>  	return VCHIQ_BULK_RECEIVE_DONE;
>  }
>  
> +static int service_notify_bulk(struct vchiq_service *service,
> +			       struct vchiq_bulk *bulk)
> +{
> +	int status = -EINVAL;
> +
> +	if (!service || !bulk)
> +		return status;

I mean, I still wish you would return -EINVAL directly here.  :/

> +
> +	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);
> +				}
                                ^
Indented too far.

regards,
dan carpenter

Re: [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks()

[Dan Carpenter]

On Sun, Oct 13, 2024 at 01:03:45PM +0530, Umang Jain wrote:
> > > +               spin_unlock(&service->state->bulk_waiter_spinlock);
> > > +
> > > +               status = 0;
> > This just looks odd here. If it weren't for this I'd have probably been
> > fine with the initialisation of status
> > 
> > > +       } 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);
> > I think I would probably just drop the int status altogether and make this
> > 
> > 		return make_service_callback(service, reason, NULL,
> > 					     bulk->userdata);
> > 
> > > +       }
> > > +
> > > +       return status;
> > And return 0 here. Then we get rid of the awkward initialisation and
> > usages above.
> 
> I usually have the tendency to minimise return  statements in a routine and
> ideally target for single return statement at the end.

I feel like the "one return per function" style rule is an anti-pattern.  I
feel like it's better to handle errors right away.  Then the code which is
indented one tab is the success path and the code which is indented more is
the edge cases.

> 
>  But I do agree on the awkward initialisation of status = 0

I sent my email and then I thought.  Actually the solution here is to do:

		status = make_service_callback(service, reason, NULL,
					       bulk->userdata);
		if (status)
			return status;
	}

	return 0;

This handles the error right away and avoids mixing the error paths with the
success paths.  Plus I like a big "return 0;" at the end of my functions.

I like Kieran's approach as well.

But, I see now that I have misread the function.  I'm not sure what is the most
readable way to write it.  Maybe:

	int status = 0;

	if (bulk->mode == VCHIQ_BULK_MODE_BLOCKING) {
		...
	} else if (bulk->mode == VCHIQ_BULK_MODE_CALLBACK) {
		...
		status = make_service_callback();
	} else {
		status = -EINVAL;
	}

	return status;

Probably whatever you decide is fine.  You care more about this code than I do
for sure.

regards,
dan carpenter