| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <172880414537.1925926.9488617715642757683@ping.linuxembedded.co.uk>
Date: Sun, 13 Oct 2024 08:22:25 +0100
From: Kieran Bingham <kieran.bingham@...asonboard.com>
To: Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Umang Jain <umang.jain@...asonboard.com>
Cc: linux-rpi-kernel@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org, linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org, Dan Carpenter <dan.carpenter@...aro.org>, Laurent Pinchart <laurent.pinchart@...asonboard.com>, kernel-list@...pberrypi.com, Stefan Wahren <wahrenst@....net>, Umang Jain <umang.jain@...asonboard.com>
Subject: Re: [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks()
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@...asonboard.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@...asonboard.com>
> }
>
> queue->remove++;
> --
> 2.45.2
>
Powered by blists - more mailing lists