vchiq: Patch to separate platform and cdev code

Hi Ojaswin,

url:    https://github.com/0day-ci/linux/commits/Ojaswin-Mujoo/vchiq-Patch-to-separate-platform-and-cdev-code/20210705-000124
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 77ad1f0e99bd00af024e650b862cfda3137af660
config: i386-randconfig-m021-20210705 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

Reported-by Dan Carpenter <[email protected]>

smatch warnings:
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c:1235 vchiq_release() warn: argument 3 to %lx specifier is cast from pointer

vim +1235 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c

62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1227  static int vchiq_release(struct inode *inode, struct file *file)
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1228  {
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1229  	struct vchiq_instance *instance = file->private_data;
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1230  	struct vchiq_state *state = vchiq_get_state();
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1231  	struct vchiq_service *service;
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1232  	int ret = 0;
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1233  	int i;
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1234  

62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04 @1235 vchiq_log_info(vchiq_arm_log_level, "%s: instance=%lx", __func__,
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04 1236 (unsigned long)instance);

This should eventually be converted to %p so it doesn't defeat KASLR. (Not that we really care on raspberry pi, I think?)

62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1237  
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1238  	if (!state) {
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1239  		ret = -EPERM;
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1240  		goto out;
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1241  	}
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1242  
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1243  	/* Ensure videocore is awake to allow termination. */
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1244  	vchiq_use_internal(instance->state, NULL, USE_TYPE_VCHIQ);
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1245  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Hello Dan,

Quoted 24 lines · Dan Carpenter

Hi Ojaswin,

url:    https://github.com/0day-ci/linux/commits/Ojaswin-Mujoo/vchiq-Patch-to-separate-platform-and-cdev-code/20210705-000124 
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git  77ad1f0e99bd00af024e650b862cfda3137af660
config: i386-randconfig-m021-20210705 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

Reported-by Dan Carpenter <[email protected]>

smatch warnings:
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c:1235 vchiq_release() warn: argument 3 to %lx specifier is cast from pointer

vim +1235 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c

62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1227  static int vchiq_release(struct inode *inode, struct file *file)
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1228  {
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1229  	struct vchiq_instance *instance = file->private_data;
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1230  	struct vchiq_state *state = vchiq_get_state();
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1231  	struct vchiq_service *service;
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1232  	int ret = 0;
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1233  	int i;
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1234  

Quoted 5 lines

62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04 @1235 vchiq_log_info(vchiq_arm_log_level, "%s: instance=%lx", __func__,
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04 1236 (unsigned long)instance);

This should eventually be converted to %p so it doesn't defeat KASLR. (Not that we really care on raspberry pi, I think?)

Yes, that does seem right, however, this patchset only moves the code from vchiq_arm.c to vchiq_dev.c and I've not really touched any of the existing code itself (Except moving it to a new file which is why it shows up in the patch).

Hence, I'm not sure if this fix is in scope of this patchset. (I also have a similar warning by kernel test robot here [1] which and I'm not sure if I need to act upon). Maybe we can look at this in a separate patch?

Quoted 37 lines · Ojaswin Mujoo

Hello Dan,

Quoted 24 lines · Dan Carpenter

Hi Ojaswin,

url:    https://github.com/0day-ci/linux/commits/Ojaswin-Mujoo/vchiq-Patch-to-separate-platform-and-cdev-code/20210705-000124 
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git  77ad1f0e99bd00af024e650b862cfda3137af660
config: i386-randconfig-m021-20210705 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

Reported-by Dan Carpenter <[email protected]>

smatch warnings:
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c:1235 vchiq_release() warn: argument 3 to %lx specifier is cast from pointer

vim +1235 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c

62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1227  static int vchiq_release(struct inode *inode, struct file *file)
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1228  {
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1229  	struct vchiq_instance *instance = file->private_data;
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1230  	struct vchiq_state *state = vchiq_get_state();
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1231  	struct vchiq_service *service;
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1232  	int ret = 0;
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1233  	int i;
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04  1234  

Quoted 5 lines

62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04 @1235 vchiq_log_info(vchiq_arm_log_level, "%s: instance=%lx", __func__,
62b5eb4fdf3f5f Ojaswin Mujoo 2021-07-04 1236 (unsigned long)instance);

This should eventually be converted to %p so it doesn't defeat KASLR. (Not that we really care on raspberry pi, I think?)

Yes, that does seem right, however, this patchset only moves the code from vchiq_arm.c to vchiq_dev.c and I've not really touched any of the existing code itself (Except moving it to a new file which is why it shows up in the patch).

Quoted 4 lines

Hence, I'm not sure if this fix is in scope of this patchset. (I also have a similar warning by kernel test robot here [1] which and I'm not sure if I need to act upon). Maybe we can look at this in a separate patch?

Yes. Correct. Don't mix this into the patch, do it "eventually". Or you don't have to do it at all since it wasn't something you introduced. Someone will check the driver for Smatch warnings before it can be moved out of staging.

(I just forwarded the kbuild bot messages for informational purposes only).

regards,
dan carpenter

Hello,

This patchset adderesses the TODO item number 10 specified at:

drivers/staging/vc04-services/interface/TODO

For reference, the task is:

10) Reorganize file structure: Move char driver to it's own file and join both platform files

The cdev is defined alongside with the platform code in vchiq_arm.c. It would be nice to completely decouple it from the actual core code. For instance to be able to use bcm2835-audio without having /dev/vchiq created. One could argue it's better for security reasons or general cleanliness. It could even be interesting to create two different kernel modules, something the likes of vchiq-core.ko and vchiq-dev.ko. This would also ease the upstreaming process.

A summary of the patches is as follows:

- Patch 1: Move cdev init code into a function
- Patch 2: Shift some devlarations from vchiq_arm.c to vchiq_arm.h for sharing
- Patch 3: Move vchiq cdev init code from vchiq_arm.c into vchiq_dev.c
- Patch 4: Decouple cdev code by defining a Kconfig entry to allow optional compilation of it.
- Patch 5: Merge code in vchiq_2835_arm.c to vchiq_arm.c

(More details can be found in the commit messages)

Changes since v2 [1]:

* In Patch 1, as suggested, I have added error handling code back to ensure the driver exits when there is an error in creating vchiq cdev

* I have built this patch against the right kernel (gregkh/staging, staging-next branch) to avoid introducing any unwanted inconsistencies like whitespace changes

I have tested the patch using vchiq_test utility on RPi 3B+.

Additionally, I had a small question regarding the next steps here (quoting my cover letter from v2):

This question is more related to the next set of patches I'm planning to submit. So the last thing left in this TODO is to completely decouple vchiq platform and cdev code into 2 separate modules and I am planning to do that in a different patchset.

The approach I have in mind is to start by using EXPORT_SYMBOL to export all the functions (and accessor functions for variables like g_state) that would be required for cdev init. Majority of these would be exported from vchiq_arm.c and vchiq_core.c, and will then be used in vchiq-dev.ko. Is this the right way to approach this?

Thank you in advance for the help. Please let me know if any changes/ additional info is required.

Regards,
Ojaswin

[1] v2: https://lkml.org/lkml/2021/6/20/63

Ojaswin Mujoo (5):
staging: vchiq: Refactor vchiq cdev code
staging: vchiq: Move certain declarations to vchiq_arm.h
staging: vchiq: Move vchiq char driver to its own file
staging: vchiq: Make creation of vchiq cdev optional
staging: vchiq: Combine vchiq platform code into single file

 drivers/staging/vc04_services/Kconfig         |   10 +
 drivers/staging/vc04_services/Makefile        |    5 +-
 .../interface/vchiq_arm/vchiq_2835_arm.c      |  564 ----
 .../interface/vchiq_arm/vchiq_arm.c           | 2344 +++++------------
 .../interface/vchiq_arm/vchiq_arm.h           |   82 +
 .../interface/vchiq_arm/vchiq_dev.c           | 1440 ++++++++++
 6 files changed, 2265 insertions(+), 2180 deletions(-)
 delete mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
 create mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c

Move the vchiq cdev initialization code to its own function for better code organization. Call the initialization function during probe, thus shifting the whole cdev creation logic (which was earlier split in vchiq_probe() and vchiq_driver_init()) to vchiq_probe().

Signed-off-by Ojaswin Mujoo <[email protected]>

Move certain declarations from vchiq_arm.c to vchiq_arm.h to allow code sharing. This will be useful when we eventually separate the vchiq char driver code from platform code, into its own file.

Signed-off-by Ojaswin Mujoo <[email protected]>

Split the initialization code of vchiq char driver and device files from that of vchiq platform. The char driver code now resides in vchiq_dev.c and the platform code resides in the original vchiq_arm.c file.

This commit focuses on separating the code into different files while maintaining the same functionality. It does not completely decouple them as the cdev init code is still called from the platform's vchiq_probe() function.

Signed-off-by Ojaswin Mujoo <[email protected]>

Before this commit, vchiq cdev (/dev/vchiq) was always created during platform initialization. Introduce a new Kconfig option CONFIG_VCHIQ_CDEV which determines if the cdev will be created or not.

Signed-off-by Ojaswin Mujoo <[email protected]>

Combine the vchiq platform initialization code into a single file by merging vchiq_2835_arm.c into vchiq_arm.c

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 drivers/staging/vc04_services/Makefile        |   1 -
 .../interface/vchiq_arm/vchiq_2835_arm.c      | 564 ------------------
 .../interface/vchiq_arm/vchiq_arm.c           | 549 +++++++++++++++++
 3 files changed, 549 insertions(+), 565 deletions(-)
 delete mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c

drivers/staging/vc04_services/Makefile +0 -1

index 0a04338fc962..1fd191e2e2a5 100644
@@ -4,7 +4,6 @@ obj-$(CONFIG_BCM2835_VCHIQ)	+= vchiq.o
 vchiq-objs := \
    interface/vchiq_arm/vchiq_core.o  \
    interface/vchiq_arm/vchiq_arm.o \
-   interface/vchiq_arm/vchiq_2835_arm.o \
    interface/vchiq_arm/vchiq_debugfs.o \
    interface/vchiq_arm/vchiq_connected.o \
 

drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +53 -0

index 0f2de571eba7..9057d01ffd48 100644
@@ -25,15 +25,32 @@
 #include <linux/rcupdate.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
+#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"
 #include "vchiq_arm.h"
 #include "vchiq_debugfs.h"
+#include "vchiq_connected.h"
+#include "vchiq_pagelist.h"
 
 #define DEVICE_NAME "vchiq"
 
+#define TOTAL_SLOTS (VCHIQ_SLOT_ZERO_SLOTS + 2 * 32)
+
+#define MAX_FRAGMENTS (VCHIQ_NUM_CURRENT_BULKS * 2)
+
+#define VCHIQ_PLATFORM_FRAGMENTS_OFFSET_IDX 0
+#define VCHIQ_PLATFORM_FRAGMENTS_COUNT_IDX  1
+
+#define BELL0	0x00
+#define BELL2	0x08
+
+#define ARM_DS_ACTIVE	BIT(2)
+
 /* Override the default prefix, which would be vchiq_arm (from the filename) */
 #undef MODULE_PARAM_PREFIX
 #define MODULE_PARAM_PREFIX DEVICE_NAME "."

@@ -59,10 +76,542 @@ static struct vchiq_drvdata bcm2836_drvdata = {
 	.cache_line_size = 64,
 };
 
+struct vchiq_2835_state {
+	int inited;
+	struct vchiq_arm_state arm_state;
+};
+
+struct vchiq_pagelist_info {
+	struct pagelist *pagelist;
+	size_t pagelist_buffer_size;
+	dma_addr_t dma_addr;
+	enum dma_data_direction dma_dir;
+	unsigned int num_pages;
+	unsigned int pages_need_release;
+	struct page **pages;
+	struct scatterlist *scatterlist;
+	unsigned int scatterlist_mapped;
+};
+
+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;
+static struct semaphore g_free_fragments_sema;
+static struct device *g_dev;
+
+static DEFINE_SEMAPHORE(g_free_fragments_mutex);

Am 04.07.21 um 17:59 schrieb Ojaswin Mujoo:

Quoted 23 lines

Combine the vchiq platform initialization code into a single file by merging vchiq_2835_arm.c into vchiq_arm.c

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 drivers/staging/vc04_services/Makefile        |   1 -
 .../interface/vchiq_arm/vchiq_2835_arm.c      | 564 ------------------
 .../interface/vchiq_arm/vchiq_arm.c           | 549 +++++++++++++++++
 3 files changed, 549 insertions(+), 565 deletions(-)
 delete mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c

drivers/staging/vc04_services/Makefile +0 -1

index 0a04338fc962..1fd191e2e2a5 100644
@@ -4,7 +4,6 @@ obj-$(CONFIG_BCM2835_VCHIQ)	+= vchiq.o
 vchiq-objs := \
    interface/vchiq_arm/vchiq_core.o  \
    interface/vchiq_arm/vchiq_arm.o \
-   interface/vchiq_arm/vchiq_2835_arm.o \
    interface/vchiq_arm/vchiq_debugfs.o \
    interface/vchiq_arm/vchiq_connected.o \
 

...

Quoted 77 lines

drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +53 -0

index 0f2de571eba7..9057d01ffd48 100644
@@ -25,15 +25,32 @@
 #include <linux/rcupdate.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
+#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"
 #include "vchiq_arm.h"
 #include "vchiq_debugfs.h"
+#include "vchiq_connected.h"
+#include "vchiq_pagelist.h"
 
 #define DEVICE_NAME "vchiq"
 
+#define TOTAL_SLOTS (VCHIQ_SLOT_ZERO_SLOTS + 2 * 32)
+
+#define MAX_FRAGMENTS (VCHIQ_NUM_CURRENT_BULKS * 2)
+
+#define VCHIQ_PLATFORM_FRAGMENTS_OFFSET_IDX 0
+#define VCHIQ_PLATFORM_FRAGMENTS_COUNT_IDX  1
+
+#define BELL0	0x00
+#define BELL2	0x08
+
+#define ARM_DS_ACTIVE	BIT(2)
+
 /* Override the default prefix, which would be vchiq_arm (from the filename) */
 #undef MODULE_PARAM_PREFIX
 #define MODULE_PARAM_PREFIX DEVICE_NAME "."

@@ -59,10 +76,542 @@ static struct vchiq_drvdata bcm2836_drvdata = {
 	.cache_line_size = 64,
 };
 
+struct vchiq_2835_state {
+	int inited;
+	struct vchiq_arm_state arm_state;
+};
+
+struct vchiq_pagelist_info {
+	struct pagelist *pagelist;
+	size_t pagelist_buffer_size;
+	dma_addr_t dma_addr;
+	enum dma_data_direction dma_dir;
+	unsigned int num_pages;
+	unsigned int pages_need_release;
+	struct page **pages;
+	struct scatterlist *scatterlist;
+	unsigned int scatterlist_mapped;
+};
+
+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;
+static struct semaphore g_free_fragments_sema;
+static struct device *g_dev;
+
+static DEFINE_SEMAPHORE(g_free_fragments_mutex);

drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c static struct vchiq_drvdata bcm2836_drvdata = { · line 124

Quoted 10 lines

drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +10 -0

+
+static irqreturn_t
+vchiq_doorbell_irq(int irq, void *dev_id);
+
+static struct vchiq_pagelist_info *
+create_pagelist(char *buf, char __user *ubuf, size_t count, unsigned short type);
+
+static void
+free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
+	      int actual);

please no forward declarations of these 3 functions. Put them into the right order instead ...

Since this patch is independent from the other ones from the series, maybe Greg can merg the rest of the series.

Quoted 116 lines · Stefan Wahren

Am 04.07.21 um 17:59 schrieb Ojaswin Mujoo:

Quoted 23 lines

Combine the vchiq platform initialization code into a single file by merging vchiq_2835_arm.c into vchiq_arm.c

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 drivers/staging/vc04_services/Makefile        |   1 -
 .../interface/vchiq_arm/vchiq_2835_arm.c      | 564 ------------------
 .../interface/vchiq_arm/vchiq_arm.c           | 549 +++++++++++++++++
 3 files changed, 549 insertions(+), 565 deletions(-)
 delete mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c

drivers/staging/vc04_services/Makefile +0 -1

index 0a04338fc962..1fd191e2e2a5 100644
@@ -4,7 +4,6 @@ obj-$(CONFIG_BCM2835_VCHIQ)	+= vchiq.o
 vchiq-objs := \
    interface/vchiq_arm/vchiq_core.o  \
    interface/vchiq_arm/vchiq_arm.o \
-   interface/vchiq_arm/vchiq_2835_arm.o \
    interface/vchiq_arm/vchiq_debugfs.o \
    interface/vchiq_arm/vchiq_connected.o \
 

...

Quoted 77 lines

drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +53 -0

index 0f2de571eba7..9057d01ffd48 100644
@@ -25,15 +25,32 @@
 #include <linux/rcupdate.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
+#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"
 #include "vchiq_arm.h"
 #include "vchiq_debugfs.h"
+#include "vchiq_connected.h"
+#include "vchiq_pagelist.h"
 
 #define DEVICE_NAME "vchiq"
 
+#define TOTAL_SLOTS (VCHIQ_SLOT_ZERO_SLOTS + 2 * 32)
+
+#define MAX_FRAGMENTS (VCHIQ_NUM_CURRENT_BULKS * 2)
+
+#define VCHIQ_PLATFORM_FRAGMENTS_OFFSET_IDX 0
+#define VCHIQ_PLATFORM_FRAGMENTS_COUNT_IDX  1
+
+#define BELL0	0x00
+#define BELL2	0x08
+
+#define ARM_DS_ACTIVE	BIT(2)
+
 /* Override the default prefix, which would be vchiq_arm (from the filename) */
 #undef MODULE_PARAM_PREFIX
 #define MODULE_PARAM_PREFIX DEVICE_NAME "."

@@ -59,10 +76,542 @@ static struct vchiq_drvdata bcm2836_drvdata = {
 	.cache_line_size = 64,
 };
 
+struct vchiq_2835_state {
+	int inited;
+	struct vchiq_arm_state arm_state;
+};
+
+struct vchiq_pagelist_info {
+	struct pagelist *pagelist;
+	size_t pagelist_buffer_size;
+	dma_addr_t dma_addr;
+	enum dma_data_direction dma_dir;
+	unsigned int num_pages;
+	unsigned int pages_need_release;
+	struct page **pages;
+	struct scatterlist *scatterlist;
+	unsigned int scatterlist_mapped;
+};
+
+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;
+static struct semaphore g_free_fragments_sema;
+static struct device *g_dev;
+
+static DEFINE_SEMAPHORE(g_free_fragments_mutex);

drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c static struct vchiq_drvdata bcm2836_drvdata = { · line 124

Quoted 10 lines

drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +10 -0

+
+static irqreturn_t
+vchiq_doorbell_irq(int irq, void *dev_id);
+
+static struct vchiq_pagelist_info *
+create_pagelist(char *buf, char __user *ubuf, size_t count, unsigned short type);
+
+static void
+free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
+	      int actual);

please no forward declarations of these 3 functions. Put them into the right order instead ...

Quoted 5 lines

Since this patch is independent from the other ones from the series, maybe Greg can merg the rest of the series.

Hello Stefan,

Thanks for the review. As for the forward declerations, sure I can fix these 3 functions and send an independent patch for this.

Quoted 123 lines · Ojaswin Mujoo

Quoted 116 lines · Stefan Wahren

Am 04.07.21 um 17:59 schrieb Ojaswin Mujoo:

Quoted 23 lines

Combine the vchiq platform initialization code into a single file by merging vchiq_2835_arm.c into vchiq_arm.c

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 drivers/staging/vc04_services/Makefile        |   1 -
 .../interface/vchiq_arm/vchiq_2835_arm.c      | 564 ------------------
 .../interface/vchiq_arm/vchiq_arm.c           | 549 +++++++++++++++++
 3 files changed, 549 insertions(+), 565 deletions(-)
 delete mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c

drivers/staging/vc04_services/Makefile +0 -1

index 0a04338fc962..1fd191e2e2a5 100644
@@ -4,7 +4,6 @@ obj-$(CONFIG_BCM2835_VCHIQ)	+= vchiq.o
 vchiq-objs := \
    interface/vchiq_arm/vchiq_core.o  \
    interface/vchiq_arm/vchiq_arm.o \
-   interface/vchiq_arm/vchiq_2835_arm.o \
    interface/vchiq_arm/vchiq_debugfs.o \
    interface/vchiq_arm/vchiq_connected.o \
 

...

Quoted 77 lines

drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +53 -0

index 0f2de571eba7..9057d01ffd48 100644
@@ -25,15 +25,32 @@
 #include <linux/rcupdate.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
+#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"
 #include "vchiq_arm.h"
 #include "vchiq_debugfs.h"
+#include "vchiq_connected.h"
+#include "vchiq_pagelist.h"
 
 #define DEVICE_NAME "vchiq"
 
+#define TOTAL_SLOTS (VCHIQ_SLOT_ZERO_SLOTS + 2 * 32)
+
+#define MAX_FRAGMENTS (VCHIQ_NUM_CURRENT_BULKS * 2)
+
+#define VCHIQ_PLATFORM_FRAGMENTS_OFFSET_IDX 0
+#define VCHIQ_PLATFORM_FRAGMENTS_COUNT_IDX  1
+
+#define BELL0	0x00
+#define BELL2	0x08
+
+#define ARM_DS_ACTIVE	BIT(2)
+
 /* Override the default prefix, which would be vchiq_arm (from the filename) */
 #undef MODULE_PARAM_PREFIX
 #define MODULE_PARAM_PREFIX DEVICE_NAME "."

@@ -59,10 +76,542 @@ static struct vchiq_drvdata bcm2836_drvdata = {
 	.cache_line_size = 64,
 };
 
+struct vchiq_2835_state {
+	int inited;
+	struct vchiq_arm_state arm_state;
+};
+
+struct vchiq_pagelist_info {
+	struct pagelist *pagelist;
+	size_t pagelist_buffer_size;
+	dma_addr_t dma_addr;
+	enum dma_data_direction dma_dir;
+	unsigned int num_pages;
+	unsigned int pages_need_release;
+	struct page **pages;
+	struct scatterlist *scatterlist;
+	unsigned int scatterlist_mapped;
+};
+
+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;
+static struct semaphore g_free_fragments_sema;
+static struct device *g_dev;
+
+static DEFINE_SEMAPHORE(g_free_fragments_mutex);

drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c static struct vchiq_drvdata bcm2836_drvdata = { · line 124

Quoted 10 lines

drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +10 -0

+
+static irqreturn_t
+vchiq_doorbell_irq(int irq, void *dev_id);
+
+static struct vchiq_pagelist_info *
+create_pagelist(char *buf, char __user *ubuf, size_t count, unsigned short type);
+
+static void
+free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
+	      int actual);

please no forward declarations of these 3 functions. Put them into the right order instead ...

Quoted 5 lines

Since this patch is independent from the other ones from the series, maybe Greg can merg the rest of the series.

Quoted 4 lines

Hello Stefan,

Thanks for the review. As for the forward declerations, sure I can fix these 3 functions and send an independent patch for this.

Please fix up and resend the whole series properly so that I can apply them that way.

thanks,

greg k-h