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

[Ojaswin Mujoo <ojaswin98@gmail.com>]

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