Re: [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks()
From: Umang Jain
Date: Sun Oct 13 2024 - 03:34:01 EST
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@xxxxxxxxxxxxxxxx>
---
.../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@xxxxxxxxxxxxxxxx>
}
queue->remove++;
--
2.45.2