[PATCH v4 00/11] staging: vc04_services: Drop non-essential global members

[PATCH v4 00/11] staging: vc04_services: Drop non-essential global members

[Umang Jain]

This series aims to drop the remaining (non-essential) global members
to address the following TODO item:
```
* Get rid of all non essential global structures and create a proper per
device structure
```

Mainly the global members are moved to be contained inside platform
driver data. They can be access via platform_get_drvdata().

More re-fractoring has gone into this version now. Please look
for individual commit for details.

---
NOTE: Series has been developed on top of [1]
"[PATCH] staging: vc04_services: Stop kthreads on .remove"

The fate of that patch is still undecided, so we need to take into
account on the direction the discussions take. That will only affect
Patch 10/11 of this series, rest is fairly independent in that regard.
---

Changes in v4:
- New patch to drop vchiq_connected.[ch] files and move the functions
  to vchiq_arm.c (5/11)
- De-globalise remapped memory region pointer (7/11)
- De-globalise global spinlocks too (8/11)
- De-globalise global vchiq_pointer g_state (10/11)
- commit message updates and trivial variable renaming

Changes in v3:
- Rework 2/6 to 5/6 as per Laurent's review in v2 [2].
- Add a comment for g_regs global __iomem ptr.

[1]: https://lore.kernel.org/linux-staging/4ba0d745-fc8d-4886-b71a-1f19962e9103@moroto.mountain/T/#m440ee992442cc82ea43092b7c895823c918d105f

Changes in v2:
- Found even more g_* global variables than v1, so new patches to drop
  them
- Introduce 1/6 as suggested during v1 review
- Introuce 6/6 to cleanup the TODO list

[1]: https://lore.kernel.org/linux-staging/171161507013.3072637.12125782507523919379@ping.linuxembedded.co.uk/T/#m1d3de7d2fa73b2447274858353bbd4a0c3a8ba14



Umang Jain (11):
  staging: vc04_services: Drop g_once_init global variable
  staging: vc04_services: vchiq_arm: Split driver static and runtime
    data
  staging: vc04_services: vchiq_arm: Drop g_cache_line_size
  staging: vc04_services: Move variables for tracking connections
  staging: vc04_services: Drop vchiq_connected.[ch] files
  staging: vc04_services: Move global variables tracking allocated pages
  staging: vc04_services: Move global memory mapped pointer
  staging: vc04_services: Move spinlocks to vchiq_state
  staging: vc04_services: vchiq_mmal: Rename service_callback()
  staging: vc04_services: Move global g_state vchiq_state pointer
  staging: vc04_services: Drop completed TODO item

 drivers/staging/vc04_services/Makefile        |   1 -
 .../bcm2835-camera/bcm2835-camera.c           |   4 +-
 .../include/linux/raspberrypi/vchiq.h         |   4 +-
 drivers/staging/vc04_services/interface/TODO  |  15 --
 .../interface/vchiq_arm/vchiq_arm.c           | 253 ++++++++++--------
 .../interface/vchiq_arm/vchiq_arm.h           |  56 +++-
 .../interface/vchiq_arm/vchiq_bus.c           |   3 +
 .../interface/vchiq_arm/vchiq_bus.h           |   3 +
 .../interface/vchiq_arm/vchiq_connected.c     |  74 -----
 .../interface/vchiq_arm/vchiq_connected.h     |  12 -
 .../interface/vchiq_arm/vchiq_core.c          |  51 ++--
 .../interface/vchiq_arm/vchiq_core.h          |  10 +-
 .../interface/vchiq_arm/vchiq_debugfs.c       |   5 +-
 .../interface/vchiq_arm/vchiq_dev.c           |  34 +--
 .../vc04_services/vchiq-mmal/mmal-vchiq.c     |  14 +-
 .../vc04_services/vchiq-mmal/mmal-vchiq.h     |   3 +-
 16 files changed, 265 insertions(+), 277 deletions(-)
 delete mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
 delete mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h

-- 
2.43.0

[PATCH v4 01/11] staging: vc04_services: Drop g_once_init global variable

[Umang Jain]

g_once_init is not used in a meaningful way anywhere. Drop it
along with connected_init() which sets it.

Suggested-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_connected.c            | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
index 3cad13f09e37..4604a2f4d2de 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
@@ -11,16 +11,8 @@
 static   int                        g_connected;
 static   int                        g_num_deferred_callbacks;
 static   void (*g_deferred_callback[MAX_CALLBACKS])(void);
-static   int                        g_once_init;
 static   DEFINE_MUTEX(g_connected_mutex);
 
-/* Function to initialize our lock */
-static void connected_init(void)
-{
-	if (!g_once_init)
-		g_once_init = 1;
-}
-
 /*
  * This function is used to defer initialization until the vchiq stack is
  * initialized. If the stack is already initialized, then the callback will
@@ -29,8 +21,6 @@ static void connected_init(void)
  */
 void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
 {
-	connected_init();
-
 	if (mutex_lock_killable(&g_connected_mutex))
 		return;
 
@@ -60,8 +50,6 @@ void vchiq_call_connected_callbacks(void)
 {
 	int i;
 
-	connected_init();
-
 	if (mutex_lock_killable(&g_connected_mutex))
 		return;
 
-- 
2.43.0

[PATCH v4 02/11] staging: vc04_services: vchiq_arm: Split driver static and runtime data

[Umang Jain]

vchiq_drvdata combines two types of book-keeping data. There is
platform-specific static data (for e.g. cache lines size) and then
data needed for book-keeping at runtime.

Split the data into two structures: struct vchiq_platform_info and
struct vchiq_drv_mgmt. The vchiq_drv_mgmt is allocated at runtime
during probe and will be extended in subsequent patches to remove
all global variables allocated.

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 42 ++++++++++---------
 .../interface/vchiq_arm/vchiq_arm.h           | 11 +++++
 2 files changed, 33 insertions(+), 20 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 9af77d17a1e8..9273767f25df 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -71,16 +71,11 @@ struct vchiq_state g_state;
 static struct vchiq_device *bcm2835_audio;
 static struct vchiq_device *bcm2835_camera;
 
-struct vchiq_drvdata {
-	const unsigned int cache_line_size;
-	struct rpi_firmware *fw;
-};
-
-static struct vchiq_drvdata bcm2835_drvdata = {
+static const struct vchiq_platform_info bcm2835_info = {
 	.cache_line_size = 32,
 };
 
-static struct vchiq_drvdata bcm2836_drvdata = {
+static const struct vchiq_platform_info bcm2836_info = {
 	.cache_line_size = 64,
 };
 
@@ -466,8 +461,8 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
 static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
 {
 	struct device *dev = &pdev->dev;
-	struct vchiq_drvdata *drvdata = platform_get_drvdata(pdev);
-	struct rpi_firmware *fw = drvdata->fw;
+	struct vchiq_drv_mgmt *drv_mgmt = platform_get_drvdata(pdev);
+	struct rpi_firmware *fw = drv_mgmt->fw;
 	struct vchiq_slot_zero *vchiq_slot_zero;
 	void *slot_mem;
 	dma_addr_t slot_phys;
@@ -484,7 +479,7 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 	if (err < 0)
 		return err;
 
-	g_cache_line_size = drvdata->cache_line_size;
+	g_cache_line_size = drv_mgmt->info->cache_line_size;
 	g_fragments_size = 2 * g_cache_line_size;
 
 	/* Allocate space for the channels in coherent memory */
@@ -1707,8 +1702,8 @@ void vchiq_platform_conn_state_changed(struct vchiq_state *state,
 }
 
 static const struct of_device_id vchiq_of_match[] = {
-	{ .compatible = "brcm,bcm2835-vchiq", .data = &bcm2835_drvdata },
-	{ .compatible = "brcm,bcm2836-vchiq", .data = &bcm2836_drvdata },
+	{ .compatible = "brcm,bcm2835-vchiq", .data = &bcm2835_info },
+	{ .compatible = "brcm,bcm2836-vchiq", .data = &bcm2836_info },
 	{},
 };
 MODULE_DEVICE_TABLE(of, vchiq_of_match);
@@ -1716,13 +1711,12 @@ MODULE_DEVICE_TABLE(of, vchiq_of_match);
 static int vchiq_probe(struct platform_device *pdev)
 {
 	struct device_node *fw_node;
-	const struct of_device_id *of_id;
-	struct vchiq_drvdata *drvdata;
+	const struct vchiq_platform_info *info;
+	struct vchiq_drv_mgmt *mgmt;
 	int err;
 
-	of_id = of_match_node(vchiq_of_match, pdev->dev.of_node);
-	drvdata = (struct vchiq_drvdata *)of_id->data;
-	if (!drvdata)
+	info = of_device_get_match_data(&pdev->dev);
+	if (!info)
 		return -EINVAL;
 
 	fw_node = of_find_compatible_node(NULL, NULL,
@@ -1732,12 +1726,17 @@ static int vchiq_probe(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
-	drvdata->fw = devm_rpi_firmware_get(&pdev->dev, fw_node);
+	mgmt = kzalloc(sizeof(*mgmt), GFP_KERNEL);
+	if (!mgmt)
+		return -ENOMEM;
+
+	mgmt->fw = devm_rpi_firmware_get(&pdev->dev, fw_node);
 	of_node_put(fw_node);
-	if (!drvdata->fw)
+	if (!mgmt->fw)
 		return -EPROBE_DEFER;
 
-	platform_set_drvdata(pdev, drvdata);
+	mgmt->info = info;
+	platform_set_drvdata(pdev, mgmt);
 
 	err = vchiq_platform_init(pdev, &g_state);
 	if (err)
@@ -1772,6 +1771,7 @@ static int vchiq_probe(struct platform_device *pdev)
 static void vchiq_remove(struct platform_device *pdev)
 {
 	struct vchiq_state *state = vchiq_get_state();
+	struct vchiq_drv_mgmt *mgmt = dev_get_drvdata(&pdev->dev);
 
 	vchiq_device_unregister(bcm2835_audio);
 	vchiq_device_unregister(bcm2835_camera);
@@ -1781,6 +1781,8 @@ static void vchiq_remove(struct platform_device *pdev)
 	kthread_stop(state->sync_thread);
 	kthread_stop(state->recycle_thread);
 	kthread_stop(state->slot_handler_thread);
+
+	kfree(mgmt);
 }
 
 static struct platform_driver vchiq_driver = {
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index 7844ef765a00..04683d98cd00 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -20,11 +20,22 @@
 #define MAX_ELEMENTS 8
 #define MSG_QUEUE_SIZE 128
 
+struct rpi_firmware;
+
 enum USE_TYPE_E {
 	USE_TYPE_SERVICE,
 	USE_TYPE_VCHIQ
 };
 
+struct vchiq_platform_info {
+	unsigned int cache_line_size;
+};
+
+struct vchiq_drv_mgmt {
+	struct rpi_firmware *fw;
+	const struct vchiq_platform_info *info;
+};
+
 struct user_service {
 	struct vchiq_service *service;
 	void __user *userdata;
-- 
2.43.0

[PATCH v4 03/11] staging: vc04_services: vchiq_arm: Drop g_cache_line_size

[Umang Jain]

The cache-line-size is cached in struct vchiq_platform_info.
There is no need to cache this again via g_cache_line_size
and use it. Instead use the value from vchiq_platform_info directly.

While at it, move the comment about L2 cache line size in the kerneldoc
block of struct vchiq_platform_info.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 34 ++++++++-----------
 .../interface/vchiq_arm/vchiq_arm.h           | 15 ++++++++
 2 files changed, 29 insertions(+), 20 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 9273767f25df..0dffc36dccd7 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -131,17 +131,6 @@ struct vchiq_pagelist_info {
 };
 
 static void __iomem *g_regs;
-/* This value is the size of the L2 cache lines as understood by the
- * VPU firmware, which determines the required alignment of the
- * offsets/sizes in pagelists.
- *
- * Modern VPU firmware looks for a DT "cache-line-size" property in
- * the VCHIQ node and will overwrite it with the actual L2 cache size,
- * which the kernel must then respect.  That property was rejected
- * upstream, so we have to use the VPU firmware's compatibility value
- * of 32.
- */
-static unsigned int g_cache_line_size = 32;
 static unsigned int g_fragments_size;
 static char *g_fragments_base;
 static char *g_free_fragments;
@@ -212,6 +201,7 @@ static struct vchiq_pagelist_info *
 create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
 		size_t count, unsigned short type)
 {
+	struct vchiq_drv_mgmt *drv_mgmt;
 	struct pagelist *pagelist;
 	struct vchiq_pagelist_info *pagelistinfo;
 	struct page **pages;
@@ -226,6 +216,8 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
 	if (count >= INT_MAX - PAGE_SIZE)
 		return NULL;
 
+	drv_mgmt = dev_get_drvdata(instance->state->dev->parent);
+
 	if (buf)
 		offset = (uintptr_t)buf & (PAGE_SIZE - 1);
 	else
@@ -368,9 +360,9 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
 
 	/* Partial cache lines (fragments) require special measures */
 	if ((type == PAGELIST_READ) &&
-	    ((pagelist->offset & (g_cache_line_size - 1)) ||
+	    ((pagelist->offset & (drv_mgmt->info->cache_line_size - 1)) ||
 	    ((pagelist->offset + pagelist->length) &
-	    (g_cache_line_size - 1)))) {
+	    (drv_mgmt->info->cache_line_size - 1)))) {
 		char *fragments;
 
 		if (down_interruptible(&g_free_fragments_sema)) {
@@ -396,12 +388,15 @@ static void
 free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagelistinfo,
 	      int actual)
 {
+	struct vchiq_drv_mgmt *drv_mgmt;
 	struct pagelist *pagelist = pagelistinfo->pagelist;
 	struct page **pages = pagelistinfo->pages;
 	unsigned int num_pages = pagelistinfo->num_pages;
 
 	dev_dbg(instance->state->dev, "arm: %pK, %d\n", pagelistinfo->pagelist, actual);
 
+	drv_mgmt = dev_get_drvdata(instance->state->dev->parent);
+
 	/*
 	 * NOTE: dma_unmap_sg must be called before the
 	 * cpu can touch any of the data/pages.
@@ -417,10 +412,10 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
 			g_fragments_size;
 		int head_bytes, tail_bytes;
 
-		head_bytes = (g_cache_line_size - pagelist->offset) &
-			(g_cache_line_size - 1);
+		head_bytes = (drv_mgmt->info->cache_line_size - pagelist->offset) &
+			(drv_mgmt->info->cache_line_size - 1);
 		tail_bytes = (pagelist->offset + actual) &
-			(g_cache_line_size - 1);
+			(drv_mgmt->info->cache_line_size - 1);
 
 		if ((actual >= 0) && (head_bytes != 0)) {
 			if (head_bytes > actual)
@@ -435,8 +430,8 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
 		    (tail_bytes != 0))
 			memcpy_to_page(pages[num_pages - 1],
 				(pagelist->offset + actual) &
-				(PAGE_SIZE - 1) & ~(g_cache_line_size - 1),
-				fragments + g_cache_line_size,
+				(PAGE_SIZE - 1) & ~(drv_mgmt->info->cache_line_size - 1),
+				fragments + drv_mgmt->info->cache_line_size,
 				tail_bytes);
 
 		down(&g_free_fragments_mutex);
@@ -479,8 +474,7 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 	if (err < 0)
 		return err;
 
-	g_cache_line_size = drv_mgmt->info->cache_line_size;
-	g_fragments_size = 2 * g_cache_line_size;
+	g_fragments_size = 2 * drv_mgmt->info->cache_line_size;
 
 	/* Allocate space for the channels in coherent memory */
 	slot_mem_size = PAGE_ALIGN(TOTAL_SLOTS * VCHIQ_SLOT_SIZE);
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index 04683d98cd00..378415b205ef 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -27,6 +27,21 @@ enum USE_TYPE_E {
 	USE_TYPE_VCHIQ
 };
 
+/**
+ * struct vchiq_platform_info - Platform information related to
+ * underlying hardware.
+ *
+ * @cache_line_size: value of L2 cache line
+ *
+ * cache_line_size is the size of the L2 cache lines as understood by the
+ * VPU firmware, which determines the required alignment of the offsets/sizes
+ * in pagelists.
+ *
+ * Modern VPU firmware looks for a DT "cache-line-size" property in the
+ * VCHIQ node and will overwrite it with the actual L2 cache size, which
+ * the kernel must then respect. That property was rejected upstream, so
+ * we use a value derived from the compatible string.
+ */
 struct vchiq_platform_info {
 	unsigned int cache_line_size;
 };
-- 
2.43.0

[PATCH v4 04/11] staging: vc04_services: Move variables for tracking connections

[Umang Jain]

Connections to the vchiq driver are tracked using global variables.
Drop those and introduce them as members of struct vchiq_drv_mgmt.
Hence, struct vchiq_drv_mgmt will be used to track the connections.

Also, store a vchiq_drv_mgmt pointer to struct vchiq_device to
have easy access to struct vchiq_drv_mgmt across vchiq devices.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           |  3 +-
 .../interface/vchiq_arm/vchiq_arm.h           |  9 +++++
 .../interface/vchiq_arm/vchiq_bus.c           |  3 ++
 .../interface/vchiq_arm/vchiq_bus.h           |  3 ++
 .../interface/vchiq_arm/vchiq_connected.c     | 36 +++++++++----------
 .../interface/vchiq_arm/vchiq_connected.h     |  4 ++-
 6 files changed, 37 insertions(+), 21 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 0dffc36dccd7..4d14ea1becaa 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -545,7 +545,8 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 	dev_dbg(&pdev->dev, "arm: vchiq_init - done (slots %pK, phys %pad)\n",
 		vchiq_slot_zero, &slot_phys);
 
-	vchiq_call_connected_callbacks();
+	mutex_init(&drv_mgmt->connected_mutex);
+	vchiq_call_connected_callbacks(drv_mgmt);
 
 	return 0;
 }
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index 378415b205ef..a774485935a2 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -20,6 +20,8 @@
 #define MAX_ELEMENTS 8
 #define MSG_QUEUE_SIZE 128
 
+#define VCHIQ_DRV_MAX_CALLBACKS 10
+
 struct rpi_firmware;
 
 enum USE_TYPE_E {
@@ -49,6 +51,13 @@ struct vchiq_platform_info {
 struct vchiq_drv_mgmt {
 	struct rpi_firmware *fw;
 	const struct vchiq_platform_info *info;
+
+	bool connected;
+	int num_deferred_callbacks;
+	/* Protects connected and num_deferred_callbacks */
+	struct mutex connected_mutex;
+
+	void (*deferred_callback[VCHIQ_DRV_MAX_CALLBACKS])(void);
 };
 
 struct user_service {
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
index 93609716df84..3f87b93c6537 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
@@ -11,6 +11,7 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 
+#include "vchiq_arm.h"
 #include "vchiq_bus.h"
 
 static int vchiq_bus_type_match(struct device *dev, struct device_driver *drv)
@@ -77,6 +78,8 @@ vchiq_device_register(struct device *parent, const char *name)
 	device->dev.dma_mask = &device->dev.coherent_dma_mask;
 	device->dev.release = vchiq_device_release;
 
+	device->drv_mgmt = dev_get_drvdata(parent);
+
 	of_dma_configure(&device->dev, parent->of_node, true);
 
 	ret = device_register(&device->dev);
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h
index 4db86e76edbd..9de179b39f85 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h
@@ -9,8 +9,11 @@
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
 
+struct vchiq_drv_mgmt;
+
 struct vchiq_device {
 	struct device dev;
+	struct vchiq_drv_mgmt *drv_mgmt;
 };
 
 struct vchiq_driver {
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
index 4604a2f4d2de..049b3f2d1bd4 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
 /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
 
+#include "vchiq_arm.h"
 #include "vchiq_connected.h"
 #include "vchiq_core.h"
 #include <linux/module.h>
@@ -8,11 +9,6 @@
 
 #define  MAX_CALLBACKS  10
 
-static   int                        g_connected;
-static   int                        g_num_deferred_callbacks;
-static   void (*g_deferred_callback[MAX_CALLBACKS])(void);
-static   DEFINE_MUTEX(g_connected_mutex);
-
 /*
  * This function is used to defer initialization until the vchiq stack is
  * initialized. If the stack is already initialized, then the callback will
@@ -21,24 +17,26 @@ static   DEFINE_MUTEX(g_connected_mutex);
  */
 void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
 {
-	if (mutex_lock_killable(&g_connected_mutex))
+	struct vchiq_drv_mgmt *drv_mgmt = device->drv_mgmt;
+
+	if (mutex_lock_killable(&drv_mgmt->connected_mutex))
 		return;
 
-	if (g_connected) {
+	if (drv_mgmt->connected) {
 		/* We're already connected. Call the callback immediately. */
 		callback();
 	} else {
-		if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
+		if (drv_mgmt->num_deferred_callbacks >= MAX_CALLBACKS) {
 			dev_err(&device->dev,
 				"core: There already %d callback registered - please increase MAX_CALLBACKS\n",
-				g_num_deferred_callbacks);
+				drv_mgmt->num_deferred_callbacks);
 		} else {
-			g_deferred_callback[g_num_deferred_callbacks] =
+			drv_mgmt->deferred_callback[drv_mgmt->num_deferred_callbacks] =
 				callback;
-			g_num_deferred_callbacks++;
+			drv_mgmt->num_deferred_callbacks++;
 		}
 	}
-	mutex_unlock(&g_connected_mutex);
+	mutex_unlock(&drv_mgmt->connected_mutex);
 }
 EXPORT_SYMBOL(vchiq_add_connected_callback);
 
@@ -46,17 +44,17 @@ EXPORT_SYMBOL(vchiq_add_connected_callback);
  * This function is called by the vchiq stack once it has been connected to
  * the videocore and clients can start to use the stack.
  */
-void vchiq_call_connected_callbacks(void)
+void vchiq_call_connected_callbacks(struct vchiq_drv_mgmt *drv_mgmt)
 {
 	int i;
 
-	if (mutex_lock_killable(&g_connected_mutex))
+	if (mutex_lock_killable(&drv_mgmt->connected_mutex))
 		return;
 
-	for (i = 0; i <  g_num_deferred_callbacks; i++)
-		g_deferred_callback[i]();
+	for (i = 0; i <  drv_mgmt->num_deferred_callbacks; i++)
+		drv_mgmt->deferred_callback[i]();
 
-	g_num_deferred_callbacks = 0;
-	g_connected = 1;
-	mutex_unlock(&g_connected_mutex);
+	drv_mgmt->num_deferred_callbacks = 0;
+	drv_mgmt->connected = true;
+	mutex_unlock(&drv_mgmt->connected_mutex);
 }
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
index e4ed56446f8a..f57a878652b1 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
@@ -6,7 +6,9 @@
 #ifndef VCHIQ_CONNECTED_H
 #define VCHIQ_CONNECTED_H
 
+struct vchiq_drv_mgmt;
+
 void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
-void vchiq_call_connected_callbacks(void);
+void vchiq_call_connected_callbacks(struct vchiq_drv_mgmt *mgmt);
 
 #endif /* VCHIQ_CONNECTED_H */
-- 
2.43.0

[PATCH v4 05/11] staging: vc04_services: Drop vchiq_connected.[ch] files

[Umang Jain]

The vchiq_connected.[ch] just implements two function:

- vchiq_add_connected_callback()
- vchiq_call_connected_callbacks()

for the deferred vchiq callbacks. Those can easily live in
vchiq_arm.[ch], hence move them.

The move doesn't copy over MAX_CALLBACKS because it is the
same as VCHIQ_DRV_MAX_CALLBACKS. Hence, it now being used in
vchiq_add_connected_callback().

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/staging/vc04_services/Makefile        |  1 -
 .../interface/vchiq_arm/vchiq_arm.c           | 51 +++++++++++++++-
 .../interface/vchiq_arm/vchiq_arm.h           |  5 ++
 .../interface/vchiq_arm/vchiq_connected.c     | 60 -------------------
 .../interface/vchiq_arm/vchiq_connected.h     | 14 -----
 5 files changed, 55 insertions(+), 76 deletions(-)
 delete mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
 delete mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h

diff --git a/drivers/staging/vc04_services/Makefile b/drivers/staging/vc04_services/Makefile
index e8b897a7b9a6..dad3789522b8 100644
--- a/drivers/staging/vc04_services/Makefile
+++ b/drivers/staging/vc04_services/Makefile
@@ -6,7 +6,6 @@ vchiq-objs := \
    interface/vchiq_arm/vchiq_arm.o \
    interface/vchiq_arm/vchiq_bus.o \
    interface/vchiq_arm/vchiq_debugfs.o \
-   interface/vchiq_arm/vchiq_connected.o \
 
 ifdef CONFIG_VCHIQ_CDEV
 vchiq-objs += interface/vchiq_arm/vchiq_dev.o
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 4d14ea1becaa..2005d392d0b7 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -36,7 +36,6 @@
 #include "vchiq_arm.h"
 #include "vchiq_bus.h"
 #include "vchiq_debugfs.h"
-#include "vchiq_connected.h"
 #include "vchiq_pagelist.h"
 
 #define DEVICE_NAME "vchiq"
@@ -189,6 +188,56 @@ is_adjacent_block(u32 *addrs, u32 addr, unsigned int k)
 	return tmp == (addr & PAGE_MASK);
 }
 
+/*
+ * This function is called by the vchiq stack once it has been connected to
+ * the videocore and clients can start to use the stack.
+ */
+static void vchiq_call_connected_callbacks(struct vchiq_drv_mgmt *drv_mgmt)
+{
+	int i;
+
+	if (mutex_lock_killable(&drv_mgmt->connected_mutex))
+		return;
+
+	for (i = 0; i < drv_mgmt->num_deferred_callbacks; i++)
+		drv_mgmt->deferred_callback[i]();
+
+	drv_mgmt->num_deferred_callbacks = 0;
+	drv_mgmt->connected = true;
+	mutex_unlock(&drv_mgmt->connected_mutex);
+}
+
+/*
+ * This function is used to defer initialization until the vchiq stack is
+ * initialized. If the stack is already initialized, then the callback will
+ * be made immediately, otherwise it will be deferred until
+ * vchiq_call_connected_callbacks is called.
+ */
+void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
+{
+	struct vchiq_drv_mgmt *drv_mgmt = device->drv_mgmt;
+
+	if (mutex_lock_killable(&drv_mgmt->connected_mutex))
+		return;
+
+	if (drv_mgmt->connected) {
+		/* We're already connected. Call the callback immediately. */
+		callback();
+	} else {
+		if (drv_mgmt->num_deferred_callbacks >= VCHIQ_DRV_MAX_CALLBACKS) {
+			dev_err(&device->dev,
+				"core: deferred callbacks(%d) exceeded the maximum limit(%d)\n",
+				drv_mgmt->num_deferred_callbacks, VCHIQ_DRV_MAX_CALLBACKS);
+		} else {
+			drv_mgmt->deferred_callback[drv_mgmt->num_deferred_callbacks] =
+				callback;
+			drv_mgmt->num_deferred_callbacks++;
+		}
+	}
+	mutex_unlock(&drv_mgmt->connected_mutex);
+}
+EXPORT_SYMBOL(vchiq_add_connected_callback);
+
 /* 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,
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index a774485935a2..4a5b5ae9625a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -23,6 +23,7 @@
 #define VCHIQ_DRV_MAX_CALLBACKS 10
 
 struct rpi_firmware;
+struct vchiq_device;
 
 enum USE_TYPE_E {
 	USE_TYPE_SERVICE,
@@ -147,6 +148,10 @@ vchiq_instance_get_trace(struct vchiq_instance *instance);
 extern void
 vchiq_instance_set_trace(struct vchiq_instance *instance, int trace);
 
+extern void
+vchiq_add_connected_callback(struct vchiq_device *device,
+			     void (*callback)(void));
+
 #if IS_ENABLED(CONFIG_VCHIQ_CDEV)
 
 extern void
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
deleted file mode 100644
index 049b3f2d1bd4..000000000000
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
+++ /dev/null
@@ -1,60 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
-/* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
-
-#include "vchiq_arm.h"
-#include "vchiq_connected.h"
-#include "vchiq_core.h"
-#include <linux/module.h>
-#include <linux/mutex.h>
-
-#define  MAX_CALLBACKS  10
-
-/*
- * This function is used to defer initialization until the vchiq stack is
- * initialized. If the stack is already initialized, then the callback will
- * be made immediately, otherwise it will be deferred until
- * vchiq_call_connected_callbacks is called.
- */
-void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
-{
-	struct vchiq_drv_mgmt *drv_mgmt = device->drv_mgmt;
-
-	if (mutex_lock_killable(&drv_mgmt->connected_mutex))
-		return;
-
-	if (drv_mgmt->connected) {
-		/* We're already connected. Call the callback immediately. */
-		callback();
-	} else {
-		if (drv_mgmt->num_deferred_callbacks >= MAX_CALLBACKS) {
-			dev_err(&device->dev,
-				"core: There already %d callback registered - please increase MAX_CALLBACKS\n",
-				drv_mgmt->num_deferred_callbacks);
-		} else {
-			drv_mgmt->deferred_callback[drv_mgmt->num_deferred_callbacks] =
-				callback;
-			drv_mgmt->num_deferred_callbacks++;
-		}
-	}
-	mutex_unlock(&drv_mgmt->connected_mutex);
-}
-EXPORT_SYMBOL(vchiq_add_connected_callback);
-
-/*
- * This function is called by the vchiq stack once it has been connected to
- * the videocore and clients can start to use the stack.
- */
-void vchiq_call_connected_callbacks(struct vchiq_drv_mgmt *drv_mgmt)
-{
-	int i;
-
-	if (mutex_lock_killable(&drv_mgmt->connected_mutex))
-		return;
-
-	for (i = 0; i <  drv_mgmt->num_deferred_callbacks; i++)
-		drv_mgmt->deferred_callback[i]();
-
-	drv_mgmt->num_deferred_callbacks = 0;
-	drv_mgmt->connected = true;
-	mutex_unlock(&drv_mgmt->connected_mutex);
-}
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
deleted file mode 100644
index f57a878652b1..000000000000
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
+++ /dev/null
@@ -1,14 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
-/* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
-
-#include "vchiq_bus.h"
-
-#ifndef VCHIQ_CONNECTED_H
-#define VCHIQ_CONNECTED_H
-
-struct vchiq_drv_mgmt;
-
-void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
-void vchiq_call_connected_callbacks(struct vchiq_drv_mgmt *mgmt);
-
-#endif /* VCHIQ_CONNECTED_H */
-- 
2.43.0

[PATCH v4 06/11] staging: vc04_services: Move global variables tracking allocated pages

[Umang Jain]

The variables tracking allocated pages fragments in vchiq_arm.c
can be easily moved to struct vchiq_drv_mgmt instead of being global.
This helps us to drop the non-essential global variables in the vchiq
interface.

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 53 +++++++++----------
 .../interface/vchiq_arm/vchiq_arm.h           |  6 +++
 2 files changed, 30 insertions(+), 29 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 2005d392d0b7..b335948f4b0c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -130,12 +130,6 @@ struct vchiq_pagelist_info {
 };
 
 static void __iomem *g_regs;
-static unsigned int g_fragments_size;
-static char *g_fragments_base;
-static char *g_free_fragments;
-static struct semaphore g_free_fragments_sema;
-
-static DEFINE_SEMAPHORE(g_free_fragments_mutex, 1);
 
 static int
 vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
@@ -414,20 +408,20 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
 	    (drv_mgmt->info->cache_line_size - 1)))) {
 		char *fragments;
 
-		if (down_interruptible(&g_free_fragments_sema)) {
+		if (down_interruptible(&drv_mgmt->free_fragments_sema)) {
 			cleanup_pagelistinfo(instance, pagelistinfo);
 			return NULL;
 		}
 
-		WARN_ON(!g_free_fragments);
+		WARN_ON(!drv_mgmt->free_fragments);
 
-		down(&g_free_fragments_mutex);
-		fragments = g_free_fragments;
+		down(&drv_mgmt->free_fragments_mutex);
+		fragments = drv_mgmt->free_fragments;
 		WARN_ON(!fragments);
-		g_free_fragments = *(char **)g_free_fragments;
-		up(&g_free_fragments_mutex);
+		drv_mgmt->free_fragments = *(char **)drv_mgmt->free_fragments;
+		up(&drv_mgmt->free_fragments_mutex);
 		pagelist->type = PAGELIST_READ_WITH_FRAGMENTS +
-			(fragments - g_fragments_base) / g_fragments_size;
+			(fragments - drv_mgmt->fragments_base) / drv_mgmt->fragments_size;
 	}
 
 	return pagelistinfo;
@@ -455,10 +449,10 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
 	pagelistinfo->scatterlist_mapped = 0;
 
 	/* Deal with any partial cache lines (fragments) */
-	if (pagelist->type >= PAGELIST_READ_WITH_FRAGMENTS && g_fragments_base) {
-		char *fragments = g_fragments_base +
+	if (pagelist->type >= PAGELIST_READ_WITH_FRAGMENTS && drv_mgmt->fragments_base) {
+		char *fragments = drv_mgmt->fragments_base +
 			(pagelist->type - PAGELIST_READ_WITH_FRAGMENTS) *
-			g_fragments_size;
+			drv_mgmt->fragments_size;
 		int head_bytes, tail_bytes;
 
 		head_bytes = (drv_mgmt->info->cache_line_size - pagelist->offset) &
@@ -483,11 +477,11 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
 				fragments + drv_mgmt->info->cache_line_size,
 				tail_bytes);
 
-		down(&g_free_fragments_mutex);
-		*(char **)fragments = g_free_fragments;
-		g_free_fragments = fragments;
-		up(&g_free_fragments_mutex);
-		up(&g_free_fragments_sema);
+		down(&drv_mgmt->free_fragments_mutex);
+		*(char **)fragments = drv_mgmt->free_fragments;
+		drv_mgmt->free_fragments = fragments;
+		up(&drv_mgmt->free_fragments_mutex);
+		up(&drv_mgmt->free_fragments_sema);
 	}
 
 	/* Need to mark all the pages dirty. */
@@ -523,11 +517,11 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 	if (err < 0)
 		return err;
 
-	g_fragments_size = 2 * drv_mgmt->info->cache_line_size;
+	drv_mgmt->fragments_size = 2 * drv_mgmt->info->cache_line_size;
 
 	/* Allocate space for the channels in coherent memory */
 	slot_mem_size = PAGE_ALIGN(TOTAL_SLOTS * VCHIQ_SLOT_SIZE);
-	frag_mem_size = PAGE_ALIGN(g_fragments_size * MAX_FRAGMENTS);
+	frag_mem_size = PAGE_ALIGN(drv_mgmt->fragments_size * MAX_FRAGMENTS);
 
 	slot_mem = dmam_alloc_coherent(dev, slot_mem_size + frag_mem_size,
 				       &slot_phys, GFP_KERNEL);
@@ -547,15 +541,16 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 	vchiq_slot_zero->platform_data[VCHIQ_PLATFORM_FRAGMENTS_COUNT_IDX] =
 		MAX_FRAGMENTS;
 
-	g_fragments_base = (char *)slot_mem + slot_mem_size;
+	drv_mgmt->fragments_base = (char *)slot_mem + slot_mem_size;
 
-	g_free_fragments = g_fragments_base;
+	drv_mgmt->free_fragments = drv_mgmt->fragments_base;
 	for (i = 0; i < (MAX_FRAGMENTS - 1); i++) {
-		*(char **)&g_fragments_base[i * g_fragments_size] =
-			&g_fragments_base[(i + 1) * g_fragments_size];
+		*(char **)&drv_mgmt->fragments_base[i * drv_mgmt->fragments_size] =
+			&drv_mgmt->fragments_base[(i + 1) * drv_mgmt->fragments_size];
 	}
-	*(char **)&g_fragments_base[i * g_fragments_size] = NULL;
-	sema_init(&g_free_fragments_sema, MAX_FRAGMENTS);
+	*(char **)&drv_mgmt->fragments_base[i * drv_mgmt->fragments_size] = NULL;
+	sema_init(&drv_mgmt->free_fragments_sema, MAX_FRAGMENTS);
+	sema_init(&drv_mgmt->free_fragments_mutex, 1);
 
 	err = vchiq_init_state(state, vchiq_slot_zero, dev);
 	if (err)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index 4a5b5ae9625a..a3bba245bfe2 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -59,6 +59,12 @@ struct vchiq_drv_mgmt {
 	struct mutex connected_mutex;
 
 	void (*deferred_callback[VCHIQ_DRV_MAX_CALLBACKS])(void);
+
+	struct semaphore free_fragments_sema;
+	struct semaphore free_fragments_mutex;
+	char *fragments_base;
+	char *free_fragments;
+	unsigned int fragments_size;
 };
 
 struct user_service {
-- 
2.43.0

[PATCH v4 07/11] staging: vc04_services: Move global memory mapped pointer

[Umang Jain]

g_regs stores the remapped memory pointer for the vchiq platform.
It can be moved to struct vchiq_drv_mgmt instead of being global.

Adjust the affected functions accordingly. Pass vchiq_state pointer
wherever necessary to access struct vchiq_drv_mgmt.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 19 +++++++++++--------
 .../interface/vchiq_arm/vchiq_arm.h           |  2 ++
 .../interface/vchiq_arm/vchiq_core.c          | 10 +++++-----
 .../interface/vchiq_arm/vchiq_core.h          |  2 +-
 4 files changed, 19 insertions(+), 14 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 b335948f4b0c..0c7dd45969ae 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -129,8 +129,6 @@ struct vchiq_pagelist_info {
 	unsigned int scatterlist_mapped;
 };
 
-static void __iomem *g_regs;
-
 static int
 vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
 			     unsigned int size, enum vchiq_bulk_dir dir);
@@ -139,11 +137,14 @@ static irqreturn_t
 vchiq_doorbell_irq(int irq, void *dev_id)
 {
 	struct vchiq_state *state = dev_id;
+	struct vchiq_drv_mgmt *mgmt;
 	irqreturn_t ret = IRQ_NONE;
 	unsigned int status;
 
+	mgmt = dev_get_drvdata(state->dev);
+
 	/* Read (and clear) the doorbell */
-	status = readl(g_regs + BELL0);
+	status = readl(mgmt->regs + BELL0);
 
 	if (status & ARM_DS_ACTIVE) {  /* Was the doorbell rung? */
 		remote_event_pollall(state);
@@ -556,9 +557,9 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 	if (err)
 		return err;
 
-	g_regs = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(g_regs))
-		return PTR_ERR(g_regs);
+	drv_mgmt->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(drv_mgmt->regs))
+		return PTR_ERR(drv_mgmt->regs);
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq <= 0)
@@ -641,8 +642,10 @@ static struct vchiq_arm_state *vchiq_platform_get_arm_state(struct vchiq_state *
 }
 
 void
-remote_event_signal(struct remote_event *event)
+remote_event_signal(struct vchiq_state *state, struct remote_event *event)
 {
+	struct vchiq_drv_mgmt *mgmt = dev_get_drvdata(state->dev);
+
 	/*
 	 * Ensure that all writes to shared data structures have completed
 	 * before signalling the peer.
@@ -654,7 +657,7 @@ remote_event_signal(struct remote_event *event)
 	dsb(sy);         /* data barrier operation */
 
 	if (event->armed)
-		writel(0, g_regs + BELL2); /* trigger vc interrupt */
+		writel(0, mgmt->regs + BELL2); /* trigger vc interrupt */
 }
 
 int
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index a3bba245bfe2..f17c22635ce5 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -65,6 +65,8 @@ struct vchiq_drv_mgmt {
 	char *fragments_base;
 	char *free_fragments;
 	unsigned int fragments_size;
+
+	void __iomem *regs;
 };
 
 struct user_service {
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 658d19f1e7e8..af633ff984f6 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -691,7 +691,7 @@ reserve_space(struct vchiq_state *state, size_t space, int is_blocking)
 			/* But first, flush through the last slot. */
 			state->local_tx_pos = tx_pos;
 			local->tx_pos = tx_pos;
-			remote_event_signal(&state->remote->trigger);
+			remote_event_signal(state, &state->remote->trigger);
 
 			if (!is_blocking ||
 			    (wait_for_completion_interruptible(&state->slot_available_event)))
@@ -1124,7 +1124,7 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
 	if (!(flags & QMFLAGS_NO_MUTEX_UNLOCK))
 		mutex_unlock(&state->slot_mutex);
 
-	remote_event_signal(&state->remote->trigger);
+	remote_event_signal(state, &state->remote->trigger);
 
 	return 0;
 }
@@ -1202,7 +1202,7 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
 		&svc_fourcc, VCHIQ_MSG_SRCPORT(msgid),
 		VCHIQ_MSG_DSTPORT(msgid), size);
 
-	remote_event_signal(&state->remote->sync_trigger);
+	remote_event_signal(state, &state->remote->sync_trigger);
 
 	if (VCHIQ_MSG_TYPE(msgid) != VCHIQ_MSG_PAUSE)
 		mutex_unlock(&state->sync_mutex);
@@ -1260,7 +1260,7 @@ release_slot(struct vchiq_state *state, struct vchiq_slot_info *slot_info,
 		 * A write barrier is necessary, but remote_event_signal
 		 * contains one.
 		 */
-		remote_event_signal(&state->remote->recycle);
+		remote_event_signal(state, &state->remote->recycle);
 	}
 
 	mutex_unlock(&state->recycle_mutex);
@@ -3240,7 +3240,7 @@ static void
 release_message_sync(struct vchiq_state *state, struct vchiq_header *header)
 {
 	header->msgid = VCHIQ_MSGID_PADDING;
-	remote_event_signal(&state->remote->sync_release);
+	remote_event_signal(state, &state->remote->sync_release);
 }
 
 int
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 5fbf173d9c56..8ca74b12427b 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -516,7 +516,7 @@ int vchiq_prepare_bulk_data(struct vchiq_instance *instance, struct vchiq_bulk *
 
 void vchiq_complete_bulk(struct vchiq_instance *instance, struct vchiq_bulk *bulk);
 
-void remote_event_signal(struct remote_event *event);
+void remote_event_signal(struct vchiq_state *state, struct remote_event *event);
 
 void vchiq_dump_platform_state(struct seq_file *f);
 
-- 
2.43.0

[PATCH v4 08/11] staging: vc04_services: Move spinlocks to vchiq_state

[Umang Jain]

The msg_queue_spinlock, quota_spinlock and bulk_waiter_spinlock
are allocated globally. Instead move them to struct vchiq_state
and initialise them in vchiq_init_state().

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 17 ++++----
 .../interface/vchiq_arm/vchiq_arm.h           |  1 -
 .../interface/vchiq_arm/vchiq_core.c          | 39 ++++++++++---------
 .../interface/vchiq_arm/vchiq_core.h          |  6 +++
 .../interface/vchiq_arm/vchiq_dev.c           | 24 ++++++------
 5 files changed, 46 insertions(+), 41 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 0c7dd45969ae..28c6357c13cd 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -59,7 +59,6 @@
 #define KEEPALIVE_VER 1
 #define KEEPALIVE_VER_MIN KEEPALIVE_VER
 
-DEFINE_SPINLOCK(msg_queue_spinlock);
 struct vchiq_state g_state;
 
 /*
@@ -989,9 +988,9 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
 				 * This is not a retry of the previous one.
 				 * Cancel the signal when the transfer completes.
 				 */
-				spin_lock(&bulk_waiter_spinlock);
+				spin_lock(&service->state->bulk_waiter_spinlock);
 				bulk->userdata = NULL;
-				spin_unlock(&bulk_waiter_spinlock);
+				spin_unlock(&service->state->bulk_waiter_spinlock);
 			}
 		}
 	} else {
@@ -1008,9 +1007,9 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
 
 		if (bulk) {
 			/* Cancel the signal when the transfer completes. */
-			spin_lock(&bulk_waiter_spinlock);
+			spin_lock(&service->state->bulk_waiter_spinlock);
 			bulk->userdata = NULL;
-			spin_unlock(&bulk_waiter_spinlock);
+			spin_unlock(&service->state->bulk_waiter_spinlock);
 		}
 		kfree(waiter);
 	} else {
@@ -1131,10 +1130,10 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
 		reason, header, instance, bulk_userdata);
 
 	if (header && user_service->is_vchi) {
-		spin_lock(&msg_queue_spinlock);
+		spin_lock(&service->state->msg_queue_spinlock);
 		while (user_service->msg_insert ==
 			(user_service->msg_remove + MSG_QUEUE_SIZE)) {
-			spin_unlock(&msg_queue_spinlock);
+			spin_unlock(&service->state->msg_queue_spinlock);
 			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 			DEBUG_COUNT(MSG_QUEUE_FULL_COUNT);
 			dev_dbg(service->state->dev, "arm: msg queue full\n");
@@ -1171,7 +1170,7 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
 				return -EINVAL;
 			}
 			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
-			spin_lock(&msg_queue_spinlock);
+			spin_lock(&service->state->msg_queue_spinlock);
 		}
 
 		user_service->msg_queue[user_service->msg_insert &
@@ -1190,7 +1189,7 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
 			skip_completion = true;
 		}
 
-		spin_unlock(&msg_queue_spinlock);
+		spin_unlock(&service->state->msg_queue_spinlock);
 		complete(&user_service->insert_event);
 
 		header = NULL;
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index f17c22635ce5..a8f7a7f7b189 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -113,7 +113,6 @@ struct vchiq_instance {
 	struct vchiq_debugfs_node debugfs_node;
 };
 
-extern spinlock_t msg_queue_spinlock;
 extern struct vchiq_state g_state;
 
 extern struct vchiq_state *
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 af633ff984f6..2e0be0194811 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -149,9 +149,6 @@ static inline void check_sizes(void)
 	BUILD_BUG_ON_NOT_POWER_OF_2(VCHIQ_MAX_SERVICES);
 }
 
-DEFINE_SPINLOCK(bulk_waiter_spinlock);
-static DEFINE_SPINLOCK(quota_spinlock);
-
 static unsigned int handle_seq;
 
 static const char *const srvstate_names[] = {
@@ -724,11 +721,11 @@ process_free_data_message(struct vchiq_state *state, u32 *service_found,
 	struct vchiq_service_quota *quota = &state->service_quotas[port];
 	int count;
 
-	spin_lock(&quota_spinlock);
+	spin_lock(&state->quota_spinlock);
 	count = quota->message_use_count;
 	if (count > 0)
 		quota->message_use_count = count - 1;
-	spin_unlock(&quota_spinlock);
+	spin_unlock(&state->quota_spinlock);
 
 	if (count == quota->message_quota) {
 		/*
@@ -747,11 +744,11 @@ process_free_data_message(struct vchiq_state *state, u32 *service_found,
 		/* Set the found bit for this service */
 		BITSET_SET(service_found, port);
 
-		spin_lock(&quota_spinlock);
+		spin_lock(&state->quota_spinlock);
 		count = quota->slot_use_count;
 		if (count > 0)
 			quota->slot_use_count = count - 1;
-		spin_unlock(&quota_spinlock);
+		spin_unlock(&state->quota_spinlock);
 
 		if (count > 0) {
 			/*
@@ -837,11 +834,11 @@ process_free_queue(struct vchiq_state *state, u32 *service_found,
 		if (data_found) {
 			int count;
 
-			spin_lock(&quota_spinlock);
+			spin_lock(&state->quota_spinlock);
 			count = state->data_use_count;
 			if (count > 0)
 				state->data_use_count = count - 1;
-			spin_unlock(&quota_spinlock);
+			spin_unlock(&state->quota_spinlock);
 			if (count == state->data_quota)
 				complete(&state->data_quota_event);
 		}
@@ -940,7 +937,7 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
 
 		quota = &state->service_quotas[service->localport];
 
-		spin_lock(&quota_spinlock);
+		spin_lock(&state->quota_spinlock);
 
 		/*
 		 * Ensure this service doesn't use more than its quota of
@@ -955,14 +952,14 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
 		while ((tx_end_index != state->previous_data_index) &&
 		       (state->data_use_count == state->data_quota)) {
 			VCHIQ_STATS_INC(state, data_stalls);
-			spin_unlock(&quota_spinlock);
+			spin_unlock(&state->quota_spinlock);
 			mutex_unlock(&state->slot_mutex);
 
 			if (wait_for_completion_interruptible(&state->data_quota_event))
 				return -EAGAIN;
 
 			mutex_lock(&state->slot_mutex);
-			spin_lock(&quota_spinlock);
+			spin_lock(&state->quota_spinlock);
 			tx_end_index = SLOT_QUEUE_INDEX_FROM_POS(state->local_tx_pos + stride - 1);
 			if ((tx_end_index == state->previous_data_index) ||
 			    (state->data_use_count < state->data_quota)) {
@@ -975,7 +972,7 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
 		while ((quota->message_use_count == quota->message_quota) ||
 		       ((tx_end_index != quota->previous_tx_index) &&
 			(quota->slot_use_count == quota->slot_quota))) {
-			spin_unlock(&quota_spinlock);
+			spin_unlock(&state->quota_spinlock);
 			dev_dbg(state->dev,
 				"core: %d: qm:%d %s,%zx - quota stall (msg %d, slot %d)\n",
 				state->id, service->localport, msg_type_str(type), size,
@@ -993,11 +990,11 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
 				mutex_unlock(&state->slot_mutex);
 				return -EHOSTDOWN;
 			}
-			spin_lock(&quota_spinlock);
+			spin_lock(&state->quota_spinlock);
 			tx_end_index = SLOT_QUEUE_INDEX_FROM_POS(state->local_tx_pos + stride - 1);
 		}
 
-		spin_unlock(&quota_spinlock);
+		spin_unlock(&state->quota_spinlock);
 	}
 
 	header = reserve_space(state, stride, flags & QMFLAGS_IS_BLOCKING);
@@ -1040,7 +1037,7 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
 				   header->data,
 				   min_t(size_t, 16, callback_result));
 
-		spin_lock(&quota_spinlock);
+		spin_lock(&state->quota_spinlock);
 		quota->message_use_count++;
 
 		tx_end_index =
@@ -1066,7 +1063,7 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
 			slot_use_count = 0;
 		}
 
-		spin_unlock(&quota_spinlock);
+		spin_unlock(&state->quota_spinlock);
 
 		if (slot_use_count)
 			dev_dbg(state->dev, "core: %d: qm:%d %s,%zx - slot_use->%d (hdr %p)\n",
@@ -1322,13 +1319,13 @@ notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
 			if (bulk->mode == VCHIQ_BULK_MODE_BLOCKING) {
 				struct bulk_waiter *waiter;
 
-				spin_lock(&bulk_waiter_spinlock);
+				spin_lock(&service->state->bulk_waiter_spinlock);
 				waiter = bulk->userdata;
 				if (waiter) {
 					waiter->actual = bulk->actual;
 					complete(&waiter->event);
 				}
-				spin_unlock(&bulk_waiter_spinlock);
+				spin_unlock(&service->state->bulk_waiter_spinlock);
 			} else if (bulk->mode == VCHIQ_BULK_MODE_CALLBACK) {
 				enum vchiq_reason reason =
 						get_bulk_reason(bulk);
@@ -2169,6 +2166,10 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero, s
 	mutex_init(&state->sync_mutex);
 	mutex_init(&state->bulk_transfer_mutex);
 
+	spin_lock_init(&state->msg_queue_spinlock);
+	spin_lock_init(&state->bulk_waiter_spinlock);
+	spin_lock_init(&state->quota_spinlock);
+
 	init_completion(&state->slot_available_event);
 	init_completion(&state->slot_remove_event);
 	init_completion(&state->data_quota_event);
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 8ca74b12427b..33d95e431717 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -348,6 +348,12 @@ struct vchiq_state {
 
 	struct mutex bulk_transfer_mutex;
 
+	struct spinlock_t msg_queue_spinlock;
+
+	struct spinlock_t bulk_waiter_spinlock;
+
+	struct spinlock_t quota_spinlock;
+
 	/*
 	 * Indicates the byte position within the stream from where the next
 	 * message will be read. The least significant bits are an index into
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 4d9deeeb637a..739ce529a71b 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -220,10 +220,10 @@ static int vchiq_ioc_dequeue_message(struct vchiq_instance *instance,
 		goto out;
 	}
 
-	spin_lock(&msg_queue_spinlock);
+	spin_lock(&service->state->msg_queue_spinlock);
 	if (user_service->msg_remove == user_service->msg_insert) {
 		if (!args->blocking) {
-			spin_unlock(&msg_queue_spinlock);
+			spin_unlock(&service->state->msg_queue_spinlock);
 			DEBUG_TRACE(DEQUEUE_MESSAGE_LINE);
 			ret = -EWOULDBLOCK;
 			goto out;
@@ -231,14 +231,14 @@ static int vchiq_ioc_dequeue_message(struct vchiq_instance *instance,
 		user_service->dequeue_pending = 1;
 		ret = 0;
 		do {
-			spin_unlock(&msg_queue_spinlock);
+			spin_unlock(&service->state->msg_queue_spinlock);
 			DEBUG_TRACE(DEQUEUE_MESSAGE_LINE);
 			if (wait_for_completion_interruptible(&user_service->insert_event)) {
 				dev_dbg(service->state->dev, "arm: DEQUEUE_MESSAGE interrupted\n");
 				ret = -EINTR;
 				break;
 			}
-			spin_lock(&msg_queue_spinlock);
+			spin_lock(&service->state->msg_queue_spinlock);
 		} while (user_service->msg_remove == user_service->msg_insert);
 
 		if (ret)
@@ -247,7 +247,7 @@ static int vchiq_ioc_dequeue_message(struct vchiq_instance *instance,
 
 	if (WARN_ON_ONCE((int)(user_service->msg_insert -
 			 user_service->msg_remove) < 0)) {
-		spin_unlock(&msg_queue_spinlock);
+		spin_unlock(&service->state->msg_queue_spinlock);
 		ret = -EINVAL;
 		goto out;
 	}
@@ -255,7 +255,7 @@ static int vchiq_ioc_dequeue_message(struct vchiq_instance *instance,
 	header = user_service->msg_queue[user_service->msg_remove &
 		(MSG_QUEUE_SIZE - 1)];
 	user_service->msg_remove++;
-	spin_unlock(&msg_queue_spinlock);
+	spin_unlock(&service->state->msg_queue_spinlock);
 
 	complete(&user_service->remove_event);
 	if (!header) {
@@ -340,9 +340,9 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 	    !waiter->bulk_waiter.bulk) {
 		if (waiter->bulk_waiter.bulk) {
 			/* Cancel the signal when the transfer completes. */
-			spin_lock(&bulk_waiter_spinlock);
+			spin_lock(&service->state->bulk_waiter_spinlock);
 			waiter->bulk_waiter.bulk->userdata = NULL;
-			spin_unlock(&bulk_waiter_spinlock);
+			spin_unlock(&service->state->bulk_waiter_spinlock);
 		}
 		kfree(waiter);
 		ret = 0;
@@ -1246,7 +1246,7 @@ static int vchiq_release(struct inode *inode, struct file *file)
 			break;
 		}
 
-		spin_lock(&msg_queue_spinlock);
+		spin_lock(&service->state->msg_queue_spinlock);
 
 		while (user_service->msg_remove != user_service->msg_insert) {
 			struct vchiq_header *header;
@@ -1254,14 +1254,14 @@ static int vchiq_release(struct inode *inode, struct file *file)
 
 			header = user_service->msg_queue[m];
 			user_service->msg_remove++;
-			spin_unlock(&msg_queue_spinlock);
+			spin_unlock(&service->state->msg_queue_spinlock);
 
 			if (header)
 				vchiq_release_message(instance, service->handle, header);
-			spin_lock(&msg_queue_spinlock);
+			spin_lock(&service->state->msg_queue_spinlock);
 		}
 
-		spin_unlock(&msg_queue_spinlock);
+		spin_unlock(&service->state->msg_queue_spinlock);
 
 		vchiq_service_put(service);
 	}
-- 
2.43.0

[PATCH v4 09/11] staging: vc04_services: vchiq_mmal: Rename service_callback()

[Umang Jain]

Rename the service_callback static function to mmal_service_callback()
since the function signature conflicts with:

extern int
service_callback(struct vchiq_instance *vchiq_instance, enum vchiq_reason reason,
                 struct vchiq_header *header, unsigned int handle, void *bulk_userdata);

in vc04_services/interface/vchiq_arm/vchiq_arm.h

In a subsequent patch, we will include vchiq_arm.h header to
mmal-vchiq.c, which will then complain of this conflict. Hence,
this patch is meant to handle the conflict beforehand.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
index 258aa0e37f55..e9cac8f3f744 100644
--- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
+++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
@@ -548,9 +548,9 @@ static void bulk_abort_cb(struct vchiq_mmal_instance *instance,
 }
 
 /* incoming event service callback */
-static int service_callback(struct vchiq_instance *vchiq_instance,
-			    enum vchiq_reason reason, struct vchiq_header *header,
-			    unsigned int handle, void *bulk_ctx)
+static int mmal_service_callback(struct vchiq_instance *vchiq_instance,
+				 enum vchiq_reason reason, struct vchiq_header *header,
+				 unsigned int handle, void *bulk_ctx)
 {
 	struct vchiq_mmal_instance *instance = vchiq_get_service_userdata(vchiq_instance, handle);
 	u32 msg_len;
@@ -1861,7 +1861,7 @@ int vchiq_mmal_init(struct vchiq_mmal_instance **out_instance)
 		.version		= VC_MMAL_VER,
 		.version_min		= VC_MMAL_MIN_VER,
 		.fourcc			= VCHIQ_MAKE_FOURCC('m', 'm', 'a', 'l'),
-		.callback		= service_callback,
+		.callback		= mmal_service_callback,
 		.userdata		= NULL,
 	};
 
-- 
2.43.0

[PATCH v4 10/11] staging: vc04_services: Move global g_state vchiq_state pointer

[Umang Jain]

The patch intended to drop the g_state pointer.
g_state is supposed to be a central place to track the state
via vchiq_state. This is now moved to be contained in the
struct vchiq_drv_mgmt. As a result vchiq_get_state() is also removed.

In order to have access to vchiq_drv_mgmt, vchiq_initialise()
and vchiq_mmal_init() are modified to receive a struct device pointer
as one of their function parameter

The vchiq_state pointer is now passed directly to
vchiq_dump_platform_instances() to get access to the state instead
getting it via vchiq_get_state().

For the char device, struct miscdevice is retrieved by struct file's
private data in vchiq_open and struct vchiq_drv_mgmt is retrieved
thereafter.

Removal of global variable members is now addressed hence, drop
the corresponding item from the TODO list.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../bcm2835-camera/bcm2835-camera.c           |  4 +-
 .../include/linux/raspberrypi/vchiq.h         |  4 +-
 drivers/staging/vc04_services/interface/TODO  |  8 ----
 .../interface/vchiq_arm/vchiq_arm.c           | 38 +++++--------------
 .../interface/vchiq_arm/vchiq_arm.h           |  7 +---
 .../interface/vchiq_arm/vchiq_core.c          |  2 +-
 .../interface/vchiq_arm/vchiq_core.h          |  2 +-
 .../interface/vchiq_arm/vchiq_debugfs.c       |  5 ++-
 .../interface/vchiq_arm/vchiq_dev.c           | 10 +++--
 .../vc04_services/vchiq-mmal/mmal-vchiq.c     |  6 ++-
 .../vc04_services/vchiq-mmal/mmal-vchiq.h     |  3 +-
 11 files changed, 34 insertions(+), 55 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
index c3ba490e53cb..b3599ec6293a 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
@@ -1555,7 +1555,7 @@ static int mmal_init(struct bcm2835_mmal_dev *dev)
 	u32 param_size;
 	struct vchiq_mmal_component  *camera;
 
-	ret = vchiq_mmal_init(&dev->instance);
+	ret = vchiq_mmal_init(dev->v4l2_dev.dev, &dev->instance);
 	if (ret < 0) {
 		v4l2_err(&dev->v4l2_dev, "%s: vchiq mmal init failed %d\n",
 			 __func__, ret);
@@ -1854,7 +1854,7 @@ static int bcm2835_mmal_probe(struct vchiq_device *device)
 		return ret;
 	}
 
-	ret = vchiq_mmal_init(&instance);
+	ret = vchiq_mmal_init(&device->dev, &instance);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h b/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
index 52e106f117da..6c40d8c1dde6 100644
--- a/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
+++ b/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
@@ -48,6 +48,7 @@ struct vchiq_element {
 };
 
 struct vchiq_instance;
+struct vchiq_state;
 
 struct vchiq_service_base {
 	int fourcc;
@@ -78,7 +79,8 @@ struct vchiq_service_params_kernel {
 	short version_min;   /* Update for incompatible changes */
 };
 
-extern int vchiq_initialise(struct vchiq_instance **pinstance);
+extern int vchiq_initialise(struct vchiq_state *state,
+			    struct vchiq_instance **pinstance);
 extern int vchiq_shutdown(struct vchiq_instance *instance);
 extern int vchiq_connect(struct vchiq_instance *instance);
 extern int vchiq_open_service(struct vchiq_instance *instance,
diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
index 57a2d6761f9b..e8acbc21ee10 100644
--- a/drivers/staging/vc04_services/interface/TODO
+++ b/drivers/staging/vc04_services/interface/TODO
@@ -34,14 +34,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.
 
-* 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
-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.
-
 * 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.
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 28c6357c13cd..b8fba6da636d 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -59,8 +59,6 @@
 #define KEEPALIVE_VER 1
 #define KEEPALIVE_VER_MIN KEEPALIVE_VER
 
-struct vchiq_state g_state;
-
 /*
  * The devices implemented in the VCHIQ firmware are not discoverable,
  * so we need to maintain a list of them in order to register them with
@@ -698,9 +696,8 @@ void vchiq_dump_platform_state(struct seq_file *f)
 }
 
 #define VCHIQ_INIT_RETRIES 10
-int vchiq_initialise(struct vchiq_instance **instance_out)
+int vchiq_initialise(struct vchiq_state *state, struct vchiq_instance **instance_out)
 {
-	struct vchiq_state *state;
 	struct vchiq_instance *instance = NULL;
 	int i, ret;
 
@@ -710,7 +707,6 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
 	 * block forever.
 	 */
 	for (i = 0; i < VCHIQ_INIT_RETRIES; i++) {
-		state = vchiq_get_state();
 		if (state)
 			break;
 		usleep_range(500, 600);
@@ -1030,9 +1026,10 @@ add_completion(struct vchiq_instance *instance, enum vchiq_reason reason,
 	       void *bulk_userdata)
 {
 	struct vchiq_completion_data_kernel *completion;
+	struct vchiq_drv_mgmt *mgmt = dev_get_drvdata(instance->state->dev);
 	int insert;
 
-	DEBUG_INITIALISE(g_state.local);
+	DEBUG_INITIALISE(mgmt->state.local);
 
 	insert = instance->completion_insert;
 	while ((insert - instance->completion_remove) >= MAX_COMPLETIONS) {
@@ -1095,11 +1092,12 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
 	 * containing the original callback and the user state structure, which
 	 * contains a circular buffer for completion records.
 	 */
+	struct vchiq_drv_mgmt *mgmt = dev_get_drvdata(instance->state->dev);
 	struct user_service *user_service;
 	struct vchiq_service *service;
 	bool skip_completion = false;
 
-	DEBUG_INITIALISE(g_state.local);
+	DEBUG_INITIALISE(mgmt->state.local);
 
 	DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 
@@ -1204,9 +1202,8 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
 		bulk_userdata);
 }
 
-void vchiq_dump_platform_instances(struct seq_file *f)
+void vchiq_dump_platform_instances(struct vchiq_state *state, struct seq_file *f)
 {
-	struct vchiq_state *state = vchiq_get_state();
 	int i;
 
 	if (!state)
@@ -1281,23 +1278,6 @@ void vchiq_dump_platform_service_state(struct seq_file *f,
 	seq_puts(f, "\n");
 }
 
-struct vchiq_state *
-vchiq_get_state(void)
-{
-	if (!g_state.remote) {
-		pr_err("%s: g_state.remote == NULL\n", __func__);
-		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;
-}
-
 /*
  * Autosuspend related functionality
  */
@@ -1331,7 +1311,7 @@ vchiq_keepalive_thread_func(void *v)
 		.version_min = KEEPALIVE_VER_MIN
 	};
 
-	ret = vchiq_initialise(&instance);
+	ret = vchiq_initialise(state, &instance);
 	if (ret) {
 		dev_err(state->dev, "suspend: %s: vchiq_initialise failed %d\n", __func__, ret);
 		goto exit;
@@ -1779,7 +1759,7 @@ static int vchiq_probe(struct platform_device *pdev)
 	mgmt->info = info;
 	platform_set_drvdata(pdev, mgmt);
 
-	err = vchiq_platform_init(pdev, &g_state);
+	err = vchiq_platform_init(pdev, &mgmt->state);
 	if (err)
 		goto failed_platform_init;
 
@@ -1811,8 +1791,8 @@ static int vchiq_probe(struct platform_device *pdev)
 
 static void vchiq_remove(struct platform_device *pdev)
 {
-	struct vchiq_state *state = vchiq_get_state();
 	struct vchiq_drv_mgmt *mgmt = dev_get_drvdata(&pdev->dev);
+	struct vchiq_state *state = &mgmt->state;
 
 	vchiq_device_unregister(bcm2835_audio);
 	vchiq_device_unregister(bcm2835_camera);
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index a8f7a7f7b189..182dacb7783b 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -67,6 +67,8 @@ struct vchiq_drv_mgmt {
 	unsigned int fragments_size;
 
 	void __iomem *regs;
+
+	struct vchiq_state state;
 };
 
 struct user_service {
@@ -113,11 +115,6 @@ struct vchiq_instance {
 	struct vchiq_debugfs_node debugfs_node;
 };
 
-extern struct vchiq_state g_state;
-
-extern struct vchiq_state *
-vchiq_get_state(void);
-
 int
 vchiq_use_service(struct vchiq_instance *instance, unsigned int handle);
 
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 2e0be0194811..8601700965a8 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3505,7 +3505,7 @@ void vchiq_dump_state(struct seq_file *f, struct vchiq_state *state)
 
 	vchiq_dump_shared_state(f, state, state->remote, "Remote");
 
-	vchiq_dump_platform_instances(f);
+	vchiq_dump_platform_instances(state, f);
 
 	for (i = 0; i < state->unused_service; i++) {
 		struct vchiq_service *service = find_service_by_port(state, i);
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 33d95e431717..62e5f3322e45 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -526,7 +526,7 @@ void remote_event_signal(struct vchiq_state *state, struct remote_event *event);
 
 void vchiq_dump_platform_state(struct seq_file *f);
 
-void vchiq_dump_platform_instances(struct seq_file *f);
+void vchiq_dump_platform_instances(struct vchiq_state *state, struct seq_file *f);
 
 void vchiq_dump_platform_service_state(struct seq_file *f, struct vchiq_service *service);
 
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 d833e4e2973a..54e7bf029d9a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_debugfs.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_debugfs.c
@@ -42,7 +42,10 @@ static int debugfs_trace_show(struct seq_file *f, void *offset)
 
 static int vchiq_dump_show(struct seq_file *f, void *offset)
 {
-	vchiq_dump_state(f, &g_state);
+	struct vchiq_instance *instance = f->private;
+
+	vchiq_dump_state(f, instance->state);
+
 	return 0;
 }
 DEFINE_SHOW_ATTRIBUTE(vchiq_dump);
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 739ce529a71b..9fe35864936c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -208,7 +208,7 @@ static int vchiq_ioc_dequeue_message(struct vchiq_instance *instance,
 	struct vchiq_header *header;
 	int ret;
 
-	DEBUG_INITIALISE(g_state.local);
+	DEBUG_INITIALISE(instance->state->local);
 	DEBUG_TRACE(DEQUEUE_MESSAGE_LINE);
 	service = find_service_for_instance(instance, args->handle);
 	if (!service)
@@ -435,7 +435,7 @@ static int vchiq_ioc_await_completion(struct vchiq_instance *instance,
 	int remove;
 	int ret;
 
-	DEBUG_INITIALISE(g_state.local);
+	DEBUG_INITIALISE(instance->state->local);
 
 	DEBUG_TRACE(AWAIT_COMPLETION_LINE);
 	if (!instance->connected)
@@ -1163,7 +1163,9 @@ vchiq_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 static int vchiq_open(struct inode *inode, struct file *file)
 {
-	struct vchiq_state *state = vchiq_get_state();
+	struct miscdevice *vchiq_miscdev = file->private_data;
+	struct vchiq_drv_mgmt *mgmt = dev_get_drvdata(vchiq_miscdev->parent);
+	struct vchiq_state *state = &mgmt->state;
 	struct vchiq_instance *instance;
 
 	dev_dbg(state->dev, "arm: vchiq open\n");
@@ -1196,7 +1198,7 @@ static int vchiq_open(struct inode *inode, struct file *file)
 static int vchiq_release(struct inode *inode, struct file *file)
 {
 	struct vchiq_instance *instance = file->private_data;
-	struct vchiq_state *state = vchiq_get_state();
+	struct vchiq_state *state = instance->state;
 	struct vchiq_service *service;
 	int ret = 0;
 	int i;
diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
index e9cac8f3f744..3f0c1a0d1c17 100644
--- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
+++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
@@ -26,6 +26,7 @@
 #include <media/videobuf2-vmalloc.h>
 
 #include "../include/linux/raspberrypi/vchiq.h"
+#include "../interface/vchiq_arm/vchiq_arm.h"
 #include "mmal-common.h"
 #include "mmal-vchiq.h"
 #include "mmal-msg.h"
@@ -1851,7 +1852,7 @@ int vchiq_mmal_finalise(struct vchiq_mmal_instance *instance)
 }
 EXPORT_SYMBOL_GPL(vchiq_mmal_finalise);
 
-int vchiq_mmal_init(struct vchiq_mmal_instance **out_instance)
+int vchiq_mmal_init(struct device *dev, struct vchiq_mmal_instance **out_instance)
 {
 	int status;
 	int err = -ENODEV;
@@ -1864,6 +1865,7 @@ int vchiq_mmal_init(struct vchiq_mmal_instance **out_instance)
 		.callback		= mmal_service_callback,
 		.userdata		= NULL,
 	};
+	struct vchiq_drv_mgmt *mgmt = dev_get_drvdata(dev->parent);
 
 	/* compile time checks to ensure structure size as they are
 	 * directly (de)serialised from memory.
@@ -1879,7 +1881,7 @@ int vchiq_mmal_init(struct vchiq_mmal_instance **out_instance)
 	BUILD_BUG_ON(sizeof(struct mmal_port) != 64);
 
 	/* create a vchi instance */
-	status = vchiq_initialise(&vchiq_instance);
+	status = vchiq_initialise(&mgmt->state, &vchiq_instance);
 	if (status) {
 		pr_err("Failed to initialise VCHI instance (status=%d)\n",
 		       status);
diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
index 09f030919d4e..8806d7b0a08e 100644
--- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
+++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
@@ -25,6 +25,7 @@
 #define MMAL_FORMAT_EXTRADATA_MAX_SIZE 128
 
 struct vchiq_mmal_instance;
+struct device;
 
 enum vchiq_mmal_es_type {
 	MMAL_ES_TYPE_UNKNOWN,     /**< Unknown elementary stream type */
@@ -95,7 +96,7 @@ struct vchiq_mmal_component {
 	u32 client_component;	/* Used to ref back to client struct */
 };
 
-int vchiq_mmal_init(struct vchiq_mmal_instance **out_instance);
+int vchiq_mmal_init(struct device *dev, struct vchiq_mmal_instance **out_instance);
 int vchiq_mmal_finalise(struct vchiq_mmal_instance *instance);
 
 /* Initialise a mmal component and its ports
-- 
2.43.0

[PATCH v4 11/11] staging: vc04_services: Drop completed TODO item

[Umang Jain]

The memory barrier comments are added in
commit f6c99d86246a ("staging: vchiq_arm: Add missing memory barrier comments")

Drop the respective item from the TODO list.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/staging/vc04_services/interface/TODO | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
index e8acbc21ee10..dfb1ee49633f 100644
--- a/drivers/staging/vc04_services/interface/TODO
+++ b/drivers/staging/vc04_services/interface/TODO
@@ -21,13 +21,6 @@ some of the ones we want:
 A short top-down description of this driver's architecture (function of
 kthreads, userspace, limitations) could be very helpful for reviewers.
 
-* 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.
-
 * Reformat core code with more sane indentations
 
 The code follows the 80 characters limitation yet tends to go 3 or 4 levels of
-- 
2.43.0

Re: [PATCH v4 02/11] staging: vc04_services: vchiq_arm: Split driver static and runtime data

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Thu, Mar 28, 2024 at 11:41:24PM +0530, Umang Jain wrote:
> vchiq_drvdata combines two types of book-keeping data. There is
> platform-specific static data (for e.g. cache lines size) and then
> data needed for book-keeping at runtime.
> 
> Split the data into two structures: struct vchiq_platform_info and
> struct vchiq_drv_mgmt. The vchiq_drv_mgmt is allocated at runtime
> during probe and will be extended in subsequent patches to remove
> all global variables allocated.
> 
> No functional changes intended in this patch.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../interface/vchiq_arm/vchiq_arm.c           | 42 ++++++++++---------
>  .../interface/vchiq_arm/vchiq_arm.h           | 11 +++++
>  2 files changed, 33 insertions(+), 20 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 9af77d17a1e8..9273767f25df 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -71,16 +71,11 @@ struct vchiq_state g_state;
>  static struct vchiq_device *bcm2835_audio;
>  static struct vchiq_device *bcm2835_camera;
>  
> -struct vchiq_drvdata {
> -	const unsigned int cache_line_size;
> -	struct rpi_firmware *fw;
> -};
> -
> -static struct vchiq_drvdata bcm2835_drvdata = {
> +static const struct vchiq_platform_info bcm2835_info = {
>  	.cache_line_size = 32,
>  };
>  
> -static struct vchiq_drvdata bcm2836_drvdata = {
> +static const struct vchiq_platform_info bcm2836_info = {
>  	.cache_line_size = 64,
>  };
>  
> @@ -466,8 +461,8 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
>  static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct vchiq_drvdata *drvdata = platform_get_drvdata(pdev);
> -	struct rpi_firmware *fw = drvdata->fw;
> +	struct vchiq_drv_mgmt *drv_mgmt = platform_get_drvdata(pdev);
> +	struct rpi_firmware *fw = drv_mgmt->fw;
>  	struct vchiq_slot_zero *vchiq_slot_zero;
>  	void *slot_mem;
>  	dma_addr_t slot_phys;
> @@ -484,7 +479,7 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>  	if (err < 0)
>  		return err;
>  
> -	g_cache_line_size = drvdata->cache_line_size;
> +	g_cache_line_size = drv_mgmt->info->cache_line_size;
>  	g_fragments_size = 2 * g_cache_line_size;
>  
>  	/* Allocate space for the channels in coherent memory */
> @@ -1707,8 +1702,8 @@ void vchiq_platform_conn_state_changed(struct vchiq_state *state,
>  }
>  
>  static const struct of_device_id vchiq_of_match[] = {
> -	{ .compatible = "brcm,bcm2835-vchiq", .data = &bcm2835_drvdata },
> -	{ .compatible = "brcm,bcm2836-vchiq", .data = &bcm2836_drvdata },
> +	{ .compatible = "brcm,bcm2835-vchiq", .data = &bcm2835_info },
> +	{ .compatible = "brcm,bcm2836-vchiq", .data = &bcm2836_info },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, vchiq_of_match);
> @@ -1716,13 +1711,12 @@ MODULE_DEVICE_TABLE(of, vchiq_of_match);
>  static int vchiq_probe(struct platform_device *pdev)
>  {
>  	struct device_node *fw_node;
> -	const struct of_device_id *of_id;
> -	struct vchiq_drvdata *drvdata;
> +	const struct vchiq_platform_info *info;
> +	struct vchiq_drv_mgmt *mgmt;
>  	int err;
>  
> -	of_id = of_match_node(vchiq_of_match, pdev->dev.of_node);
> -	drvdata = (struct vchiq_drvdata *)of_id->data;
> -	if (!drvdata)
> +	info = of_device_get_match_data(&pdev->dev);
> +	if (!info)
>  		return -EINVAL;
>  
>  	fw_node = of_find_compatible_node(NULL, NULL,
> @@ -1732,12 +1726,17 @@ static int vchiq_probe(struct platform_device *pdev)
>  		return -ENOENT;
>  	}
>  
> -	drvdata->fw = devm_rpi_firmware_get(&pdev->dev, fw_node);
> +	mgmt = kzalloc(sizeof(*mgmt), GFP_KERNEL);
> +	if (!mgmt)
> +		return -ENOMEM;
> +
> +	mgmt->fw = devm_rpi_firmware_get(&pdev->dev, fw_node);
>  	of_node_put(fw_node);
> -	if (!drvdata->fw)
> +	if (!mgmt->fw)
>  		return -EPROBE_DEFER;
>  
> -	platform_set_drvdata(pdev, drvdata);
> +	mgmt->info = info;
> +	platform_set_drvdata(pdev, mgmt);
>  
>  	err = vchiq_platform_init(pdev, &g_state);
>  	if (err)
> @@ -1772,6 +1771,7 @@ static int vchiq_probe(struct platform_device *pdev)
>  static void vchiq_remove(struct platform_device *pdev)
>  {
>  	struct vchiq_state *state = vchiq_get_state();
> +	struct vchiq_drv_mgmt *mgmt = dev_get_drvdata(&pdev->dev);
>  
>  	vchiq_device_unregister(bcm2835_audio);
>  	vchiq_device_unregister(bcm2835_camera);
> @@ -1781,6 +1781,8 @@ static void vchiq_remove(struct platform_device *pdev)
>  	kthread_stop(state->sync_thread);
>  	kthread_stop(state->recycle_thread);
>  	kthread_stop(state->slot_handler_thread);
> +
> +	kfree(mgmt);
>  }
>  
>  static struct platform_driver vchiq_driver = {
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> index 7844ef765a00..04683d98cd00 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> @@ -20,11 +20,22 @@
>  #define MAX_ELEMENTS 8
>  #define MSG_QUEUE_SIZE 128
>  
> +struct rpi_firmware;
> +
>  enum USE_TYPE_E {
>  	USE_TYPE_SERVICE,
>  	USE_TYPE_VCHIQ
>  };
>  
> +struct vchiq_platform_info {
> +	unsigned int cache_line_size;
> +};
> +
> +struct vchiq_drv_mgmt {
> +	struct rpi_firmware *fw;
> +	const struct vchiq_platform_info *info;
> +};
> +
>  struct user_service {
>  	struct vchiq_service *service;
>  	void __user *userdata;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 04/11] staging: vc04_services: Move variables for tracking connections

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Thu, Mar 28, 2024 at 11:41:26PM +0530, Umang Jain wrote:
> Connections to the vchiq driver are tracked using global variables.
> Drop those and introduce them as members of struct vchiq_drv_mgmt.
> Hence, struct vchiq_drv_mgmt will be used to track the connections.
> 
> Also, store a vchiq_drv_mgmt pointer to struct vchiq_device to
> have easy access to struct vchiq_drv_mgmt across vchiq devices.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../interface/vchiq_arm/vchiq_arm.c           |  3 +-
>  .../interface/vchiq_arm/vchiq_arm.h           |  9 +++++
>  .../interface/vchiq_arm/vchiq_bus.c           |  3 ++
>  .../interface/vchiq_arm/vchiq_bus.h           |  3 ++
>  .../interface/vchiq_arm/vchiq_connected.c     | 36 +++++++++----------
>  .../interface/vchiq_arm/vchiq_connected.h     |  4 ++-
>  6 files changed, 37 insertions(+), 21 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 0dffc36dccd7..4d14ea1becaa 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -545,7 +545,8 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>  	dev_dbg(&pdev->dev, "arm: vchiq_init - done (slots %pK, phys %pad)\n",
>  		vchiq_slot_zero, &slot_phys);
>  
> -	vchiq_call_connected_callbacks();
> +	mutex_init(&drv_mgmt->connected_mutex);
> +	vchiq_call_connected_callbacks(drv_mgmt);
>  
>  	return 0;
>  }
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> index 378415b205ef..a774485935a2 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> @@ -20,6 +20,8 @@
>  #define MAX_ELEMENTS 8
>  #define MSG_QUEUE_SIZE 128
>  
> +#define VCHIQ_DRV_MAX_CALLBACKS 10
> +
>  struct rpi_firmware;
>  
>  enum USE_TYPE_E {
> @@ -49,6 +51,13 @@ struct vchiq_platform_info {
>  struct vchiq_drv_mgmt {
>  	struct rpi_firmware *fw;
>  	const struct vchiq_platform_info *info;
> +
> +	bool connected;
> +	int num_deferred_callbacks;
> +	/* Protects connected and num_deferred_callbacks */
> +	struct mutex connected_mutex;
> +
> +	void (*deferred_callback[VCHIQ_DRV_MAX_CALLBACKS])(void);
>  };
>  
>  struct user_service {
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
> index 93609716df84..3f87b93c6537 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
> @@ -11,6 +11,7 @@
>  #include <linux/slab.h>
>  #include <linux/string.h>
>  
> +#include "vchiq_arm.h"
>  #include "vchiq_bus.h"
>  
>  static int vchiq_bus_type_match(struct device *dev, struct device_driver *drv)
> @@ -77,6 +78,8 @@ vchiq_device_register(struct device *parent, const char *name)
>  	device->dev.dma_mask = &device->dev.coherent_dma_mask;
>  	device->dev.release = vchiq_device_release;
>  
> +	device->drv_mgmt = dev_get_drvdata(parent);
> +
>  	of_dma_configure(&device->dev, parent->of_node, true);
>  
>  	ret = device_register(&device->dev);
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h
> index 4db86e76edbd..9de179b39f85 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h
> @@ -9,8 +9,11 @@
>  #include <linux/device.h>
>  #include <linux/mod_devicetable.h>
>  
> +struct vchiq_drv_mgmt;
> +
>  struct vchiq_device {
>  	struct device dev;
> +	struct vchiq_drv_mgmt *drv_mgmt;
>  };
>  
>  struct vchiq_driver {
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> index 4604a2f4d2de..049b3f2d1bd4 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
>  /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
>  
> +#include "vchiq_arm.h"
>  #include "vchiq_connected.h"
>  #include "vchiq_core.h"
>  #include <linux/module.h>
> @@ -8,11 +9,6 @@
>  
>  #define  MAX_CALLBACKS  10
>  
> -static   int                        g_connected;
> -static   int                        g_num_deferred_callbacks;
> -static   void (*g_deferred_callback[MAX_CALLBACKS])(void);
> -static   DEFINE_MUTEX(g_connected_mutex);
> -
>  /*
>   * This function is used to defer initialization until the vchiq stack is
>   * initialized. If the stack is already initialized, then the callback will
> @@ -21,24 +17,26 @@ static   DEFINE_MUTEX(g_connected_mutex);
>   */
>  void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
>  {
> -	if (mutex_lock_killable(&g_connected_mutex))
> +	struct vchiq_drv_mgmt *drv_mgmt = device->drv_mgmt;
> +
> +	if (mutex_lock_killable(&drv_mgmt->connected_mutex))
>  		return;
>  
> -	if (g_connected) {
> +	if (drv_mgmt->connected) {
>  		/* We're already connected. Call the callback immediately. */
>  		callback();
>  	} else {
> -		if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
> +		if (drv_mgmt->num_deferred_callbacks >= MAX_CALLBACKS) {
>  			dev_err(&device->dev,
>  				"core: There already %d callback registered - please increase MAX_CALLBACKS\n",
> -				g_num_deferred_callbacks);
> +				drv_mgmt->num_deferred_callbacks);
>  		} else {
> -			g_deferred_callback[g_num_deferred_callbacks] =
> +			drv_mgmt->deferred_callback[drv_mgmt->num_deferred_callbacks] =
>  				callback;
> -			g_num_deferred_callbacks++;
> +			drv_mgmt->num_deferred_callbacks++;
>  		}
>  	}
> -	mutex_unlock(&g_connected_mutex);
> +	mutex_unlock(&drv_mgmt->connected_mutex);
>  }
>  EXPORT_SYMBOL(vchiq_add_connected_callback);
>  
> @@ -46,17 +44,17 @@ EXPORT_SYMBOL(vchiq_add_connected_callback);
>   * This function is called by the vchiq stack once it has been connected to
>   * the videocore and clients can start to use the stack.
>   */
> -void vchiq_call_connected_callbacks(void)
> +void vchiq_call_connected_callbacks(struct vchiq_drv_mgmt *drv_mgmt)
>  {
>  	int i;
>  
> -	if (mutex_lock_killable(&g_connected_mutex))
> +	if (mutex_lock_killable(&drv_mgmt->connected_mutex))
>  		return;
>  
> -	for (i = 0; i <  g_num_deferred_callbacks; i++)
> -		g_deferred_callback[i]();
> +	for (i = 0; i <  drv_mgmt->num_deferred_callbacks; i++)
> +		drv_mgmt->deferred_callback[i]();
>  
> -	g_num_deferred_callbacks = 0;
> -	g_connected = 1;
> -	mutex_unlock(&g_connected_mutex);
> +	drv_mgmt->num_deferred_callbacks = 0;
> +	drv_mgmt->connected = true;
> +	mutex_unlock(&drv_mgmt->connected_mutex);
>  }
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> index e4ed56446f8a..f57a878652b1 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> @@ -6,7 +6,9 @@
>  #ifndef VCHIQ_CONNECTED_H
>  #define VCHIQ_CONNECTED_H
>  
> +struct vchiq_drv_mgmt;
> +
>  void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
> -void vchiq_call_connected_callbacks(void);
> +void vchiq_call_connected_callbacks(struct vchiq_drv_mgmt *mgmt);
>  
>  #endif /* VCHIQ_CONNECTED_H */

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 05/11] staging: vc04_services: Drop vchiq_connected.[ch] files

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Thu, Mar 28, 2024 at 11:41:27PM +0530, Umang Jain wrote:
> The vchiq_connected.[ch] just implements two function:
> 
> - vchiq_add_connected_callback()
> - vchiq_call_connected_callbacks()
> 
> for the deferred vchiq callbacks. Those can easily live in
> vchiq_arm.[ch], hence move them.

I would add

This allows making the vchiq_call_connected_callbacks() function static.

> The move doesn't copy over MAX_CALLBACKS because it is the
> same as VCHIQ_DRV_MAX_CALLBACKS. Hence, it now being used in
> vchiq_add_connected_callback().

You can reflow to 72 columns.

> 
> No functional changes intended in this patch.
> 

In case you didn't know, there's a Suggested-by tag you can use to
indicate who the idea for a patch came from.

> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/staging/vc04_services/Makefile        |  1 -
>  .../interface/vchiq_arm/vchiq_arm.c           | 51 +++++++++++++++-
>  .../interface/vchiq_arm/vchiq_arm.h           |  5 ++
>  .../interface/vchiq_arm/vchiq_connected.c     | 60 -------------------
>  .../interface/vchiq_arm/vchiq_connected.h     | 14 -----
>  5 files changed, 55 insertions(+), 76 deletions(-)
>  delete mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>  delete mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> 
> diff --git a/drivers/staging/vc04_services/Makefile b/drivers/staging/vc04_services/Makefile
> index e8b897a7b9a6..dad3789522b8 100644
> --- a/drivers/staging/vc04_services/Makefile
> +++ b/drivers/staging/vc04_services/Makefile
> @@ -6,7 +6,6 @@ vchiq-objs := \
>     interface/vchiq_arm/vchiq_arm.o \
>     interface/vchiq_arm/vchiq_bus.o \
>     interface/vchiq_arm/vchiq_debugfs.o \
> -   interface/vchiq_arm/vchiq_connected.o \
>  
>  ifdef CONFIG_VCHIQ_CDEV
>  vchiq-objs += interface/vchiq_arm/vchiq_dev.o
> 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 4d14ea1becaa..2005d392d0b7 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -36,7 +36,6 @@
>  #include "vchiq_arm.h"
>  #include "vchiq_bus.h"
>  #include "vchiq_debugfs.h"
> -#include "vchiq_connected.h"
>  #include "vchiq_pagelist.h"
>  
>  #define DEVICE_NAME "vchiq"
> @@ -189,6 +188,56 @@ is_adjacent_block(u32 *addrs, u32 addr, unsigned int k)
>  	return tmp == (addr & PAGE_MASK);
>  }
>  
> +/*
> + * This function is called by the vchiq stack once it has been connected to
> + * the videocore and clients can start to use the stack.
> + */
> +static void vchiq_call_connected_callbacks(struct vchiq_drv_mgmt *drv_mgmt)
> +{
> +	int i;
> +
> +	if (mutex_lock_killable(&drv_mgmt->connected_mutex))
> +		return;
> +
> +	for (i = 0; i < drv_mgmt->num_deferred_callbacks; i++)
> +		drv_mgmt->deferred_callback[i]();
> +
> +	drv_mgmt->num_deferred_callbacks = 0;
> +	drv_mgmt->connected = true;
> +	mutex_unlock(&drv_mgmt->connected_mutex);
> +}
> +
> +/*
> + * This function is used to defer initialization until the vchiq stack is
> + * initialized. If the stack is already initialized, then the callback will
> + * be made immediately, otherwise it will be deferred until
> + * vchiq_call_connected_callbacks is called.
> + */
> +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
> +{
> +	struct vchiq_drv_mgmt *drv_mgmt = device->drv_mgmt;
> +
> +	if (mutex_lock_killable(&drv_mgmt->connected_mutex))
> +		return;
> +
> +	if (drv_mgmt->connected) {
> +		/* We're already connected. Call the callback immediately. */
> +		callback();
> +	} else {
> +		if (drv_mgmt->num_deferred_callbacks >= VCHIQ_DRV_MAX_CALLBACKS) {
> +			dev_err(&device->dev,
> +				"core: deferred callbacks(%d) exceeded the maximum limit(%d)\n",
> +				drv_mgmt->num_deferred_callbacks, VCHIQ_DRV_MAX_CALLBACKS);
> +		} else {
> +			drv_mgmt->deferred_callback[drv_mgmt->num_deferred_callbacks] =
> +				callback;
> +			drv_mgmt->num_deferred_callbacks++;
> +		}
> +	}
> +	mutex_unlock(&drv_mgmt->connected_mutex);
> +}
> +EXPORT_SYMBOL(vchiq_add_connected_callback);
> +
>  /* 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,
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> index a774485935a2..4a5b5ae9625a 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> @@ -23,6 +23,7 @@
>  #define VCHIQ_DRV_MAX_CALLBACKS 10
>  
>  struct rpi_firmware;
> +struct vchiq_device;
>  
>  enum USE_TYPE_E {
>  	USE_TYPE_SERVICE,
> @@ -147,6 +148,10 @@ vchiq_instance_get_trace(struct vchiq_instance *instance);
>  extern void
>  vchiq_instance_set_trace(struct vchiq_instance *instance, int trace);
>  
> +extern void
> +vchiq_add_connected_callback(struct vchiq_device *device,
> +			     void (*callback)(void));
> +
>  #if IS_ENABLED(CONFIG_VCHIQ_CDEV)
>  
>  extern void
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> deleted file mode 100644
> index 049b3f2d1bd4..000000000000
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> +++ /dev/null
> @@ -1,60 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> -/* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
> -
> -#include "vchiq_arm.h"
> -#include "vchiq_connected.h"
> -#include "vchiq_core.h"
> -#include <linux/module.h>
> -#include <linux/mutex.h>
> -
> -#define  MAX_CALLBACKS  10
> -
> -/*
> - * This function is used to defer initialization until the vchiq stack is
> - * initialized. If the stack is already initialized, then the callback will
> - * be made immediately, otherwise it will be deferred until
> - * vchiq_call_connected_callbacks is called.
> - */
> -void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
> -{
> -	struct vchiq_drv_mgmt *drv_mgmt = device->drv_mgmt;
> -
> -	if (mutex_lock_killable(&drv_mgmt->connected_mutex))
> -		return;
> -
> -	if (drv_mgmt->connected) {
> -		/* We're already connected. Call the callback immediately. */
> -		callback();
> -	} else {
> -		if (drv_mgmt->num_deferred_callbacks >= MAX_CALLBACKS) {
> -			dev_err(&device->dev,
> -				"core: There already %d callback registered - please increase MAX_CALLBACKS\n",
> -				drv_mgmt->num_deferred_callbacks);
> -		} else {
> -			drv_mgmt->deferred_callback[drv_mgmt->num_deferred_callbacks] =
> -				callback;
> -			drv_mgmt->num_deferred_callbacks++;
> -		}
> -	}
> -	mutex_unlock(&drv_mgmt->connected_mutex);
> -}
> -EXPORT_SYMBOL(vchiq_add_connected_callback);
> -
> -/*
> - * This function is called by the vchiq stack once it has been connected to
> - * the videocore and clients can start to use the stack.
> - */
> -void vchiq_call_connected_callbacks(struct vchiq_drv_mgmt *drv_mgmt)
> -{
> -	int i;
> -
> -	if (mutex_lock_killable(&drv_mgmt->connected_mutex))
> -		return;
> -
> -	for (i = 0; i <  drv_mgmt->num_deferred_callbacks; i++)
> -		drv_mgmt->deferred_callback[i]();
> -
> -	drv_mgmt->num_deferred_callbacks = 0;
> -	drv_mgmt->connected = true;
> -	mutex_unlock(&drv_mgmt->connected_mutex);
> -}
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> deleted file mode 100644
> index f57a878652b1..000000000000
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> +++ /dev/null
> @@ -1,14 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> -/* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
> -
> -#include "vchiq_bus.h"
> -
> -#ifndef VCHIQ_CONNECTED_H
> -#define VCHIQ_CONNECTED_H
> -
> -struct vchiq_drv_mgmt;
> -
> -void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
> -void vchiq_call_connected_callbacks(struct vchiq_drv_mgmt *mgmt);
> -
> -#endif /* VCHIQ_CONNECTED_H */

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 06/11] staging: vc04_services: Move global variables tracking allocated pages

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Thu, Mar 28, 2024 at 11:41:28PM +0530, Umang Jain wrote:
> The variables tracking allocated pages fragments in vchiq_arm.c
> can be easily moved to struct vchiq_drv_mgmt instead of being global.
> This helps us to drop the non-essential global variables in the vchiq
> interface.
> 
> No functional changes intended in this patch.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../interface/vchiq_arm/vchiq_arm.c           | 53 +++++++++----------
>  .../interface/vchiq_arm/vchiq_arm.h           |  6 +++
>  2 files changed, 30 insertions(+), 29 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 2005d392d0b7..b335948f4b0c 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -130,12 +130,6 @@ struct vchiq_pagelist_info {
>  };
>  
>  static void __iomem *g_regs;
> -static unsigned int g_fragments_size;
> -static char *g_fragments_base;
> -static char *g_free_fragments;
> -static struct semaphore g_free_fragments_sema;
> -
> -static DEFINE_SEMAPHORE(g_free_fragments_mutex, 1);
>  
>  static int
>  vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
> @@ -414,20 +408,20 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
>  	    (drv_mgmt->info->cache_line_size - 1)))) {
>  		char *fragments;
>  
> -		if (down_interruptible(&g_free_fragments_sema)) {
> +		if (down_interruptible(&drv_mgmt->free_fragments_sema)) {
>  			cleanup_pagelistinfo(instance, pagelistinfo);
>  			return NULL;
>  		}
>  
> -		WARN_ON(!g_free_fragments);
> +		WARN_ON(!drv_mgmt->free_fragments);
>  
> -		down(&g_free_fragments_mutex);
> -		fragments = g_free_fragments;
> +		down(&drv_mgmt->free_fragments_mutex);
> +		fragments = drv_mgmt->free_fragments;
>  		WARN_ON(!fragments);
> -		g_free_fragments = *(char **)g_free_fragments;
> -		up(&g_free_fragments_mutex);
> +		drv_mgmt->free_fragments = *(char **)drv_mgmt->free_fragments;
> +		up(&drv_mgmt->free_fragments_mutex);
>  		pagelist->type = PAGELIST_READ_WITH_FRAGMENTS +
> -			(fragments - g_fragments_base) / g_fragments_size;
> +			(fragments - drv_mgmt->fragments_base) / drv_mgmt->fragments_size;
>  	}
>  
>  	return pagelistinfo;
> @@ -455,10 +449,10 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
>  	pagelistinfo->scatterlist_mapped = 0;
>  
>  	/* Deal with any partial cache lines (fragments) */
> -	if (pagelist->type >= PAGELIST_READ_WITH_FRAGMENTS && g_fragments_base) {
> -		char *fragments = g_fragments_base +
> +	if (pagelist->type >= PAGELIST_READ_WITH_FRAGMENTS && drv_mgmt->fragments_base) {
> +		char *fragments = drv_mgmt->fragments_base +
>  			(pagelist->type - PAGELIST_READ_WITH_FRAGMENTS) *
> -			g_fragments_size;
> +			drv_mgmt->fragments_size;
>  		int head_bytes, tail_bytes;
>  
>  		head_bytes = (drv_mgmt->info->cache_line_size - pagelist->offset) &
> @@ -483,11 +477,11 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
>  				fragments + drv_mgmt->info->cache_line_size,
>  				tail_bytes);
>  
> -		down(&g_free_fragments_mutex);
> -		*(char **)fragments = g_free_fragments;
> -		g_free_fragments = fragments;
> -		up(&g_free_fragments_mutex);
> -		up(&g_free_fragments_sema);
> +		down(&drv_mgmt->free_fragments_mutex);
> +		*(char **)fragments = drv_mgmt->free_fragments;
> +		drv_mgmt->free_fragments = fragments;
> +		up(&drv_mgmt->free_fragments_mutex);
> +		up(&drv_mgmt->free_fragments_sema);
>  	}
>  
>  	/* Need to mark all the pages dirty. */
> @@ -523,11 +517,11 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>  	if (err < 0)
>  		return err;
>  
> -	g_fragments_size = 2 * drv_mgmt->info->cache_line_size;
> +	drv_mgmt->fragments_size = 2 * drv_mgmt->info->cache_line_size;
>  
>  	/* Allocate space for the channels in coherent memory */
>  	slot_mem_size = PAGE_ALIGN(TOTAL_SLOTS * VCHIQ_SLOT_SIZE);
> -	frag_mem_size = PAGE_ALIGN(g_fragments_size * MAX_FRAGMENTS);
> +	frag_mem_size = PAGE_ALIGN(drv_mgmt->fragments_size * MAX_FRAGMENTS);
>  
>  	slot_mem = dmam_alloc_coherent(dev, slot_mem_size + frag_mem_size,
>  				       &slot_phys, GFP_KERNEL);
> @@ -547,15 +541,16 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>  	vchiq_slot_zero->platform_data[VCHIQ_PLATFORM_FRAGMENTS_COUNT_IDX] =
>  		MAX_FRAGMENTS;
>  
> -	g_fragments_base = (char *)slot_mem + slot_mem_size;
> +	drv_mgmt->fragments_base = (char *)slot_mem + slot_mem_size;
>  
> -	g_free_fragments = g_fragments_base;
> +	drv_mgmt->free_fragments = drv_mgmt->fragments_base;
>  	for (i = 0; i < (MAX_FRAGMENTS - 1); i++) {
> -		*(char **)&g_fragments_base[i * g_fragments_size] =
> -			&g_fragments_base[(i + 1) * g_fragments_size];
> +		*(char **)&drv_mgmt->fragments_base[i * drv_mgmt->fragments_size] =
> +			&drv_mgmt->fragments_base[(i + 1) * drv_mgmt->fragments_size];
>  	}
> -	*(char **)&g_fragments_base[i * g_fragments_size] = NULL;
> -	sema_init(&g_free_fragments_sema, MAX_FRAGMENTS);
> +	*(char **)&drv_mgmt->fragments_base[i * drv_mgmt->fragments_size] = NULL;
> +	sema_init(&drv_mgmt->free_fragments_sema, MAX_FRAGMENTS);
> +	sema_init(&drv_mgmt->free_fragments_mutex, 1);
>  
>  	err = vchiq_init_state(state, vchiq_slot_zero, dev);
>  	if (err)
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> index 4a5b5ae9625a..a3bba245bfe2 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> @@ -59,6 +59,12 @@ struct vchiq_drv_mgmt {
>  	struct mutex connected_mutex;
>  
>  	void (*deferred_callback[VCHIQ_DRV_MAX_CALLBACKS])(void);
> +
> +	struct semaphore free_fragments_sema;
> +	struct semaphore free_fragments_mutex;
> +	char *fragments_base;
> +	char *free_fragments;
> +	unsigned int fragments_size;
>  };
>  
>  struct user_service {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 07/11] staging: vc04_services: Move global memory mapped pointer

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Thu, Mar 28, 2024 at 11:41:29PM +0530, Umang Jain wrote:
> g_regs stores the remapped memory pointer for the vchiq platform.
> It can be moved to struct vchiq_drv_mgmt instead of being global.
> 
> Adjust the affected functions accordingly. Pass vchiq_state pointer
> wherever necessary to access struct vchiq_drv_mgmt.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../interface/vchiq_arm/vchiq_arm.c           | 19 +++++++++++--------
>  .../interface/vchiq_arm/vchiq_arm.h           |  2 ++
>  .../interface/vchiq_arm/vchiq_core.c          | 10 +++++-----
>  .../interface/vchiq_arm/vchiq_core.h          |  2 +-
>  4 files changed, 19 insertions(+), 14 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 b335948f4b0c..0c7dd45969ae 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -129,8 +129,6 @@ struct vchiq_pagelist_info {
>  	unsigned int scatterlist_mapped;
>  };
>  
> -static void __iomem *g_regs;
> -
>  static int
>  vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
>  			     unsigned int size, enum vchiq_bulk_dir dir);
> @@ -139,11 +137,14 @@ static irqreturn_t
>  vchiq_doorbell_irq(int irq, void *dev_id)
>  {
>  	struct vchiq_state *state = dev_id;
> +	struct vchiq_drv_mgmt *mgmt;
>  	irqreturn_t ret = IRQ_NONE;
>  	unsigned int status;
>  
> +	mgmt = dev_get_drvdata(state->dev);
> +
>  	/* Read (and clear) the doorbell */
> -	status = readl(g_regs + BELL0);
> +	status = readl(mgmt->regs + BELL0);
>  
>  	if (status & ARM_DS_ACTIVE) {  /* Was the doorbell rung? */
>  		remote_event_pollall(state);
> @@ -556,9 +557,9 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>  	if (err)
>  		return err;
>  
> -	g_regs = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(g_regs))
> -		return PTR_ERR(g_regs);
> +	drv_mgmt->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(drv_mgmt->regs))
> +		return PTR_ERR(drv_mgmt->regs);
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq <= 0)
> @@ -641,8 +642,10 @@ static struct vchiq_arm_state *vchiq_platform_get_arm_state(struct vchiq_state *
>  }
>  
>  void
> -remote_event_signal(struct remote_event *event)
> +remote_event_signal(struct vchiq_state *state, struct remote_event *event)
>  {
> +	struct vchiq_drv_mgmt *mgmt = dev_get_drvdata(state->dev);
> +
>  	/*
>  	 * Ensure that all writes to shared data structures have completed
>  	 * before signalling the peer.
> @@ -654,7 +657,7 @@ remote_event_signal(struct remote_event *event)
>  	dsb(sy);         /* data barrier operation */
>  
>  	if (event->armed)
> -		writel(0, g_regs + BELL2); /* trigger vc interrupt */
> +		writel(0, mgmt->regs + BELL2); /* trigger vc interrupt */
>  }
>  
>  int
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> index a3bba245bfe2..f17c22635ce5 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> @@ -65,6 +65,8 @@ struct vchiq_drv_mgmt {
>  	char *fragments_base;
>  	char *free_fragments;
>  	unsigned int fragments_size;
> +
> +	void __iomem *regs;
>  };
>  
>  struct user_service {
> 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 658d19f1e7e8..af633ff984f6 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -691,7 +691,7 @@ reserve_space(struct vchiq_state *state, size_t space, int is_blocking)
>  			/* But first, flush through the last slot. */
>  			state->local_tx_pos = tx_pos;
>  			local->tx_pos = tx_pos;
> -			remote_event_signal(&state->remote->trigger);
> +			remote_event_signal(state, &state->remote->trigger);
>  
>  			if (!is_blocking ||
>  			    (wait_for_completion_interruptible(&state->slot_available_event)))
> @@ -1124,7 +1124,7 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
>  	if (!(flags & QMFLAGS_NO_MUTEX_UNLOCK))
>  		mutex_unlock(&state->slot_mutex);
>  
> -	remote_event_signal(&state->remote->trigger);
> +	remote_event_signal(state, &state->remote->trigger);
>  
>  	return 0;
>  }
> @@ -1202,7 +1202,7 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
>  		&svc_fourcc, VCHIQ_MSG_SRCPORT(msgid),
>  		VCHIQ_MSG_DSTPORT(msgid), size);
>  
> -	remote_event_signal(&state->remote->sync_trigger);
> +	remote_event_signal(state, &state->remote->sync_trigger);
>  
>  	if (VCHIQ_MSG_TYPE(msgid) != VCHIQ_MSG_PAUSE)
>  		mutex_unlock(&state->sync_mutex);
> @@ -1260,7 +1260,7 @@ release_slot(struct vchiq_state *state, struct vchiq_slot_info *slot_info,
>  		 * A write barrier is necessary, but remote_event_signal
>  		 * contains one.
>  		 */
> -		remote_event_signal(&state->remote->recycle);
> +		remote_event_signal(state, &state->remote->recycle);
>  	}
>  
>  	mutex_unlock(&state->recycle_mutex);
> @@ -3240,7 +3240,7 @@ static void
>  release_message_sync(struct vchiq_state *state, struct vchiq_header *header)
>  {
>  	header->msgid = VCHIQ_MSGID_PADDING;
> -	remote_event_signal(&state->remote->sync_release);
> +	remote_event_signal(state, &state->remote->sync_release);
>  }
>  
>  int
> 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 5fbf173d9c56..8ca74b12427b 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> @@ -516,7 +516,7 @@ int vchiq_prepare_bulk_data(struct vchiq_instance *instance, struct vchiq_bulk *
>  
>  void vchiq_complete_bulk(struct vchiq_instance *instance, struct vchiq_bulk *bulk);
>  
> -void remote_event_signal(struct remote_event *event);
> +void remote_event_signal(struct vchiq_state *state, struct remote_event *event);
>  
>  void vchiq_dump_platform_state(struct seq_file *f);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 09/11] staging: vc04_services: vchiq_mmal: Rename service_callback()

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Thu, Mar 28, 2024 at 11:41:31PM +0530, Umang Jain wrote:
> Rename the service_callback static function to mmal_service_callback()
> since the function signature conflicts with:
> 
> extern int
> service_callback(struct vchiq_instance *vchiq_instance, enum vchiq_reason reason,
>                  struct vchiq_header *header, unsigned int handle, void *bulk_userdata);
> 
> in vc04_services/interface/vchiq_arm/vchiq_arm.h

I would rename this function too. It can be done in a separate patch.

> 
> In a subsequent patch, we will include vchiq_arm.h header to
> mmal-vchiq.c, which will then complain of this conflict. Hence,
> this patch is meant to handle the conflict beforehand.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> index 258aa0e37f55..e9cac8f3f744 100644
> --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> @@ -548,9 +548,9 @@ static void bulk_abort_cb(struct vchiq_mmal_instance *instance,
>  }
>  
>  /* incoming event service callback */
> -static int service_callback(struct vchiq_instance *vchiq_instance,
> -			    enum vchiq_reason reason, struct vchiq_header *header,
> -			    unsigned int handle, void *bulk_ctx)
> +static int mmal_service_callback(struct vchiq_instance *vchiq_instance,
> +				 enum vchiq_reason reason, struct vchiq_header *header,
> +				 unsigned int handle, void *bulk_ctx)
>  {
>  	struct vchiq_mmal_instance *instance = vchiq_get_service_userdata(vchiq_instance, handle);
>  	u32 msg_len;
> @@ -1861,7 +1861,7 @@ int vchiq_mmal_init(struct vchiq_mmal_instance **out_instance)
>  		.version		= VC_MMAL_VER,
>  		.version_min		= VC_MMAL_MIN_VER,
>  		.fourcc			= VCHIQ_MAKE_FOURCC('m', 'm', 'a', 'l'),
> -		.callback		= service_callback,
> +		.callback		= mmal_service_callback,
>  		.userdata		= NULL,
>  	};
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 06/11] staging: vc04_services: Move global variables tracking allocated pages

[Laurent Pinchart]

On Fri, Mar 29, 2024 at 12:59:59AM +0200, Laurent Pinchart wrote:
> Hi Umang,
> 
> Thank you for the patch.
> 
> On Thu, Mar 28, 2024 at 11:41:28PM +0530, Umang Jain wrote:
> > The variables tracking allocated pages fragments in vchiq_arm.c
> > can be easily moved to struct vchiq_drv_mgmt instead of being global.
> > This helps us to drop the non-essential global variables in the vchiq
> > interface.
> > 
> > No functional changes intended in this patch.
> > 
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > ---
> >  .../interface/vchiq_arm/vchiq_arm.c           | 53 +++++++++----------
> >  .../interface/vchiq_arm/vchiq_arm.h           |  6 +++
> >  2 files changed, 30 insertions(+), 29 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 2005d392d0b7..b335948f4b0c 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -130,12 +130,6 @@ struct vchiq_pagelist_info {
> >  };
> >  
> >  static void __iomem *g_regs;
> > -static unsigned int g_fragments_size;
> > -static char *g_fragments_base;
> > -static char *g_free_fragments;
> > -static struct semaphore g_free_fragments_sema;
> > -
> > -static DEFINE_SEMAPHORE(g_free_fragments_mutex, 1);
> >  
> >  static int
> >  vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
> > @@ -414,20 +408,20 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
> >  	    (drv_mgmt->info->cache_line_size - 1)))) {
> >  		char *fragments;
> >  
> > -		if (down_interruptible(&g_free_fragments_sema)) {
> > +		if (down_interruptible(&drv_mgmt->free_fragments_sema)) {
> >  			cleanup_pagelistinfo(instance, pagelistinfo);
> >  			return NULL;
> >  		}
> >  
> > -		WARN_ON(!g_free_fragments);
> > +		WARN_ON(!drv_mgmt->free_fragments);
> >  
> > -		down(&g_free_fragments_mutex);
> > -		fragments = g_free_fragments;
> > +		down(&drv_mgmt->free_fragments_mutex);
> > +		fragments = drv_mgmt->free_fragments;
> >  		WARN_ON(!fragments);
> > -		g_free_fragments = *(char **)g_free_fragments;
> > -		up(&g_free_fragments_mutex);
> > +		drv_mgmt->free_fragments = *(char **)drv_mgmt->free_fragments;
> > +		up(&drv_mgmt->free_fragments_mutex);
> >  		pagelist->type = PAGELIST_READ_WITH_FRAGMENTS +
> > -			(fragments - g_fragments_base) / g_fragments_size;
> > +			(fragments - drv_mgmt->fragments_base) / drv_mgmt->fragments_size;
> >  	}
> >  
> >  	return pagelistinfo;
> > @@ -455,10 +449,10 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
> >  	pagelistinfo->scatterlist_mapped = 0;
> >  
> >  	/* Deal with any partial cache lines (fragments) */
> > -	if (pagelist->type >= PAGELIST_READ_WITH_FRAGMENTS && g_fragments_base) {
> > -		char *fragments = g_fragments_base +
> > +	if (pagelist->type >= PAGELIST_READ_WITH_FRAGMENTS && drv_mgmt->fragments_base) {
> > +		char *fragments = drv_mgmt->fragments_base +
> >  			(pagelist->type - PAGELIST_READ_WITH_FRAGMENTS) *
> > -			g_fragments_size;
> > +			drv_mgmt->fragments_size;
> >  		int head_bytes, tail_bytes;
> >  
> >  		head_bytes = (drv_mgmt->info->cache_line_size - pagelist->offset) &
> > @@ -483,11 +477,11 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
> >  				fragments + drv_mgmt->info->cache_line_size,
> >  				tail_bytes);
> >  
> > -		down(&g_free_fragments_mutex);
> > -		*(char **)fragments = g_free_fragments;
> > -		g_free_fragments = fragments;
> > -		up(&g_free_fragments_mutex);
> > -		up(&g_free_fragments_sema);
> > +		down(&drv_mgmt->free_fragments_mutex);
> > +		*(char **)fragments = drv_mgmt->free_fragments;
> > +		drv_mgmt->free_fragments = fragments;
> > +		up(&drv_mgmt->free_fragments_mutex);
> > +		up(&drv_mgmt->free_fragments_sema);
> >  	}
> >  
> >  	/* Need to mark all the pages dirty. */
> > @@ -523,11 +517,11 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
> >  	if (err < 0)
> >  		return err;
> >  
> > -	g_fragments_size = 2 * drv_mgmt->info->cache_line_size;
> > +	drv_mgmt->fragments_size = 2 * drv_mgmt->info->cache_line_size;
> >  
> >  	/* Allocate space for the channels in coherent memory */
> >  	slot_mem_size = PAGE_ALIGN(TOTAL_SLOTS * VCHIQ_SLOT_SIZE);
> > -	frag_mem_size = PAGE_ALIGN(g_fragments_size * MAX_FRAGMENTS);
> > +	frag_mem_size = PAGE_ALIGN(drv_mgmt->fragments_size * MAX_FRAGMENTS);
> >  
> >  	slot_mem = dmam_alloc_coherent(dev, slot_mem_size + frag_mem_size,
> >  				       &slot_phys, GFP_KERNEL);
> > @@ -547,15 +541,16 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
> >  	vchiq_slot_zero->platform_data[VCHIQ_PLATFORM_FRAGMENTS_COUNT_IDX] =
> >  		MAX_FRAGMENTS;
> >  
> > -	g_fragments_base = (char *)slot_mem + slot_mem_size;
> > +	drv_mgmt->fragments_base = (char *)slot_mem + slot_mem_size;
> >  
> > -	g_free_fragments = g_fragments_base;
> > +	drv_mgmt->free_fragments = drv_mgmt->fragments_base;
> >  	for (i = 0; i < (MAX_FRAGMENTS - 1); i++) {
> > -		*(char **)&g_fragments_base[i * g_fragments_size] =
> > -			&g_fragments_base[(i + 1) * g_fragments_size];
> > +		*(char **)&drv_mgmt->fragments_base[i * drv_mgmt->fragments_size] =
> > +			&drv_mgmt->fragments_base[(i + 1) * drv_mgmt->fragments_size];
> >  	}
> > -	*(char **)&g_fragments_base[i * g_fragments_size] = NULL;
> > -	sema_init(&g_free_fragments_sema, MAX_FRAGMENTS);
> > +	*(char **)&drv_mgmt->fragments_base[i * drv_mgmt->fragments_size] = NULL;
> > +	sema_init(&drv_mgmt->free_fragments_sema, MAX_FRAGMENTS);
> > +	sema_init(&drv_mgmt->free_fragments_mutex, 1);
> >  
> >  	err = vchiq_init_state(state, vchiq_slot_zero, dev);
> >  	if (err)
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> > index 4a5b5ae9625a..a3bba245bfe2 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> > @@ -59,6 +59,12 @@ struct vchiq_drv_mgmt {
> >  	struct mutex connected_mutex;
> >  
> >  	void (*deferred_callback[VCHIQ_DRV_MAX_CALLBACKS])(void);
> > +
> > +	struct semaphore free_fragments_sema;
> > +	struct semaphore free_fragments_mutex;

Could we turn the latter into a mutex (in a separate patch) ?

> > +	char *fragments_base;
> > +	char *free_fragments;
> > +	unsigned int fragments_size;
> >  };
> >  
> >  struct user_service {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 06/11] staging: vc04_services: Move global variables tracking allocated pages

[Umang Jain]

Hi Laurent,

On 29/03/24 4:34 am, Laurent Pinchart wrote:
> On Fri, Mar 29, 2024 at 12:59:59AM +0200, Laurent Pinchart wrote:
>> Hi Umang,
>>
>> Thank you for the patch.
>>
>> On Thu, Mar 28, 2024 at 11:41:28PM +0530, Umang Jain wrote:
>>> The variables tracking allocated pages fragments in vchiq_arm.c
>>> can be easily moved to struct vchiq_drv_mgmt instead of being global.
>>> This helps us to drop the non-essential global variables in the vchiq
>>> interface.
>>>
>>> No functional changes intended in this patch.
>>>
>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>>> ---
>>>   .../interface/vchiq_arm/vchiq_arm.c           | 53 +++++++++----------
>>>   .../interface/vchiq_arm/vchiq_arm.h           |  6 +++
>>>   2 files changed, 30 insertions(+), 29 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 2005d392d0b7..b335948f4b0c 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>> @@ -130,12 +130,6 @@ struct vchiq_pagelist_info {
>>>   };
>>>   
>>>   static void __iomem *g_regs;
>>> -static unsigned int g_fragments_size;
>>> -static char *g_fragments_base;
>>> -static char *g_free_fragments;
>>> -static struct semaphore g_free_fragments_sema;
>>> -
>>> -static DEFINE_SEMAPHORE(g_free_fragments_mutex, 1);
>>>   
>>>   static int
>>>   vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
>>> @@ -414,20 +408,20 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
>>>   	    (drv_mgmt->info->cache_line_size - 1)))) {
>>>   		char *fragments;
>>>   
>>> -		if (down_interruptible(&g_free_fragments_sema)) {
>>> +		if (down_interruptible(&drv_mgmt->free_fragments_sema)) {
>>>   			cleanup_pagelistinfo(instance, pagelistinfo);
>>>   			return NULL;
>>>   		}
>>>   
>>> -		WARN_ON(!g_free_fragments);
>>> +		WARN_ON(!drv_mgmt->free_fragments);
>>>   
>>> -		down(&g_free_fragments_mutex);
>>> -		fragments = g_free_fragments;
>>> +		down(&drv_mgmt->free_fragments_mutex);
>>> +		fragments = drv_mgmt->free_fragments;
>>>   		WARN_ON(!fragments);
>>> -		g_free_fragments = *(char **)g_free_fragments;
>>> -		up(&g_free_fragments_mutex);
>>> +		drv_mgmt->free_fragments = *(char **)drv_mgmt->free_fragments;
>>> +		up(&drv_mgmt->free_fragments_mutex);
>>>   		pagelist->type = PAGELIST_READ_WITH_FRAGMENTS +
>>> -			(fragments - g_fragments_base) / g_fragments_size;
>>> +			(fragments - drv_mgmt->fragments_base) / drv_mgmt->fragments_size;
>>>   	}
>>>   
>>>   	return pagelistinfo;
>>> @@ -455,10 +449,10 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
>>>   	pagelistinfo->scatterlist_mapped = 0;
>>>   
>>>   	/* Deal with any partial cache lines (fragments) */
>>> -	if (pagelist->type >= PAGELIST_READ_WITH_FRAGMENTS && g_fragments_base) {
>>> -		char *fragments = g_fragments_base +
>>> +	if (pagelist->type >= PAGELIST_READ_WITH_FRAGMENTS && drv_mgmt->fragments_base) {
>>> +		char *fragments = drv_mgmt->fragments_base +
>>>   			(pagelist->type - PAGELIST_READ_WITH_FRAGMENTS) *
>>> -			g_fragments_size;
>>> +			drv_mgmt->fragments_size;
>>>   		int head_bytes, tail_bytes;
>>>   
>>>   		head_bytes = (drv_mgmt->info->cache_line_size - pagelist->offset) &
>>> @@ -483,11 +477,11 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
>>>   				fragments + drv_mgmt->info->cache_line_size,
>>>   				tail_bytes);
>>>   
>>> -		down(&g_free_fragments_mutex);
>>> -		*(char **)fragments = g_free_fragments;
>>> -		g_free_fragments = fragments;
>>> -		up(&g_free_fragments_mutex);
>>> -		up(&g_free_fragments_sema);
>>> +		down(&drv_mgmt->free_fragments_mutex);
>>> +		*(char **)fragments = drv_mgmt->free_fragments;
>>> +		drv_mgmt->free_fragments = fragments;
>>> +		up(&drv_mgmt->free_fragments_mutex);
>>> +		up(&drv_mgmt->free_fragments_sema);
>>>   	}
>>>   
>>>   	/* Need to mark all the pages dirty. */
>>> @@ -523,11 +517,11 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>>>   	if (err < 0)
>>>   		return err;
>>>   
>>> -	g_fragments_size = 2 * drv_mgmt->info->cache_line_size;
>>> +	drv_mgmt->fragments_size = 2 * drv_mgmt->info->cache_line_size;
>>>   
>>>   	/* Allocate space for the channels in coherent memory */
>>>   	slot_mem_size = PAGE_ALIGN(TOTAL_SLOTS * VCHIQ_SLOT_SIZE);
>>> -	frag_mem_size = PAGE_ALIGN(g_fragments_size * MAX_FRAGMENTS);
>>> +	frag_mem_size = PAGE_ALIGN(drv_mgmt->fragments_size * MAX_FRAGMENTS);
>>>   
>>>   	slot_mem = dmam_alloc_coherent(dev, slot_mem_size + frag_mem_size,
>>>   				       &slot_phys, GFP_KERNEL);
>>> @@ -547,15 +541,16 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>>>   	vchiq_slot_zero->platform_data[VCHIQ_PLATFORM_FRAGMENTS_COUNT_IDX] =
>>>   		MAX_FRAGMENTS;
>>>   
>>> -	g_fragments_base = (char *)slot_mem + slot_mem_size;
>>> +	drv_mgmt->fragments_base = (char *)slot_mem + slot_mem_size;
>>>   
>>> -	g_free_fragments = g_fragments_base;
>>> +	drv_mgmt->free_fragments = drv_mgmt->fragments_base;
>>>   	for (i = 0; i < (MAX_FRAGMENTS - 1); i++) {
>>> -		*(char **)&g_fragments_base[i * g_fragments_size] =
>>> -			&g_fragments_base[(i + 1) * g_fragments_size];
>>> +		*(char **)&drv_mgmt->fragments_base[i * drv_mgmt->fragments_size] =
>>> +			&drv_mgmt->fragments_base[(i + 1) * drv_mgmt->fragments_size];
>>>   	}
>>> -	*(char **)&g_fragments_base[i * g_fragments_size] = NULL;
>>> -	sema_init(&g_free_fragments_sema, MAX_FRAGMENTS);
>>> +	*(char **)&drv_mgmt->fragments_base[i * drv_mgmt->fragments_size] = NULL;
>>> +	sema_init(&drv_mgmt->free_fragments_sema, MAX_FRAGMENTS);
>>> +	sema_init(&drv_mgmt->free_fragments_mutex, 1);
>>>   
>>>   	err = vchiq_init_state(state, vchiq_slot_zero, dev);
>>>   	if (err)
>>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
>>> index 4a5b5ae9625a..a3bba245bfe2 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
>>> @@ -59,6 +59,12 @@ struct vchiq_drv_mgmt {
>>>   	struct mutex connected_mutex;
>>>   
>>>   	void (*deferred_callback[VCHIQ_DRV_MAX_CALLBACKS])(void);
>>> +
>>> +	struct semaphore free_fragments_sema;
>>> +	struct semaphore free_fragments_mutex;
> Could we turn the latter into a mutex (in a separate patch) ?

It can be on top. I have noted it down for next tranch of cleanups.
>
>>> +	char *fragments_base;
>>> +	char *free_fragments;
>>> +	unsigned int fragments_size;
>>>   };
>>>   
>>>   struct user_service {

Re: [PATCH v4 00/11] staging: vc04_services: Drop non-essential global members

[Greg KH]

On Thu, Mar 28, 2024 at 11:41:22PM +0530, Umang Jain wrote:
> This series aims to drop the remaining (non-essential) global members
> to address the following TODO item:
> ```
> * Get rid of all non essential global structures and create a proper per
> device structure
> ```
> 
> Mainly the global members are moved to be contained inside platform
> driver data. They can be access via platform_get_drvdata().
> 
> More re-fractoring has gone into this version now. Please look
> for individual commit for details.
> 
> ---
> NOTE: Series has been developed on top of [1]
> "[PATCH] staging: vc04_services: Stop kthreads on .remove"
> 
> The fate of that patch is still undecided, so we need to take into
> account on the direction the discussions take. That will only affect
> Patch 10/11 of this series, rest is fairly independent in that regard.

As that patch was not accepted, but the revert was this series needs to
be rebased.

thanks,

greg k-h