Re: [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks()
From: Kieran Bingham <kieran.bingham@ideasonboard.com>
Date: 2024-10-13 07:22:29
Also in:
linux-staging, lkml
Quoting Umang Jain (2024-10-12 19:56:50)
quoted hunk ↗ jump to hunk
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 <redacted> --- .../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.
quoted hunk ↗ jump to hunk
+} + /* 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