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

[Umang Jain <umang.jain@ideasonboard.com>]

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