[PATCH 0/3] staging: vc04_services: Drop remaining global members

[PATCH 0/3] staging: vc04_services: Drop remaining global members

[Umang Jain]

This series works to drop the remaining (non-essential) global members
in vchiq_connected.[ch] and address the following TODO:

```
* Get rid of all non essential global structures and create a proper per
device structure
```

The global members tracks the connections to the vchiq platform driver.
The logic has been moved to wrap all this within the platform driver
data instead so it can be get()ed whenever necessary.

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


Umang Jain (3):
  staging: vc04_services: Move struct vchiq_drvdata to vchiq_core header
  staging: v04_services: Add connection structure to driver data
  staging: vc04_services: Drop global members for tracking connections

 drivers/staging/vc04_services/interface/TODO  |  8 ----
 .../interface/vchiq_arm/vchiq_arm.c           | 18 +++++---
 .../interface/vchiq_arm/vchiq_connected.c     | 44 +++++++++----------
 .../interface/vchiq_arm/vchiq_connected.h     | 16 ++++++-
 .../interface/vchiq_arm/vchiq_core.h          |  9 ++++
 5 files changed, 55 insertions(+), 40 deletions(-)

-- 
2.43.0

[PATCH 1/3] staging: vc04_services: Move struct vchiq_drvdata to vchiq_core header

[Umang Jain]

The vchiq_drvdata structure will be extended in subsequent patches,
to track the vchiq connections (currently tracked by global members).
to the vchiq platform driver. Hence, prepare the struct vchiq_drvdata
by moving it vchiq_core.h header, so it can be shared across.

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c  | 6 ------
 .../staging/vc04_services/interface/vchiq_arm/vchiq_core.h | 7 +++++++
 2 files changed, 7 insertions(+), 6 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..52569517ba4e 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,11 +70,6 @@ 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 = {
 	.cache_line_size = 32,
 };
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index c8527551b58c..331959a91102 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -13,6 +13,8 @@
 #include <linux/rcupdate.h>
 #include <linux/wait.h>
 
+#include <soc/bcm2835/raspberrypi-firmware.h>
+
 #include "../../include/linux/raspberrypi/vchiq.h"
 #include "vchiq_cfg.h"
 
@@ -424,6 +426,11 @@ struct vchiq_config {
 	short version_min;  /* The minimum compatible version of VCHIQ */
 };
 
+struct vchiq_drvdata {
+	const unsigned int cache_line_size;
+	struct rpi_firmware *fw;
+};
+
 extern spinlock_t bulk_waiter_spinlock;
 
 extern const char *
-- 
2.43.0

[PATCH 2/3] staging: v04_services: Add connection structure to driver data

[Umang Jain]

Introduce a new struct vchiq_connected, responsible to track
the connections to the vchiq platform driver. The struct is added
as part of vchiq platform driver data.

Adding struct vchiq_connected will help us to move away from global
variables members being used to track the connections in
vchiq_connected.[ch]. This will be done in a subsequent patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_arm.c      | 10 ++++++++++
 .../interface/vchiq_arm/vchiq_connected.h              | 10 ++++++++++
 .../vc04_services/interface/vchiq_arm/vchiq_core.h     |  2 ++
 3 files changed, 22 insertions(+)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 52569517ba4e..b8b51267bcde 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -72,10 +72,20 @@ static struct vchiq_device *bcm2835_camera;
 
 static struct vchiq_drvdata bcm2835_drvdata = {
 	.cache_line_size = 32,
+	.drv_connected = {
+		.connected = 0,
+		.num_deferred_callbacks = 0,
+		.once_init = 0,
+	},
 };
 
 static struct vchiq_drvdata bcm2836_drvdata = {
 	.cache_line_size = 64,
+	.drv_connected = {
+		.connected = 0,
+		.num_deferred_callbacks = 0,
+		.once_init = 0,
+	},
 };
 
 struct vchiq_arm_state {
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..cb5cba94dd54 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
@@ -6,6 +6,16 @@
 #ifndef VCHIQ_CONNECTED_H
 #define VCHIQ_CONNECTED_H
 
+#define  VCHIQ_DRV_MAX_CALLBACKS  10
+
+struct vchiq_connected {
+	int connected;
+	int num_deferred_callbacks;
+	int once_init;
+
+	void (*deferred_callback[VCHIQ_DRV_MAX_CALLBACKS])(void);
+};
+
 void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
 void vchiq_call_connected_callbacks(void);
 
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 331959a91102..e36a8cd8533a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -17,6 +17,7 @@
 
 #include "../../include/linux/raspberrypi/vchiq.h"
 #include "vchiq_cfg.h"
+#include "vchiq_connected.h"
 
 /* Do this so that we can test-build the code on non-rpi systems */
 #if IS_ENABLED(CONFIG_RASPBERRYPI_FIRMWARE)
@@ -429,6 +430,7 @@ struct vchiq_config {
 struct vchiq_drvdata {
 	const unsigned int cache_line_size;
 	struct rpi_firmware *fw;
+	struct vchiq_connected drv_connected;
 };
 
 extern spinlock_t bulk_waiter_spinlock;
-- 
2.43.0

[PATCH 3/3] 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_connected present
as a part of vchiq platform driver data.

All calls have been adjusted to use the struct vchiq_connected.

Since the non-essential global members have been eliminated,
drop the respective TODO item.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/staging/vc04_services/interface/TODO  |  8 ----
 .../interface/vchiq_arm/vchiq_arm.c           |  2 +-
 .../interface/vchiq_arm/vchiq_connected.c     | 44 +++++++++----------
 .../interface/vchiq_arm/vchiq_connected.h     |  6 ++-
 4 files changed, 26 insertions(+), 34 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 b8b51267bcde..7daad927207a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -560,7 +560,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(&drvdata->drv_connected);
 
 	return 0;
 }
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..4b79fccaa95e 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
@@ -6,19 +6,13 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 
-#define  MAX_CALLBACKS  10
-
-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)
+static void connected_init(struct vchiq_connected *drv_connected)
 {
-	if (!g_once_init)
-		g_once_init = 1;
+	if (!drv_connected->once_init)
+		drv_connected->once_init = 1;
 }
 
 /*
@@ -27,25 +21,29 @@ static void connected_init(void)
  * be made immediately, otherwise it will be deferred until
  * vchiq_call_connected_callbacks is called.
  */
-void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
+void vchiq_add_connected_callback(struct vchiq_device *device,
+				  void (*callback)(void),
+				  struct vchiq_connected *drv_connected)
 {
-	connected_init();
+	unsigned int index;
+
+	connected_init(drv_connected);
 
 	if (mutex_lock_killable(&g_connected_mutex))
 		return;
 
-	if (g_connected) {
+	if (drv_connected->connected) {
 		/* We're already connected. Call the callback immediately. */
 		callback();
 	} else {
-		if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
+		if (drv_connected->num_deferred_callbacks >= VCHIQ_DRV_MAX_CALLBACKS) {
 			dev_err(&device->dev,
 				"core: There already %d callback registered - please increase MAX_CALLBACKS\n",
-				g_num_deferred_callbacks);
+				drv_connected->num_deferred_callbacks);
 		} else {
-			g_deferred_callback[g_num_deferred_callbacks] =
-				callback;
-			g_num_deferred_callbacks++;
+			index = drv_connected->num_deferred_callbacks;
+			drv_connected->deferred_callback[index] = callback;
+			drv_connected->num_deferred_callbacks++;
 		}
 	}
 	mutex_unlock(&g_connected_mutex);
@@ -56,19 +54,19 @@ 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_connected *drv_connected)
 {
 	int i;
 
-	connected_init();
+	connected_init(drv_connected);
 
 	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_connected->num_deferred_callbacks; i++)
+		drv_connected->deferred_callback[i]();
 
-	g_num_deferred_callbacks = 0;
-	g_connected = 1;
+	drv_connected->num_deferred_callbacks = 0;
+	drv_connected->connected = 1;
 	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 cb5cba94dd54..f40d68fc44df 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
@@ -16,7 +16,9 @@ struct vchiq_connected {
 	void (*deferred_callback[VCHIQ_DRV_MAX_CALLBACKS])(void);
 };
 
-void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
-void vchiq_call_connected_callbacks(void);
+void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void),
+				  struct vchiq_connected *drv_connected);
+void vchiq_call_connected_callbacks(struct vchiq_connected *drv_connected);
+
 
 #endif /* VCHIQ_CONNECTED_H */
-- 
2.43.0

Re: [PATCH 2/3] staging: v04_services: Add connection structure to driver data

[Dan Carpenter]

On Tue, Mar 12, 2024 at 04:46:06AM +0530, Umang Jain wrote:
> 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 52569517ba4e..b8b51267bcde 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -72,10 +72,20 @@ static struct vchiq_device *bcm2835_camera;
>  
>  static struct vchiq_drvdata bcm2835_drvdata = {
>  	.cache_line_size = 32,
> +	.drv_connected = {
> +		.connected = 0,
> +		.num_deferred_callbacks = 0,
> +		.once_init = 0,
> +	},
>  };
>  
>  static struct vchiq_drvdata bcm2836_drvdata = {
>  	.cache_line_size = 64,
> +	.drv_connected = {
> +		.connected = 0,
> +		.num_deferred_callbacks = 0,
> +		.once_init = 0,
> +	},
>  };
>  

Modifying vchiq_arm.c is unnecessary, because these are initialized to
zero by default.  Once you remove that then this is quite small and
easier to review when it's merged together with patch 3.



>  struct vchiq_arm_state {
> 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..cb5cba94dd54 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> @@ -6,6 +6,16 @@
>  #ifndef VCHIQ_CONNECTED_H
>  #define VCHIQ_CONNECTED_H
>  
> +#define  VCHIQ_DRV_MAX_CALLBACKS  10
> +
> +struct vchiq_connected {
> +	int connected;

connected should be type bool.

> +	int num_deferred_callbacks;
> +	int once_init;
> +
> +	void (*deferred_callback[VCHIQ_DRV_MAX_CALLBACKS])(void);
> +};

regards,
dan carpenter

Re: [PATCH 3/3] staging: vc04_services: Drop global members for tracking connections

[Dan Carpenter]

On Tue, Mar 12, 2024 at 04:46:07AM +0530, Umang Jain wrote:
> 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..4b79fccaa95e 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> @@ -6,19 +6,13 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  
> -#define  MAX_CALLBACKS  10
> -
> -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)
> +static void connected_init(struct vchiq_connected *drv_connected)
>  {
> -	if (!g_once_init)
> -		g_once_init = 1;
> +	if (!drv_connected->once_init)
> +		drv_connected->once_init = 1;


This is a nonsense function...  once_init is never used.  Could you
delete this function in a separate patch at the start?

>  }
>  

Otherwise, I think it looks okay.  How are you testing this, btw?

regards,
dan carpenter