| 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: <14912c78-32c4-4c61-97db-c9f6dbbd3bb1@ideasonboard.com>
Date: Sun, 13 Oct 2024 13:03:45 +0530
From: Umang Jain <umang.jain@...asonboard.com>
To: Kieran Bingham <kieran.bingham@...asonboard.com>,
Broadcom internal kernel review list
<bcm-kernel-feedback-list@...adcom.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
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>
Subject: Re: [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks()
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@...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.
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@...asonboard.com>
>
>
>
>> }
>>
>> queue->remove++;
>> --
>> 2.45.2
>>
Powered by blists - more mailing lists