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

[PATCH v5 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.

Testing on top of RPi 3 with staging next.

Changes in v5:
- Rebase over latest staging-testing that contains kthreads revert [2]
- commit messages fixup
- Add suggested-by in 5/11
- Use spinlock_t(typedef) instead of 'struct spinlock_t' in 8/11

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 [1].
- Add a comment for g_regs global __iomem ptr.

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/4ba0d745-fc8d-4886-b71a-1f19962e9103@moroto.mountain/T/#m440ee992442cc82ea43092b7c895823c918d105f

[2]: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=ebee9ca2f59e35a60a6704a79df6477b3c84ac96

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 to vchiq_state
  staging: vc04_services: Drop completed TODO item

 drivers/staging/vc04_services/Makefile        |   1 -
 .../bcm2835-audio/bcm2835-vchiq.c             |   5 +-
 .../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           | 252 ++++++++++--------
 .../interface/vchiq_arm/vchiq_arm.h           |  41 ++-
 .../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          |  11 +-
 .../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 +-
 17 files changed, 255 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


base-commit: e4d5e3a9ae68250f7cc7e930ffeecba2c9f32832
-- 
2.44.0

[PATCH v5 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Umang Jain <umang.jain@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.44.0

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

[Stefan Wahren]

Am 12.04.24 um 09:57 schrieb 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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>
Yes, this cleans up the rest after commit 1790f2be41e4 ("staging:
vc04_services: use DEFINE_MUTEX() for mutex lock")

Reviewed-by: Stefan Wahren <wahrenst@gmx.net>

[PATCH v5 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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 43 ++++++++++---------
 1 file changed, 23 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 ad506016fc93..7cf38f8581fa 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 */
@@ -1703,8 +1698,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);
@@ -1712,13 +1707,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,
@@ -1728,12 +1722,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)
@@ -1767,10 +1766,14 @@ static int vchiq_probe(struct platform_device *pdev)
 
 static void vchiq_remove(struct platform_device *pdev)
 {
+	struct vchiq_drv_mgmt *mgmt = dev_get_drvdata(&pdev->dev);
+
 	vchiq_device_unregister(bcm2835_audio);
 	vchiq_device_unregister(bcm2835_camera);
 	vchiq_debugfs_deinit();
 	vchiq_deregister_chrdev();
+
+	kfree(mgmt);
 }
 
 static struct platform_driver vchiq_driver = {
-- 
2.44.0

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

[Stefan Wahren]

Am 12.04.24 um 09:57 schrieb 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.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>

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

[Stefan Wahren]

Hi Umang,

Am 12.04.24 um 09:57 schrieb 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.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   .../interface/vchiq_arm/vchiq_arm.c           | 43 ++++++++++---------
>   1 file changed, 23 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 ad506016fc93..7cf38f8581fa 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;
i know this patch has been applied, but this patch doesn't compile
because the necessary structs are defined in the following patch. This
is bad because it's not bisectable :-(

Please always make sure that every single patch is able to compile.

[PATCH v5 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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 34 ++++++++-----------
 .../interface/vchiq_arm/vchiq_arm.h           | 11 ++++++
 2 files changed, 25 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 7cf38f8581fa..af74d7268717 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 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.44.0

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

[Stefan Wahren]

Am 12.04.24 um 09:57 schrieb 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.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>

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

[Stefan Wahren]

Hi Umang,

Am 12.04.24 um 09:57 schrieb 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.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   .../interface/vchiq_arm/vchiq_arm.c           | 34 ++++++++-----------
>   .../interface/vchiq_arm/vchiq_arm.h           | 11 ++++++
>   2 files changed, 25 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 7cf38f8581fa..af74d7268717 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);
this patch is wrong and cause a null pointer dereference:

[  232.297800] Unable to handle kernel NULL pointer dereference at
virtual address 00000004 when read
[  232.317576] [00000004] *pgd=02963831, *pte=00000000, *ppte=00000000
[  232.334240] Internal error: Oops: 17 [#1] ARM
[  232.348423] Modules linked in: bcm2835_v4l2(C) videobuf2_vmalloc
videobuf2_memops bcm2835_mmal_vchiq(C) videobuf2_v4l2 videobuf2_common
snd_bcm2835(C) raspberrypi_hwmon bcm2835_rng rng_core vchiq(C) configfs
[  232.377176] CPU: 0 PID: 504 Comm: VCHIQ completio Tainted: G        
C         6.9.0-rc2+ #1
[  232.395690] Hardware name: BCM2835
[  232.409044] PC is at vchiq_prepare_bulk_data+0x26c/0x528 [vchiq]
[  232.425441] LR is at vchiq_prepare_bulk_data+0x4ec/0x528 [vchiq]
[  232.441815] pc : [<bf0154f8>]    lr : [<bf015778>] psr: 60000013
[  232.458395] sp : ccc29d80  ip : 00000000  fp : c7c43000
[  232.473982] r10: bf022fc8  r9 : 43cd7000  r8 : c64a4000
[  232.489690] r7 : 00000001  r6 : 00000000  r5 : c7c43020  r4 : 43cd7000
[  232.506316] r3 : 00000000  r2 : 0000028c  r1 : c3cd7a80  r0 : 00000000
[  232.522865] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM
Segment none
[  232.540123] Control: 00c5387d  Table: 02828008  DAC: 00000051
[  232.556061] Register r0 information: NULL pointer
[  232.570951] Register r1 information: non-slab/vmalloc memory
[  232.586845] Register r2 information: non-paged memory
[  232.602036] Register r3 information: NULL pointer
[  232.616689] Register r4 information: non-paged memory
[  232.631750] Register r5 information: 0-page vmalloc region starting
at 0xc7c00000 allocated at iotable_init+0x0/0xf8
[  232.652526] Register r6 information: NULL pointer
[  232.667439] Register r7 information: non-paged memory
[  232.682558] Register r8 information: slab kmalloc-4k start c64a4000
pointer offset 0 size 4096
[  232.701421] Register r9 information: non-paged memory
[  232.716545] Register r10 information: 4-page vmalloc region starting
at 0xbf021000 allocated at load_module+0x73c/0x16e0
[  232.737816] Register r11 information: 0-page vmalloc region starting
at 0xc7c00000 allocated at iotable_init+0x0/0xf8
[  232.758991] Register r12 information: NULL pointer
[  232.774079] Process VCHIQ completio (pid: 504, stack limit = 0xe510e83c)
[  232.791136] Stack: (0xccc29d80 to 0xccc2a000)
[  232.805806] 9d80: 00000000 00000000 00000036 00000000 00000001
00000001 c7c43008 00000001
[  232.824634] 9da0: c1d610e8 0002b28c 00000001 00000010 47c43000
10a55ba2 c2291744 c1d61000
[  232.843077] 9dc0: c1d610d4 00000000 00000000 00001002 00000000
c1d610e8 c1d61170 bf012bd8
[  232.861553] 9de0: 00000800 00000001 6c6b6a69 706f6e6d c8209748
ccc29e28 74737271 c236006c
[  232.880103] 9e00: 00000000 00000000 00000072 0002b28c 00000006
c64a4000 00000051 c2c9e580
[  232.898623] 9e20: ccc29e90 10a55ba2 00000000 c22b2dc0 c64a4000
c014c406 b6ceaca8 00000000
[  232.917119] 9e40: c2c9e580 00000036 c1d61000 bf0173a0 00000800
00001002 00000000 00000001
[  232.935668] 9e60: 00000002 c8200194 00000007 00000003 c1d61000
c64a4020 00000002 c2c9e580
[  232.954296] 9e80: 00000003 00000000 b6eaf22c 00001001 0000d001
0002b28c 00000800 00001002
[  232.973059] 9ea0: 00000000 c2643420 cbfc8160 c0112bec 00000000
c644fc18 cbfc8160 00000255
[  232.991818] 9ec0: 0b98b34f c0227c9c 00000001 c1c6d200 00000255
00001020 b6308000 ccc29fb0
[  233.010716] 9ee0: c1c6d204 c644fc18 00000cc0 10a55ba2 b6308000
c22b2dc0 c22dd600 b6ceaca8
[  233.029603] 9f00: c014c406 c2c9e580 00000003 00000036 b6eaf0d8
c026d4d0 c22b2dc0 c026de50
[  233.048517] 9f20: b6321000 ccc29fb0 00000817 b6308734 c2c9e580
c22dd601 00000255 c096a730
[  233.067479] 9f40: ccc29fb0 c096a730 00000000 ccc29f50 00000000
c010271c 40000000 ee08aa10
[  233.086503] 9f60: ccc29fb0 c010271c ccc29fb0 b6e9af40 c0102648
c2c9e580 00c5387d 10a55ba2
[  233.105615] 9f80: b6cead48 b6eaf130 b6ceaca8 c014c406 00000036
c01002c0 c2c9e580 00000036
[  233.124926] 9fa0: b6eaf0d8 c0100060 b6eaf130 b6ceaca8 00000003
c014c406 b6ceaca8 00000000
[  233.144213] 9fc0: b6eaf130 b6ceaca8 c014c406 00000036 00000800
00001002 0002b28c b6eaf0d8
[  233.163431] 9fe0: b6eaf060 b6ceac9c b6e9c0c8 b6dd353c 80000010
00000003 00000000 00000000
[  233.182630] Call trace:
[  233.182680]  vchiq_prepare_bulk_data [vchiq] from
vchiq_bulk_transfer+0x1e4/0x3d4 [vchiq]
[  233.215907]  vchiq_bulk_transfer [vchiq] from
vchiq_ioctl+0x1d4/0x1114 [vchiq]
[  233.234685]  vchiq_ioctl [vchiq] from vfs_ioctl+0x28/0x3c
[  233.251519]  vfs_ioctl from sys_ioctl+0xc4/0x928
[  233.267303]  sys_ioctl from ret_fast_syscall+0x0/0x54
[  233.283472] Exception stack(0xccc29fa8 to 0xccc29ff0)
[  233.299605] 9fa0:                   b6eaf130 b6ceaca8 00000003
c014c406 b6ceaca8 00000000
[  233.318897] 9fc0: b6eaf130 b6ceaca8 c014c406 00000036 00000800
00001002 0002b28c b6eaf0d8
[  233.338126] 9fe0: b6eaf060 b6ceac9c b6e9c0c8 b6dd353c
[  233.354204] Code: e3530001 1a000032 e59d300c e1db20b6 (e5933004)

Please always test your changes as documented in interface/TESTING

I will send a follow-up fix soon

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

[Umang Jain]

Hi Stefan

On 19/04/24 7:34 pm, Stefan Wahren wrote:
> Hi Umang,
>
> Am 12.04.24 um 09:57 schrieb 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.
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   .../interface/vchiq_arm/vchiq_arm.c           | 34 ++++++++-----------
>>   .../interface/vchiq_arm/vchiq_arm.h           | 11 ++++++
>>   2 files changed, 25 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 7cf38f8581fa..af74d7268717 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);
> this patch is wrong and cause a null pointer dereference:
>
> [  232.297800] Unable to handle kernel NULL pointer dereference at
> virtual address 00000004 when read
> [  232.317576] [00000004] *pgd=02963831, *pte=00000000, *ppte=00000000
> [  232.334240] Internal error: Oops: 17 [#1] ARM
> [  232.348423] Modules linked in: bcm2835_v4l2(C) videobuf2_vmalloc
> videobuf2_memops bcm2835_mmal_vchiq(C) videobuf2_v4l2 videobuf2_common
> snd_bcm2835(C) raspberrypi_hwmon bcm2835_rng rng_core vchiq(C) configfs
> [  232.377176] CPU: 0 PID: 504 Comm: VCHIQ completio Tainted: G
> C         6.9.0-rc2+ #1
> [  232.395690] Hardware name: BCM2835
> [  232.409044] PC is at vchiq_prepare_bulk_data+0x26c/0x528 [vchiq]
> [  232.425441] LR is at vchiq_prepare_bulk_data+0x4ec/0x528 [vchiq]
> [  232.441815] pc : [<bf0154f8>]    lr : [<bf015778>] psr: 60000013
> [  232.458395] sp : ccc29d80  ip : 00000000  fp : c7c43000
> [  232.473982] r10: bf022fc8  r9 : 43cd7000  r8 : c64a4000
> [  232.489690] r7 : 00000001  r6 : 00000000  r5 : c7c43020  r4 : 43cd7000
> [  232.506316] r3 : 00000000  r2 : 0000028c  r1 : c3cd7a80  r0 : 00000000
> [  232.522865] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM
> Segment none
> [  232.540123] Control: 00c5387d  Table: 02828008  DAC: 00000051
> [  232.556061] Register r0 information: NULL pointer
> [  232.570951] Register r1 information: non-slab/vmalloc memory
> [  232.586845] Register r2 information: non-paged memory
> [  232.602036] Register r3 information: NULL pointer
> [  232.616689] Register r4 information: non-paged memory
> [  232.631750] Register r5 information: 0-page vmalloc region starting
> at 0xc7c00000 allocated at iotable_init+0x0/0xf8
> [  232.652526] Register r6 information: NULL pointer
> [  232.667439] Register r7 information: non-paged memory
> [  232.682558] Register r8 information: slab kmalloc-4k start c64a4000
> pointer offset 0 size 4096
> [  232.701421] Register r9 information: non-paged memory
> [  232.716545] Register r10 information: 4-page vmalloc region starting
> at 0xbf021000 allocated at load_module+0x73c/0x16e0
> [  232.737816] Register r11 information: 0-page vmalloc region starting
> at 0xc7c00000 allocated at iotable_init+0x0/0xf8
> [  232.758991] Register r12 information: NULL pointer
> [  232.774079] Process VCHIQ completio (pid: 504, stack limit = 
> 0xe510e83c)
> [  232.791136] Stack: (0xccc29d80 to 0xccc2a000)
> [  232.805806] 9d80: 00000000 00000000 00000036 00000000 00000001
> 00000001 c7c43008 00000001
> [  232.824634] 9da0: c1d610e8 0002b28c 00000001 00000010 47c43000
> 10a55ba2 c2291744 c1d61000
> [  232.843077] 9dc0: c1d610d4 00000000 00000000 00001002 00000000
> c1d610e8 c1d61170 bf012bd8
> [  232.861553] 9de0: 00000800 00000001 6c6b6a69 706f6e6d c8209748
> ccc29e28 74737271 c236006c
> [  232.880103] 9e00: 00000000 00000000 00000072 0002b28c 00000006
> c64a4000 00000051 c2c9e580
> [  232.898623] 9e20: ccc29e90 10a55ba2 00000000 c22b2dc0 c64a4000
> c014c406 b6ceaca8 00000000
> [  232.917119] 9e40: c2c9e580 00000036 c1d61000 bf0173a0 00000800
> 00001002 00000000 00000001
> [  232.935668] 9e60: 00000002 c8200194 00000007 00000003 c1d61000
> c64a4020 00000002 c2c9e580
> [  232.954296] 9e80: 00000003 00000000 b6eaf22c 00001001 0000d001
> 0002b28c 00000800 00001002
> [  232.973059] 9ea0: 00000000 c2643420 cbfc8160 c0112bec 00000000
> c644fc18 cbfc8160 00000255
> [  232.991818] 9ec0: 0b98b34f c0227c9c 00000001 c1c6d200 00000255
> 00001020 b6308000 ccc29fb0
> [  233.010716] 9ee0: c1c6d204 c644fc18 00000cc0 10a55ba2 b6308000
> c22b2dc0 c22dd600 b6ceaca8
> [  233.029603] 9f00: c014c406 c2c9e580 00000003 00000036 b6eaf0d8
> c026d4d0 c22b2dc0 c026de50
> [  233.048517] 9f20: b6321000 ccc29fb0 00000817 b6308734 c2c9e580
> c22dd601 00000255 c096a730
> [  233.067479] 9f40: ccc29fb0 c096a730 00000000 ccc29f50 00000000
> c010271c 40000000 ee08aa10
> [  233.086503] 9f60: ccc29fb0 c010271c ccc29fb0 b6e9af40 c0102648
> c2c9e580 00c5387d 10a55ba2
> [  233.105615] 9f80: b6cead48 b6eaf130 b6ceaca8 c014c406 00000036
> c01002c0 c2c9e580 00000036
> [  233.124926] 9fa0: b6eaf0d8 c0100060 b6eaf130 b6ceaca8 00000003
> c014c406 b6ceaca8 00000000
> [  233.144213] 9fc0: b6eaf130 b6ceaca8 c014c406 00000036 00000800
> 00001002 0002b28c b6eaf0d8
> [  233.163431] 9fe0: b6eaf060 b6ceac9c b6e9c0c8 b6dd353c 80000010
> 00000003 00000000 00000000
> [  233.182630] Call trace:
> [  233.182680]  vchiq_prepare_bulk_data [vchiq] from
> vchiq_bulk_transfer+0x1e4/0x3d4 [vchiq]
> [  233.215907]  vchiq_bulk_transfer [vchiq] from
> vchiq_ioctl+0x1d4/0x1114 [vchiq]
> [  233.234685]  vchiq_ioctl [vchiq] from vfs_ioctl+0x28/0x3c
> [  233.251519]  vfs_ioctl from sys_ioctl+0xc4/0x928
> [  233.267303]  sys_ioctl from ret_fast_syscall+0x0/0x54
> [  233.283472] Exception stack(0xccc29fa8 to 0xccc29ff0)
> [  233.299605] 9fa0:                   b6eaf130 b6ceaca8 00000003
> c014c406 b6ceaca8 00000000
> [  233.318897] 9fc0: b6eaf130 b6ceaca8 c014c406 00000036 00000800
> 00001002 0002b28c b6eaf0d8
> [  233.338126] 9fe0: b6eaf060 b6ceac9c b6e9c0c8 b6dd353c
> [  233.354204] Code: e3530001 1a000032 e59d300c e1db20b6 (e5933004)
>
> Please always test your changes as documented in interface/TESTING
>
> I will send a follow-up fix soon

thanks for the fixup!

I was testing it till v3, and I see that v4 where the bug was introduced 
by me. And I must have completely missed testing it as per interface/TESTING

Shouldn't happen again, I should streamlining process for testing VC04 
services patches now (including compile-test and functional-test). It 
has been very adhoc (Rpi3 / Rpi4 / 32-bit / 64-bit / ) from a long time 
now :(

Apologies again for the pain.

[PATCH v5 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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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 af74d7268717..ac12ed0784a4 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 04683d98cd00..b4ed50e4072d 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 {
@@ -34,6 +36,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.44.0

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

[Stefan Wahren]

Am 12.04.24 um 09:57 schrieb 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.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 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 af74d7268717..ac12ed0784a4 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 04683d98cd00..b4ed50e4072d 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 {
> @@ -34,6 +36,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
I think with the introduction of VCHIQ_DRV_MAX_CALLBACKS this define
should be removed in this patch and not in patch 5. So you will avoid
two different things in patch 5.

[PATCH v5 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. 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().

No functional changes intended in this patch.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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 ac12ed0784a4..396c686b1978 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 b4ed50e4072d..48f6abee591f 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,
@@ -132,6 +133,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.44.0

[PATCH v5 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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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 396c686b1978..29f9affe5304 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 48f6abee591f..52013bbc5171 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -44,6 +44,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.44.0

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

[Stefan Wahren]

Am 12.04.24 um 09:57 schrieb 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.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>

[PATCH v5 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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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 29f9affe5304..9fc98411a2b8 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 52013bbc5171..10c1bdc50faf 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -50,6 +50,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 76c27778154a..8c339aebbc99 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.44.0

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

[Stefan Wahren]

Hi Umang,

Am 12.04.24 um 09:57 schrieb 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.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 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 29f9affe5304..9fc98411a2b8 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);
Was there a reason against moving it in the declaration?
> +
>   	/* 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 52013bbc5171..10c1bdc50faf 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> @@ -50,6 +50,8 @@ struct vchiq_drv_mgmt {
>   	char *fragments_base;
>   	char *free_fragments;
>   	unsigned int fragments_size;
> +
> +	void __iomem *regs;
I'm not a expert about cache line alignment, but maybe this should be
moved at the beginning of struct vchiq_drv_mgmt?

Not sure this would have a performance impact.

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

[Laurent Pinchart]

Hello,

On Sun, Apr 14, 2024 at 01:58:04PM +0200, Stefan Wahren wrote:
> Am 12.04.24 um 09:57 schrieb 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.
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 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 29f9affe5304..9fc98411a2b8 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);
> Was there a reason against moving it in the declaration?
> > +
> >   	/* 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 52013bbc5171..10c1bdc50faf 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> > @@ -50,6 +50,8 @@ struct vchiq_drv_mgmt {
> >   	char *fragments_base;
> >   	char *free_fragments;
> >   	unsigned int fragments_size;
> > +
> > +	void __iomem *regs;
>
> I'm not a expert about cache line alignment, but maybe this should be
> moved at the beginning of struct vchiq_drv_mgmt?
> 
> Not sure this would have a performance impact.

I don't think it will help much, but it shouldn't hurt.

-- 
Regards,

Laurent Pinchart

[PATCH v5 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          |  7 ++++
 .../interface/vchiq_arm/vchiq_dev.c           | 24 ++++++------
 5 files changed, 47 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 9fc98411a2b8..4bdd383a060c 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;
 
 /*
@@ -985,9 +984,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 {
@@ -1004,9 +1003,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 {
@@ -1127,10 +1126,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");
@@ -1167,7 +1166,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 &
@@ -1186,7 +1185,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 10c1bdc50faf..127e2d6b1d51 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -98,7 +98,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 8c339aebbc99..52a11c743bd6 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..96299e2a2984 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -11,6 +11,7 @@
 #include <linux/kthread.h>
 #include <linux/kref.h>
 #include <linux/rcupdate.h>
+#include <linux/spinlock_types.h>
 #include <linux/wait.h>
 
 #include "../../include/linux/raspberrypi/vchiq.h"
@@ -348,6 +349,12 @@ struct vchiq_state {
 
 	struct mutex bulk_transfer_mutex;
 
+	spinlock_t msg_queue_spinlock;
+
+	spinlock_t bulk_waiter_spinlock;
+
+	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.44.0

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

[Stefan Wahren]

Am 12.04.24 um 09:57 schrieb 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>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>

[PATCH v5 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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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 4c3684dd902e..680961a7c61b 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;
@@ -1862,7 +1862,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.44.0

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

[Stefan Wahren]

Am 12.04.24 um 09:57 schrieb 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.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>
Thanks for addressing the conflict which was introduced by c405028f471d
("staging: vchiq: Move certain declarations to vchiq_arm.h")

Reviewed-by: Stefan Wahren <wahrenst@gmx.net>

[PATCH v5 10/11] staging: vc04_services: Move global g_state to vchiq_state

[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-audio/bcm2835-vchiq.c             |  5 ++-
 .../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           | 36 +++++--------------
 .../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 +-
 12 files changed, 37 insertions(+), 55 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index d74110ca17ab..133ed15f3dbc 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -7,6 +7,8 @@
 #include "bcm2835.h"
 #include "vc_vchi_audioserv_defs.h"
 
+#include "../interface/vchiq_arm/vchiq_arm.h"
+
 struct bcm2835_audio_instance {
 	struct device *dev;
 	unsigned int service_handle;
@@ -175,10 +177,11 @@ static void vc_vchi_audio_deinit(struct bcm2835_audio_instance *instance)
 
 int bcm2835_new_vchi_ctx(struct device *dev, struct bcm2835_vchi_ctx *vchi_ctx)
 {
+	struct vchiq_drv_mgmt *mgmt = dev_get_drvdata(dev->parent);
 	int ret;
 
 	/* Initialize and create a VCHI connection */
-	ret = vchiq_initialise(&vchi_ctx->instance);
+	ret = vchiq_initialise(&mgmt->state, &vchi_ctx->instance);
 	if (ret) {
 		dev_err(dev, "failed to initialise VCHI instance (ret=%d)\n",
 			ret);
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 05eb5140d096..15f12b8f213e 100644
--- a/drivers/staging/vc04_services/interface/TODO
+++ b/drivers/staging/vc04_services/interface/TODO
@@ -41,14 +41,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 4bdd383a060c..502ddc0f6e46 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);
@@ -1026,9 +1022,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) {
@@ -1091,11 +1088,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);
 
@@ -1200,9 +1198,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)
@@ -1277,23 +1274,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
  */
@@ -1327,7 +1307,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;
@@ -1775,7 +1755,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;
 
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 127e2d6b1d51..fd1b9d3555ce 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -52,6 +52,8 @@ struct vchiq_drv_mgmt {
 	unsigned int fragments_size;
 
 	void __iomem *regs;
+
+	struct vchiq_state state;
 };
 
 struct user_service {
@@ -98,11 +100,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 52a11c743bd6..3397365e551e 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 96299e2a2984..8af209e34fb2 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -527,7 +527,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 680961a7c61b..fca920d41e4f 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"
@@ -1852,7 +1853,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;
@@ -1865,6 +1866,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.
@@ -1880,7 +1882,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.44.0

Re: [PATCH v5 10/11] staging: vc04_services: Move global g_state to vchiq_state

[Stefan Wahren]

Hi Umang,

just 2 nits

Am 12.04.24 um 09:57 schrieb 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-audio/bcm2835-vchiq.c             |  5 ++-
>   .../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           | 36 +++++--------------
>   .../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 +-
>   12 files changed, 37 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> index d74110ca17ab..133ed15f3dbc 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> @@ -7,6 +7,8 @@
>   #include "bcm2835.h"
>   #include "vc_vchi_audioserv_defs.h"
>
> +#include "../interface/vchiq_arm/vchiq_arm.h"
> +
>   struct bcm2835_audio_instance {
>   	struct device *dev;
>   	unsigned int service_handle;
> @@ -175,10 +177,11 @@ static void vc_vchi_audio_deinit(struct bcm2835_audio_instance *instance)
>
>   int bcm2835_new_vchi_ctx(struct device *dev, struct bcm2835_vchi_ctx *vchi_ctx)
>   {
> +	struct vchiq_drv_mgmt *mgmt = dev_get_drvdata(dev->parent);
>   	int ret;
>
>   	/* Initialize and create a VCHI connection */
> -	ret = vchiq_initialise(&vchi_ctx->instance);
> +	ret = vchiq_initialise(&mgmt->state, &vchi_ctx->instance);
>   	if (ret) {
>   		dev_err(dev, "failed to initialise VCHI instance (ret=%d)\n",
>   			ret);
> 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 05eb5140d096..15f12b8f213e 100644
> --- a/drivers/staging/vc04_services/interface/TODO
> +++ b/drivers/staging/vc04_services/interface/TODO
> @@ -41,14 +41,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 4bdd383a060c..502ddc0f6e46 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);
> @@ -1026,9 +1022,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);
Please make it the first line of the declarations
>   	int insert;
>
> -	DEBUG_INITIALISE(g_state.local);
> +	DEBUG_INITIALISE(mgmt->state.local);
>
>   	insert = instance->completion_insert;
>   	while ((insert - instance->completion_remove) >= MAX_COMPLETIONS) {
...
> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> index 680961a7c61b..fca920d41e4f 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"
> @@ -1852,7 +1853,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;
> @@ -1865,6 +1866,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);
here too

[PATCH v5 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.

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 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 15f12b8f213e..05f129c0c254 100644
--- a/drivers/staging/vc04_services/interface/TODO
+++ b/drivers/staging/vc04_services/interface/TODO
@@ -28,13 +28,6 @@ variables avoided.
 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.44.0