[PATCH 00/18] staging: vchiq_arm: minor adjustments

[PATCH 00/18] staging: vchiq_arm: minor adjustments

[Stefan Wahren]

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

 drivers/staging/vc04_services/interface/TODO       |  56 +++------
 .../vc04_services/interface/vchiq_arm/vchiq_arm.c  |  34 +++--
 .../vc04_services/interface/vchiq_arm/vchiq_core.c | 137 +++++++++------------
 .../vc04_services/interface/vchiq_arm/vchiq_core.h |   2 +-
 .../interface/vchiq_arm/vchiq_debugfs.c            |   4 +-
 .../vc04_services/interface/vchiq_arm/vchiq_dev.c  |   7 +-
 6 files changed, 107 insertions(+), 133 deletions(-)

-- 
2.7.4

[PATCH 01/18] staging: vchiq_core: fix indentation in vchiq_log_dump_mem

[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

[PATCH 02/18] staging: vchiq_debugfs: get the rid of n_log_entries

[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

[PATCH 03/18] staging: vchiq_arm: introduce is_adjacent_block

[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

[PATCH 04/18] staging: vchiq: convert TODOs into unordered list

[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

[PATCH 05/18] staging: vchiq: drop completed tasks from TODO

[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

[PATCH 06/18] staging: vchiq: add message handling to TODO list

[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

[PATCH 07/18] staging: vchiq_core: fix type of parameter localport

[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

[PATCH 08/18] staging: vchiq_core: simplify vchiq_add_service_internal

[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

[PATCH 09/18] staging: vchiq_core: align return statements in msg_type_str

[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

[PATCH 10/18] staging: vchiq_core: drop prefix of vchiq_set_service_state

[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

[PATCH 11/18] staging: vchiq_core: reduce multi-line statements

[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

[PATCH 12/18] staging: vchiq_core: fix alignment

[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

[PATCH 13/18] staging: vchiq_core: avoid ternary operator for set_service_state

[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

[PATCH 14/18] staging: vchiq_core: use min_t macro

[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(&quota_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

[PATCH 15/18] staging: vchiq_arm: make vchiq_get_state return early

[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

[PATCH 16/18] staging: vchiq_arm: Avoid NULL ptr deref in vchiq_dump_platform_instances

[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

[PATCH 17/18] staging: vchiq_core: handle NULL result of find_service_by_handle

[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

[PATCH 18/18] staging: vchiq_dev: Avoid unnecessary alloc in vchiq_ioc_create_service

[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

Re: [PATCH 00/18] staging: vchiq_arm: minor adjustments

[nicolas saenz julienne]

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

Re: [PATCH 08/18] staging: vchiq_core: simplify vchiq_add_service_internal

[Pavel Skripkin]

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

Re: [PATCH 08/18] staging: vchiq_core: simplify vchiq_add_service_internal

[Dan Carpenter]

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

Re: [PATCH 08/18] staging: vchiq_core: simplify vchiq_add_service_internal

[Pavel Skripkin]

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

Re: [PATCH 08/18] staging: vchiq_core: simplify vchiq_add_service_internal

[Dan Carpenter]

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

Re: [PATCH 08/18] staging: vchiq_core: simplify vchiq_add_service_internal

[Pavel Skripkin]

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