* [PATCH v3 1/6] staging: vchiq_core: Locally cache cache_line_size information
2024-10-12 18:56 [PATCH v3 0/6] staging: vchiq: Lower indentation at various places Umang Jain
@ 2024-10-12 18:56 ` Umang Jain
2024-10-12 18:56 ` [PATCH v3 2/6] staging: vchiq_core: Do not log debug in a separate scope Umang Jain
` (4 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Umang Jain @ 2024-10-12 18:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Broadcom internal kernel review list
Cc: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
Kieran Bingham, Dan Carpenter, Laurent Pinchart, kernel-list,
Stefan Wahren, Umang Jain
Locally cache 'cache_line_size' information in a variable instead of
repeatedly accessing it from drv_mgmt->info. This helps to reflow lines
under 80 columns.
No functional change intended in this patch.
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
---
.../interface/vchiq_arm/vchiq_core.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 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 1e4b2978c186..e9b60dd8d419 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1490,6 +1490,7 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
size_t pagelist_size;
struct scatterlist *scatterlist, *sg;
int dma_buffers;
+ unsigned int cache_line_size;
dma_addr_t dma_addr;
if (count >= INT_MAX - PAGE_SIZE)
@@ -1638,10 +1639,10 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
}
/* Partial cache lines (fragments) require special measures */
+ cache_line_size = drv_mgmt->info->cache_line_size;
if ((type == PAGELIST_READ) &&
- ((pagelist->offset & (drv_mgmt->info->cache_line_size - 1)) ||
- ((pagelist->offset + pagelist->length) &
- (drv_mgmt->info->cache_line_size - 1)))) {
+ ((pagelist->offset & (cache_line_size - 1)) ||
+ ((pagelist->offset + pagelist->length) & (cache_line_size - 1)))) {
char *fragments;
if (down_interruptible(&drv_mgmt->free_fragments_sema)) {
@@ -1671,6 +1672,7 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
struct pagelist *pagelist = pagelistinfo->pagelist;
struct page **pages = pagelistinfo->pages;
unsigned int num_pages = pagelistinfo->num_pages;
+ unsigned int cache_line_size;
dev_dbg(instance->state->dev, "arm: %pK, %d\n", pagelistinfo->pagelist, actual);
@@ -1685,16 +1687,17 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
pagelistinfo->scatterlist_mapped = 0;
/* Deal with any partial cache lines (fragments) */
+ cache_line_size = drv_mgmt->info->cache_line_size;
if (pagelist->type >= PAGELIST_READ_WITH_FRAGMENTS && drv_mgmt->fragments_base) {
char *fragments = drv_mgmt->fragments_base +
(pagelist->type - PAGELIST_READ_WITH_FRAGMENTS) *
drv_mgmt->fragments_size;
int head_bytes, tail_bytes;
- head_bytes = (drv_mgmt->info->cache_line_size - pagelist->offset) &
- (drv_mgmt->info->cache_line_size - 1);
+ head_bytes = (cache_line_size - pagelist->offset) &
+ (cache_line_size - 1);
tail_bytes = (pagelist->offset + actual) &
- (drv_mgmt->info->cache_line_size - 1);
+ (cache_line_size - 1);
if ((actual >= 0) && (head_bytes != 0)) {
if (head_bytes > actual)
@@ -1707,8 +1710,8 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
(tail_bytes != 0))
memcpy_to_page(pages[num_pages - 1],
(pagelist->offset + actual) &
- (PAGE_SIZE - 1) & ~(drv_mgmt->info->cache_line_size - 1),
- fragments + drv_mgmt->info->cache_line_size,
+ (PAGE_SIZE - 1) & ~(cache_line_size - 1),
+ fragments + cache_line_size,
tail_bytes);
down(&drv_mgmt->free_fragments_mutex);
--
2.45.2
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v3 2/6] staging: vchiq_core: Do not log debug in a separate scope
2024-10-12 18:56 [PATCH v3 0/6] staging: vchiq: Lower indentation at various places Umang Jain
2024-10-12 18:56 ` [PATCH v3 1/6] staging: vchiq_core: Locally cache cache_line_size information Umang Jain
@ 2024-10-12 18:56 ` Umang Jain
2024-10-12 18:56 ` [PATCH v3 3/6] staging: vchiq_core: Indent copy_message_data() on a single line Umang Jain
` (3 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Umang Jain @ 2024-10-12 18:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Broadcom internal kernel review list
Cc: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
Kieran Bingham, Dan Carpenter, Laurent Pinchart, kernel-list,
Stefan Wahren, Umang Jain
Do not log a dev_dbg() with a separate scope. Drop the {..}
scope and align the dev_dbg() to make it more readable.
No functional changes intended in this patch.
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
---
.../interface/vchiq_arm/vchiq_core.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 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 e9b60dd8d419..0324dfe59dca 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -936,6 +936,7 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
struct vchiq_service_quota *quota = NULL;
struct vchiq_header *header;
int type = VCHIQ_MSG_TYPE(msgid);
+ int svc_fourcc;
size_t stride;
@@ -1128,17 +1129,13 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
header->msgid = msgid;
header->size = size;
- {
- int svc_fourcc;
-
- svc_fourcc = service
- ? service->base.fourcc
- : VCHIQ_MAKE_FOURCC('?', '?', '?', '?');
+ svc_fourcc = service ? service->base.fourcc
+ : VCHIQ_MAKE_FOURCC('?', '?', '?', '?');
- dev_dbg(state->dev, "core_msg: Sent Msg %s(%u) to %p4cc s:%u d:%d len:%zu\n",
- msg_type_str(VCHIQ_MSG_TYPE(msgid)), VCHIQ_MSG_TYPE(msgid),
- &svc_fourcc, VCHIQ_MSG_SRCPORT(msgid), VCHIQ_MSG_DSTPORT(msgid), size);
- }
+ dev_dbg(state->dev, "core_msg: Sent Msg %s(%u) to %p4cc s:%u d:%d len:%zu\n",
+ msg_type_str(VCHIQ_MSG_TYPE(msgid)),
+ VCHIQ_MSG_TYPE(msgid), &svc_fourcc,
+ VCHIQ_MSG_SRCPORT(msgid), VCHIQ_MSG_DSTPORT(msgid), size);
/* Make sure the new header is visible to the peer. */
wmb();
--
2.45.2
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v3 3/6] staging: vchiq_core: Indent copy_message_data() on a single line
2024-10-12 18:56 [PATCH v3 0/6] staging: vchiq: Lower indentation at various places Umang Jain
2024-10-12 18:56 ` [PATCH v3 1/6] staging: vchiq_core: Locally cache cache_line_size information Umang Jain
2024-10-12 18:56 ` [PATCH v3 2/6] staging: vchiq_core: Do not log debug in a separate scope Umang Jain
@ 2024-10-12 18:56 ` Umang Jain
2024-10-12 18:56 ` [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks() Umang Jain
` (2 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Umang Jain @ 2024-10-12 18:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Broadcom internal kernel review list
Cc: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
Kieran Bingham, Dan Carpenter, Laurent Pinchart, kernel-list,
Stefan Wahren, Umang Jain
Fix the copy_message_data() indentation in queue_message_sync().
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 5 ++---
1 file changed, 2 insertions(+), 3 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 0324dfe59dca..e9cd012e2b5f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1197,9 +1197,8 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
state->id, msg_type_str(VCHIQ_MSG_TYPE(msgid)), header, size,
VCHIQ_MSG_SRCPORT(msgid), VCHIQ_MSG_DSTPORT(msgid));
- callback_result =
- copy_message_data(copy_callback, context,
- header->data, size);
+ callback_result = copy_message_data(copy_callback, context,
+ header->data, size);
if (callback_result < 0) {
mutex_unlock(&state->slot_mutex);
--
2.45.2
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks()
2024-10-12 18:56 [PATCH v3 0/6] staging: vchiq: Lower indentation at various places Umang Jain
` (2 preceding siblings ...)
2024-10-12 18:56 ` [PATCH v3 3/6] staging: vchiq_core: Indent copy_message_data() on a single line Umang Jain
@ 2024-10-12 18:56 ` Umang Jain
2024-10-13 7:22 ` Kieran Bingham
2024-10-13 11:59 ` Dan Carpenter
2024-10-12 18:56 ` [PATCH v3 5/6] staging: vchiq_core: Lower indentation in parse_open() Umang Jain
2024-10-12 18:56 ` [PATCH v3 6/6] staging: vchiq_core: Lower indentation in vchiq_close_service_internal Umang Jain
5 siblings, 2 replies; 11+ messages in thread
From: Umang Jain @ 2024-10-12 18:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Broadcom internal kernel review list
Cc: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
Kieran Bingham, Dan Carpenter, Laurent Pinchart, kernel-list,
Stefan Wahren, Umang Jain
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.2
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks()
2024-10-12 18:56 ` [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks() Umang Jain
@ 2024-10-13 7:22 ` Kieran Bingham
2024-10-13 7:33 ` Umang Jain
2024-10-13 11:59 ` Dan Carpenter
1 sibling, 1 reply; 11+ messages in thread
From: Kieran Bingham @ 2024-10-13 7:22 UTC (permalink / raw)
To: Broadcom internal kernel review list, Greg Kroah-Hartman, Umang Jain
Cc: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
Dan Carpenter, Laurent Pinchart, kernel-list, Stefan Wahren,
Umang Jain
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks()
2024-10-13 7:22 ` Kieran Bingham
@ 2024-10-13 7:33 ` Umang Jain
2024-10-13 12:14 ` Dan Carpenter
0 siblings, 1 reply; 11+ messages in thread
From: Umang Jain @ 2024-10-13 7:33 UTC (permalink / raw)
To: Kieran Bingham, Broadcom internal kernel review list, Greg Kroah-Hartman
Cc: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
Dan Carpenter, Laurent Pinchart, kernel-list, Stefan Wahren
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
>>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks()
2024-10-13 7:33 ` Umang Jain
@ 2024-10-13 12:14 ` Dan Carpenter
0 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2024-10-13 12:14 UTC (permalink / raw)
To: Umang Jain
Cc: Kieran Bingham, Broadcom internal kernel review list,
Greg Kroah-Hartman, linux-rpi-kernel, linux-arm-kernel,
linux-staging, linux-kernel, Laurent Pinchart, kernel-list,
Stefan Wahren
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks()
2024-10-12 18:56 ` [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks() Umang Jain
2024-10-13 7:22 ` Kieran Bingham
@ 2024-10-13 11:59 ` Dan Carpenter
1 sibling, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2024-10-13 11:59 UTC (permalink / raw)
To: Umang Jain
Cc: Greg Kroah-Hartman, Broadcom internal kernel review list,
linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
Kieran Bingham, Laurent Pinchart, kernel-list, Stefan Wahren
On 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 5/6] staging: vchiq_core: Lower indentation in parse_open()
2024-10-12 18:56 [PATCH v3 0/6] staging: vchiq: Lower indentation at various places Umang Jain
` (3 preceding siblings ...)
2024-10-12 18:56 ` [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks() Umang Jain
@ 2024-10-12 18:56 ` Umang Jain
2024-10-12 18:56 ` [PATCH v3 6/6] staging: vchiq_core: Lower indentation in vchiq_close_service_internal Umang Jain
5 siblings, 0 replies; 11+ messages in thread
From: Umang Jain @ 2024-10-12 18:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Broadcom internal kernel review list
Cc: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
Kieran Bingham, Dan Carpenter, Laurent Pinchart, kernel-list,
Stefan Wahren, Umang Jain
If the service is not in VCHIQ_SRVSTATE_LISTENING state, it is
implied that the message is dealt with and parse_open() should return.
If this is the case, simply jump the code flow to return site using
'goto done;' statement.
This helps to lower the indentation of
if (service->srvstate == VCHIQ_SRVSTATE_LISTENING)
conditional branch.
No functional changes intended in this patch.
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
---
.../interface/vchiq_arm/vchiq_core.c | 48 ++++++++++---------
1 file changed, 26 insertions(+), 22 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 19dfcd98dcde..e2183d55e5cf 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1830,8 +1830,10 @@ static int
parse_open(struct vchiq_state *state, struct vchiq_header *header)
{
const struct vchiq_open_payload *payload;
+ struct vchiq_openack_payload ack_payload;
struct vchiq_service *service = NULL;
int msgid, size;
+ int openack_id;
unsigned int localport, remoteport, fourcc;
short version, version_min;
@@ -1866,34 +1868,36 @@ parse_open(struct vchiq_state *state, struct vchiq_header *header)
}
service->peer_version = version;
- if (service->srvstate == VCHIQ_SRVSTATE_LISTENING) {
- struct vchiq_openack_payload ack_payload = {
- service->version
- };
- int openack_id = MAKE_OPENACK(service->localport, remoteport);
+ if (service->srvstate != VCHIQ_SRVSTATE_LISTENING)
+ goto done;
- if (state->version_common <
- VCHIQ_VERSION_SYNCHRONOUS_MODE)
- service->sync = 0;
+ ack_payload.version = service->version;
+ openack_id = MAKE_OPENACK(service->localport, remoteport);
- /* Acknowledge the OPEN */
- if (service->sync) {
- if (queue_message_sync(state, NULL, openack_id, memcpy_copy_callback,
- &ack_payload, sizeof(ack_payload)) == -EAGAIN)
- goto bail_not_ready;
+ if (state->version_common < VCHIQ_VERSION_SYNCHRONOUS_MODE)
+ service->sync = 0;
- /* The service is now open */
- set_service_state(service, VCHIQ_SRVSTATE_OPENSYNC);
- } else {
- if (queue_message(state, NULL, openack_id, memcpy_copy_callback,
- &ack_payload, sizeof(ack_payload), 0) == -EINTR)
- goto bail_not_ready;
+ /* Acknowledge the OPEN */
+ if (service->sync) {
+ if (queue_message_sync(state, NULL, openack_id,
+ memcpy_copy_callback,
+ &ack_payload,
+ sizeof(ack_payload)) == -EAGAIN)
+ goto bail_not_ready;
- /* The service is now open */
- set_service_state(service, VCHIQ_SRVSTATE_OPEN);
- }
+ /* The service is now open */
+ set_service_state(service, VCHIQ_SRVSTATE_OPENSYNC);
+ } else {
+ if (queue_message(state, NULL, openack_id,
+ memcpy_copy_callback, &ack_payload,
+ sizeof(ack_payload), 0) == -EINTR)
+ goto bail_not_ready;
+
+ /* The service is now open */
+ set_service_state(service, VCHIQ_SRVSTATE_OPEN);
}
+done:
/* Success - the message has been dealt with */
vchiq_service_put(service);
return 1;
--
2.45.2
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v3 6/6] staging: vchiq_core: Lower indentation in vchiq_close_service_internal
2024-10-12 18:56 [PATCH v3 0/6] staging: vchiq: Lower indentation at various places Umang Jain
` (4 preceding siblings ...)
2024-10-12 18:56 ` [PATCH v3 5/6] staging: vchiq_core: Lower indentation in parse_open() Umang Jain
@ 2024-10-12 18:56 ` Umang Jain
5 siblings, 0 replies; 11+ messages in thread
From: Umang Jain @ 2024-10-12 18:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Broadcom internal kernel review list
Cc: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
Kieran Bingham, Dan Carpenter, Laurent Pinchart, kernel-list,
Stefan Wahren, Umang Jain
Reduce indentation of the conditional nesting in
vchiq_close_service_internal() switch case by checking the error paths
first and break early. This helps to reduce conditional branching and
reduce indentation levels.
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
.../interface/vchiq_arm/vchiq_core.c | 24 ++++++++++---------
1 file changed, 13 insertions(+), 11 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 e2183d55e5cf..a2bb368836e4 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3169,19 +3169,21 @@ vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
if (close_recvd) {
dev_err(state->dev, "core: (1) called in state %s\n",
srvstate_names[service->srvstate]);
- } else if (is_server) {
- if (service->srvstate == VCHIQ_SRVSTATE_LISTENING) {
- status = -EINVAL;
- } else {
- service->client_id = 0;
- service->remoteport = VCHIQ_PORT_FREE;
- if (service->srvstate == VCHIQ_SRVSTATE_CLOSEWAIT)
- set_service_state(service, VCHIQ_SRVSTATE_LISTENING);
- }
- complete(&service->remove_event);
- } else {
+ break;
+ } else if (!is_server) {
vchiq_free_service_internal(service);
+ break;
+ }
+
+ if (service->srvstate == VCHIQ_SRVSTATE_LISTENING) {
+ status = -EINVAL;
+ } else {
+ service->client_id = 0;
+ service->remoteport = VCHIQ_PORT_FREE;
+ if (service->srvstate == VCHIQ_SRVSTATE_CLOSEWAIT)
+ set_service_state(service, VCHIQ_SRVSTATE_LISTENING);
}
+ complete(&service->remove_event);
break;
case VCHIQ_SRVSTATE_OPENING:
if (close_recvd) {
--
2.45.2
^ permalink raw reply [flat|nested] 11+ messages in thread