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@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.

> +}
> +
>  /* 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@xxxxxxxxxxxxxxxx>



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