[PATCH v3 0/6] staging: vc04_services: Drop non-essential global members

[PATCH v3 0/6] 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().

Patch 1/6 drops the g_once_init variable as it is not used meaningfully.

Patch 2/6 is a splits static platform data and runtime data.
This is prep-up patch for removing global variables

Patch 3/6 drops g_cache_line_size and use direct value from
vchiq_platform_info

Patch 4/6 drops global variables tracking connections to vchiq driver
through vchiq_connected[.ch].

Patch 5/6 drops global variables pertaining to track memory pages

Patch 6/6 is already a completed item - so just a TODO cleanup

The user of vchiq_connected.[ch] is not present currently, but the user
will be present when we will upstream bcm2835-isp via the vc-sm-cma
driver. Posting a branch for reference [1] to provide and test the idea
behind this series.

[1]: https://git.uk.ideasonboard.com/uajain/linux/commits/branch/uajain/remove-global

Changes in v3:
- Rework 2/6 to 5/6 as per Laurent's review in v2 [2].
- Add a comment for g_regs global __iomem ptr.
[2]: https://lore.kernel.org/linux-staging/4ba0d745-fc8d-4886-b71a-1f19962e9103@moroto.mountain/T/#m440ee992442cc82ea43092b7c895823c918d105f

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

Umang Jain (6):
  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: Drop global members for tracking connections
  staging: vc04_services: Drop global variables tracking allocated pages
  staging: vc04_services: Drop completed TODO item

 drivers/staging/vc04_services/interface/TODO  |  15 --
 .../interface/vchiq_arm/vchiq_arm.c           | 142 +++++++++---------
 .../interface/vchiq_arm/vchiq_arm.h           |  24 +++
 .../interface/vchiq_arm/vchiq_bus.c           |   4 +
 .../interface/vchiq_arm/vchiq_bus.h           |   3 +
 .../interface/vchiq_arm/vchiq_connected.c     |  35 ++---
 .../interface/vchiq_arm/vchiq_connected.h     |   3 +-
 7 files changed, 119 insertions(+), 107 deletions(-)


base-commit: 68bb540b1aefded1d58a9f956568d5316643d291
-- 
2.43.0

[PATCH v3 1/6] staging: vc04_services: Drop g_once_init global variable

[Umang Jain]

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

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

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

[PATCH v3 2/6] 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 responsible for book-keeping.

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 38 +++++++++----------
 .../interface/vchiq_arm/vchiq_arm.h           | 12 ++++++
 2 files changed, 31 insertions(+), 19 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 1579bd4e5263..73405a1f50ee 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -29,7 +29,6 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/uaccess.h>
-#include <soc/bcm2835/raspberrypi-firmware.h>
 
 #include "vchiq_core.h"
 #include "vchiq_ioctl.h"
@@ -71,16 +70,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 struct vchiq_platform_info bcm2835_info = {
 	.cache_line_size = 32,
 };
 
-static struct vchiq_drvdata bcm2836_drvdata = {
+static struct vchiq_platform_info bcm2836_info = {
 	.cache_line_size = 64,
 };
 
@@ -466,8 +460,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 +478,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->pinfo->cache_line_size;
 	g_fragments_size = 2 * g_cache_line_size;
 
 	/* Allocate space for the channels in coherent memory */
@@ -1706,8 +1700,8 @@ void vchiq_platform_conn_state_changed(struct vchiq_state *state,
 }
 
 static const struct of_device_id vchiq_of_match[] = {
-	{ .compatible = "brcm,bcm2835-vchiq", .data = &bcm2835_drvdata },
-	{ .compatible = "brcm,bcm2836-vchiq", .data = &bcm2836_drvdata },
+	{ .compatible = "brcm,bcm2835-vchiq", .data = &bcm2835_info },
+	{ .compatible = "brcm,bcm2836-vchiq", .data = &bcm2836_info },
 	{},
 };
 MODULE_DEVICE_TABLE(of, vchiq_of_match);
@@ -1716,12 +1710,13 @@ 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 *platform_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)
+	platform_info = (struct vchiq_platform_info *)of_id->data;
+	if (!platform_info)
 		return -EINVAL;
 
 	fw_node = of_find_compatible_node(NULL, NULL,
@@ -1731,12 +1726,17 @@ static int vchiq_probe(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
-	drvdata->fw = devm_rpi_firmware_get(&pdev->dev, fw_node);
+	mgmt = kzalloc(sizeof(struct vchiq_drv_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->pinfo = platform_info;
+	platform_set_drvdata(pdev, mgmt);
 
 	err = vchiq_platform_init(pdev, &g_state);
 	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 7844ef765a00..fc4122c27e94 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -11,6 +11,9 @@
 #include <linux/platform_device.h>
 #include <linux/semaphore.h>
 #include <linux/atomic.h>
+
+#include <soc/bcm2835/raspberrypi-firmware.h>
+
 #include "vchiq_core.h"
 #include "vchiq_debugfs.h"
 
@@ -25,6 +28,15 @@ enum USE_TYPE_E {
 	USE_TYPE_VCHIQ
 };
 
+struct vchiq_platform_info {
+	const unsigned int cache_line_size;
+};
+
+struct vchiq_drv_mgmt {
+	struct rpi_firmware *fw;
+	const struct vchiq_platform_info *pinfo;
+};
+
 struct user_service {
 	struct vchiq_service *service;
 	void __user *userdata;
-- 
2.43.0

Re: [PATCH v3 2/6] staging: vc04_services: vchiq_arm: Split driver static and runtime data

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Thu, Mar 21, 2024 at 04:07:36PM +0530, Umang Jain wrote:
> vchiq_drvdata combines two types of book-keeping data. There is
> platform-specific static data (for e.g. cache lines size) and then
> data needed for book-keeping at runtime.
> 
> Split the data into two structures: struct vchiq_platform_info and
> struct vchiq_drv_mgmt. The vchiq_drv_mgmt is allocated at runtime
> during probe and will be extended in subsequent patches to remove
> all global variables responsible for book-keeping.
> 
> No functional changes intended in this patch.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../interface/vchiq_arm/vchiq_arm.c           | 38 +++++++++----------
>  .../interface/vchiq_arm/vchiq_arm.h           | 12 ++++++
>  2 files changed, 31 insertions(+), 19 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 1579bd4e5263..73405a1f50ee 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -29,7 +29,6 @@
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/uaccess.h>
> -#include <soc/bcm2835/raspberrypi-firmware.h>
>  
>  #include "vchiq_core.h"
>  #include "vchiq_ioctl.h"
> @@ -71,16 +70,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 struct vchiq_platform_info bcm2835_info = {

static const

>  	.cache_line_size = 32,
>  };
>  
> -static struct vchiq_drvdata bcm2836_drvdata = {
> +static struct vchiq_platform_info bcm2836_info = {

Ditto.

>  	.cache_line_size = 64,
>  };
>  
> @@ -466,8 +460,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 +478,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->pinfo->cache_line_size;
>  	g_fragments_size = 2 * g_cache_line_size;
>  
>  	/* Allocate space for the channels in coherent memory */
> @@ -1706,8 +1700,8 @@ void vchiq_platform_conn_state_changed(struct vchiq_state *state,
>  }
>  
>  static const struct of_device_id vchiq_of_match[] = {
> -	{ .compatible = "brcm,bcm2835-vchiq", .data = &bcm2835_drvdata },
> -	{ .compatible = "brcm,bcm2836-vchiq", .data = &bcm2836_drvdata },
> +	{ .compatible = "brcm,bcm2835-vchiq", .data = &bcm2835_info },
> +	{ .compatible = "brcm,bcm2836-vchiq", .data = &bcm2836_info },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, vchiq_of_match);
> @@ -1716,12 +1710,13 @@ 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 *platform_info;

Or just 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)
> +	platform_info = (struct vchiq_platform_info *)of_id->data;

	platform_info = of_device_get_match_data(&pdev->dev);

and drop the of_id variable.

> +	if (!platform_info)
>  		return -EINVAL;
>  
>  	fw_node = of_find_compatible_node(NULL, NULL,
> @@ -1731,12 +1726,17 @@ static int vchiq_probe(struct platform_device *pdev)
>  		return -ENOENT;
>  	}
>  
> -	drvdata->fw = devm_rpi_firmware_get(&pdev->dev, fw_node);
> +	mgmt = kzalloc(sizeof(struct vchiq_drv_mgmt), GFP_KERNEL);

	mgmt = kzalloc(sizeof(*mgmt), GFP_KERNEL);

Where is this freed ?

> +	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->pinfo = platform_info;
> +	platform_set_drvdata(pdev, mgmt);
>  
>  	err = vchiq_platform_init(pdev, &g_state);
>  	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 7844ef765a00..fc4122c27e94 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> @@ -11,6 +11,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/semaphore.h>
>  #include <linux/atomic.h>
> +
> +#include <soc/bcm2835/raspberrypi-firmware.h>

You can also simply add a forward-declaration:

struct rpi_firmware;

This reduces compilation time by lowering the number of headers that are
included.

> +
>  #include "vchiq_core.h"
>  #include "vchiq_debugfs.h"
>  
> @@ -25,6 +28,15 @@ enum USE_TYPE_E {
>  	USE_TYPE_VCHIQ
>  };
>  
> +struct vchiq_platform_info {
> +	const unsigned int cache_line_size;

const doesn't help muc here.

> +};
> +
> +struct vchiq_drv_mgmt {

The name sounds a bit weird but I can live with it :-)

> +	struct rpi_firmware *fw;
> +	const struct vchiq_platform_info *pinfo;

Unless you expect another kind of info later in this structure, I'd name
the field just 'info'.

> +};
> +
>  struct user_service {
>  	struct vchiq_service *service;
>  	void __user *userdata;

-- 
Regards,

Laurent Pinchart

[PATCH v3 3/6] 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.
Instead use the value from vchiq_platform_info directly.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 49 +++++++++++--------
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 73405a1f50ee..01b4e4b010c6 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -71,6 +71,17 @@ static struct vchiq_device *bcm2835_audio;
 static struct vchiq_device *bcm2835_camera;
 
 static struct vchiq_platform_info bcm2835_info = {
+	/*
+	 * 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.
+	 */
 	.cache_line_size = 32,
 };
 
@@ -130,17 +141,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;
@@ -211,6 +211,8 @@ static struct vchiq_pagelist_info *
 create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
 		size_t count, unsigned short type)
 {
+	struct platform_device *pdev;
+	struct vchiq_drv_mgmt *drv_mgmt;
 	struct pagelist *pagelist;
 	struct vchiq_pagelist_info *pagelistinfo;
 	struct page **pages;
@@ -225,6 +227,9 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
 	if (count >= INT_MAX - PAGE_SIZE)
 		return NULL;
 
+	pdev = to_platform_device(instance->state->dev->parent);
+	drv_mgmt = platform_get_drvdata(pdev);
+
 	if (buf)
 		offset = (uintptr_t)buf & (PAGE_SIZE - 1);
 	else
@@ -367,9 +372,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->pinfo->cache_line_size - 1)) ||
 	    ((pagelist->offset + pagelist->length) &
-	    (g_cache_line_size - 1)))) {
+	    (drv_mgmt->pinfo->cache_line_size - 1)))) {
 		char *fragments;
 
 		if (down_interruptible(&g_free_fragments_sema)) {
@@ -395,12 +400,17 @@ static void
 free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagelistinfo,
 	      int actual)
 {
+	struct platform_device *pdev;
+	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);
 
+	pdev = to_platform_device(instance->state->dev->parent);
+	drv_mgmt = platform_get_drvdata(pdev);
+
 	/*
 	 * NOTE: dma_unmap_sg must be called before the
 	 * cpu can touch any of the data/pages.
@@ -416,10 +426,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->pinfo->cache_line_size - pagelist->offset) &
+			(drv_mgmt->pinfo->cache_line_size - 1);
 		tail_bytes = (pagelist->offset + actual) &
-			(g_cache_line_size - 1);
+			(drv_mgmt->pinfo->cache_line_size - 1);
 
 		if ((actual >= 0) && (head_bytes != 0)) {
 			if (head_bytes > actual)
@@ -434,8 +444,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->pinfo->cache_line_size - 1),
+				fragments + drv_mgmt->pinfo->cache_line_size,
 				tail_bytes);
 
 		down(&g_free_fragments_mutex);
@@ -478,8 +488,7 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 	if (err < 0)
 		return err;
 
-	g_cache_line_size = drv_mgmt->pinfo->cache_line_size;
-	g_fragments_size = 2 * g_cache_line_size;
+	g_fragments_size = 2 * drv_mgmt->pinfo->cache_line_size;
 
 	/* Allocate space for the channels in coherent memory */
 	slot_mem_size = PAGE_ALIGN(TOTAL_SLOTS * VCHIQ_SLOT_SIZE);
-- 
2.43.0

Re: [PATCH v3 3/6] staging: vc04_services: vchiq_arm: Drop g_cache_line_size

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Thu, Mar 21, 2024 at 04:07:37PM +0530, Umang Jain wrote:
> The cache-line-size is cached in struct vchiq_platform_info.
> There is no need to cache this again via g_cache_line_size.
> Instead use the value from vchiq_platform_info directly.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../interface/vchiq_arm/vchiq_arm.c           | 49 +++++++++++--------
>  1 file changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 73405a1f50ee..01b4e4b010c6 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -71,6 +71,17 @@ static struct vchiq_device *bcm2835_audio;
>  static struct vchiq_device *bcm2835_camera;
>  
>  static struct vchiq_platform_info bcm2835_info = {
> +	/*
> +	 * 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.

This belongs to a kerneldoc block for the vchiq_drvdata structure.

> +	 *
> +	 * 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.

This should be moved to the same location too, but the comment is a bit
outdated, as the driver uses 64 for bcm2836 and compatible. You could
write

	 * Modern VPU firmware looks for a DT "cache-line-size" property in the
	 * VCHIQ node and will overwrite it with the actual L2 cache size, which
	 * the kernel must then respect. That property was rejected upstream, so
	 * we use a value derived from the compatible string.

> +	 */
>  	.cache_line_size = 32,
>  };
>  
> @@ -130,17 +141,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;
> @@ -211,6 +211,8 @@ static struct vchiq_pagelist_info *
>  create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
>  		size_t count, unsigned short type)
>  {
> +	struct platform_device *pdev;
> +	struct vchiq_drv_mgmt *drv_mgmt;
>  	struct pagelist *pagelist;
>  	struct vchiq_pagelist_info *pagelistinfo;
>  	struct page **pages;
> @@ -225,6 +227,9 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
>  	if (count >= INT_MAX - PAGE_SIZE)
>  		return NULL;
>  
> +	pdev = to_platform_device(instance->state->dev->parent);
> +	drv_mgmt = platform_get_drvdata(pdev);

	drv_mgmt = dev_get_drvdata(instance->state->dev->parent);

Same below.

On a related note, I think you should move the contents of
vchiq_connected.c to vchiq_arm.c. It's a small file, containing two
functions only, with one of them being called only from vchiq_arm.c. It
would allow you to make the vchiq_drv_mgmt structure private to
vchiq_arm.c. This can be done on top of this series.

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

> +
>  	if (buf)
>  		offset = (uintptr_t)buf & (PAGE_SIZE - 1);
>  	else
> @@ -367,9 +372,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->pinfo->cache_line_size - 1)) ||
>  	    ((pagelist->offset + pagelist->length) &
> -	    (g_cache_line_size - 1)))) {
> +	    (drv_mgmt->pinfo->cache_line_size - 1)))) {
>  		char *fragments;
>  
>  		if (down_interruptible(&g_free_fragments_sema)) {
> @@ -395,12 +400,17 @@ static void
>  free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagelistinfo,
>  	      int actual)
>  {
> +	struct platform_device *pdev;
> +	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);
>  
> +	pdev = to_platform_device(instance->state->dev->parent);
> +	drv_mgmt = platform_get_drvdata(pdev);
> +
>  	/*
>  	 * NOTE: dma_unmap_sg must be called before the
>  	 * cpu can touch any of the data/pages.
> @@ -416,10 +426,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->pinfo->cache_line_size - pagelist->offset) &
> +			(drv_mgmt->pinfo->cache_line_size - 1);
>  		tail_bytes = (pagelist->offset + actual) &
> -			(g_cache_line_size - 1);
> +			(drv_mgmt->pinfo->cache_line_size - 1);
>  
>  		if ((actual >= 0) && (head_bytes != 0)) {
>  			if (head_bytes > actual)
> @@ -434,8 +444,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->pinfo->cache_line_size - 1),
> +				fragments + drv_mgmt->pinfo->cache_line_size,
>  				tail_bytes);
>  
>  		down(&g_free_fragments_mutex);
> @@ -478,8 +488,7 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>  	if (err < 0)
>  		return err;
>  
> -	g_cache_line_size = drv_mgmt->pinfo->cache_line_size;
> -	g_fragments_size = 2 * g_cache_line_size;
> +	g_fragments_size = 2 * drv_mgmt->pinfo->cache_line_size;
>  
>  	/* Allocate space for the channels in coherent memory */
>  	slot_mem_size = PAGE_ALIGN(TOTAL_SLOTS * VCHIQ_SLOT_SIZE);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/6] staging: vc04_services: vchiq_arm: Drop g_cache_line_size

[Laurent Pinchart]

On Thu, Mar 21, 2024 at 09:04:32PM +0200, Laurent Pinchart wrote:
> Hi Umang,
> 
> Thank you for the patch.
> 
> On Thu, Mar 21, 2024 at 04:07:37PM +0530, Umang Jain wrote:
> > The cache-line-size is cached in struct vchiq_platform_info.
> > There is no need to cache this again via g_cache_line_size.
> > Instead use the value from vchiq_platform_info directly.
> > 
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  .../interface/vchiq_arm/vchiq_arm.c           | 49 +++++++++++--------
> >  1 file changed, 29 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > index 73405a1f50ee..01b4e4b010c6 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -71,6 +71,17 @@ static struct vchiq_device *bcm2835_audio;
> >  static struct vchiq_device *bcm2835_camera;
> >  
> >  static struct vchiq_platform_info bcm2835_info = {
> > +	/*
> > +	 * 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.
> 
> This belongs to a kerneldoc block for the vchiq_drvdata structure.
> 
> > +	 *
> > +	 * 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.
> 
> This should be moved to the same location too, but the comment is a bit
> outdated, as the driver uses 64 for bcm2836 and compatible. You could
> write
> 
> 	 * Modern VPU firmware looks for a DT "cache-line-size" property in the
> 	 * VCHIQ node and will overwrite it with the actual L2 cache size, which
> 	 * the kernel must then respect. That property was rejected upstream, so
> 	 * we use a value derived from the compatible string.
> 
> > +	 */
> >  	.cache_line_size = 32,
> >  };
> >  
> > @@ -130,17 +141,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;
> > @@ -211,6 +211,8 @@ static struct vchiq_pagelist_info *
> >  create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
> >  		size_t count, unsigned short type)
> >  {
> > +	struct platform_device *pdev;
> > +	struct vchiq_drv_mgmt *drv_mgmt;
> >  	struct pagelist *pagelist;
> >  	struct vchiq_pagelist_info *pagelistinfo;
> >  	struct page **pages;
> > @@ -225,6 +227,9 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
> >  	if (count >= INT_MAX - PAGE_SIZE)
> >  		return NULL;
> >  
> > +	pdev = to_platform_device(instance->state->dev->parent);
> > +	drv_mgmt = platform_get_drvdata(pdev);
> 
> 	drv_mgmt = dev_get_drvdata(instance->state->dev->parent);
> 
> Same below.
> 
> On a related note, I think you should move the contents of
> vchiq_connected.c to vchiq_arm.c. It's a small file, containing two
> functions only, with one of them being called only from vchiq_arm.c. It
> would allow you to make the vchiq_drv_mgmt structure private to
> vchiq_arm.c. This can be done on top of this series.

In which case vchiq_call_connected_callbacks() would become a static
function, the vchiq_add_connected_callback() function declaration could
go to vchiq.h or vchiq_bus.h, and vchiq_connected.h should be dropped.

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +
> >  	if (buf)
> >  		offset = (uintptr_t)buf & (PAGE_SIZE - 1);
> >  	else
> > @@ -367,9 +372,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->pinfo->cache_line_size - 1)) ||
> >  	    ((pagelist->offset + pagelist->length) &
> > -	    (g_cache_line_size - 1)))) {
> > +	    (drv_mgmt->pinfo->cache_line_size - 1)))) {
> >  		char *fragments;
> >  
> >  		if (down_interruptible(&g_free_fragments_sema)) {
> > @@ -395,12 +400,17 @@ static void
> >  free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagelistinfo,
> >  	      int actual)
> >  {
> > +	struct platform_device *pdev;
> > +	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);
> >  
> > +	pdev = to_platform_device(instance->state->dev->parent);
> > +	drv_mgmt = platform_get_drvdata(pdev);
> > +
> >  	/*
> >  	 * NOTE: dma_unmap_sg must be called before the
> >  	 * cpu can touch any of the data/pages.
> > @@ -416,10 +426,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->pinfo->cache_line_size - pagelist->offset) &
> > +			(drv_mgmt->pinfo->cache_line_size - 1);
> >  		tail_bytes = (pagelist->offset + actual) &
> > -			(g_cache_line_size - 1);
> > +			(drv_mgmt->pinfo->cache_line_size - 1);
> >  
> >  		if ((actual >= 0) && (head_bytes != 0)) {
> >  			if (head_bytes > actual)
> > @@ -434,8 +444,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->pinfo->cache_line_size - 1),
> > +				fragments + drv_mgmt->pinfo->cache_line_size,
> >  				tail_bytes);
> >  
> >  		down(&g_free_fragments_mutex);
> > @@ -478,8 +488,7 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
> >  	if (err < 0)
> >  		return err;
> >  
> > -	g_cache_line_size = drv_mgmt->pinfo->cache_line_size;
> > -	g_fragments_size = 2 * g_cache_line_size;
> > +	g_fragments_size = 2 * drv_mgmt->pinfo->cache_line_size;
> >  
> >  	/* Allocate space for the channels in coherent memory */
> >  	slot_mem_size = PAGE_ALIGN(TOTAL_SLOTS * VCHIQ_SLOT_SIZE);

-- 
Regards,

Laurent Pinchart

[PATCH v3 4/6] staging: vc04_services: Drop global members for tracking connections

[Umang Jain]

Drop global members for tracking vchiq driver connections in
vchiq_connected.[ch] and use the struct vchiq_drv_mgmt to
track the connections.

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

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           |  2 +-
 .../interface/vchiq_arm/vchiq_arm.h           |  6 +++++
 .../interface/vchiq_arm/vchiq_bus.c           |  4 +++
 .../interface/vchiq_arm/vchiq_bus.h           |  3 +++
 .../interface/vchiq_arm/vchiq_connected.c     | 25 +++++++++----------
 .../interface/vchiq_arm/vchiq_connected.h     |  3 ++-
 6 files changed, 28 insertions(+), 15 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 01b4e4b010c6..ba096bcb32c8 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -559,7 +559,7 @@ 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();
+	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 fc4122c27e94..1190fab2efc4 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,8 @@
 #define MAX_ELEMENTS 8
 #define MSG_QUEUE_SIZE 128
 
+#define VCHIQ_DRV_MAX_CALLBACKS 10
+
 enum USE_TYPE_E {
 	USE_TYPE_SERVICE,
 	USE_TYPE_VCHIQ
@@ -35,6 +37,10 @@ struct vchiq_platform_info {
 struct vchiq_drv_mgmt {
 	struct rpi_firmware *fw;
 	const struct vchiq_platform_info *pinfo;
+
+	bool connected;
+	int num_deferred_callbacks;
+	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 68f830d75531..fb837e64838b 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
@@ -54,6 +54,7 @@ static void vchiq_device_release(struct device *dev)
 struct vchiq_device *
 vchiq_device_register(struct device *parent, const char *name)
 {
+	struct platform_device *pdev;
 	struct vchiq_device *device;
 	int ret;
 
@@ -67,6 +68,9 @@ vchiq_device_register(struct device *parent, const char *name)
 	device->dev.dma_mask = &device->dev.coherent_dma_mask;
 	device->dev.release = vchiq_device_release;
 
+	pdev = to_platform_device(parent);
+	device->drv_mgmt = platform_get_drvdata(pdev);
+
 	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..b0e85ca2365e 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>
 
+#include "vchiq_arm.h"
+
 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..ec5e9107868e 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
@@ -8,9 +8,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);
 
 /*
@@ -21,21 +18,23 @@ static   DEFINE_MUTEX(g_connected_mutex);
  */
 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(&g_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);
@@ -46,17 +45,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))
 		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;
+	drv_mgmt->num_deferred_callbacks = 0;
+	drv_mgmt->connected = true;
 	mutex_unlock(&g_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..0a3adefc69e0 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
@@ -1,12 +1,13 @@
 /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
 /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
 
+#include "vchiq_arm.h"
 #include "vchiq_bus.h"
 
 #ifndef VCHIQ_CONNECTED_H
 #define VCHIQ_CONNECTED_H
 
 void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
-void vchiq_call_connected_callbacks(void);
+void vchiq_call_connected_callbacks(struct vchiq_drv_mgmt *mgmt);
 
 #endif /* VCHIQ_CONNECTED_H */
-- 
2.43.0

Re: [PATCH v3 4/6] staging: vc04_services: Drop global members for tracking connections

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Thu, Mar 21, 2024 at 04:07:38PM +0530, Umang Jain wrote:
> Drop global members for tracking vchiq driver connections in
> vchiq_connected.[ch] and use the struct vchiq_drv_mgmt to
> track the connections.

I'd write "Move global connection tracking to vchiq_drv_mgmt" in the
subject line, as you're not just dropping them completely. Same in the
commit message.

> Also, store a vchiq_drv_mgmt pointer to struct vchiq_device to
> have easy access to struct vchiq_drv_mgmt across vchiq devices.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../interface/vchiq_arm/vchiq_arm.c           |  2 +-
>  .../interface/vchiq_arm/vchiq_arm.h           |  6 +++++
>  .../interface/vchiq_arm/vchiq_bus.c           |  4 +++
>  .../interface/vchiq_arm/vchiq_bus.h           |  3 +++
>  .../interface/vchiq_arm/vchiq_connected.c     | 25 +++++++++----------
>  .../interface/vchiq_arm/vchiq_connected.h     |  3 ++-
>  6 files changed, 28 insertions(+), 15 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 01b4e4b010c6..ba096bcb32c8 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -559,7 +559,7 @@ 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();
> +	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 fc4122c27e94..1190fab2efc4 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,8 @@
>  #define MAX_ELEMENTS 8
>  #define MSG_QUEUE_SIZE 128
>  
> +#define VCHIQ_DRV_MAX_CALLBACKS 10

It would be nice to remove this, but I think we can live with it for
now.

> +
>  enum USE_TYPE_E {
>  	USE_TYPE_SERVICE,
>  	USE_TYPE_VCHIQ
> @@ -35,6 +37,10 @@ struct vchiq_platform_info {
>  struct vchiq_drv_mgmt {
>  	struct rpi_firmware *fw;
>  	const struct vchiq_platform_info *pinfo;
> +
> +	bool connected;
> +	int num_deferred_callbacks;
> +	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 68f830d75531..fb837e64838b 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
> @@ -54,6 +54,7 @@ static void vchiq_device_release(struct device *dev)
>  struct vchiq_device *
>  vchiq_device_register(struct device *parent, const char *name)
>  {
> +	struct platform_device *pdev;
>  	struct vchiq_device *device;
>  	int ret;
>  
> @@ -67,6 +68,9 @@ vchiq_device_register(struct device *parent, const char *name)
>  	device->dev.dma_mask = &device->dev.coherent_dma_mask;
>  	device->dev.release = vchiq_device_release;
>  
> +	pdev = to_platform_device(parent);
> +	device->drv_mgmt = platform_get_drvdata(pdev);

	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..b0e85ca2365e 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>
>  
> +#include "vchiq_arm.h"
> +

Use a declaration instead.

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..ec5e9107868e 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> @@ -8,9 +8,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);

Any reason not to move this to vchiq_drv_mgmt ?

>  
>  /*
> @@ -21,21 +18,23 @@ static   DEFINE_MUTEX(g_connected_mutex);
>   */
>  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(&g_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);
> @@ -46,17 +45,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))
>  		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;
> +	drv_mgmt->num_deferred_callbacks = 0;
> +	drv_mgmt->connected = true;
>  	mutex_unlock(&g_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..0a3adefc69e0 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> @@ -1,12 +1,13 @@
>  /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
>  /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
>  
> +#include "vchiq_arm.h"

Structure declaration too.

>  #include "vchiq_bus.h"
>  
>  #ifndef VCHIQ_CONNECTED_H
>  #define VCHIQ_CONNECTED_H
>  
>  void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
> -void vchiq_call_connected_callbacks(void);
> +void vchiq_call_connected_callbacks(struct vchiq_drv_mgmt *mgmt);
>  
>  #endif /* VCHIQ_CONNECTED_H */

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 4/6] staging: vc04_services: Drop global members for tracking connections

[Umang Jain]

Hi Laurent,

On 22/03/24 12:42 am, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Thu, Mar 21, 2024 at 04:07:38PM +0530, Umang Jain wrote:
>> Drop global members for tracking vchiq driver connections in
>> vchiq_connected.[ch] and use the struct vchiq_drv_mgmt to
>> track the connections.
> I'd write "Move global connection tracking to vchiq_drv_mgmt" in the
> subject line, as you're not just dropping them completely. Same in the
> commit message.
>
>> Also, store a vchiq_drv_mgmt pointer to struct vchiq_device to
>> have easy access to struct vchiq_drv_mgmt across vchiq devices.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   .../interface/vchiq_arm/vchiq_arm.c           |  2 +-
>>   .../interface/vchiq_arm/vchiq_arm.h           |  6 +++++
>>   .../interface/vchiq_arm/vchiq_bus.c           |  4 +++
>>   .../interface/vchiq_arm/vchiq_bus.h           |  3 +++
>>   .../interface/vchiq_arm/vchiq_connected.c     | 25 +++++++++----------
>>   .../interface/vchiq_arm/vchiq_connected.h     |  3 ++-
>>   6 files changed, 28 insertions(+), 15 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 01b4e4b010c6..ba096bcb32c8 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -559,7 +559,7 @@ 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();
>> +	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 fc4122c27e94..1190fab2efc4 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,8 @@
>>   #define MAX_ELEMENTS 8
>>   #define MSG_QUEUE_SIZE 128
>>   
>> +#define VCHIQ_DRV_MAX_CALLBACKS 10
> It would be nice to remove this, but I think we can live with it for
> now.
>
>> +
>>   enum USE_TYPE_E {
>>   	USE_TYPE_SERVICE,
>>   	USE_TYPE_VCHIQ
>> @@ -35,6 +37,10 @@ struct vchiq_platform_info {
>>   struct vchiq_drv_mgmt {
>>   	struct rpi_firmware *fw;
>>   	const struct vchiq_platform_info *pinfo;
>> +
>> +	bool connected;
>> +	int num_deferred_callbacks;
>> +	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 68f830d75531..fb837e64838b 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
>> @@ -54,6 +54,7 @@ static void vchiq_device_release(struct device *dev)
>>   struct vchiq_device *
>>   vchiq_device_register(struct device *parent, const char *name)
>>   {
>> +	struct platform_device *pdev;
>>   	struct vchiq_device *device;
>>   	int ret;
>>   
>> @@ -67,6 +68,9 @@ vchiq_device_register(struct device *parent, const char *name)
>>   	device->dev.dma_mask = &device->dev.coherent_dma_mask;
>>   	device->dev.release = vchiq_device_release;
>>   
>> +	pdev = to_platform_device(parent);
>> +	device->drv_mgmt = platform_get_drvdata(pdev);
> 	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..b0e85ca2365e 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>
>>   
>> +#include "vchiq_arm.h"
>> +
> Use a declaration instead.
>
> 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..ec5e9107868e 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>> @@ -8,9 +8,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);
> Any reason not to move this to vchiq_drv_mgmt ?

yes, I looked into this - and it turned out the DEFINE_MUTEX 
implementation differs based on whether CONFIG_PREEMPT_RT is enabled or 
not, and I am not very sure if replacing it with struct mutex would have 
repercussions. I don't understand these bits very well yet and it seems 
(from a cursory look that DEFINE_MUTEX seems to be statically declared...)


>
>>   
>>   /*
>> @@ -21,21 +18,23 @@ static   DEFINE_MUTEX(g_connected_mutex);
>>    */
>>   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(&g_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);
>> @@ -46,17 +45,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))
>>   		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;
>> +	drv_mgmt->num_deferred_callbacks = 0;
>> +	drv_mgmt->connected = true;
>>   	mutex_unlock(&g_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..0a3adefc69e0 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>> @@ -1,12 +1,13 @@
>>   /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
>>   /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
>>   
>> +#include "vchiq_arm.h"
> Structure declaration too.
>
>>   #include "vchiq_bus.h"
>>   
>>   #ifndef VCHIQ_CONNECTED_H
>>   #define VCHIQ_CONNECTED_H
>>   
>>   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 */

Re: [PATCH v3 4/6] staging: vc04_services: Drop global members for tracking connections

[Laurent Pinchart]

Hi Umang,

On Fri, Mar 22, 2024 at 10:25:49AM +0530, Umang Jain wrote:
> On 22/03/24 12:42 am, Laurent Pinchart wrote:
> > On Thu, Mar 21, 2024 at 04:07:38PM +0530, Umang Jain wrote:
> >> Drop global members for tracking vchiq driver connections in
> >> vchiq_connected.[ch] and use the struct vchiq_drv_mgmt to
> >> track the connections.
> >
> > I'd write "Move global connection tracking to vchiq_drv_mgmt" in the
> > subject line, as you're not just dropping them completely. Same in the
> > commit message.
> >
> >> Also, store a vchiq_drv_mgmt pointer to struct vchiq_device to
> >> have easy access to struct vchiq_drv_mgmt across vchiq devices.
> >>
> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >> ---
> >>   .../interface/vchiq_arm/vchiq_arm.c           |  2 +-
> >>   .../interface/vchiq_arm/vchiq_arm.h           |  6 +++++
> >>   .../interface/vchiq_arm/vchiq_bus.c           |  4 +++
> >>   .../interface/vchiq_arm/vchiq_bus.h           |  3 +++
> >>   .../interface/vchiq_arm/vchiq_connected.c     | 25 +++++++++----------
> >>   .../interface/vchiq_arm/vchiq_connected.h     |  3 ++-
> >>   6 files changed, 28 insertions(+), 15 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 01b4e4b010c6..ba096bcb32c8 100644
> >> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >> @@ -559,7 +559,7 @@ 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();
> >> +	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 fc4122c27e94..1190fab2efc4 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,8 @@
> >>   #define MAX_ELEMENTS 8
> >>   #define MSG_QUEUE_SIZE 128
> >>   
> >> +#define VCHIQ_DRV_MAX_CALLBACKS 10
> >
> > It would be nice to remove this, but I think we can live with it for
> > now.
> >
> >> +
> >>   enum USE_TYPE_E {
> >>   	USE_TYPE_SERVICE,
> >>   	USE_TYPE_VCHIQ
> >> @@ -35,6 +37,10 @@ struct vchiq_platform_info {
> >>   struct vchiq_drv_mgmt {
> >>   	struct rpi_firmware *fw;
> >>   	const struct vchiq_platform_info *pinfo;
> >> +
> >> +	bool connected;
> >> +	int num_deferred_callbacks;
> >> +	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 68f830d75531..fb837e64838b 100644
> >> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
> >> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
> >> @@ -54,6 +54,7 @@ static void vchiq_device_release(struct device *dev)
> >>   struct vchiq_device *
> >>   vchiq_device_register(struct device *parent, const char *name)
> >>   {
> >> +	struct platform_device *pdev;
> >>   	struct vchiq_device *device;
> >>   	int ret;
> >>   
> >> @@ -67,6 +68,9 @@ vchiq_device_register(struct device *parent, const char *name)
> >>   	device->dev.dma_mask = &device->dev.coherent_dma_mask;
> >>   	device->dev.release = vchiq_device_release;
> >>   
> >> +	pdev = to_platform_device(parent);
> >> +	device->drv_mgmt = platform_get_drvdata(pdev);
> >
> > 	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..b0e85ca2365e 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>
> >>   
> >> +#include "vchiq_arm.h"
> >> +
> >
> > Use a declaration instead.
> >
> > 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..ec5e9107868e 100644
> >> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> >> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> >> @@ -8,9 +8,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);
> >
> > Any reason not to move this to vchiq_drv_mgmt ?
> 
> yes, I looked into this - and it turned out the DEFINE_MUTEX 
> implementation differs based on whether CONFIG_PREEMPT_RT is enabled or 
> not, and I am not very sure if replacing it with struct mutex would have 
> repercussions. I don't understand these bits very well yet and it seems 
> (from a cursory look that DEFINE_MUTEX seems to be statically declared...)

I don't think there's any issue moving the mutex to the vchiq_drv_mgmt
structure.

> >>   
> >>   /*
> >> @@ -21,21 +18,23 @@ static   DEFINE_MUTEX(g_connected_mutex);
> >>    */
> >>   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(&g_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);
> >> @@ -46,17 +45,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))
> >>   		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;
> >> +	drv_mgmt->num_deferred_callbacks = 0;
> >> +	drv_mgmt->connected = true;
> >>   	mutex_unlock(&g_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..0a3adefc69e0 100644
> >> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> >> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> >> @@ -1,12 +1,13 @@
> >>   /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> >>   /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
> >>   
> >> +#include "vchiq_arm.h"
> >
> > Structure declaration too.
> >
> >>   #include "vchiq_bus.h"
> >>   
> >>   #ifndef VCHIQ_CONNECTED_H
> >>   #define VCHIQ_CONNECTED_H
> >>   
> >>   void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
> >> -void vchiq_call_connected_callbacks(void);
> >> +void vchiq_call_connected_callbacks(struct vchiq_drv_mgmt *mgmt);
> >>   
> >>   #endif /* VCHIQ_CONNECTED_H */

-- 
Regards,

Laurent Pinchart

[PATCH v3 5/6] staging: vc04_services: Drop 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.

However, g_regs is intentionally kept global since it is read/write
in fast paths (for e.g. doorbell IRQs). Introducing it in struct
vchiq_drv_mgmt and getting it in remote_event_signal handler, turns
out to be quite tedious. Add a comment for the same.

Drop the TODO item as well, as it is now totally addressed:
> Get rid of all non essential global structures and create a proper per
  device structure

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/staging/vc04_services/interface/TODO  |  8 ---
 .../interface/vchiq_arm/vchiq_arm.c           | 57 +++++++++----------
 .../interface/vchiq_arm/vchiq_arm.h           |  6 ++
 3 files changed, 34 insertions(+), 37 deletions(-)

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 ba096bcb32c8..c4807b302718 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -140,13 +140,11 @@ struct vchiq_pagelist_info {
 	unsigned int scatterlist_mapped;
 };
 
+/*
+ * Essential global pointer since read/written on fast paths
+ * (doorbell IRQs).
+ */
 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,
@@ -377,20 +375,20 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
 	    (drv_mgmt->pinfo->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;
@@ -420,10 +418,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->pinfo->cache_line_size - pagelist->offset) &
@@ -448,11 +446,11 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
 				fragments + drv_mgmt->pinfo->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. */
@@ -488,11 +486,11 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 	if (err < 0)
 		return err;
 
-	g_fragments_size = 2 * drv_mgmt->pinfo->cache_line_size;
+	drv_mgmt->fragments_size = 2 * drv_mgmt->pinfo->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);
@@ -512,15 +510,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 1190fab2efc4..1134187da6ab 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -41,6 +41,12 @@ struct vchiq_drv_mgmt {
 	bool connected;
 	int num_deferred_callbacks;
 	void (*deferred_callback[VCHIQ_DRV_MAX_CALLBACKS])(void);
+
+	struct semaphore free_fragments_sema;
+	struct semaphore free_fragments_mutex;
+	char *fragments_base;
+	char *free_fragments;
+	unsigned int fragments_size;
 };
 
 struct user_service {
-- 
2.43.0

Re: [PATCH v3 5/6] staging: vc04_services: Drop global variables tracking allocated pages

[Laurent Pinchart]

Hi Uamng,

Thank you for the patch.

On Thu, Mar 21, 2024 at 04:07:39PM +0530, Umang Jain wrote:
> The variables tracking allocated pages fragments in vchiq_arm.c
> can be easily moved to struct vchiq_drv_mgmt instead of being global.
> This helps us to drop the non-essential global variables in the vchiq
> interface.
> 
> However, g_regs is intentionally kept global since it is read/write
> in fast paths (for e.g. doorbell IRQs). Introducing it in struct
> vchiq_drv_mgmt and getting it in remote_event_signal handler, turns
> out to be quite tedious. Add a comment for the same.

The fast path argument doesn't really hold in my opinion, I think you
can retrieve the variable through vchiq_state in vchiq_doorbell_irq().

All callers for remote_event_signal have a vchiq_state pointer, so it
shouldn't be that difficult to get rid of that global variable. It
doesn't have to be done in this patch, it can be done on top.

> 
> Drop the TODO item as well, as it is now totally addressed:
> > Get rid of all non essential global structures and create a proper per
>   device structure

Don't you need to also drop the g_state variable ?

> 
> No functional changes intended in this patch.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  drivers/staging/vc04_services/interface/TODO  |  8 ---
>  .../interface/vchiq_arm/vchiq_arm.c           | 57 +++++++++----------
>  .../interface/vchiq_arm/vchiq_arm.h           |  6 ++
>  3 files changed, 34 insertions(+), 37 deletions(-)
> 
> 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 ba096bcb32c8..c4807b302718 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -140,13 +140,11 @@ struct vchiq_pagelist_info {
>  	unsigned int scatterlist_mapped;
>  };
>  
> +/*
> + * Essential global pointer since read/written on fast paths
> + * (doorbell IRQs).
> + */
>  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);

Why is this code using a semaphore as a mutex ?? It should be replaced
with a real mutex. This can be done on top of this patch.

>  
>  static int
>  vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
> @@ -377,20 +375,20 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
>  	    (drv_mgmt->pinfo->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;
> @@ -420,10 +418,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->pinfo->cache_line_size - pagelist->offset) &
> @@ -448,11 +446,11 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
>  				fragments + drv_mgmt->pinfo->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. */
> @@ -488,11 +486,11 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>  	if (err < 0)
>  		return err;
>  
> -	g_fragments_size = 2 * drv_mgmt->pinfo->cache_line_size;
> +	drv_mgmt->fragments_size = 2 * drv_mgmt->pinfo->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);
> @@ -512,15 +510,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 1190fab2efc4..1134187da6ab 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> @@ -41,6 +41,12 @@ struct vchiq_drv_mgmt {
>  	bool connected;
>  	int num_deferred_callbacks;
>  	void (*deferred_callback[VCHIQ_DRV_MAX_CALLBACKS])(void);
> +
> +	struct semaphore free_fragments_sema;
> +	struct semaphore free_fragments_mutex;
> +	char *fragments_base;
> +	char *free_fragments;
> +	unsigned int fragments_size;
>  };
>  
>  struct user_service {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 5/6] staging: vc04_services: Drop global variables tracking allocated pages

[Umang Jain]

Hi Laurent,

On 22/03/24 1:20 am, Laurent Pinchart wrote:
> Hi Uamng,
>
> Thank you for the patch.
>
> On Thu, Mar 21, 2024 at 04:07:39PM +0530, Umang Jain wrote:
>> The variables tracking allocated pages fragments in vchiq_arm.c
>> can be easily moved to struct vchiq_drv_mgmt instead of being global.
>> This helps us to drop the non-essential global variables in the vchiq
>> interface.
>>
>> However, g_regs is intentionally kept global since it is read/write
>> in fast paths (for e.g. doorbell IRQs). Introducing it in struct
>> vchiq_drv_mgmt and getting it in remote_event_signal handler, turns
>> out to be quite tedious. Add a comment for the same.
> The fast path argument doesn't really hold in my opinion, I think you
> can retrieve the variable through vchiq_state in vchiq_doorbell_irq().
>
> All callers for remote_event_signal have a vchiq_state pointer, so it
> shouldn't be that difficult to get rid of that global variable. It
> doesn't have to be done in this patch, it can be done on top.
>
>> Drop the TODO item as well, as it is now totally addressed:
>>> Get rid of all non essential global structures and create a proper per
>>    device structure
> Don't you need to also drop the g_state variable ?

Seems essential to me looking at vchiq_get_state() helper

Especially in the vchiq_open() to get a starting state when the char 
device is opened..
>> No functional changes intended in this patch.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   drivers/staging/vc04_services/interface/TODO  |  8 ---
>>   .../interface/vchiq_arm/vchiq_arm.c           | 57 +++++++++----------
>>   .../interface/vchiq_arm/vchiq_arm.h           |  6 ++
>>   3 files changed, 34 insertions(+), 37 deletions(-)
>>
>> 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 ba096bcb32c8..c4807b302718 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -140,13 +140,11 @@ struct vchiq_pagelist_info {
>>   	unsigned int scatterlist_mapped;
>>   };
>>   
>> +/*
>> + * Essential global pointer since read/written on fast paths
>> + * (doorbell IRQs).
>> + */
>>   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);
> Why is this code using a semaphore as a mutex ?? It should be replaced
> with a real mutex. This can be done on top of this patch.
>
>>   
>>   static int
>>   vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
>> @@ -377,20 +375,20 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
>>   	    (drv_mgmt->pinfo->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;
>> @@ -420,10 +418,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->pinfo->cache_line_size - pagelist->offset) &
>> @@ -448,11 +446,11 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
>>   				fragments + drv_mgmt->pinfo->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. */
>> @@ -488,11 +486,11 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>>   	if (err < 0)
>>   		return err;
>>   
>> -	g_fragments_size = 2 * drv_mgmt->pinfo->cache_line_size;
>> +	drv_mgmt->fragments_size = 2 * drv_mgmt->pinfo->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);
>> @@ -512,15 +510,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 1190fab2efc4..1134187da6ab 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
>> @@ -41,6 +41,12 @@ struct vchiq_drv_mgmt {
>>   	bool connected;
>>   	int num_deferred_callbacks;
>>   	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 {

Re: [PATCH v3 5/6] staging: vc04_services: Drop global variables tracking allocated pages

[Laurent Pinchart]

On Fri, Mar 22, 2024 at 02:45:11PM +0530, Umang Jain wrote:
> On 22/03/24 1:20 am, Laurent Pinchart wrote:
> > On Thu, Mar 21, 2024 at 04:07:39PM +0530, Umang Jain wrote:
> >> The variables tracking allocated pages fragments in vchiq_arm.c
> >> can be easily moved to struct vchiq_drv_mgmt instead of being global.
> >> This helps us to drop the non-essential global variables in the vchiq
> >> interface.
> >>
> >> However, g_regs is intentionally kept global since it is read/write
> >> in fast paths (for e.g. doorbell IRQs). Introducing it in struct
> >> vchiq_drv_mgmt and getting it in remote_event_signal handler, turns
> >> out to be quite tedious. Add a comment for the same.
> > The fast path argument doesn't really hold in my opinion, I think you
> > can retrieve the variable through vchiq_state in vchiq_doorbell_irq().
> >
> > All callers for remote_event_signal have a vchiq_state pointer, so it
> > shouldn't be that difficult to get rid of that global variable. It
> > doesn't have to be done in this patch, it can be done on top.
> >
> >> Drop the TODO item as well, as it is now totally addressed:
> >>> Get rid of all non essential global structures and create a proper per
> >>    device structure
> >
> > Don't you need to also drop the g_state variable ?
> 
> Seems essential to me looking at vchiq_get_state() helper
> 
> Especially in the vchiq_open() to get a starting state when the char 
> device is opened..

Nobody said it would be a 5 minutes job :-) There seems to be an endless
amount of refactoring that vchiq could benefit from. It doesn't have to
be all done in one go of course.

> >> No functional changes intended in this patch.
> >>
> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >> ---
> >>   drivers/staging/vc04_services/interface/TODO  |  8 ---
> >>   .../interface/vchiq_arm/vchiq_arm.c           | 57 +++++++++----------
> >>   .../interface/vchiq_arm/vchiq_arm.h           |  6 ++
> >>   3 files changed, 34 insertions(+), 37 deletions(-)
> >>
> >> 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 ba096bcb32c8..c4807b302718 100644
> >> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >> @@ -140,13 +140,11 @@ struct vchiq_pagelist_info {
> >>   	unsigned int scatterlist_mapped;
> >>   };
> >>   
> >> +/*
> >> + * Essential global pointer since read/written on fast paths
> >> + * (doorbell IRQs).
> >> + */
> >>   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);
> >
> > Why is this code using a semaphore as a mutex ?? It should be replaced
> > with a real mutex. This can be done on top of this patch.
> >
> >>   
> >>   static int
> >>   vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
> >> @@ -377,20 +375,20 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
> >>   	    (drv_mgmt->pinfo->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;
> >> @@ -420,10 +418,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->pinfo->cache_line_size - pagelist->offset) &
> >> @@ -448,11 +446,11 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
> >>   				fragments + drv_mgmt->pinfo->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. */
> >> @@ -488,11 +486,11 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
> >>   	if (err < 0)
> >>   		return err;
> >>   
> >> -	g_fragments_size = 2 * drv_mgmt->pinfo->cache_line_size;
> >> +	drv_mgmt->fragments_size = 2 * drv_mgmt->pinfo->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);
> >> @@ -512,15 +510,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 1190fab2efc4..1134187da6ab 100644
> >> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> >> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> >> @@ -41,6 +41,12 @@ struct vchiq_drv_mgmt {
> >>   	bool connected;
> >>   	int num_deferred_callbacks;
> >>   	void (*deferred_callback[VCHIQ_DRV_MAX_CALLBACKS])(void);
> >> +
> >> +	struct semaphore free_fragments_sema;
> >> +	struct semaphore free_fragments_mutex;
> >> +	char *fragments_base;
> >> +	char *free_fragments;
> >> +	unsigned int fragments_size;
> >>   };
> >>   
> >>   struct user_service {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 5/6] staging: vc04_services: Drop global variables tracking allocated pages

[Umang Jain]

Hi Laurent,

On 22/03/24 1:20 am, Laurent Pinchart wrote:
> Hi Uamng,
>
> Thank you for the patch.
>
> On Thu, Mar 21, 2024 at 04:07:39PM +0530, Umang Jain wrote:
>> The variables tracking allocated pages fragments in vchiq_arm.c
>> can be easily moved to struct vchiq_drv_mgmt instead of being global.
>> This helps us to drop the non-essential global variables in the vchiq
>> interface.
>>
>> However, g_regs is intentionally kept global since it is read/write
>> in fast paths (for e.g. doorbell IRQs). Introducing it in struct
>> vchiq_drv_mgmt and getting it in remote_event_signal handler, turns
>> out to be quite tedious. Add a comment for the same.
> The fast path argument doesn't really hold in my opinion, I think you
> can retrieve the variable through vchiq_state in vchiq_doorbell_irq().
>
> All callers for remote_event_signal have a vchiq_state pointer, so it
> shouldn't be that difficult to get rid of that global variable. It
> doesn't have to be done in this patch, it can be done on top.
>
>> Drop the TODO item as well, as it is now totally addressed:
>>> Get rid of all non essential global structures and create a proper per
>>    device structure
> Don't you need to also drop the g_state variable ?
>
>> No functional changes intended in this patch.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   drivers/staging/vc04_services/interface/TODO  |  8 ---
>>   .../interface/vchiq_arm/vchiq_arm.c           | 57 +++++++++----------
>>   .../interface/vchiq_arm/vchiq_arm.h           |  6 ++
>>   3 files changed, 34 insertions(+), 37 deletions(-)
>>
>> 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 ba096bcb32c8..c4807b302718 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -140,13 +140,11 @@ struct vchiq_pagelist_info {
>>   	unsigned int scatterlist_mapped;
>>   };
>>   
>> +/*
>> + * Essential global pointer since read/written on fast paths
>> + * (doorbell IRQs).
>> + */
>>   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);
> Why is this code using a semaphore as a mutex ?? It should be replaced
> with a real mutex. This can be done on top of this patch.

Probably the RPi people can answer this (they're cc'ed to the series) 
but what I found  in `Documentation/locking/mutex-design.rst`

```
Disadvantages
-------------

Unlike its original design and purpose, 'struct mutex' is among the largest
locks in the kernel. E.g: on x86-64 it is 32 bytes, where 'struct semaphore'
is 24 bytes and rw_semaphore is 40 bytes. Larger structure sizes mean 
more CPU
cache and memory footprint.
```

So probably a resource constraint / resource-saving step?
>
>>   
>>   static int
>>   vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
>> @@ -377,20 +375,20 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
>>   	    (drv_mgmt->pinfo->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;
>> @@ -420,10 +418,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->pinfo->cache_line_size - pagelist->offset) &
>> @@ -448,11 +446,11 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
>>   				fragments + drv_mgmt->pinfo->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. */
>> @@ -488,11 +486,11 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>>   	if (err < 0)
>>   		return err;
>>   
>> -	g_fragments_size = 2 * drv_mgmt->pinfo->cache_line_size;
>> +	drv_mgmt->fragments_size = 2 * drv_mgmt->pinfo->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);
>> @@ -512,15 +510,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 1190fab2efc4..1134187da6ab 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
>> @@ -41,6 +41,12 @@ struct vchiq_drv_mgmt {
>>   	bool connected;
>>   	int num_deferred_callbacks;
>>   	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 {

[PATCH v3 6/6] staging: vc04_services: Drop completed TODO item

[Umang Jain]

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

Drop the respective item from the TODO list.

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

diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
index 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.43.0