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

[PATCH v2 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 prep-up patch for subsequent patches.

Patch 3/6 drops global variables tracking connections to vchiq driver
through vchiq_connected[.ch]. New struct vchiq_connected is introduced
and added to vchiq_drvdata.

Patch 4/6 drops g_cache_line_size and use direct value from
vchiq_drvdata

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 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: Move struct vchiq_drvdata to header
  staging: vc04_services: Drop global members for tracking connections
  staging: vc04_services: vchiq_arm: Drop g_cache_line_size
  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           | 109 +++++++++---------
 .../interface/vchiq_arm/vchiq_arm.h           |  17 +++
 .../interface/vchiq_arm/vchiq_connected.c     |  43 +++----
 .../interface/vchiq_arm/vchiq_connected.h     |  15 ++-
 5 files changed, 99 insertions(+), 100 deletions(-)


base-commit: 68bb540b1aefded1d58a9f956568d5316643d291
-- 
2.43.0

[PATCH v2 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>
---
 .../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

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

[Kieran Bingham]

Quoting Umang Jain (2024-03-14 10:06:02)
> g_once_init is not used in a meaningful way anywhere. Drop it
> along with connected_init() which sets it.

It certainly looks lonely and useless.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Suggested-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../interface/vchiq_arm/vchiq_connected.c            | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> index 3cad13f09e37..4604a2f4d2de 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> @@ -11,16 +11,8 @@
>  static   int                        g_connected;
>  static   int                        g_num_deferred_callbacks;
>  static   void (*g_deferred_callback[MAX_CALLBACKS])(void);
> -static   int                        g_once_init;
>  static   DEFINE_MUTEX(g_connected_mutex);
>  
> -/* Function to initialize our lock */
> -static void connected_init(void)
> -{
> -       if (!g_once_init)
> -               g_once_init = 1;
> -}
> -
>  /*
>   * This function is used to defer initialization until the vchiq stack is
>   * initialized. If the stack is already initialized, then the callback will
> @@ -29,8 +21,6 @@ static void connected_init(void)
>   */
>  void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
>  {
> -       connected_init();
> -
>         if (mutex_lock_killable(&g_connected_mutex))
>                 return;
>  
> @@ -60,8 +50,6 @@ void vchiq_call_connected_callbacks(void)
>  {
>         int i;
>  
> -       connected_init();
> -
>         if (mutex_lock_killable(&g_connected_mutex))
>                 return;
>  
> -- 
> 2.43.0
>

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

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Thu, Mar 14, 2024 at 03:36:02PM +0530, Umang Jain wrote:
> 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>

An easy one :-)

Reviewed-by: Laurent Pinchart <laurent.pinchart@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;
>  

-- 
Regards,

Laurent Pinchart

[PATCH v2 2/6] staging: vc04_services: vchiq_arm: Move struct vchiq_drvdata to 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 to vchiq_arm.h, 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_arm.h | 8 ++++++++
 2 files changed, 8 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_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index 7844ef765a00..7fef05e7389c 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,11 @@ enum USE_TYPE_E {
 	USE_TYPE_VCHIQ
 };
 
+struct vchiq_drvdata {
+	const unsigned int cache_line_size;
+	struct rpi_firmware *fw;
+};
+
 struct user_service {
 	struct vchiq_service *service;
 	void __user *userdata;
-- 
2.43.0

Re: [PATCH v2 2/6] staging: vc04_services: vchiq_arm: Move struct vchiq_drvdata to header

[Kieran Bingham]

Quoting Umang Jain (2024-03-14 10:06:03)
> 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 to vchiq_arm.h, so it can be shared across.
> 
> No functional changes intended in this patch.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> 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_arm.h | 8 ++++++++
>  2 files changed, 8 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_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> index 7844ef765a00..7fef05e7389c 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,11 @@ enum USE_TYPE_E {
>         USE_TYPE_VCHIQ
>  };
>  
> +struct vchiq_drvdata {
> +       const unsigned int cache_line_size;
> +       struct rpi_firmware *fw;
> +};
> +
>  struct user_service {
>         struct vchiq_service *service;
>         void __user *userdata;
> -- 
> 2.43.0
>

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

[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 (struct vchiq_drvdata).

Drop global members for tracking vchiq driver connections in
vchiq_connected.[ch] and use the struct vchiq_connected present
in struct vchiq_drvdata.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           |  2 +-
 .../interface/vchiq_arm/vchiq_arm.h           |  3 ++
 .../interface/vchiq_arm/vchiq_connected.c     | 33 +++++++++----------
 .../interface/vchiq_arm/vchiq_connected.h     | 15 +++++++--
 4 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 52569517ba4e..8c7520dee5f4 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -550,7 +550,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_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index 7fef05e7389c..f2fd572df2b3 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -16,6 +16,7 @@
 
 #include "vchiq_core.h"
 #include "vchiq_debugfs.h"
+#include "vchiq_connected.h"
 
 /* Some per-instance constants */
 #define MAX_COMPLETIONS 128
@@ -31,6 +32,8 @@ enum USE_TYPE_E {
 struct vchiq_drvdata {
 	const unsigned int cache_line_size;
 	struct rpi_firmware *fw;
+
+	struct vchiq_connected drv_connected;
 };
 
 struct user_service {
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..bc21910fe823 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
@@ -6,11 +6,6 @@
 #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   DEFINE_MUTEX(g_connected_mutex);
 
 /*
@@ -19,23 +14,27 @@ static   DEFINE_MUTEX(g_connected_mutex);
  * 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)
 {
+	unsigned int index;
+
 	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);
@@ -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_connected *drv_connected)
 {
 	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_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 = 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..66e56b5af46e 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
@@ -6,7 +6,18 @@
 #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);
+#define  VCHIQ_DRV_MAX_CALLBACKS  10
+
+struct vchiq_connected {
+	bool connected;
+	int num_deferred_callbacks;
+
+	void (*deferred_callback[VCHIQ_DRV_MAX_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 v2 3/6] staging: vc04_services: Drop global members for tracking connections

[Kieran Bingham]

Quoting Umang Jain (2024-03-14 10:06:04)
> 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 (struct vchiq_drvdata).
> 
> Drop global members for tracking vchiq driver connections in
> vchiq_connected.[ch] and use the struct vchiq_connected present
> in struct vchiq_drvdata.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../interface/vchiq_arm/vchiq_arm.c           |  2 +-
>  .../interface/vchiq_arm/vchiq_arm.h           |  3 ++
>  .../interface/vchiq_arm/vchiq_connected.c     | 33 +++++++++----------
>  .../interface/vchiq_arm/vchiq_connected.h     | 15 +++++++--
>  4 files changed, 33 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 52569517ba4e..8c7520dee5f4 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -550,7 +550,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_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> index 7fef05e7389c..f2fd572df2b3 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> @@ -16,6 +16,7 @@
>  
>  #include "vchiq_core.h"
>  #include "vchiq_debugfs.h"
> +#include "vchiq_connected.h"
>  
>  /* Some per-instance constants */
>  #define MAX_COMPLETIONS 128
> @@ -31,6 +32,8 @@ enum USE_TYPE_E {
>  struct vchiq_drvdata {
>         const unsigned int cache_line_size;
>         struct rpi_firmware *fw;
> +
> +       struct vchiq_connected drv_connected;
>  };
>  
>  struct user_service {
> 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..bc21910fe823 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> @@ -6,11 +6,6 @@
>  #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   DEFINE_MUTEX(g_connected_mutex);
>  
>  /*
> @@ -19,23 +14,27 @@ static   DEFINE_MUTEX(g_connected_mutex);
>   * 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)

I think I would have called this 'struct vchiq_connection *connection'
but it doesn't really matter, so don't rework unless there's a 'need'
for another iteration.



Aha though I see vchiq_connected matches the name of this c module, so
maybe that's just as/more suitable.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  {
> +       unsigned int index;
> +
>         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);
> @@ -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_connected *drv_connected)
>  {
>         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_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 = 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..66e56b5af46e 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> @@ -6,7 +6,18 @@
>  #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);
> +#define  VCHIQ_DRV_MAX_CALLBACKS  10
> +
> +struct vchiq_connected {
> +       bool connected;
> +       int num_deferred_callbacks;
> +
> +       void (*deferred_callback[VCHIQ_DRV_MAX_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 v2 3/6] staging: vc04_services: Drop global members for tracking connections

[Laurent Pinchart]

On Thu, Mar 14, 2024 at 01:16:06PM +0000, Kieran Bingham wrote:
> Quoting Umang Jain (2024-03-14 10:06:04)
> > 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 (struct vchiq_drvdata).
> > 
> > Drop global members for tracking vchiq driver connections in
> > vchiq_connected.[ch] and use the struct vchiq_connected present
> > in struct vchiq_drvdata.
> > 
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  .../interface/vchiq_arm/vchiq_arm.c           |  2 +-
> >  .../interface/vchiq_arm/vchiq_arm.h           |  3 ++
> >  .../interface/vchiq_arm/vchiq_connected.c     | 33 +++++++++----------
> >  .../interface/vchiq_arm/vchiq_connected.h     | 15 +++++++--
> >  4 files changed, 33 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > index 52569517ba4e..8c7520dee5f4 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -550,7 +550,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_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> > index 7fef05e7389c..f2fd572df2b3 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> > @@ -16,6 +16,7 @@
> >  
> >  #include "vchiq_core.h"
> >  #include "vchiq_debugfs.h"
> > +#include "vchiq_connected.h"
> >  
> >  /* Some per-instance constants */
> >  #define MAX_COMPLETIONS 128
> > @@ -31,6 +32,8 @@ enum USE_TYPE_E {
> >  struct vchiq_drvdata {
> >         const unsigned int cache_line_size;
> >         struct rpi_firmware *fw;
> > +
> > +       struct vchiq_connected drv_connected;
> >  };
> >  
> >  struct user_service {
> > 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..bc21910fe823 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> > @@ -6,11 +6,6 @@
> >  #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   DEFINE_MUTEX(g_connected_mutex);
> >  
> >  /*
> > @@ -19,23 +14,27 @@ static   DEFINE_MUTEX(g_connected_mutex);
> >   * 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)
> 
> I think I would have called this 'struct vchiq_connection *connection'
> but it doesn't really matter, so don't rework unless there's a 'need'
> for another iteration.

Yes, "connected" sounds weird.

This being said, I don't think we should add a new structure, and I also
don't think we should move this to vchiq_drvdata as-is.

vchiq_drvdata combines two things (in just two fields, quite an
achievement :-)):

- The cache_line_size field is static information about the platform. It
  should be moved to a vchiq_platform_info or similar structure, as
  that's what the .data field of the of_device_it entries should point
  to.

- The fw field is runtime data belonging to the vchiq "controller" or
  "bus" device (not sure how to name it). It's the equivalent, in a
  camera sensor driver for instance, of the private structure allocated
  per sensor instance at probe time. The vchiq_drvdata structure should
  be renamed accordingly (e.g. vchiq_controller, ...), and allocated
  dynamically in vchiq_probe().

The global variables related to connections can then move to that
controller structure, there's no need to create a sub-structure. It
should also not be passed explicitly to vchiq_add_connected_callback(),
but should be retrieved from vchiq_device. The vchiq_device structure
should store a pointer to vchiq_controller, which should be initialized
in vchiq_device_register(), using platform_get_drvdata(parent).

Does this make sense ?

> Aha though I see vchiq_connected matches the name of this c module, so
> maybe that's just as/more suitable.
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >  {
> > +       unsigned int index;
> > +
> >         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);
> > @@ -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_connected *drv_connected)
> >  {
> >         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_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 = 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..66e56b5af46e 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> > @@ -6,7 +6,18 @@
> >  #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);
> > +#define  VCHIQ_DRV_MAX_CALLBACKS  10
> > +
> > +struct vchiq_connected {
> > +       bool connected;
> > +       int num_deferred_callbacks;
> > +
> > +       void (*deferred_callback[VCHIQ_DRV_MAX_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 */

-- 
Regards,

Laurent Pinchart

[PATCH v2 4/6] staging: vc04_services: vchiq_arm: Drop g_cache_line_size

[Umang Jain]

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

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 50 +++++++++++--------
 1 file changed, 30 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 8c7520dee5f4..282f83b335d4 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,16 @@ static struct vchiq_device *bcm2835_audio;
 static struct vchiq_device *bcm2835_camera;
 
 static struct vchiq_drvdata bcm2835_drvdata = {
+	/* 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 +140,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 +210,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_drvdata *drvdata;
 	struct pagelist *pagelist;
 	struct vchiq_pagelist_info *pagelistinfo;
 	struct page **pages;
@@ -225,6 +226,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);
+	drvdata = platform_get_drvdata(pdev);
+
 	if (buf)
 		offset = (uintptr_t)buf & (PAGE_SIZE - 1);
 	else
@@ -367,9 +371,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 & (drvdata->cache_line_size - 1)) ||
 	    ((pagelist->offset + pagelist->length) &
-	    (g_cache_line_size - 1)))) {
+	    (drvdata->cache_line_size - 1)))) {
 		char *fragments;
 
 		if (down_interruptible(&g_free_fragments_sema)) {
@@ -395,12 +399,19 @@ static void
 free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagelistinfo,
 	      int actual)
 {
+	struct platform_device *pdev;
+	struct vchiq_drvdata *drvdata;
 	struct pagelist *pagelist = pagelistinfo->pagelist;
 	struct page **pages = pagelistinfo->pages;
 	unsigned int num_pages = pagelistinfo->num_pages;
+	unsigned int cache_line_size;
 
 	dev_dbg(instance->state->dev, "arm: %pK, %d\n", pagelistinfo->pagelist, actual);
 
+	pdev = to_platform_device(instance->state->dev->parent);
+	drvdata = platform_get_drvdata(pdev);
+	cache_line_size = drvdata->cache_line_size;
+
 	/*
 	 * NOTE: dma_unmap_sg must be called before the
 	 * cpu can touch any of the data/pages.
@@ -416,10 +427,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 = (cache_line_size - pagelist->offset) &
+			(cache_line_size - 1);
 		tail_bytes = (pagelist->offset + actual) &
-			(g_cache_line_size - 1);
+			(cache_line_size - 1);
 
 		if ((actual >= 0) && (head_bytes != 0)) {
 			if (head_bytes > actual)
@@ -434,8 +445,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) & ~(cache_line_size - 1),
+				fragments + cache_line_size,
 				tail_bytes);
 
 		down(&g_free_fragments_mutex);
@@ -478,8 +489,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_fragments_size = 2 * g_cache_line_size;
+	g_fragments_size = 2 * drvdata->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 v2 4/6] staging: vc04_services: vchiq_arm: Drop g_cache_line_size

[Kieran Bingham]

Quoting Umang Jain (2024-03-14 10:06:05)
> The cache-line-size is cached in struct vchiq_drvdata.
> There is no need to cache this again via g_cache_line_size
> and use it. Instead use the value from vchiq_drvdata directly.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../interface/vchiq_arm/vchiq_arm.c           | 50 +++++++++++--------
>  1 file changed, 30 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 8c7520dee5f4..282f83b335d4 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,16 @@ static struct vchiq_device *bcm2835_audio;
>  static struct vchiq_device *bcm2835_camera;
>  
>  static struct vchiq_drvdata bcm2835_drvdata = {
> +       /* This value is the size of the L2 cache lines as understood by the

This file has a mix of A)
 /* bulk multiline
  * comments like this
  */


and B)
 /*
  * Bulk multiline
  * comments like this
  */

I'd probably use B) when moving this, but again, no point for a respin
just for that. Only if there's another revision. Or a patch on top to
normalise throughout if you felt like it, as this is only a move anyway.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +        * 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 +140,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 +210,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_drvdata *drvdata;
>         struct pagelist *pagelist;
>         struct vchiq_pagelist_info *pagelistinfo;
>         struct page **pages;
> @@ -225,6 +226,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);
> +       drvdata = platform_get_drvdata(pdev);
> +
>         if (buf)
>                 offset = (uintptr_t)buf & (PAGE_SIZE - 1);
>         else
> @@ -367,9 +371,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 & (drvdata->cache_line_size - 1)) ||
>             ((pagelist->offset + pagelist->length) &
> -           (g_cache_line_size - 1)))) {
> +           (drvdata->cache_line_size - 1)))) {
>                 char *fragments;
>  
>                 if (down_interruptible(&g_free_fragments_sema)) {
> @@ -395,12 +399,19 @@ static void
>  free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagelistinfo,
>               int actual)
>  {
> +       struct platform_device *pdev;
> +       struct vchiq_drvdata *drvdata;
>         struct pagelist *pagelist = pagelistinfo->pagelist;
>         struct page **pages = pagelistinfo->pages;
>         unsigned int num_pages = pagelistinfo->num_pages;
> +       unsigned int cache_line_size;
>  
>         dev_dbg(instance->state->dev, "arm: %pK, %d\n", pagelistinfo->pagelist, actual);
>  
> +       pdev = to_platform_device(instance->state->dev->parent);
> +       drvdata = platform_get_drvdata(pdev);
> +       cache_line_size = drvdata->cache_line_size;
> +
>         /*
>          * NOTE: dma_unmap_sg must be called before the
>          * cpu can touch any of the data/pages.
> @@ -416,10 +427,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 = (cache_line_size - pagelist->offset) &
> +                       (cache_line_size - 1);
>                 tail_bytes = (pagelist->offset + actual) &
> -                       (g_cache_line_size - 1);
> +                       (cache_line_size - 1);
>  
>                 if ((actual >= 0) && (head_bytes != 0)) {
>                         if (head_bytes > actual)
> @@ -434,8 +445,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) & ~(cache_line_size - 1),
> +                               fragments + cache_line_size,
>                                 tail_bytes);
>  
>                 down(&g_free_fragments_mutex);
> @@ -478,8 +489,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_fragments_size = 2 * g_cache_line_size;
> +       g_fragments_size = 2 * drvdata->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 v2 4/6] staging: vc04_services: vchiq_arm: Drop g_cache_line_size

[Dan Carpenter]

On Thu, Mar 14, 2024 at 02:51:19PM +0000, Kieran Bingham wrote:
> Quoting Umang Jain (2024-03-14 10:06:05)
> > The cache-line-size is cached in struct vchiq_drvdata.
> > There is no need to cache this again via g_cache_line_size
> > and use it. Instead use the value from vchiq_drvdata directly.
> > 
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  .../interface/vchiq_arm/vchiq_arm.c           | 50 +++++++++++--------
> >  1 file changed, 30 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 8c7520dee5f4..282f83b335d4 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,16 @@ static struct vchiq_device *bcm2835_audio;
> >  static struct vchiq_device *bcm2835_camera;
> >  
> >  static struct vchiq_drvdata bcm2835_drvdata = {
> > +       /* This value is the size of the L2 cache lines as understood by the
> 
> This file has a mix of A)
>  /* bulk multiline
>   * comments like this
>   */
> 
> 
> and B)
>  /*
>   * Bulk multiline
>   * comments like this
>   */
> 
> I'd probably use B) when moving this, but again, no point for a respin
> just for that. Only if there's another revision. Or a patch on top to
> normalise throughout if you felt like it, as this is only a move anyway.

Yeah.  It should be B.   A is only for in networking.  But we can change
it later.

regards,
dan carpenter

[PATCH v2 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_drvdata instead of being global.
This helps us to drop the non-essential global variables in the vchiq
interface.

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           | 53 +++++++++----------
 .../interface/vchiq_arm/vchiq_arm.h           |  6 +++
 3 files changed, 30 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 282f83b335d4..666ab73ce0d1 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -140,12 +140,6 @@ struct vchiq_pagelist_info {
 };
 
 static void __iomem *g_regs;
-static unsigned int g_fragments_size;
-static char *g_fragments_base;
-static char *g_free_fragments;
-static struct semaphore g_free_fragments_sema;
-
-static DEFINE_SEMAPHORE(g_free_fragments_mutex, 1);
 
 static int
 vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
@@ -376,20 +370,20 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
 	    (drvdata->cache_line_size - 1)))) {
 		char *fragments;
 
-		if (down_interruptible(&g_free_fragments_sema)) {
+		if (down_interruptible(&drvdata->free_fragments_sema)) {
 			cleanup_pagelistinfo(instance, pagelistinfo);
 			return NULL;
 		}
 
-		WARN_ON(!g_free_fragments);
+		WARN_ON(!drvdata->free_fragments);
 
-		down(&g_free_fragments_mutex);
-		fragments = g_free_fragments;
+		down(&drvdata->free_fragments_mutex);
+		fragments = drvdata->free_fragments;
 		WARN_ON(!fragments);
-		g_free_fragments = *(char **)g_free_fragments;
-		up(&g_free_fragments_mutex);
+		drvdata->free_fragments = *(char **)drvdata->free_fragments;
+		up(&drvdata->free_fragments_mutex);
 		pagelist->type = PAGELIST_READ_WITH_FRAGMENTS +
-			(fragments - g_fragments_base) / g_fragments_size;
+			(fragments - drvdata->fragments_base) / drvdata->fragments_size;
 	}
 
 	return pagelistinfo;
@@ -421,10 +415,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 && drvdata->fragments_base) {
+		char *fragments = drvdata->fragments_base +
 			(pagelist->type - PAGELIST_READ_WITH_FRAGMENTS) *
-			g_fragments_size;
+			drvdata->fragments_size;
 		int head_bytes, tail_bytes;
 
 		head_bytes = (cache_line_size - pagelist->offset) &
@@ -449,11 +443,11 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
 				fragments + 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(&drvdata->free_fragments_mutex);
+		*(char **)fragments = drvdata->free_fragments;
+		drvdata->free_fragments = fragments;
+		up(&drvdata->free_fragments_mutex);
+		up(&drvdata->free_fragments_sema);
 	}
 
 	/* Need to mark all the pages dirty. */
@@ -489,11 +483,11 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 	if (err < 0)
 		return err;
 
-	g_fragments_size = 2 * drvdata->cache_line_size;
+	drvdata->fragments_size = 2 * drvdata->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(drvdata->fragments_size * MAX_FRAGMENTS);
 
 	slot_mem = dmam_alloc_coherent(dev, slot_mem_size + frag_mem_size,
 				       &slot_phys, GFP_KERNEL);
@@ -513,15 +507,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;
+	drvdata->fragments_base = (char *)slot_mem + slot_mem_size;
 
-	g_free_fragments = g_fragments_base;
+	drvdata->free_fragments = drvdata->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 **)&drvdata->fragments_base[i * drvdata->fragments_size] =
+			&drvdata->fragments_base[(i + 1) * drvdata->fragments_size];
 	}
-	*(char **)&g_fragments_base[i * g_fragments_size] = NULL;
-	sema_init(&g_free_fragments_sema, MAX_FRAGMENTS);
+	*(char **)&drvdata->fragments_base[i * drvdata->fragments_size] = NULL;
+	sema_init(&drvdata->free_fragments_sema, MAX_FRAGMENTS);
+	sema_init(&drvdata->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 f2fd572df2b3..d5c0a8e9fa2b 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -34,6 +34,12 @@ struct vchiq_drvdata {
 	struct rpi_firmware *fw;
 
 	struct vchiq_connected drv_connected;
+
+	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 v2 5/6] staging: vc04_services: Drop global variables tracking allocated pages

[Kieran Bingham]

Quoting Umang Jain (2024-03-14 10:06:06)
> The variables tracking allocated pages fragments in vchiq_arm.c
> can be easily moved to struct vchiq_drvdata instead of being global.
> This helps us to drop the non-essential global variables in the vchiq
> interface.
> 
> 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           | 53 +++++++++----------
>  .../interface/vchiq_arm/vchiq_arm.h           |  6 +++
>  3 files changed, 30 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 282f83b335d4..666ab73ce0d1 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -140,12 +140,6 @@ struct vchiq_pagelist_info {
>  };
>  
>  static void __iomem *g_regs;

What happens with this one ? Is it essential to be global? Can it be
part of the device? Who's mapping it? or unmapping?

If we're keeping it global, I'd add a comment...

> -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,
> @@ -376,20 +370,20 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
>             (drvdata->cache_line_size - 1)))) {
>                 char *fragments;
>  
> -               if (down_interruptible(&g_free_fragments_sema)) {
> +               if (down_interruptible(&drvdata->free_fragments_sema)) {
>                         cleanup_pagelistinfo(instance, pagelistinfo);
>                         return NULL;
>                 }
>  
> -               WARN_ON(!g_free_fragments);
> +               WARN_ON(!drvdata->free_fragments);
>  
> -               down(&g_free_fragments_mutex);
> -               fragments = g_free_fragments;
> +               down(&drvdata->free_fragments_mutex);
> +               fragments = drvdata->free_fragments;
>                 WARN_ON(!fragments);
> -               g_free_fragments = *(char **)g_free_fragments;
> -               up(&g_free_fragments_mutex);
> +               drvdata->free_fragments = *(char **)drvdata->free_fragments;
> +               up(&drvdata->free_fragments_mutex);
>                 pagelist->type = PAGELIST_READ_WITH_FRAGMENTS +
> -                       (fragments - g_fragments_base) / g_fragments_size;
> +                       (fragments - drvdata->fragments_base) / drvdata->fragments_size;
>         }
>  
>         return pagelistinfo;
> @@ -421,10 +415,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 && drvdata->fragments_base) {
> +               char *fragments = drvdata->fragments_base +
>                         (pagelist->type - PAGELIST_READ_WITH_FRAGMENTS) *
> -                       g_fragments_size;
> +                       drvdata->fragments_size;
>                 int head_bytes, tail_bytes;
>  
>                 head_bytes = (cache_line_size - pagelist->offset) &
> @@ -449,11 +443,11 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
>                                 fragments + 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(&drvdata->free_fragments_mutex);
> +               *(char **)fragments = drvdata->free_fragments;
> +               drvdata->free_fragments = fragments;
> +               up(&drvdata->free_fragments_mutex);
> +               up(&drvdata->free_fragments_sema);
>         }
>  
>         /* Need to mark all the pages dirty. */
> @@ -489,11 +483,11 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>         if (err < 0)
>                 return err;
>  
> -       g_fragments_size = 2 * drvdata->cache_line_size;
> +       drvdata->fragments_size = 2 * drvdata->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(drvdata->fragments_size * MAX_FRAGMENTS);
>  
>         slot_mem = dmam_alloc_coherent(dev, slot_mem_size + frag_mem_size,
>                                        &slot_phys, GFP_KERNEL);
> @@ -513,15 +507,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;
> +       drvdata->fragments_base = (char *)slot_mem + slot_mem_size;
>  
> -       g_free_fragments = g_fragments_base;
> +       drvdata->free_fragments = drvdata->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 **)&drvdata->fragments_base[i * drvdata->fragments_size] =
> +                       &drvdata->fragments_base[(i + 1) * drvdata->fragments_size];
>         }
> -       *(char **)&g_fragments_base[i * g_fragments_size] = NULL;
> -       sema_init(&g_free_fragments_sema, MAX_FRAGMENTS);
> +       *(char **)&drvdata->fragments_base[i * drvdata->fragments_size] = NULL;
> +       sema_init(&drvdata->free_fragments_sema, MAX_FRAGMENTS);
> +       sema_init(&drvdata->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 f2fd572df2b3..d5c0a8e9fa2b 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> @@ -34,6 +34,12 @@ struct vchiq_drvdata {
>         struct rpi_firmware *fw;
>  
>         struct vchiq_connected drv_connected;
> +
> +       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 v2 5/6] staging: vc04_services: Drop global variables tracking allocated pages

[Dan Carpenter]

On Thu, Mar 14, 2024 at 02:54:34PM +0000, Kieran Bingham wrote:
> Quoting Umang Jain (2024-03-14 10:06:06)
> > The variables tracking allocated pages fragments in vchiq_arm.c
> > can be easily moved to struct vchiq_drvdata instead of being global.
> > This helps us to drop the non-essential global variables in the vchiq
> > interface.
> > 
> > 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           | 53 +++++++++----------
> >  .../interface/vchiq_arm/vchiq_arm.h           |  6 +++
> >  3 files changed, 30 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 282f83b335d4..666ab73ce0d1 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -140,12 +140,6 @@ struct vchiq_pagelist_info {
> >  };
> >  
> >  static void __iomem *g_regs;
> 
> What happens with this one ? Is it essential to be global? Can it be
> part of the device? Who's mapping it? or unmapping?
> 
> If we're keeping it global, I'd add a comment...
> 

Ugh...  If we hadn't updated the TODO list then we could have fixed this
in a follow on patch instead of redoing the series...  To be honest, I'd
prefer if it in a separate patch instead of mixed in to patch 5.

regards,
dan carpenter

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

[Umang Jain]

Hi,

On 14/03/24 8:24 pm, Kieran Bingham wrote:
> Quoting Umang Jain (2024-03-14 10:06:06)
>> The variables tracking allocated pages fragments in vchiq_arm.c
>> can be easily moved to struct vchiq_drvdata instead of being global.
>> This helps us to drop the non-essential global variables in the vchiq
>> interface.
>>
>> 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           | 53 +++++++++----------
>>   .../interface/vchiq_arm/vchiq_arm.h           |  6 +++
>>   3 files changed, 30 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 282f83b335d4..666ab73ce0d1 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -140,12 +140,6 @@ struct vchiq_pagelist_info {
>>   };
>>   
>>   static void __iomem *g_regs;
> What happens with this one ? Is it essential to be global? Can it be
> part of the device? Who's mapping it? or unmapping?
>
> If we're keeping it global, I'd add a comment...

The keyword in the series in 'non-essential' and I feel this one is 
essential one.

It's on the irq request path so encapsulating it in the platform driver 
data and then again trying to get it from there, will make this affect 
performance.

See remote_event_signal() and vchiq_doorbell_irq()  in vchiq_arm.c

(For testing, a added a dev_info() in the vchiq_doorbell_irq() and it 
brought down the consistent 30fps IMX219 streaming to (15-26)fps 
inconsistent range)

>
>> -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,
>> @@ -376,20 +370,20 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
>>              (drvdata->cache_line_size - 1)))) {
>>                  char *fragments;
>>   
>> -               if (down_interruptible(&g_free_fragments_sema)) {
>> +               if (down_interruptible(&drvdata->free_fragments_sema)) {
>>                          cleanup_pagelistinfo(instance, pagelistinfo);
>>                          return NULL;
>>                  }
>>   
>> -               WARN_ON(!g_free_fragments);
>> +               WARN_ON(!drvdata->free_fragments);
>>   
>> -               down(&g_free_fragments_mutex);
>> -               fragments = g_free_fragments;
>> +               down(&drvdata->free_fragments_mutex);
>> +               fragments = drvdata->free_fragments;
>>                  WARN_ON(!fragments);
>> -               g_free_fragments = *(char **)g_free_fragments;
>> -               up(&g_free_fragments_mutex);
>> +               drvdata->free_fragments = *(char **)drvdata->free_fragments;
>> +               up(&drvdata->free_fragments_mutex);
>>                  pagelist->type = PAGELIST_READ_WITH_FRAGMENTS +
>> -                       (fragments - g_fragments_base) / g_fragments_size;
>> +                       (fragments - drvdata->fragments_base) / drvdata->fragments_size;
>>          }
>>   
>>          return pagelistinfo;
>> @@ -421,10 +415,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 && drvdata->fragments_base) {
>> +               char *fragments = drvdata->fragments_base +
>>                          (pagelist->type - PAGELIST_READ_WITH_FRAGMENTS) *
>> -                       g_fragments_size;
>> +                       drvdata->fragments_size;
>>                  int head_bytes, tail_bytes;
>>   
>>                  head_bytes = (cache_line_size - pagelist->offset) &
>> @@ -449,11 +443,11 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
>>                                  fragments + 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(&drvdata->free_fragments_mutex);
>> +               *(char **)fragments = drvdata->free_fragments;
>> +               drvdata->free_fragments = fragments;
>> +               up(&drvdata->free_fragments_mutex);
>> +               up(&drvdata->free_fragments_sema);
>>          }
>>   
>>          /* Need to mark all the pages dirty. */
>> @@ -489,11 +483,11 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>>          if (err < 0)
>>                  return err;
>>   
>> -       g_fragments_size = 2 * drvdata->cache_line_size;
>> +       drvdata->fragments_size = 2 * drvdata->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(drvdata->fragments_size * MAX_FRAGMENTS);
>>   
>>          slot_mem = dmam_alloc_coherent(dev, slot_mem_size + frag_mem_size,
>>                                         &slot_phys, GFP_KERNEL);
>> @@ -513,15 +507,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;
>> +       drvdata->fragments_base = (char *)slot_mem + slot_mem_size;
>>   
>> -       g_free_fragments = g_fragments_base;
>> +       drvdata->free_fragments = drvdata->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 **)&drvdata->fragments_base[i * drvdata->fragments_size] =
>> +                       &drvdata->fragments_base[(i + 1) * drvdata->fragments_size];
>>          }
>> -       *(char **)&g_fragments_base[i * g_fragments_size] = NULL;
>> -       sema_init(&g_free_fragments_sema, MAX_FRAGMENTS);
>> +       *(char **)&drvdata->fragments_base[i * drvdata->fragments_size] = NULL;
>> +       sema_init(&drvdata->free_fragments_sema, MAX_FRAGMENTS);
>> +       sema_init(&drvdata->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 f2fd572df2b3..d5c0a8e9fa2b 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
>> @@ -34,6 +34,12 @@ struct vchiq_drvdata {
>>          struct rpi_firmware *fw;
>>   
>>          struct vchiq_connected drv_connected;
>> +
>> +       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 v2 5/6] staging: vc04_services: Drop global variables tracking allocated pages

[Dan Carpenter]

On Fri, Mar 15, 2024 at 11:17:15AM +0530, Umang Jain wrote:
> Hi,
> 
> On 14/03/24 8:24 pm, Kieran Bingham wrote:
> > Quoting Umang Jain (2024-03-14 10:06:06)
> > > 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 282f83b335d4..666ab73ce0d1 100644
> > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > @@ -140,12 +140,6 @@ struct vchiq_pagelist_info {
> > >   };
> > >   static void __iomem *g_regs;
> > What happens with this one ? Is it essential to be global? Can it be
> > part of the device? Who's mapping it? or unmapping?
> > 
> > If we're keeping it global, I'd add a comment...
> 
> The keyword in the series in 'non-essential' and I feel this one is
> essential one.
> 
> It's on the irq request path so encapsulating it in the platform driver data
> and then again trying to get it from there, will make this affect
> performance.
> 
> See remote_event_signal() and vchiq_doorbell_irq()  in vchiq_arm.c
> 

The fast path argument was convincing up to here

> (For testing, a added a dev_info() in the vchiq_doorbell_irq() and it
> brought down the consistent 30fps IMX219 streaming to (15-26)fps
> inconsistent range)
> 

but doing a dev_info() seems way way more expensive than dereferencing
in some pointers in driver data...

Anyway, adding a comment seems like a good idea.

regards,
dan carpenter

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

[Kieran Bingham]

Quoting Dan Carpenter (2024-03-16 08:41:50)
> On Fri, Mar 15, 2024 at 11:17:15AM +0530, Umang Jain wrote:
> > Hi,
> > 
> > On 14/03/24 8:24 pm, Kieran Bingham wrote:
> > > Quoting Umang Jain (2024-03-14 10:06:06)
> > > > 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 282f83b335d4..666ab73ce0d1 100644
> > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > @@ -140,12 +140,6 @@ struct vchiq_pagelist_info {
> > > >   };
> > > >   static void __iomem *g_regs;
> > > What happens with this one ? Is it essential to be global? Can it be
> > > part of the device? Who's mapping it? or unmapping?
> > > 
> > > If we're keeping it global, I'd add a comment...
> > 
> > The keyword in the series in 'non-essential' and I feel this one is
> > essential one.
> > 
> > It's on the irq request path so encapsulating it in the platform driver data
> > and then again trying to get it from there, will make this affect
> > performance.
> > 
> > See remote_event_signal() and vchiq_doorbell_irq()  in vchiq_arm.c
> > 
> 
> The fast path argument was convincing up to here
> 
> > (For testing, a added a dev_info() in the vchiq_doorbell_irq() and it
> > brought down the consistent 30fps IMX219 streaming to (15-26)fps
> > inconsistent range)
> > 
> 
> but doing a dev_info() seems way way more expensive than dereferencing
> in some pointers in driver data...

Indeed, I think even in IRQ context here we should be fine to
dereference the pointers here, and can get rid of the last global.

> Anyway, adding a comment seems like a good idea.

If there's a reason this doesn't work, or if it does break the tests
then a comment should describe that for sure.

--
Kieran


> 
> regards,
> dan carpenter
>

[PATCH v2 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>
---
 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

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

[Dan Carpenter]

Thanks!

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter