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.2On 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
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
>
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
>>
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
© 2016 - 2024 Red Hat, Inc.