* [PATCH 01/18] staging: vchiq_core: fix indentation in vchiq_log_dump_mem
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 02/18] staging: vchiq_debugfs: get the rid of n_log_entries Stefan Wahren
` (17 subsequent siblings)
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
This align the indentation in the var declaration of vchiq_log_dump_mem to
the other code.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 8 ++++----
1 file changed, 4 insertions(+), 4 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 7fe20d4..2cbbbb6 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3666,10 +3666,10 @@ enum vchiq_status vchiq_send_remote_use_active(struct vchiq_state *state)
void vchiq_log_dump_mem(const char *label, u32 addr, const void *void_mem, size_t num_bytes)
{
- const u8 *mem = void_mem;
- size_t offset;
- char line_buf[100];
- char *s;
+ const u8 *mem = void_mem;
+ size_t offset;
+ char line_buf[100];
+ char *s;
while (num_bytes > 0) {
s = line_buf;
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 02/18] staging: vchiq_debugfs: get the rid of n_log_entries
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
2022-01-23 20:02 ` [PATCH 01/18] staging: vchiq_core: fix indentation in vchiq_log_dump_mem Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 03/18] staging: vchiq_arm: introduce is_adjacent_block Stefan Wahren
` (16 subsequent siblings)
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
There is no need for this static variable, so use the macro directly.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_debugfs.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_debugfs.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_debugfs.c
index 8f3d9cb..dc667af 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_debugfs.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_debugfs.c
@@ -36,8 +36,6 @@ static struct vchiq_debugfs_log_entry vchiq_debugfs_log_entries[] = {
{ "arm", &vchiq_arm_log_level },
};
-static int n_log_entries = ARRAY_SIZE(vchiq_debugfs_log_entries);
-
static int debugfs_log_show(struct seq_file *f, void *offset)
{
int *levp = f->private;
@@ -216,7 +214,7 @@ void vchiq_debugfs_init(void)
/* create an entry under <debugfs>/vchiq/log for each log category */
dir = debugfs_create_dir("log", vchiq_dbg_dir);
- for (i = 0; i < n_log_entries; i++)
+ for (i = 0; i < ARRAY_SIZE(vchiq_debugfs_log_entries); i++)
debugfs_create_file(vchiq_debugfs_log_entries[i].name, 0644,
dir, vchiq_debugfs_log_entries[i].plevel,
&debugfs_log_fops);
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 03/18] staging: vchiq_arm: introduce is_adjacent_block
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
2022-01-23 20:02 ` [PATCH 01/18] staging: vchiq_core: fix indentation in vchiq_log_dump_mem Stefan Wahren
2022-01-23 20:02 ` [PATCH 02/18] staging: vchiq_debugfs: get the rid of n_log_entries Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 04/18] staging: vchiq: convert TODOs into unordered list Stefan Wahren
` (15 subsequent siblings)
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
The check for an adjacent block is hard to read. So move it into a separate
inline function.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 6759a62..9748044 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -189,6 +189,20 @@ cleanup_pagelistinfo(struct vchiq_pagelist_info *pagelistinfo)
pagelistinfo->pagelist, pagelistinfo->dma_addr);
}
+static inline bool
+is_adjacent_block(u32 *addrs, u32 addr, unsigned int k)
+{
+ u32 tmp;
+
+ if (!k)
+ return false;
+
+ tmp = (addrs[k - 1] & PAGE_MASK) +
+ (((addrs[k - 1] & ~PAGE_MASK) + 1) << PAGE_SHIFT);
+
+ return tmp == (addr & PAGE_MASK);
+}
+
/* There is a potential problem with partial cache lines (pages?)
* at the ends of the block when reading. If the CPU accessed anything in
* the same line (page?) then it may have pulled old data into the cache,
@@ -349,10 +363,7 @@ create_pagelist(char *buf, char __user *ubuf,
WARN_ON(len == 0);
WARN_ON(i && (i != (dma_buffers - 1)) && (len & ~PAGE_MASK));
WARN_ON(i && (addr & ~PAGE_MASK));
- if (k > 0 &&
- ((addrs[k - 1] & PAGE_MASK) +
- (((addrs[k - 1] & ~PAGE_MASK) + 1) << PAGE_SHIFT))
- == (addr & PAGE_MASK))
+ if (is_adjacent_block(addrs, addr, k))
addrs[k - 1] += ((len + PAGE_SIZE - 1) >> PAGE_SHIFT);
else
addrs[k++] = (addr & PAGE_MASK) |
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 04/18] staging: vchiq: convert TODOs into unordered list
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (2 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 03/18] staging: vchiq_arm: introduce is_adjacent_block Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 05/18] staging: vchiq: drop completed tasks from TODO Stefan Wahren
` (14 subsequent siblings)
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
Keeping the order of the TODO items is hard and also unnecessary.
So convert it to an unordered list.
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/staging/vc04_services/interface/TODO | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
index 241ca00..2e4ec58 100644
--- a/drivers/staging/vc04_services/interface/TODO
+++ b/drivers/staging/vc04_services/interface/TODO
@@ -1,4 +1,4 @@
-1) Import drivers using VCHI.
+* Import drivers using VCHI.
VCHI is just a tool to let drivers talk to the firmware. Here are
some of the ones we want:
@@ -16,7 +16,7 @@ some of the ones we want:
to manage these buffers as dmabufs so that we can zero-copy import
camera images into vc4 for rendering/display.
-2) Garbage-collect unused code
+* Garbage-collect unused code
One of the reasons this driver wasn't upstreamed previously was that
there's a lot code that got built that's probably unnecessary these
@@ -24,7 +24,7 @@ days. Once we have the set of VCHI-using drivers we want in tree, we
should be able to do a sweep of the code to see what's left that's
unused.
-3) Make driver more portable
+* Make driver more portable
Building this driver with arm/multi_v7_defconfig or arm64/defconfig
leads to data corruption during the following command:
@@ -33,42 +33,42 @@ leads to data corruption during the following command:
This should be fixed.
-4) Fix kernel module support
+* Fix kernel module support
Even the VPU firmware doesn't support a VCHI re-connect, the driver
should properly handle a module unload. This also includes that all
resources must be freed (kthreads, debugfs entries, ...) and global
variables avoided.
-5) Cleanup logging mechanism
+* Cleanup logging mechanism
The driver should probably be using the standard kernel logging mechanisms
such as dev_info, dev_dbg, and friends.
-6) Documentation
+* Documentation
A short top-down description of this driver's architecture (function of
kthreads, userspace, limitations) could be very helpful for reviewers.
-7) Review and comment memory barriers
+* Review and comment memory barriers
There is a heavy use of memory barriers in this driver, it would be very
beneficial to go over all of them and, if correct, comment on their merits.
Extra points to whomever confidently reviews the remote_event_*() family of
functions.
-8) Get rid of custom function return values
+* Get rid of custom function return values
Most functions use a custom set of return values, we should force proper Linux
error numbers. Special care is needed for VCHIQ_RETRY.
-9) Reformat core code with more sane indentations
+* Reformat core code with more sane indentations
The code follows the 80 characters limitation yet tends to go 3 or 4 levels of
indentation deep making it very unpleasant to read. This is specially relevant
in the character driver ioctl code and in the core thread functions.
-10) Reorganize file structure: Move char driver to it's own file and join both
+* Reorganize file structure: Move char driver to it's own file and join both
platform files
The cdev is defined alongside with the platform code in vchiq_arm.c. It would
@@ -80,7 +80,7 @@ vchiq-core.ko and vchiq-dev.ko. This would also ease the upstreaming process.
The code in vchiq_bcm2835_arm.c should fit in the generic platform file.
-11) Get rid of all non essential global structures and create a proper per
+* Get rid of all non essential global structures and create a proper per
device structure
The first thing one generally sees in a probe function is a memory allocation
@@ -88,6 +88,6 @@ for all the device specific data. This structure is then passed all over the
driver. This is good practice since it makes the driver work regardless of the
number of devices probed.
-12) Clean up Sparse warnings from __user annotations. See
+* Clean up Sparse warnings from __user annotations. See
vchiq_irq_queue_bulk_tx_rx(). Ensure that the address of "&waiter->bulk_waiter"
is never disclosed to userspace.
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 05/18] staging: vchiq: drop completed tasks from TODO
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (3 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 04/18] staging: vchiq: convert TODOs into unordered list Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 06/18] staging: vchiq: add message handling to TODO list Stefan Wahren
` (13 subsequent siblings)
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
This removes all already completed tasks from the TODO file.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/staging/vc04_services/interface/TODO | 29 ----------------------------
1 file changed, 29 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
index 2e4ec58..e2d44a4 100644
--- a/drivers/staging/vc04_services/interface/TODO
+++ b/drivers/staging/vc04_services/interface/TODO
@@ -16,23 +16,6 @@ some of the ones we want:
to manage these buffers as dmabufs so that we can zero-copy import
camera images into vc4 for rendering/display.
-* Garbage-collect unused code
-
-One of the reasons this driver wasn't upstreamed previously was that
-there's a lot code that got built that's probably unnecessary these
-days. Once we have the set of VCHI-using drivers we want in tree, we
-should be able to do a sweep of the code to see what's left that's
-unused.
-
-* Make driver more portable
-
-Building this driver with arm/multi_v7_defconfig or arm64/defconfig
-leads to data corruption during the following command:
-
- vchiq_test -f 1
-
-This should be fixed.
-
* Fix kernel module support
Even the VPU firmware doesn't support a VCHI re-connect, the driver
@@ -68,18 +51,6 @@ The code follows the 80 characters limitation yet tends to go 3 or 4 levels of
indentation deep making it very unpleasant to read. This is specially relevant
in the character driver ioctl code and in the core thread functions.
-* Reorganize file structure: Move char driver to it's own file and join both
-platform files
-
-The cdev is defined alongside with the platform code in vchiq_arm.c. It would
-be nice to completely decouple it from the actual core code. For instance to be
-able to use bcm2835-audio without having /dev/vchiq created. One could argue
-it's better for security reasons or general cleanliness. It could even be
-interesting to create two different kernel modules, something the likes of
-vchiq-core.ko and vchiq-dev.ko. This would also ease the upstreaming process.
-
-The code in vchiq_bcm2835_arm.c should fit in the generic platform file.
-
* Get rid of all non essential global structures and create a proper per
device structure
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 06/18] staging: vchiq: add message handling to TODO list
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (4 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 05/18] staging: vchiq: drop completed tasks from TODO Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 07/18] staging: vchiq_core: fix type of parameter localport Stefan Wahren
` (12 subsequent siblings)
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
Recently Arnd Bergmann noticed that the message handling looks broken
to him. So add this point on the TODO list.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/staging/vc04_services/interface/TODO | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
index e2d44a4..97085a0 100644
--- a/drivers/staging/vc04_services/interface/TODO
+++ b/drivers/staging/vc04_services/interface/TODO
@@ -62,3 +62,12 @@ number of devices probed.
* Clean up Sparse warnings from __user annotations. See
vchiq_irq_queue_bulk_tx_rx(). Ensure that the address of "&waiter->bulk_waiter"
is never disclosed to userspace.
+
+* Fix behavior of message handling
+
+The polling behavior of vchiq_bulk_transmit(), vchiq_bulk_receive() and
+vchiq_queue_kernel_message() looks broken. A possible signal should be
+propagated back to user space to let the calling task handle it before
+retrying. Hopefully these msleep(1) shouldn't be necessary anymore.
+
+https://lore.kernel.org/linux-staging/CAK8P3a3HGm1cPo4sW9fOY4E8AN8yAq3tevXxU5m8bmtmsU8WKw@mail.gmail.com/
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 07/18] staging: vchiq_core: fix type of parameter localport
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (5 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 06/18] staging: vchiq: add message handling to TODO list Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 08/18] staging: vchiq_core: simplify vchiq_add_service_internal Stefan Wahren
` (11 subsequent siblings)
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
The whole vchiq driver uses unsigned int for "localport" except of
find_service_by_port(). So fix this and get the rid of this suspicous
cast.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 6 +++---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h | 2 +-
2 files changed, 4 insertions(+), 4 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 2cbbbb6..6fa9fee 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -255,9 +255,9 @@ find_service_by_handle(unsigned int handle)
}
struct vchiq_service *
-find_service_by_port(struct vchiq_state *state, int localport)
+find_service_by_port(struct vchiq_state *state, unsigned int localport)
{
- if ((unsigned int)localport <= VCHIQ_PORT_MAX) {
+ if (localport <= VCHIQ_PORT_MAX) {
struct vchiq_service *service;
rcu_read_lock();
@@ -271,7 +271,7 @@ find_service_by_port(struct vchiq_state *state, int localport)
rcu_read_unlock();
}
vchiq_log_info(vchiq_core_log_level,
- "Invalid port %d", localport);
+ "Invalid port %u", localport);
return NULL;
}
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 55abaf0..82b7bd7b 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -524,7 +524,7 @@ extern struct vchiq_service *
find_service_by_handle(unsigned int handle);
extern struct vchiq_service *
-find_service_by_port(struct vchiq_state *state, int localport);
+find_service_by_port(struct vchiq_state *state, unsigned int localport);
extern struct vchiq_service *
find_service_for_instance(struct vchiq_instance *instance, unsigned int handle);
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 08/18] staging: vchiq_core: simplify vchiq_add_service_internal
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (6 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 07/18] staging: vchiq_core: fix type of parameter localport Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-24 15:26 ` Pavel Skripkin
2022-01-23 20:02 ` [PATCH 09/18] staging: vchiq_core: align return statements in msg_type_str Stefan Wahren
` (10 subsequent siblings)
18 siblings, 1 reply; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
Better use kzalloc to properly init vchiq_service with zero. As a result
this saves us all the zero assignments.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_core.c | 23 +---------------------
1 file changed, 1 insertion(+), 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 6fa9fee..a13a076 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2097,16 +2097,6 @@ sync_func(void *v)
return 0;
}
-static void
-init_bulk_queue(struct vchiq_bulk_queue *queue)
-{
- queue->local_insert = 0;
- queue->remote_insert = 0;
- queue->process = 0;
- queue->remote_notify = 0;
- queue->remove = 0;
-}
-
inline const char *
get_conn_state_name(enum vchiq_connstate conn_state)
{
@@ -2371,7 +2361,7 @@ vchiq_add_service_internal(struct vchiq_state *state,
if (ret)
return NULL;
- service = kmalloc(sizeof(*service), GFP_KERNEL);
+ service = kzalloc(sizeof(*service), GFP_KERNEL);
if (!service)
return service;
@@ -2387,28 +2377,17 @@ vchiq_add_service_internal(struct vchiq_state *state,
service->public_fourcc = (srvstate == VCHIQ_SRVSTATE_OPENING) ?
VCHIQ_FOURCC_INVALID : params->fourcc;
- service->client_id = 0;
service->auto_close = 1;
- service->sync = 0;
- service->closing = 0;
- service->trace = 0;
atomic_set(&service->poll_flags, 0);
service->version = params->version;
service->version_min = params->version_min;
service->state = state;
service->instance = instance;
- service->service_use_count = 0;
- service->msg_queue_read = 0;
- service->msg_queue_write = 0;
- init_bulk_queue(&service->bulk_tx);
- init_bulk_queue(&service->bulk_rx);
init_completion(&service->remove_event);
init_completion(&service->bulk_remove_event);
init_completion(&service->msg_queue_pop);
init_completion(&service->msg_queue_push);
mutex_init(&service->bulk_mutex);
- memset(&service->stats, 0, sizeof(service->stats));
- memset(&service->msg_queue, 0, sizeof(service->msg_queue));
/*
* Although it is perfectly possible to use a spinlock
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 08/18] staging: vchiq_core: simplify vchiq_add_service_internal
2022-01-23 20:02 ` [PATCH 08/18] staging: vchiq_core: simplify vchiq_add_service_internal Stefan Wahren
@ 2022-01-24 15:26 ` Pavel Skripkin
2022-01-25 9:59 ` Dan Carpenter
0 siblings, 1 reply; 25+ messages in thread
From: Pavel Skripkin @ 2022-01-24 15:26 UTC (permalink / raw)
To: Stefan Wahren, Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging
Hi Stefan,
On 1/23/22 23:02, Stefan Wahren wrote:
> Better use kzalloc to properly init vchiq_service with zero. As a result
> this saves us all the zero assignments.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
> .../vc04_services/interface/vchiq_arm/vchiq_core.c | 23 +---------------------
> 1 file changed, 1 insertion(+), 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 6fa9fee..a13a076 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -2097,16 +2097,6 @@ sync_func(void *v)
> return 0;
> }
>
> -static void
> -init_bulk_queue(struct vchiq_bulk_queue *queue)
> -{
> - queue->local_insert = 0;
> - queue->remote_insert = 0;
> - queue->process = 0;
> - queue->remote_notify = 0;
> - queue->remove = 0;
> -}
> -
> inline const char *
> get_conn_state_name(enum vchiq_connstate conn_state)
> {
> @@ -2371,7 +2361,7 @@ vchiq_add_service_internal(struct vchiq_state *state,
> if (ret)
> return NULL;
>
> - service = kmalloc(sizeof(*service), GFP_KERNEL);
> + service = kzalloc(sizeof(*service), GFP_KERNEL);
> if (!service)
> return service;
>
> @@ -2387,28 +2377,17 @@ vchiq_add_service_internal(struct vchiq_state *state,
>
> service->public_fourcc = (srvstate == VCHIQ_SRVSTATE_OPENING) ?
> VCHIQ_FOURCC_INVALID : params->fourcc;
> - service->client_id = 0;
> service->auto_close = 1;
> - service->sync = 0;
> - service->closing = 0;
> - service->trace = 0;
> atomic_set(&service->poll_flags, 0);
Nit: atomic_set(0) can be also removed
With regards,
Pavel Skripkin
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 08/18] staging: vchiq_core: simplify vchiq_add_service_internal
2022-01-24 15:26 ` Pavel Skripkin
@ 2022-01-25 9:59 ` Dan Carpenter
2022-01-25 10:06 ` Pavel Skripkin
0 siblings, 1 reply; 25+ messages in thread
From: Dan Carpenter @ 2022-01-25 9:59 UTC (permalink / raw)
To: Pavel Skripkin
Cc: Stefan Wahren, Nicolas Saenz Julienne, Greg Kroah-Hartman,
bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging
On Mon, Jan 24, 2022 at 06:26:45PM +0300, Pavel Skripkin wrote:
> Hi Stefan,
>
> On 1/23/22 23:02, Stefan Wahren wrote:
> > Better use kzalloc to properly init vchiq_service with zero. As a result
> > this saves us all the zero assignments.
> >
> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > ---
> > .../vc04_services/interface/vchiq_arm/vchiq_core.c | 23 +---------------------
> > 1 file changed, 1 insertion(+), 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 6fa9fee..a13a076 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> > @@ -2097,16 +2097,6 @@ sync_func(void *v)
> > return 0;
> > }
> > -static void
> > -init_bulk_queue(struct vchiq_bulk_queue *queue)
> > -{
> > - queue->local_insert = 0;
> > - queue->remote_insert = 0;
> > - queue->process = 0;
> > - queue->remote_notify = 0;
> > - queue->remove = 0;
> > -}
> > -
> > inline const char *
> > get_conn_state_name(enum vchiq_connstate conn_state)
> > {
> > @@ -2371,7 +2361,7 @@ vchiq_add_service_internal(struct vchiq_state *state,
> > if (ret)
> > return NULL;
> > - service = kmalloc(sizeof(*service), GFP_KERNEL);
> > + service = kzalloc(sizeof(*service), GFP_KERNEL);
> > if (!service)
> > return service;
> > @@ -2387,28 +2377,17 @@ vchiq_add_service_internal(struct vchiq_state *state,
> > service->public_fourcc = (srvstate == VCHIQ_SRVSTATE_OPENING) ?
> > VCHIQ_FOURCC_INVALID : params->fourcc;
> > - service->client_id = 0;
> > service->auto_close = 1;
> > - service->sync = 0;
> > - service->closing = 0;
> > - service->trace = 0;
> > atomic_set(&service->poll_flags, 0);
>
> Nit: atomic_set(0) can be also removed
>
No, let's not write code that assumes it knows atomic_t internals.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 08/18] staging: vchiq_core: simplify vchiq_add_service_internal
2022-01-25 9:59 ` Dan Carpenter
@ 2022-01-25 10:06 ` Pavel Skripkin
2022-01-25 10:20 ` Dan Carpenter
0 siblings, 1 reply; 25+ messages in thread
From: Pavel Skripkin @ 2022-01-25 10:06 UTC (permalink / raw)
To: Dan Carpenter
Cc: Stefan Wahren, Nicolas Saenz Julienne, Greg Kroah-Hartman,
bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging
Hi Dan,
On 1/25/22 12:59, Dan Carpenter wrote:
[...]
>> > - service->trace = 0;
>> > atomic_set(&service->poll_flags, 0);
>>
>> Nit: atomic_set(0) can be also removed
>>
>
> No, let's not write code that assumes it knows atomic_t internals.
>
Isn't atomic_set() just (let's say) "magic" write to memory in all
cases? Am I missing some design issue here?
With regards,
Pavel Skripkin
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 08/18] staging: vchiq_core: simplify vchiq_add_service_internal
2022-01-25 10:06 ` Pavel Skripkin
@ 2022-01-25 10:20 ` Dan Carpenter
2022-01-25 10:22 ` Pavel Skripkin
0 siblings, 1 reply; 25+ messages in thread
From: Dan Carpenter @ 2022-01-25 10:20 UTC (permalink / raw)
To: Pavel Skripkin
Cc: Stefan Wahren, Nicolas Saenz Julienne, Greg Kroah-Hartman,
bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging
On Tue, Jan 25, 2022 at 01:06:14PM +0300, Pavel Skripkin wrote:
> Hi Dan,
>
> On 1/25/22 12:59, Dan Carpenter wrote:
> [...]
>
> > > > - service->trace = 0;
> > > > atomic_set(&service->poll_flags, 0);
> > >
> > > Nit: atomic_set(0) can be also removed
> > >
> >
> > No, let's not write code that assumes it knows atomic_t internals.
> >
>
> Isn't atomic_set() just (let's say) "magic" write to memory in all cases? Am
> I missing some design issue here?
>
The point is atomic values should only be set using atomic_set/inc/dec()
etc. Anything else is wrong even if it works.
I think it might trigger a kcsan warning these days? I don't know what
that instrumentation does. But even in olden times, the rule was you
always had to use the accessor functions.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 08/18] staging: vchiq_core: simplify vchiq_add_service_internal
2022-01-25 10:20 ` Dan Carpenter
@ 2022-01-25 10:22 ` Pavel Skripkin
0 siblings, 0 replies; 25+ messages in thread
From: Pavel Skripkin @ 2022-01-25 10:22 UTC (permalink / raw)
To: Dan Carpenter
Cc: Stefan Wahren, Nicolas Saenz Julienne, Greg Kroah-Hartman,
bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging
Hi Dan,
On 1/25/22 13:20, Dan Carpenter wrote:
> On Tue, Jan 25, 2022 at 01:06:14PM +0300, Pavel Skripkin wrote:
>> Hi Dan,
>>
>> On 1/25/22 12:59, Dan Carpenter wrote:
>> [...]
>>
>> > > > - service->trace = 0;
>> > > > atomic_set(&service->poll_flags, 0);
>> > >
>> > > Nit: atomic_set(0) can be also removed
>> > >
>> >
>> > No, let's not write code that assumes it knows atomic_t internals.
>> >
>>
>> Isn't atomic_set() just (let's say) "magic" write to memory in all cases? Am
>> I missing some design issue here?
>>
>
> The point is atomic values should only be set using atomic_set/inc/dec()
> etc. Anything else is wrong even if it works.
>
> I think it might trigger a kcsan warning these days? I don't know what
> that instrumentation does. But even in olden times, the rule was you
> always had to use the accessor functions.
>
Ok, got it, thanks for explanations!
With regards,
Pavel Skripkin
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 09/18] staging: vchiq_core: align return statements in msg_type_str
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (7 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 08/18] staging: vchiq_core: simplify vchiq_add_service_internal Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 10/18] staging: vchiq_core: drop prefix of vchiq_set_service_state Stefan Wahren
` (9 subsequent siblings)
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
The return statements aren't properly aligned. So fix this by using tabs.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_core.c | 30 +++++++++++-----------
1 file changed, 15 insertions(+), 15 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 a13a076..e500cef 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -205,21 +205,21 @@ release_message_sync(struct vchiq_state *state, struct vchiq_header *header);
static const char *msg_type_str(unsigned int msg_type)
{
switch (msg_type) {
- case VCHIQ_MSG_PADDING: return "PADDING";
- case VCHIQ_MSG_CONNECT: return "CONNECT";
- case VCHIQ_MSG_OPEN: return "OPEN";
- case VCHIQ_MSG_OPENACK: return "OPENACK";
- case VCHIQ_MSG_CLOSE: return "CLOSE";
- case VCHIQ_MSG_DATA: return "DATA";
- case VCHIQ_MSG_BULK_RX: return "BULK_RX";
- case VCHIQ_MSG_BULK_TX: return "BULK_TX";
- case VCHIQ_MSG_BULK_RX_DONE: return "BULK_RX_DONE";
- case VCHIQ_MSG_BULK_TX_DONE: return "BULK_TX_DONE";
- case VCHIQ_MSG_PAUSE: return "PAUSE";
- case VCHIQ_MSG_RESUME: return "RESUME";
- case VCHIQ_MSG_REMOTE_USE: return "REMOTE_USE";
- case VCHIQ_MSG_REMOTE_RELEASE: return "REMOTE_RELEASE";
- case VCHIQ_MSG_REMOTE_USE_ACTIVE: return "REMOTE_USE_ACTIVE";
+ case VCHIQ_MSG_PADDING: return "PADDING";
+ case VCHIQ_MSG_CONNECT: return "CONNECT";
+ case VCHIQ_MSG_OPEN: return "OPEN";
+ case VCHIQ_MSG_OPENACK: return "OPENACK";
+ case VCHIQ_MSG_CLOSE: return "CLOSE";
+ case VCHIQ_MSG_DATA: return "DATA";
+ case VCHIQ_MSG_BULK_RX: return "BULK_RX";
+ case VCHIQ_MSG_BULK_TX: return "BULK_TX";
+ case VCHIQ_MSG_BULK_RX_DONE: return "BULK_RX_DONE";
+ case VCHIQ_MSG_BULK_TX_DONE: return "BULK_TX_DONE";
+ case VCHIQ_MSG_PAUSE: return "PAUSE";
+ case VCHIQ_MSG_RESUME: return "RESUME";
+ case VCHIQ_MSG_REMOTE_USE: return "REMOTE_USE";
+ case VCHIQ_MSG_REMOTE_RELEASE: return "REMOTE_RELEASE";
+ case VCHIQ_MSG_REMOTE_USE_ACTIVE: return "REMOTE_USE_ACTIVE";
}
return "???";
}
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 10/18] staging: vchiq_core: drop prefix of vchiq_set_service_state
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (8 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 09/18] staging: vchiq_core: align return statements in msg_type_str Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 11/18] staging: vchiq_core: reduce multi-line statements Stefan Wahren
` (8 subsequent siblings)
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
The name of this static function is unnecessary longish. So drop the prefix.
This gives us the chance to avoid some multi-line statements or fix some
indentations later.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_core.c | 31 +++++++++++-----------
1 file changed, 15 insertions(+), 16 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 e500cef..d861857 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -225,7 +225,7 @@ static const char *msg_type_str(unsigned int msg_type)
}
static inline void
-vchiq_set_service_state(struct vchiq_service *service, int newstate)
+set_service_state(struct vchiq_service *service, int newstate)
{
vchiq_log_info(vchiq_core_log_level, "%d: srv:%d %s->%s",
service->state->id, service->localport,
@@ -1122,7 +1122,7 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
wmb();
if (service && (type == VCHIQ_MSG_CLOSE))
- vchiq_set_service_state(service, VCHIQ_SRVSTATE_CLOSESENT);
+ set_service_state(service, VCHIQ_SRVSTATE_CLOSESENT);
if (!(flags & QMFLAGS_NO_MUTEX_UNLOCK))
mutex_unlock(&state->slot_mutex);
@@ -1531,7 +1531,7 @@ parse_open(struct vchiq_state *state, struct vchiq_header *header)
}
/* The service is now open */
- vchiq_set_service_state(service, service->sync ? VCHIQ_SRVSTATE_OPENSYNC
+ set_service_state(service, service->sync ? VCHIQ_SRVSTATE_OPENSYNC
: VCHIQ_SRVSTATE_OPEN);
}
@@ -1666,7 +1666,7 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
service->peer_version);
if (service->srvstate == VCHIQ_SRVSTATE_OPENING) {
service->remoteport = remoteport;
- vchiq_set_service_state(service, VCHIQ_SRVSTATE_OPEN);
+ set_service_state(service, VCHIQ_SRVSTATE_OPEN);
complete(&service->remove_event);
} else {
vchiq_log_error(vchiq_core_log_level, "OPENACK received in state %s",
@@ -2063,7 +2063,7 @@ sync_func(void *v)
service->peer_version);
if (service->srvstate == VCHIQ_SRVSTATE_OPENING) {
service->remoteport = remoteport;
- vchiq_set_service_state(service, VCHIQ_SRVSTATE_OPENSYNC);
+ set_service_state(service, VCHIQ_SRVSTATE_OPENSYNC);
service->sync = 1;
complete(&service->remove_event);
}
@@ -2465,7 +2465,7 @@ vchiq_add_service_internal(struct vchiq_state *state,
- 1;
/* Bring this service online */
- vchiq_set_service_state(service, srvstate);
+ set_service_state(service, srvstate);
vchiq_log_info(vchiq_core_msg_log_level, "%s Service %c%c%c%c SrcPort:%d",
(srvstate == VCHIQ_SRVSTATE_OPENING) ? "Open" : "Add",
@@ -2621,7 +2621,7 @@ close_service_complete(struct vchiq_service *service, int failstate)
} else {
newstate = VCHIQ_SRVSTATE_CLOSED;
}
- vchiq_set_service_state(service, newstate);
+ set_service_state(service, newstate);
break;
case VCHIQ_SRVSTATE_LISTENING:
break;
@@ -2657,7 +2657,7 @@ close_service_complete(struct vchiq_service *service, int failstate)
complete(&service->remove_event);
}
} else {
- vchiq_set_service_state(service, failstate);
+ set_service_state(service, failstate);
}
return status;
@@ -2692,7 +2692,7 @@ vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
service->remoteport = VCHIQ_PORT_FREE;
if (service->srvstate ==
VCHIQ_SRVSTATE_CLOSEWAIT)
- vchiq_set_service_state(service, VCHIQ_SRVSTATE_LISTENING);
+ set_service_state(service, VCHIQ_SRVSTATE_LISTENING);
}
complete(&service->remove_event);
} else {
@@ -2702,7 +2702,7 @@ vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
case VCHIQ_SRVSTATE_OPENING:
if (close_recvd) {
/* The open was rejected - tell the user */
- vchiq_set_service_state(service, VCHIQ_SRVSTATE_CLOSEWAIT);
+ set_service_state(service, VCHIQ_SRVSTATE_CLOSEWAIT);
complete(&service->remove_event);
} else {
/* Shutdown mid-open - let the other side know */
@@ -2733,8 +2733,7 @@ vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
if (!close_recvd) {
/* Change the state while the mutex is still held */
- vchiq_set_service_state(service,
- VCHIQ_SRVSTATE_CLOSESENT);
+ set_service_state(service, VCHIQ_SRVSTATE_CLOSESENT);
mutex_unlock(&state->slot_mutex);
if (service->sync)
mutex_unlock(&state->sync_mutex);
@@ -2742,7 +2741,7 @@ vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
}
/* Change the state while the mutex is still held */
- vchiq_set_service_state(service, VCHIQ_SRVSTATE_CLOSERECVD);
+ set_service_state(service, VCHIQ_SRVSTATE_CLOSERECVD);
mutex_unlock(&state->slot_mutex);
if (service->sync)
mutex_unlock(&state->sync_mutex);
@@ -2767,7 +2766,7 @@ vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
case VCHIQ_SRVSTATE_CLOSERECVD:
if (!close_recvd && is_server)
/* Force into LISTENING mode */
- vchiq_set_service_state(service, VCHIQ_SRVSTATE_LISTENING);
+ set_service_state(service, VCHIQ_SRVSTATE_LISTENING);
status = close_service_complete(service, VCHIQ_SRVSTATE_CLOSERECVD);
break;
@@ -2816,7 +2815,7 @@ vchiq_free_service_internal(struct vchiq_service *service)
return;
}
- vchiq_set_service_state(service, VCHIQ_SRVSTATE_FREE);
+ set_service_state(service, VCHIQ_SRVSTATE_FREE);
complete(&service->remove_event);
@@ -2834,7 +2833,7 @@ vchiq_connect_internal(struct vchiq_state *state, struct vchiq_instance *instanc
i = 0;
while ((service = next_service_by_instance(state, instance, &i)) != NULL) {
if (service->srvstate == VCHIQ_SRVSTATE_HIDDEN)
- vchiq_set_service_state(service, VCHIQ_SRVSTATE_LISTENING);
+ set_service_state(service, VCHIQ_SRVSTATE_LISTENING);
vchiq_service_put(service);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 11/18] staging: vchiq_core: reduce multi-line statements
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (9 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 10/18] staging: vchiq_core: drop prefix of vchiq_set_service_state Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 12/18] staging: vchiq_core: fix alignment Stefan Wahren
` (7 subsequent siblings)
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
In order to improvement readability try to reduce the multi-line statements.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 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 d861857..fbc44e7 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -872,9 +872,8 @@ copy_message_data(ssize_t (*copy_callback)(void *context, void *dest, size_t off
ssize_t callback_result;
size_t max_bytes = size - pos;
- callback_result =
- copy_callback(context, dest + pos,
- pos, max_bytes);
+ callback_result = copy_callback(context, dest + pos, pos,
+ max_bytes);
if (callback_result < 0)
return callback_result;
@@ -1028,8 +1027,7 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
if (callback_result < 0) {
mutex_unlock(&state->slot_mutex);
- VCHIQ_SERVICE_STATS_INC(service,
- error_count);
+ VCHIQ_SERVICE_STATS_INC(service, error_count);
return VCHIQ_ERROR;
}
@@ -1177,8 +1175,7 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
if (callback_result < 0) {
mutex_unlock(&state->slot_mutex);
- VCHIQ_SERVICE_STATS_INC(service,
- error_count);
+ VCHIQ_SERVICE_STATS_INC(service, error_count);
return VCHIQ_ERROR;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 12/18] staging: vchiq_core: fix alignment
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (10 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 11/18] staging: vchiq_core: reduce multi-line statements Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 13/18] staging: vchiq_core: avoid ternary operator for set_service_state Stefan Wahren
` (6 subsequent siblings)
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
This fixes some alignment found by checkpatch.pl.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/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 fbc44e7..7e901be 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2687,8 +2687,7 @@ vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
} else {
service->client_id = 0;
service->remoteport = VCHIQ_PORT_FREE;
- if (service->srvstate ==
- VCHIQ_SRVSTATE_CLOSEWAIT)
+ if (service->srvstate == VCHIQ_SRVSTATE_CLOSEWAIT)
set_service_state(service, VCHIQ_SRVSTATE_LISTENING);
}
complete(&service->remove_event);
@@ -2893,7 +2892,7 @@ vchiq_close_service(unsigned int handle)
status = vchiq_close_service_internal(service, NO_CLOSE_RECVD);
WARN_ON(status == VCHIQ_RETRY);
} else {
- /* Mark the service for termination by the slot handler */
+ /* Mark the service for termination by the slot handler */
request_poll(service->state, service, VCHIQ_POLL_TERMINATE);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 13/18] staging: vchiq_core: avoid ternary operator for set_service_state
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (11 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 12/18] staging: vchiq_core: fix alignment Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 14/18] staging: vchiq_core: use min_t macro Stefan Wahren
` (5 subsequent siblings)
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
There is already a check for service->sync, so use this instead of a
separate ternary operator to update the service state.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 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 7e901be..6127244 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1521,15 +1521,17 @@ parse_open(struct vchiq_state *state, struct vchiq_header *header)
if (queue_message_sync(state, NULL, openack_id, memcpy_copy_callback,
&ack_payload, sizeof(ack_payload), 0) == VCHIQ_RETRY)
goto bail_not_ready;
+
+ /* 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) == VCHIQ_RETRY)
goto bail_not_ready;
- }
- /* The service is now open */
- set_service_state(service, service->sync ? VCHIQ_SRVSTATE_OPENSYNC
- : VCHIQ_SRVSTATE_OPEN);
+ /* The service is now open */
+ set_service_state(service, VCHIQ_SRVSTATE_OPEN);
+ }
}
/* Success - the message has been dealt with */
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 14/18] staging: vchiq_core: use min_t macro
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (12 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 13/18] staging: vchiq_core: avoid ternary operator for set_service_state Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 15/18] staging: vchiq_arm: make vchiq_get_state return early Stefan Wahren
` (4 subsequent siblings)
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
Don't try to open code min_t().
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 9 +++------
1 file changed, 3 insertions(+), 6 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 6127244..b32cd12 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1035,8 +1035,7 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
VCHIQ_LOG_INFO))
vchiq_log_dump_mem("Sent", 0,
header->data,
- min((size_t)16,
- (size_t)callback_result));
+ min_t(size_t, 16, callback_result));
spin_lock("a_spinlock);
quota->message_use_count++;
@@ -1184,8 +1183,7 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
VCHIQ_LOG_INFO))
vchiq_log_dump_mem("Sent", 0,
header->data,
- min((size_t)16,
- (size_t)callback_result));
+ min_t(size_t, 16, callback_result));
VCHIQ_SERVICE_STATS_INC(service, ctrl_tx_count);
VCHIQ_SERVICE_STATS_ADD(service, ctrl_tx_bytes, size);
@@ -2209,8 +2207,7 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
state->default_slot_quota = state->slot_queue_available / 2;
state->default_message_quota =
- min((unsigned short)(state->default_slot_quota * 256),
- (unsigned short)~0);
+ min_t(unsigned short, state->default_slot_quota * 256, ~0);
state->previous_data_index = -1;
state->data_use_count = 0;
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 15/18] staging: vchiq_arm: make vchiq_get_state return early
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (13 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 14/18] staging: vchiq_core: use min_t macro Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 16/18] staging: vchiq_arm: Avoid NULL ptr deref in vchiq_dump_platform_instances Stefan Wahren
` (3 subsequent siblings)
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
Make vchiq_get_state return early with NULL and improve the readability.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 9748044..6ef8f19 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1285,14 +1285,18 @@ int vchiq_dump_platform_service_state(void *dump_context,
struct vchiq_state *
vchiq_get_state(void)
{
- if (!g_state.remote)
+ if (!g_state.remote) {
pr_err("%s: g_state.remote == NULL\n", __func__);
- else if (g_state.remote->initialised != 1)
+ return NULL;
+ }
+
+ if (g_state.remote->initialised != 1) {
pr_notice("%s: g_state.remote->initialised != 1 (%d)\n",
__func__, g_state.remote->initialised);
+ return NULL;
+ }
- return (g_state.remote &&
- (g_state.remote->initialised == 1)) ? &g_state : NULL;
+ return &g_state;
}
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 16/18] staging: vchiq_arm: Avoid NULL ptr deref in vchiq_dump_platform_instances
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (14 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 15/18] staging: vchiq_arm: make vchiq_get_state return early Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 17/18] staging: vchiq_core: handle NULL result of find_service_by_handle Stefan Wahren
` (2 subsequent siblings)
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
vchiq_get_state() can return a NULL pointer. So handle this cases and
avoid a NULL pointer derefence in vchiq_dump_platform_instances.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 6ef8f19..a9b6e4a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1204,6 +1204,9 @@ int vchiq_dump_platform_instances(void *dump_context)
int len;
int i;
+ if (!state)
+ return -ENOTCONN;
+
/*
* There is no list of instances, so instead scan all services,
* marking those that have been dumped.
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 17/18] staging: vchiq_core: handle NULL result of find_service_by_handle
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (15 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 16/18] staging: vchiq_arm: Avoid NULL ptr deref in vchiq_dump_platform_instances Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-23 20:02 ` [PATCH 18/18] staging: vchiq_dev: Avoid unnecessary alloc in vchiq_ioc_create_service Stefan Wahren
2022-01-24 9:52 ` [PATCH 00/18] staging: vchiq_arm: minor adjustments nicolas saenz julienne
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
In case of an invalid handle the function find_servive_by_handle
returns NULL. So take care of this and avoid a NULL pointer dereference.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 6 ++++++
1 file changed, 6 insertions(+)
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 b32cd12..8f99272 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2292,6 +2292,9 @@ void vchiq_msg_queue_push(unsigned int handle, struct vchiq_header *header)
struct vchiq_service *service = find_service_by_handle(handle);
int pos;
+ if (!service)
+ return;
+
while (service->msg_queue_write == service->msg_queue_read +
VCHIQ_MAX_SLOTS) {
if (wait_for_completion_interruptible(&service->msg_queue_pop))
@@ -2312,6 +2315,9 @@ struct vchiq_header *vchiq_msg_hold(unsigned int handle)
struct vchiq_header *header;
int pos;
+ if (!service)
+ return NULL;
+
if (service->msg_queue_write == service->msg_queue_read)
return NULL;
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 18/18] staging: vchiq_dev: Avoid unnecessary alloc in vchiq_ioc_create_service
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (16 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 17/18] staging: vchiq_core: handle NULL result of find_service_by_handle Stefan Wahren
@ 2022-01-23 20:02 ` Stefan Wahren
2022-01-24 9:52 ` [PATCH 00/18] staging: vchiq_arm: minor adjustments nicolas saenz julienne
18 siblings, 0 replies; 25+ messages in thread
From: Stefan Wahren @ 2022-01-23 20:02 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging, Stefan Wahren
There is no need to allocate the user service in case there is a VCHIQ
connection required, but not available. So simply check the error conditions
before doing an expensive memory allocation.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
index 2325844..b41c2a2 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -146,15 +146,14 @@ static int vchiq_ioc_create_service(struct vchiq_instance *instance,
struct vchiq_service_params_kernel params;
int srvstate;
+ if (args->is_open && !instance->connected)
+ return -ENOTCONN;
+
user_service = kmalloc(sizeof(*user_service), GFP_KERNEL);
if (!user_service)
return -ENOMEM;
if (args->is_open) {
- if (!instance->connected) {
- kfree(user_service);
- return -ENOTCONN;
- }
srvstate = VCHIQ_SRVSTATE_OPENING;
} else {
srvstate = instance->connected ?
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 00/18] staging: vchiq_arm: minor adjustments
2022-01-23 20:02 [PATCH 00/18] staging: vchiq_arm: minor adjustments Stefan Wahren
` (17 preceding siblings ...)
2022-01-23 20:02 ` [PATCH 18/18] staging: vchiq_dev: Avoid unnecessary alloc in vchiq_ioc_create_service Stefan Wahren
@ 2022-01-24 9:52 ` nicolas saenz julienne
18 siblings, 0 replies; 25+ messages in thread
From: nicolas saenz julienne @ 2022-01-24 9:52 UTC (permalink / raw)
To: Stefan Wahren, Greg Kroah-Hartman
Cc: bcm-kernel-feedback-list, Phil Elwell, Gaston Gonzalez,
linux-staging
On Sun, 2022-01-23 at 21:02 +0100, Stefan Wahren wrote:
> This is a loose collection of small adjustments to the VCHIQ code
> (mostly coding style, but also bugfixes).
>
> Stefan Wahren (18):
> staging: vchiq_core: fix indentation in vchiq_log_dump_mem
> staging: vchiq_debugfs: get the rid of n_log_entries
> staging: vchiq_arm: introduce is_adjacent_block
> staging: vchiq: convert TODOs into unordered list
> staging: vchiq: drop completed tasks from TODO
> staging: vchiq: add message handling to TODO list
> staging: vchiq_core: fix type of parameter localport
> staging: vchiq_core: simplify vchiq_add_service_internal
> staging: vchiq_core: align return statements in msg_type_str
> staging: vchiq_core: drop prefix of vchiq_set_service_state
> staging: vchiq_core: reduce multi-line statements
> staging: vchiq_core: fix alignment
> staging: vchiq_core: avoid ternary operator for set_service_state
> staging: vchiq_core: use min_t macro
> staging: vchiq_arm: make vchiq_get_state return early
> staging: vchiq_arm: Avoid NULL ptr deref in
> vchiq_dump_platform_instances
> staging: vchiq_core: handle NULL result of find_service_by_handle
> staging: vchiq_dev: Avoid unnecessary alloc in
> vchiq_ioc_create_service
For the whole series:
Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Thanks!
Nicolas
^ permalink raw reply [flat|nested] 25+ messages in thread