vchiq: Patch to separate platform and cdev code
Subsystems (3)
v2 cover vchiq: Patch to separate platform and cdev code
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.
As Stefan suggested in the last revision, I have split the commits into more finer parts for ease of readability and maintainability. I have also added 2 more patches to define a KConfig entry for vchiq cdev, and to merge the code in vchiq_2835_arm.c to vchiq_arm.c
A summary of the patches is now 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)
NOTE: This patchset is built against the raspberry pi mainline kernel at
https://github.com/raspberrypi/linux/blob/rpi-5.10.y, and has been tested on Raspberry Pi 3B+
At this point, I have some questions and ideas and would like to hear your thoughts and suggestions on them:
1. So as mentioned, I have built this against the raspberry pi kernel, since I was not able to figure out a way to build the vanilla mainline kernel for Raspberry Pi. However, I understand that since this will be applied to the mainline, I need to make sure it is consistent with it.
Hence to confirm that, I tried to "git am" this patchset to the mainline kernel but there are some merge conflicts in doing so. I have an idea how to resolve most of them except the following:
- The mainline vchiq_arm.c differs from the one in rapberry pi mainline which caused conflict in Patch 3.
I'm not sure which vchiq_arm.c to treat as the base for my patches. The one in mainline? or the one in raspberry pi's git tree?
2. 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 looking into this and best regards! Ojaswin
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
arch/arm/configs/bcm2709_defconfig | 1 + arch/arm/configs/bcm2711_defconfig | 1 + arch/arm/configs/bcmrpi_defconfig | 1 + drivers/staging/vc04_services/Kconfig | 10 + drivers/staging/vc04_services/Makefile | 5 +- .../interface/vchiq_arm/vchiq_2835_arm.c | 651 ----- .../interface/vchiq_arm/vchiq_arm.c | 2477 ++++++----------- .../interface/vchiq_arm/vchiq_arm.h | 79 + .../interface/vchiq_arm/vchiq_dev.c | 1488 ++++++++++ 9 files changed, 2402 insertions(+), 2311 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
Signature · 1 line
2.25.1
PATCH v2 1/5 staging: vchiq: Refactor vchiq cdev code
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().
--- .../interface/vchiq_arm/vchiq_arm.c | 137 +++++++++++------- 1 file changed, 87 insertions(+), 50 deletions(-)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +87 -50
@@ -2207,6 +2207,81 @@ vchiq_fops = { .read = vchiq_read }; +/** +* vchiq_register_chrdev - Register the char driver for vchiq +* and create the necessary class and +* device files in userspace. +* @parent The parent of the char device. +* +* Returns 0 on success else returns the error code. +*/ +static int vchiq_register_chrdev (struct device *parent) +{ +struct device *vchiq_dev; +int ret; + + vchiq_class =class_create (THIS_MODULE ,DEVICE_NAME ); +if (IS_ERR (vchiq_class)) { +pr_err ("Failed to create vchiq class\n" ); + ret =PTR_ERR (vchiq_class); + goto error_exit; + } + + ret =alloc_chrdev_region (&vchiq_devid,0 ,1 ,DEVICE_NAME ); +if (ret) { +pr_err ("vchiq: Failed to allocate vchiq's chrdev region\n" ); + goto alloc_region_error; + } + +cdev_init (&vchiq_cdev, &vchiq_fops); + vchiq_cdev.owner =THIS_MODULE ; + ret =cdev_add (&vchiq_cdev, vchiq_devid,1 ); +if (ret) { +vchiq_log_error (vchiq_arm_log_level, +"Unable to register vchiq char device" ); + goto cdev_add_error; + } + + vchiq_dev =device_create (vchiq_class, parent, vchiq_devid,NULL , +DEVICE_NAME ); +if (IS_ERR (vchiq_dev)) { +vchiq_log_error (vchiq_arm_log_level, +"Failed to create vchiq char device node" ); + ret =PTR_ERR (vchiq_dev); + goto device_create_error; + } + +vchiq_log_info (vchiq_arm_log_level, +"vchiq char dev initialised successfully - device %d.%d" , +MAJOR (vchiq_devid),MINOR (vchiq_devid)); + +return 0 ; + +device_create_error: +cdev_del (&vchiq_cdev); + +cdev_add_error: +unregister_chrdev_region (vchiq_devid,1 ); + +alloc_region_error: +class_destroy (vchiq_class); + +error_exit: +return ret; +} + +/** +* vchiq_deregister_chrdev - Deregister and cleanup the vchiq char +* driver and device files +*/ +static void vchiq_deregister_chrdev (void ) +{ +device_destroy (vchiq_class, vchiq_devid); +cdev_del (&vchiq_cdev); +unregister_chrdev_region (vchiq_devid,1 ); +class_destroy (vchiq_class); +} +/* * Autosuspend related functionality */
@@ -2740,7 +2815,6 @@ static int vchiq_probe(struct platform_device *pdev)struct device_node *fw_node;const struct of_device_id *of_id;struct vchiq_drvdata *drvdata; -struct device *vchiq_dev;int err; of_id =of_match_node (vchiq_of_match, pdev->dev.of_node);
@@ -2766,28 +2840,18 @@ static int vchiq_probe(struct platform_device *pdev)if (err) goto failed_platform_init; -cdev_init (&vchiq_cdev, &vchiq_fops); - vchiq_cdev.owner =THIS_MODULE ; - err =cdev_add (&vchiq_cdev, vchiq_devid,1 ); -if (err) { -vchiq_log_error (vchiq_arm_log_level, -"Unable to register device" ); - goto failed_platform_init; - } - - vchiq_dev =device_create (vchiq_class, &pdev->dev, vchiq_devid,NULL , -"vchiq" ); -if (IS_ERR (vchiq_dev)) { - err =PTR_ERR (vchiq_dev); - goto failed_device_create; - } -vchiq_debugfs_init ();vchiq_log_info (vchiq_arm_log_level, -"vchiq: initialised - version %d (min %d), device %d.%d" , -VCHIQ_VERSION ,VCHIQ_VERSION_MIN , -MAJOR (vchiq_devid),MINOR (vchiq_devid)); +"vchiq: platform initialised - version %d (min %d)" , +VCHIQ_VERSION ,VCHIQ_VERSION_MIN ); + +/* +* We don't handle error here since the function handles +* cleanup in cases of failure. Further, we can proceed +* even if this function fails. +*/ +vchiq_register_chrdev (&pdev->dev); vcsm_cma =vchiq_register_child (pdev,"vcsm-cma" ); bcm2835_codec =vchiq_register_child (pdev,"bcm2835-codec" );
@@ -2797,10 +2861,8 @@ static int vchiq_probe(struct platform_device *pdev)return 0 ; -failed_device_create: -cdev_del (&vchiq_cdev); failed_platform_init: -vchiq_log_warning (vchiq_arm_log_level,"could not load vchiq" ); +vchiq_log_warning (vchiq_arm_log_level,"could not initialize vchiq platform" );return err; }
@@ -2812,8 +2874,7 @@ static int vchiq_remove(struct platform_device *pdev)platform_device_unregister (bcm2835_codec);platform_device_unregister (vcsm_cma);vchiq_debugfs_deinit (); -device_destroy (vchiq_class, vchiq_devid); -cdev_del (&vchiq_cdev); +vchiq_deregister_chrdev ();return 0 ; }
@@ -2831,31 +2892,9 @@ static int __init vchiq_driver_init(void) {int ret; - vchiq_class =class_create (THIS_MODULE ,DEVICE_NAME ); -if (IS_ERR (vchiq_class)) { -pr_err ("Failed to create vchiq class\n" ); -return PTR_ERR (vchiq_class); - } - - ret =alloc_chrdev_region (&vchiq_devid,0 ,1 ,DEVICE_NAME ); -if (ret) { -pr_err ("Failed to allocate vchiq's chrdev region\n" ); - goto class_destroy; - } - ret =platform_driver_register (&vchiq_driver); -if (ret) { +if (ret)pr_err ("Failed to register vchiq driver\n" ); - goto region_unregister; - } - -return 0 ; - -region_unregister: -unregister_chrdev_region (vchiq_devid,1 ); - -class_destroy: -class_destroy (vchiq_class);return ret; }
@@ -2864,8 +2903,6 @@ module_init(vchiq_driver_init); static void __exit vchiq_driver_exit(void) {platform_driver_unregister (&vchiq_driver); -unregister_chrdev_region (vchiq_devid,1 ); -class_destroy (vchiq_class); }module_exit (vchiq_driver_exit);
Signature · 1 line
2.25.1
PATCH v2 2/5 staging: vchiq: Move certain declarations to vchiq_arm.h
In reply to Ojaswin MujooMove 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.
--- .../interface/vchiq_arm/vchiq_arm.c | 66 ++--------------- .../interface/vchiq_arm/vchiq_arm.h | 70 +++++++++++++++++++ 2 files changed, 75 insertions(+), 61 deletions(-)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +5 -61
@@ -38,12 +38,6 @@#undef MODULE_PARAM_PREFIX#define MODULE_PARAM_PREFIX DEVICE_NAME "." -/* Some per-instance constants */ -#define MAX_COMPLETIONS 128 -#define MAX_SERVICES 64 -#define MAX_ELEMENTS 8 -#define MSG_QUEUE_SIZE 128 -#define KEEPALIVE_VER 1#define KEEPALIVE_VER_MIN KEEPALIVE_VER
@@ -51,62 +45,12 @@int vchiq_arm_log_level =VCHIQ_LOG_DEFAULT ;int vchiq_susp_log_level =VCHIQ_LOG_ERROR ; -struct user_service { -struct vchiq_service *service; -void __user *userdata; -struct vchiq_instance *instance; -char is_vchi; -char dequeue_pending; -char close_pending; -int message_available_pos; -int msg_insert; -int msg_remove; -struct completion insert_event; -struct completion remove_event; -struct completion close_event; -struct vchiq_header *msg_queue[MSG_QUEUE_SIZE ]; -}; - -struct bulk_waiter_node { -struct bulk_waiter bulk_waiter; -int pid; -struct list_head list; -}; - -struct vchiq_instance { -struct vchiq_state *state; -struct vchiq_completion_data_kernel completions[MAX_COMPLETIONS ]; -int completion_insert; -int completion_remove; -struct completion insert_event; -struct completion remove_event; -struct mutex completion_mutex; - -int connected; -int closing; -int pid; -int mark; -int use_close_delivered; -int trace; - -struct list_head bulk_waiter_list; -struct mutex bulk_waiter_list_mutex; - -struct vchiq_debugfs_node debugfs_node; -}; - -struct dump_context { -char __user *buf; -size_t actual; -size_t space; -loff_t offset; -}; +DEFINE_SPINLOCK (msg_queue_spinlock); +struct vchiq_state g_state;static struct cdev vchiq_cdev;static dev_t vchiq_devid; -static struct vchiq_state g_state;static struct class *vchiq_class; -static DEFINE_SPINLOCK (msg_queue_spinlock);static struct platform_device *bcm2835_camera;static struct platform_device *bcm2835_audio;static struct platform_device *bcm2835_codec;
@@ -587,7 +531,7 @@ add_completion(struct vchiq_instance *instance, enum vchiq_reason reason,* ***************************************************************************/ -static enum vchiq_status +enum vchiq_status service_callback (enum vchiq_reason reason,struct vchiq_header *header,unsigned int handle,void *bulk_userdata) {
@@ -2215,7 +2159,7 @@ vchiq_fops = {* * Returns 0 on success else returns the error code. */ -static int vchiq_register_chrdev (struct device *parent) +int vchiq_register_chrdev (struct device *parent) {struct device *vchiq_dev;int ret;
@@ -2274,7 +2218,7 @@ static int vchiq_register_chrdev(struct device *parent)* vchiq_deregister_chrdev - Deregister and cleanup the vchiq char * driver and device files */ -static void vchiq_deregister_chrdev (void ) +void vchiq_deregister_chrdev (void ) {device_destroy (vchiq_class, vchiq_devid);cdev_del (&vchiq_cdev);
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h +70 -0
89 lines not on this page · raw message
Signature · 1 line
2.25.1
PATCH v2 3/5 staging: vchiq: Move vchiq char driver to its own file
In reply to Ojaswin MujooSplit 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.
--- drivers/staging/vc04_services/Makefile | 1 + .../interface/vchiq_arm/vchiq_arm.c | 1675 ++--------------- .../interface/vchiq_arm/vchiq_dev.c | 1488 +++++++++++++++ 3 files changed, 1598 insertions(+), 1566 deletions(-) create mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
drivers/staging/vc04_services/Makefile +1 -0
@@ -7,6 +7,7 @@ vchiq-objs := \ interface/vchiq_arm/vchiq_2835_arm.o \ interface/vchiq_arm/vchiq_debugfs.o \ interface/vchiq_arm/vchiq_connected.o \ + interface/vchiq_arm/vchiq_dev.o \ obj-$(CONFIG_SND_BCM2835) += bcm2835-audio/ obj-$(CONFIG_VIDEO_BCM2835) += bcm2835-camera/
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +109 -1566
1,740 lines not on this page · raw message
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c new file +1488 -0
1,253 lines not on this page · raw message
The rest of this diff is not shown. View the raw message.
Signature · 1 line
2.25.1
Part of the diff is not shown here. View the raw message.
PATCH v2 4/5 staging: vchiq: Make creation of vchiq cdev optional
In reply to Ojaswin MujooBefore this commit, vchiq cdev (/dev/vchiq) was always created during platform initialization. Introduce a new Kconfig option CONFIG_VCHIQ_CDEV which determines the cdev needs to be created or not. Also modify the predefined config files to have this option set to "yes".
--- arch/arm/configs/bcm2709_defconfig | 1 + arch/arm/configs/bcm2711_defconfig | 1 + arch/arm/configs/bcmrpi_defconfig | 1 + drivers/staging/vc04_services/Kconfig | 10 ++++++++++ drivers/staging/vc04_services/Makefile | 5 ++++- .../vc04_services/interface/vchiq_arm/vchiq_arm.h | 9 +++++++++ 6 files changed, 26 insertions(+), 1 deletion(-)
arch/arm/configs/bcm2709_defconfig +1 -0
@@ -1314,6 +1314,7 @@ CONFIG_FB_TFT_UC1701=m CONFIG_FB_TFT_UPD161704=m CONFIG_FB_TFT_WATTEROTT=m CONFIG_BCM2835_VCHIQ=y +CONFIG_VCHIQ_CDEV=y CONFIG_SND_BCM2835=m CONFIG_VIDEO_BCM2835=m CONFIG_VIDEO_CODEC_BCM2835=m
arch/arm/configs/bcm2711_defconfig +1 -0
@@ -1352,6 +1352,7 @@ CONFIG_FB_TFT_UC1701=m CONFIG_FB_TFT_UPD161704=m CONFIG_FB_TFT_WATTEROTT=m CONFIG_BCM2835_VCHIQ=y +CONFIG_VCHIQ_CDEV=y CONFIG_SND_BCM2835=m CONFIG_VIDEO_BCM2835=m CONFIG_VIDEO_CODEC_BCM2835=m
arch/arm/configs/bcmrpi_defconfig +1 -0
@@ -1325,6 +1325,7 @@ CONFIG_FB_TFT_UC1701=m CONFIG_FB_TFT_UPD161704=m CONFIG_FB_TFT_WATTEROTT=m CONFIG_BCM2835_VCHIQ=y +CONFIG_VCHIQ_CDEV=y CONFIG_SND_BCM2835=m CONFIG_VIDEO_BCM2835=m CONFIG_VIDEO_CODEC_BCM2835=m
drivers/staging/vc04_services/Kconfig +10 -0
@@ -19,6 +19,16 @@ config BCM2835_VCHIQ Defaults to Y when the Broadcom Videocore services are included in the build, N otherwise. +if BCM2835_VCHIQ + +config VCHIQ_CDEV + bool "VCHIQ Character Driver" + help + Enable the creation of VCHIQ character driver to help + communicate with the Videocore platform. + +endif + source "drivers/staging/vc04_services/bcm2835-audio/Kconfig" source "drivers/staging/vc04_services/bcm2835-camera/Kconfig"
drivers/staging/vc04_services/Makefile +4 -1
@@ -7,7 +7,10 @@ vchiq-objs := \ interface/vchiq_arm/vchiq_2835_arm.o \ interface/vchiq_arm/vchiq_debugfs.o \ interface/vchiq_arm/vchiq_connected.o \ - interface/vchiq_arm/vchiq_dev.o \ + +ifdef CONFIG_VCHIQ_CDEV +vchiq-objs += interface/vchiq_arm/vchiq_dev.o +endif obj-$(CONFIG_SND_BCM2835) += bcm2835-audio/ obj-$(CONFIG_VIDEO_BCM2835) += bcm2835-camera/
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h +9 -0
@@ -172,12 +172,21 @@ vchiq_instance_get_trace(struct vchiq_instance *instance);extern void vchiq_instance_set_trace (struct vchiq_instance *instance,int trace); +#if IS_ENABLED (CONFIG_VCHIQ_CDEV ) +extern void vchiq_deregister_chrdev (void );extern int vchiq_register_chrdev (struct device *parent); +#else + +static inline void vchiq_deregister_chrdev (void ) { } +static inline int vchiq_register_chrdev (struct device *parent) {return 0 ; } + +#endif /* IS_ENABLED(CONFIG_VCHIQ_CDEV) */ +extern enum vchiq_status service_callback (enum vchiq_reason reason,struct vchiq_header *header,unsigned int handle,void *bulk_userdata);
Signature · 1 line
2.25.1
PATCH v2 5/5 staging: vchiq: Combine vchiq platform code into single file
In reply to Ojaswin MujooCombine the vchiq platform initialization code into a single file. The file vchiq_2835_arm.c is now merged in vchiq_arm.c
--- drivers/staging/vc04_services/Makefile | 1 - .../interface/vchiq_arm/vchiq_2835_arm.c | 651 ------------------ .../interface/vchiq_arm/vchiq_arm.c | 635 +++++++++++++++++ 3 files changed, 635 insertions(+), 652 deletions(-) delete mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
drivers/staging/vc04_services/Makefile +0 -1
@@ -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_2835_arm.c deleted +0 -651
651 lines not on this page · raw message
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +635 -0
660 lines not on this page · raw message
Signature · 1 line
2.25.1
Hi Ojaswin,
Am 20.06.21 um 14:55 schrieb Ojaswin Mujoo:
Quoted 34 lines
Hello,
This patchset adderesses the TODO item number 10 specified at:
drivers/staging/vc04-services/interface/TODOFor 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/vchiqcreated. 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.As Stefan suggested in the last revision, I have split the commits into more finer parts for ease of readability and maintainability. I have also added 2 more patches to define a KConfig entry for vchiq cdev, and to merge the code in
vchiq_2835_arm.ctovchiq_arm.cA summary of the patches is now as follows:
- Patch 1: Move cdev init code into a function
- Patch 2: Shift some devlarations fromvchiq_arm.ctovchiq_arm.hfor sharing
- Patch 3: Move vchiq cdev init code fromvchiq_arm.cintovchiq_dev.c
- Patch 4: Decouple cdev code by defining aKconfigentry to allow optional compilation of it.
- Patch 5: Merge code invchiq_2835_arm.ctovchiq_arm.c
Quoted 6 lines
(More details can be found in the commit messages)
NOTE: This patchset is built against the raspberry pi mainline kernel at
https://github.com/raspberrypi/linux/blob/rpi-5.10.y, and has been tested on Raspberry Pi 3B+
please don't do this, because it's a waste of time. Greg can only apply patches against the mainline kernel and the patches must be tested with the mainline kernel. Additionally i've sent a lot of patches recently which are not applied against the vendor tree.
Quoted 9 lines
At this point, I have some questions and ideas and would like to hear your thoughts and suggestions on them:
1. So as mentioned, I have built this against the raspberry pi kernel, since I was not able to figure out a way to build the vanilla mainline kernel for Raspberry Pi. However, I understand that since this will be applied to the mainline, I need to make sure it is consistent with it.
Can you please describe the issue in detail?
Or try this older guide [1]
Best regards
Stefan Wahren
[1] - https://gist.github.com/lategoodbye/c7317a42bf7f9c07f5a91baed8c68f75
Quoted 47 lines
Hence to confirm that, I tried to "git am" this patchset to the mainline kernel but there are some merge conflicts in doing so. I have an idea how to resolve most of them except the following:
- The mainline
vchiq_arm.cdiffers from the one in rapberry pi mainline which caused conflict in Patch 3.I'm not sure which
vchiq_arm.cto treat as the base for my patches. The one in mainline? or the one in raspberry pi's git tree?2. 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_SYMBOLto export all the functions (and accessor functions for variables likeg_state) that would be required for cdev init. Majority of these would be exported fromvchiq_arm.candvchiq_core.c, and will then be used in vchiq-dev.ko. Is this the right way to approach this?Thank you in advance for looking into this and best regards! Ojaswin
Ojaswin Mujoo (5):
staging: vchiq: Refactor vchiq cdev code
staging: vchiq: Move certain declarations tovchiq_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 filearch/arm/configs/bcm2709_defconfig | 1 + arch/arm/configs/bcm2711_defconfig | 1 + arch/arm/configs/bcmrpi_defconfig | 1 + drivers/staging/vc04_services/Kconfig | 10 + drivers/staging/vc04_services/Makefile | 5 +- .../interface/vchiq_arm/vchiq_2835_arm.c | 651 ----- .../interface/vchiq_arm/vchiq_arm.c | 2477 ++++++----------- .../interface/vchiq_arm/vchiq_arm.h | 79 + .../interface/vchiq_arm/vchiq_dev.c | 1488 ++++++++++ 9 files changed, 2402 insertions(+), 2311 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
Re: [PATCH v2 1/5] staging: vchiq: Refactor vchiq cdev code
In reply to Ojaswin MujooQuoted 5 lines · Ojaswin Mujoo
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +0
-2vchiq_debugfs_init (); vchiq_log_info(vchiq_arm_log_level, - "vchiq: initialised - version %d (min %d), device %d.%d", - VCHIQ_VERSION, VCHIQ_VERSION_MIN,
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c static int vchiq_probe(struct platform_device *pdev) · line 2854
Quoted 10 lines
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +9
-1- MAJOR(vchiq_devid), MINOR(vchiq_devid)); +"vchiq: platform initialised - version %d (min %d)" , + VCHIQ_VERSION, VCHIQ_VERSION_MIN); + +/* +* We don't handle error here since the function handles +* cleanup in cases of failure. Further, we can proceed +* even if this function fails. +*/ +vchiq_register_chrdev (&pdev->dev);
I feel like ignoring errors and just continuing seems helpful, but it's actually doing the users a disservice. If it's an error during, boot that's different, in that case it's better to get some kind of minimally useful boot so the user can debug the problem. But if the error isn't going to prevent the system from booting then it's better to just return an error so they can fix the problem and try again.
regards,
dan carpenter
Re: [PATCH v2 3/5] staging: vchiq: Move vchiq char driver to its own file
In reply to Ojaswin Mujoodrivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c line 148
Quoted 8 lines · Ojaswin Mujoo
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c new file +8
-0+/* read a user pointer value from an array pointers in user space */ +static inline int vchiq_get_user_ptr (void __user **buf,void __user *ubuf,int index) +{ +int ret; + +if (in_compat_syscall ()) { +compat_uptr_t ptr32; +
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
When I'm reviewing these sorts of patches then what I'm trying to verify is that you are not a UMN "researcher" trying to change the code without anyone noticing. In the orignal code, there was no blank line here
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c line 149
Quoted 1 line
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c new file +1
-0+compat_uptr_t __user *uptr = ubuf;
but there was a blank line here.
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c line 152
Quoted 3 lines
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c new file +3
-0+ ret =get_user (ptr32, uptr + index); + *buf =compat_ptr (ptr32); + }else {
These sorts of minor white space changes make it hard to verify the code in an automated way.
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c line 161
Quoted 9 lines
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c new file +9
-0+uintptr_t ptr,__user *uptr = ubuf; + + ret =get_user (ptr, uptr + index); + *buf = (void __user *)ptr; + } + +return ret; +} +
regards,
dan carpenter
On Sun, Jun 20, 2021 at 03:28:43PM +0200, Stefan Wahren wrote:
Hello Stefan!
Quoted 40 lines
Hi Ojaswin,
Am 20.06.21 um 14:55 schrieb Ojaswin Mujoo:
Quoted 37 lines
Hello,
This patchset adderesses the TODO item number 10 specified at:
drivers/staging/vc04-services/interface/TODOFor 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/vchiqcreated. 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.As Stefan suggested in the last revision, I have split the commits into more finer parts for ease of readability and maintainability. I have also added 2 more patches to define a KConfig entry for vchiq cdev, and to merge the code in
vchiq_2835_arm.ctovchiq_arm.cA summary of the patches is now as follows:
- Patch 1: Move cdev init code into a function
- Patch 2: Shift some devlarations fromvchiq_arm.ctovchiq_arm.hfor sharing
- Patch 3: Move vchiq cdev init code fromvchiq_arm.cintovchiq_dev.c
- Patch 4: Decouple cdev code by defining aKconfigentry to allow optional compilation of it.
- Patch 5: Merge code invchiq_2835_arm.ctovchiq_arm.c(More details can be found in the commit messages)
Quoted 7 lines
Quoted 3 lines
NOTE: This patchset is built against the raspberry pi mainline kernel at
https://github.com/raspberrypi/linux/blob/rpi-5.10.y, and has been tested on Raspberry Pi 3B+please don't do this, because it's a waste of time. Greg can only apply patches against the mainline kernel and the patches must be tested with the mainline kernel. Additionally i've sent a lot of patches recently which are not applied against the vendor tree.
Understood Stefan, I'll keep that in mind going forward.
Quoted 10 lines
Quoted 9 lines
At this point, I have some questions and ideas and would like to hear your thoughts and suggestions on them:
1. So as mentioned, I have built this against the raspberry pi kernel, since I was not able to figure out a way to build the vanilla mainline kernel for Raspberry Pi. However, I understand that since this will be applied to the mainline, I need to make sure it is consistent with it.
Quoted 3 lines
Can you please describe the issue in detail?
Or try this older guide [1]
Thanks for the guide, I tried this out (along with some other things) but I'm not able to get the Pi to boot. I'm using a headless setup with Wifi+SSH, due to lack of hardware, and I'm not able to SSH/ping when using mainline. This works correctly with the downstream kernel + Raspbian. (Maybe because of missing drivers and/or config options).
I'm still looking into this but a I'm a bit in the dark till I can get my hands on a UART to USB cable although they are difficult to get around here. I'm also trying to maybe figure out if I can somehow use my Arduino to capture the serial output.
May I ask what setup you are using for kernel development on pi?
Quoted 6 lines
Best regards
Stefan Wahren[1] - https://gist.github.com/lategoodbye/c7317a42bf7f9c07f5a91baed8c68f75
Thank you again for the help!
Ojaswin
Quoted 48 lines
Quoted 47 lines
Hence to confirm that, I tried to "git am" this patchset to the mainline kernel but there are some merge conflicts in doing so. I have an idea how to resolve most of them except the following:
- The mainline
vchiq_arm.cdiffers from the one in rapberry pi mainline which caused conflict in Patch 3.I'm not sure which
vchiq_arm.cto treat as the base for my patches. The one in mainline? or the one in raspberry pi's git tree?2. 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_SYMBOLto export all the functions (and accessor functions for variables likeg_state) that would be required for cdev init. Majority of these would be exported fromvchiq_arm.candvchiq_core.c, and will then be used in vchiq-dev.ko. Is this the right way to approach this?Thank you in advance for looking into this and best regards! Ojaswin
Ojaswin Mujoo (5):
staging: vchiq: Refactor vchiq cdev code
staging: vchiq: Move certain declarations tovchiq_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 filearch/arm/configs/bcm2709_defconfig | 1 + arch/arm/configs/bcm2711_defconfig | 1 + arch/arm/configs/bcmrpi_defconfig | 1 + drivers/staging/vc04_services/Kconfig | 10 + drivers/staging/vc04_services/Makefile | 5 +- .../interface/vchiq_arm/vchiq_2835_arm.c | 651 ----- .../interface/vchiq_arm/vchiq_arm.c | 2477 ++++++----------- .../interface/vchiq_arm/vchiq_arm.h | 79 + .../interface/vchiq_arm/vchiq_dev.c | 1488 ++++++++++ 9 files changed, 2402 insertions(+), 2311 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
Re: [PATCH v2 1/5] staging: vchiq: Refactor vchiq cdev code
In reply to Dan CarpenterHello Dan,
Quoted 15 lines · Dan Carpenter
Quoted 14 lines · Ojaswin Mujoo
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +8
-3vchiq_debugfs_init ();vchiq_log_info (vchiq_arm_log_level, -"vchiq: initialised - version %d (min %d), device %d.%d" , -VCHIQ_VERSION ,VCHIQ_VERSION_MIN , -MAJOR (vchiq_devid),MINOR (vchiq_devid)); +"vchiq: platform initialised - version %d (min %d)" , +VCHIQ_VERSION ,VCHIQ_VERSION_MIN ); + +/* +* We don't handle error here since the function handles +* cleanup in cases of failure. Further, we can proceed +* even if this function fails. +*/
Quoted 8 lines
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c static int vchiq_probe(struct platform_device *pdev) · line 2854
Quoted 1 line
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +1
-0+vchiq_register_chrdev (&pdev->dev);I feel like ignoring errors and just continuing seems helpful, but it's actually doing the users a disservice. If it's an error during, boot that's different, in that case it's better to get some kind of minimally useful boot so the user can debug the problem. But if the error isn't going to prevent the system from booting then it's better to just return an error so they can fix the problem and try again.
Got it, I'll fix this in the next revision.
Thank you,
Ojaswin
Quoted 4 lines
regards,
dan carpenter
Re: [PATCH v2 3/5] staging: vchiq: Move vchiq char driver to its own file
In reply to Dan CarpenterQuoted 18 lines · Dan Carpenter
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c line 148
Quoted 8 lines · Ojaswin Mujoo
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c new file +8
-0+/* read a user pointer value from an array pointers in user space */ +static inline int vchiq_get_user_ptr (void __user **buf,void __user *ubuf,int index) +{ +int ret; + +if (in_compat_syscall ()) { +compat_uptr_t ptr32; +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
When I'm reviewing these sorts of patches then what I'm trying to verify is that you are not a UMN "researcher" trying to change the code without anyone noticing. In the orignal code, there was no blank line heredrivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c line 149
Quoted 1 line
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c new file +1
-0+compat_uptr_t __user *uptr = ubuf;but there was a blank line here.
Quoted 7 lines
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c line 152
Quoted 3 lines
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c new file +3
-0+ ret =get_user (ptr32, uptr + index); + *buf =compat_ptr (ptr32); + }else {These sorts of minor white space changes make it hard to verify the code in an automated way.
Ohh I see, I get the point. I'm not sure how this creeped in here. Especially with this patch, the git diff looks very ugly and confusing to me. I'm not sure if that is because I unknownigly introduced some whitespace changes or if thats normal when shifting a lof of code around. Anyways, I'll fix this in the next revision.
PS: I was unable to get mainline kernel working on my Pi and instead submitted this patch which is built against the raspberry downstream kernel. I now know this definitely not the right thing to do and am trying to recreate this patchset with mainline. Hopefully I'll be able to iron out all such anomalies in the next revision
Quoted 4 lines
Quoted 4 lines
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c new file +4
-0+uintptr_t ptr,__user *uptr = ubuf; + + ret =get_user (ptr, uptr + index); + *buf = (void __user *)ptr;
Quoted 9 lines
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c line 161
Quoted 5 lines
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c new file +5
-0+ } + +return ret; +} +regards,
dan carpenter
Thank you for the help!
Ojaswin