staging: vchiq: convert to use a miscdevice

Using a struct class, a cdev, and another device just for a single minor device is total overkill. Just use a dynamic misc device instead, saving lots of logic and memory.

Cc Nicolas Saenz Julienne <[email protected]>

Cc Stefan Wahren <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Dan Carpenter <[email protected]>

Cc Ojaswin Mujoo <[email protected]>

Cc Phil Elwell <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 .../interface/vchiq_arm/vchiq_dev.c           | 71 +++----------------
 1 file changed, 11 insertions(+), 60 deletions(-)

drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c +11 -60

index bf1a88c9d1ee..788fa5a987a3 100644
@@ -9,18 +9,13 @@
 #include <linux/device.h>
 #include <linux/slab.h>
 #include <linux/compat.h>
+#include <linux/miscdevice.h>
 
 #include "vchiq_core.h"
 #include "vchiq_ioctl.h"
 #include "vchiq_arm.h"
 #include "vchiq_debugfs.h"
 
-#define DEVICE_NAME "vchiq"
-
-static struct cdev    vchiq_cdev;
-static dev_t          vchiq_devid;
-static struct class  *vchiq_class;
-
 static const char *const ioctl_names[] = {
 	"CONNECT",
 	"SHUTDOWN",

@@ -1364,6 +1359,13 @@ vchiq_fops = {
 	.read = vchiq_read
 };
 
+static struct miscdevice vchiq_miscdev = {
+	.fops = &vchiq_fops,
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "vchiq",
+
+};
+
 /**
  *	vchiq_register_chrdev - Register the char driver for vchiq
  *				and create the necessary class and

@@ -1374,57 +1376,9 @@ vchiq_fops = {
  */
 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));
+	vchiq_miscdev.parent = parent;
 
-	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;
+	return misc_register(&vchiq_miscdev);
 }
 
 /**

@@ -1433,8 +1387,5 @@ int vchiq_register_chrdev(struct device *parent)
  */
 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);
+	misc_deregister(&vchiq_miscdev);
 }

Signature · 1 line

2.33.0

Quoted 3 lines · Greg Kroah-Hartman

Using a struct class, a cdev, and another device just for a single minor device is total overkill. Just use a dynamic misc device instead, saving lots of logic and memory.

Hello Greg,

I got some time to test this out at my end. This seems to work correctly however there's a small change in permissions applied to /dev/vchiq that is causing tests to break.

* Permissions before the patch *

$ ls -l /dev/vchiq 
crw-rw---- 1 root video 235, 0 May  7 17:33 vchiq

* Permissions after the patch *

$ ls -l /dev/vchiq 
crw------- 1 root root 10, 125 May  7 17:30 vchiq

As seen above, after the patch, the cdev is only accessible by root user, which is causing the tests ($ vchiq_test -f 10) to fail when run as non-root.

I believe assigning the permission and "video" group to /dev/vchiq is handled by udev, in the downstream pi OS, as seen in this line in /lib/udev/rules.d/10-local-rpi.rules file:

SUBSYSTEM=="vchiq", GROUP="video", MODE="0660"

I'm not completely sure how the SUBSYSTEM part is passed to udev from the kernel modules, however seems like the miscdevice is not notifying udev correctly (?).

Quoted 15 lines · Ojaswin Mujoo

Quoted 3 lines · Greg Kroah-Hartman

Using a struct class, a cdev, and another device just for a single minor device is total overkill. Just use a dynamic misc device instead, saving lots of logic and memory.

Hello Greg,

I got some time to test this out at my end. This seems to work correctly however there's a small change in permissions applied to /dev/vchiq that is causing tests to break.

* Permissions before the patch *

$ ls -l /dev/vchiq 
crw-rw---- 1 root video 235, 0 May  7 17:33 vchiq

Quoted 7 lines

* Permissions after the patch *

$ ls -l /dev/vchiq 
crw------- 1 root root 10, 125 May  7 17:30 vchiq

As seen above, after the patch, the cdev is only accessible by root user, which is causing the tests ($ vchiq_test -f 10) to fail when run as non-root.

Ah, that's not under the kernel's control, but as you point out, it's a udev issue.

Quoted 9 lines

I believe assigning the permission and "video" group to /dev/vchiq is handled by udev, in the downstream pi OS, as seen in this line in /lib/udev/rules.d/10-local-rpi.rules file:

SUBSYSTEM=="vchiq", GROUP="video", MODE="0660"

I'm not completely sure how the SUBSYSTEM part is passed to udev from the kernel modules, however seems like the miscdevice is not notifying udev correctly (?).

No, the SUBSYSTEM for this device has changed from "vchiq" to "misc".

Having a whole subsystem for just one character device is crazy, which is why I did the kernel change.

Try changing the line in the udev file to:

	NAME=="vchiq", GROUP="video", MODE="0660"

(SUBSYSTEM changes to NAME) and see if that works both on the newer kernel, and on the older ones as
well.

thanks,

greg k-h

Quoted 42 lines · Greg Kroah-Hartman

Quoted 15 lines · Ojaswin Mujoo

Quoted 3 lines · Greg Kroah-Hartman

Using a struct class, a cdev, and another device just for a single minor device is total overkill. Just use a dynamic misc device instead, saving lots of logic and memory.

Hello Greg,

I got some time to test this out at my end. This seems to work correctly however there's a small change in permissions applied to /dev/vchiq that is causing tests to break.

* Permissions before the patch *

$ ls -l /dev/vchiq 
crw-rw---- 1 root video 235, 0 May  7 17:33 vchiq

Quoted 7 lines

* Permissions after the patch *

$ ls -l /dev/vchiq 
crw------- 1 root root 10, 125 May  7 17:30 vchiq

As seen above, after the patch, the cdev is only accessible by root user, which is causing the tests ($ vchiq_test -f 10) to fail when run as non-root.

Ah, that's not under the kernel's control, but as you point out, it's a udev issue.

Quoted 9 lines

I believe assigning the permission and "video" group to /dev/vchiq is handled by udev, in the downstream pi OS, as seen in this line in /lib/udev/rules.d/10-local-rpi.rules file:

SUBSYSTEM=="vchiq", GROUP="video", MODE="0660"

I'm not completely sure how the SUBSYSTEM part is passed to udev from the kernel modules, however seems like the miscdevice is not notifying udev correctly (?).

No, the SUBSYSTEM for this device has changed from "vchiq" to "misc".

Having a whole subsystem for just one character device is crazy, which is why I did the kernel change.

Quoted 7 lines

Try changing the line in the udev file to:

	NAME=="vchiq", GROUP="video", MODE="0660"

(SUBSYSTEM changes to NAME) and see if that works both on the newer kernel, and on the older ones as
well.

Hello, thanks for the explanation and pointers.

The "NAME==vchiq" change doesn't seem to work but I was able to get it correctly working by using "KERNEL=vchiq" instead:

   KERNEL=="vchiq", GROUP="video", MODE="0660"

I tested this with and without this patch and it works as expected.

Quoted 57 lines · Ojaswin Mujoo

Quoted 42 lines · Greg Kroah-Hartman

Quoted 15 lines · Ojaswin Mujoo

Quoted 3 lines · Greg Kroah-Hartman

Using a struct class, a cdev, and another device just for a single minor device is total overkill. Just use a dynamic misc device instead, saving lots of logic and memory.

Hello Greg,

I got some time to test this out at my end. This seems to work correctly however there's a small change in permissions applied to /dev/vchiq that is causing tests to break.

* Permissions before the patch *

$ ls -l /dev/vchiq 
crw-rw---- 1 root video 235, 0 May  7 17:33 vchiq

Quoted 7 lines

* Permissions after the patch *

$ ls -l /dev/vchiq 
crw------- 1 root root 10, 125 May  7 17:30 vchiq

As seen above, after the patch, the cdev is only accessible by root user, which is causing the tests ($ vchiq_test -f 10) to fail when run as non-root.

Ah, that's not under the kernel's control, but as you point out, it's a udev issue.

Quoted 9 lines

I believe assigning the permission and "video" group to /dev/vchiq is handled by udev, in the downstream pi OS, as seen in this line in /lib/udev/rules.d/10-local-rpi.rules file:

SUBSYSTEM=="vchiq", GROUP="video", MODE="0660"

I'm not completely sure how the SUBSYSTEM part is passed to udev from the kernel modules, however seems like the miscdevice is not notifying udev correctly (?).

No, the SUBSYSTEM for this device has changed from "vchiq" to "misc".

Having a whole subsystem for just one character device is crazy, which is why I did the kernel change.

Quoted 7 lines

Try changing the line in the udev file to:

	NAME=="vchiq", GROUP="video", MODE="0660"

(SUBSYSTEM changes to NAME) and see if that works both on the newer kernel, and on the older ones as
well.

Hello, thanks for the explanation and pointers.

The "NAME==vchiq" change doesn't seem to work but I was able to get it correctly working by using "KERNEL=vchiq" instead:

   KERNEL=="vchiq", GROUP="video", MODE="0660"

Quoted 1 line

I tested this with and without this patch and it works as expected.

Ah, yes, you are right, I was just guessing, I should have read the udev documentation :)

Any chance you can send a patch for this to whatever package that file comes from?

thanks,

greg k-h