[PATCH] staging: vchiq: convert to use a miscdevice

[PATCH] staging: vchiq: convert to use a miscdevice

[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.

Cc: Nicolas Saenz Julienne <nsaenz@kernel.org>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Ojaswin Mujoo <ojaswin98@gmail.com>
Cc: Phil Elwell <phil@raspberrypi.com>
Cc: bcm-kernel-feedback-list@broadcom.com
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 .../interface/vchiq_arm/vchiq_dev.c           | 71 +++----------------
 1 file changed, 11 insertions(+), 60 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
index bf1a88c9d1ee..788fa5a987a3 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -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);
 }
-- 
2.33.0

[PATCH] staging: vchiq: convert to use a miscdevice

[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.

Cc: Nicolas Saenz Julienne <nsaenz@kernel.org>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Ojaswin Mujoo <ojaswin98@gmail.com>
Cc: Phil Elwell <phil@raspberrypi.com>
Cc: bcm-kernel-feedback-list@broadcom.com
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 .../interface/vchiq_arm/vchiq_dev.c           | 71 +++----------------
 1 file changed, 11 insertions(+), 60 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
index bf1a88c9d1ee..788fa5a987a3 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -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);
 }
-- 
2.33.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: vchiq: convert to use a miscdevice

[Ojaswin Mujoo]

On Tue, Sep 07, 2021 at 01:50:45PM +0200, Greg Kroah-Hartman wrote:
> 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 (?).

I'm still looking into this but thought I'd point this out so someone
more experienced can check and see if this could be an issue. 

Regards, Ojaswin

> 
> Cc: Nicolas Saenz Julienne <nsaenz@kernel.org>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Ojaswin Mujoo <ojaswin98@gmail.com>
> Cc: Phil Elwell <phil@raspberrypi.com>
> Cc: bcm-kernel-feedback-list@broadcom.com
> Cc: linux-rpi-kernel@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  .../interface/vchiq_arm/vchiq_dev.c           | 71 +++----------------
>  1 file changed, 11 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> index bf1a88c9d1ee..788fa5a987a3 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> @@ -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);
>  }
> -- 
> 2.33.0
> 

Re: [PATCH] staging: vchiq: convert to use a miscdevice

[Ojaswin Mujoo]

On Tue, Sep 07, 2021 at 01:50:45PM +0200, Greg Kroah-Hartman wrote:
> 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 (?).

I'm still looking into this but thought I'd point this out so someone
more experienced can check and see if this could be an issue. 

Regards, Ojaswin

> 
> Cc: Nicolas Saenz Julienne <nsaenz@kernel.org>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Ojaswin Mujoo <ojaswin98@gmail.com>
> Cc: Phil Elwell <phil@raspberrypi.com>
> Cc: bcm-kernel-feedback-list@broadcom.com
> Cc: linux-rpi-kernel@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  .../interface/vchiq_arm/vchiq_dev.c           | 71 +++----------------
>  1 file changed, 11 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> index bf1a88c9d1ee..788fa5a987a3 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> @@ -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);
>  }
> -- 
> 2.33.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: vchiq: convert to use a miscdevice

[Greg Kroah-Hartman]

On Fri, Sep 10, 2021 at 05:10:04PM +0530, Ojaswin Mujoo wrote:
> On Tue, Sep 07, 2021 at 01:50:45PM +0200, Greg Kroah-Hartman wrote:
> > 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.

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

> 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

Re: [PATCH] staging: vchiq: convert to use a miscdevice

[Greg Kroah-Hartman]

On Fri, Sep 10, 2021 at 05:10:04PM +0530, Ojaswin Mujoo wrote:
> On Tue, Sep 07, 2021 at 01:50:45PM +0200, Greg Kroah-Hartman wrote:
> > 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.

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

> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: vchiq: convert to use a miscdevice

[Ojaswin Mujoo]

On Fri, Sep 10, 2021 at 02:05:19PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 10, 2021 at 05:10:04PM +0530, Ojaswin Mujoo wrote:
> > On Tue, Sep 07, 2021 at 01:50:45PM +0200, Greg Kroah-Hartman wrote:
> > > 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.
> 
> Ah, that's not under the kernel's control, but as you point out, it's a
> udev issue.
> 
> > 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.
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. 

Regards,
Ojaswin
> 
> thanks,
> 
> greg k-h

Re: [PATCH] staging: vchiq: convert to use a miscdevice

[Ojaswin Mujoo]

On Fri, Sep 10, 2021 at 02:05:19PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 10, 2021 at 05:10:04PM +0530, Ojaswin Mujoo wrote:
> > On Tue, Sep 07, 2021 at 01:50:45PM +0200, Greg Kroah-Hartman wrote:
> > > 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.
> 
> Ah, that's not under the kernel's control, but as you point out, it's a
> udev issue.
> 
> > 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.
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. 

Regards,
Ojaswin
> 
> thanks,
> 
> greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: vchiq: convert to use a miscdevice

[Greg Kroah-Hartman]

On Sat, Sep 11, 2021 at 04:59:11PM +0530, Ojaswin Mujoo wrote:
> On Fri, Sep 10, 2021 at 02:05:19PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Sep 10, 2021 at 05:10:04PM +0530, Ojaswin Mujoo wrote:
> > > On Tue, Sep 07, 2021 at 01:50:45PM +0200, Greg Kroah-Hartman wrote:
> > > > 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.
> > 
> > Ah, that's not under the kernel's control, but as you point out, it's a
> > udev issue.
> > 
> > > 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.
> 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. 

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

Re: [PATCH] staging: vchiq: convert to use a miscdevice

[Greg Kroah-Hartman]

On Sat, Sep 11, 2021 at 04:59:11PM +0530, Ojaswin Mujoo wrote:
> On Fri, Sep 10, 2021 at 02:05:19PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Sep 10, 2021 at 05:10:04PM +0530, Ojaswin Mujoo wrote:
> > > On Tue, Sep 07, 2021 at 01:50:45PM +0200, Greg Kroah-Hartman wrote:
> > > > 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.
> > 
> > Ah, that's not under the kernel's control, but as you point out, it's a
> > udev issue.
> > 
> > > 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.
> 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. 

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: vchiq: convert to use a miscdevice

[Ojaswin Mujoo]

On Sat, Sep 11, 2021 at 01:56:08PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Sep 11, 2021 at 04:59:11PM +0530, Ojaswin Mujoo wrote:
> > On Fri, Sep 10, 2021 at 02:05:19PM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Sep 10, 2021 at 05:10:04PM +0530, Ojaswin Mujoo wrote:
> > > > On Tue, Sep 07, 2021 at 01:50:45PM +0200, Greg Kroah-Hartman wrote:
> > > > > 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.
> > > 
> > > Ah, that's not under the kernel's control, but as you point out, it's a
> > > udev issue.
> > > 
> > > > 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.
> > 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. 
> 
> 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?

Sure Greg, I can do that.

Thanks,
ojaswin
> 
> thanks,
> 
> greg k-h

Re: [PATCH] staging: vchiq: convert to use a miscdevice

[Ojaswin Mujoo]

On Sat, Sep 11, 2021 at 01:56:08PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Sep 11, 2021 at 04:59:11PM +0530, Ojaswin Mujoo wrote:
> > On Fri, Sep 10, 2021 at 02:05:19PM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Sep 10, 2021 at 05:10:04PM +0530, Ojaswin Mujoo wrote:
> > > > On Tue, Sep 07, 2021 at 01:50:45PM +0200, Greg Kroah-Hartman wrote:
> > > > > 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.
> > > 
> > > Ah, that's not under the kernel's control, but as you point out, it's a
> > > udev issue.
> > > 
> > > > 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.
> > 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. 
> 
> 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?

Sure Greg, I can do that.

Thanks,
ojaswin
> 
> thanks,
> 
> greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel