epics-modules / mrfioc2

EPICS driver for Micro Research Finland event timing system devices
http://epics-modules.github.io/mrfioc2/
Other
8 stars 30 forks source link

Install kernel driver as kernel version independent package #45

Closed Araneidae closed 3 years ago

Araneidae commented 3 years ago

This pull request adds support for building an RPM package for installing the kernel driver as a kernel version independent package using DKMS.

mdavidsaver commented 3 years ago

This looks useful. A README would be helpful to give others a starting point for how to make use of these changes. Doesn't need to be exhaustive, but at least an example invocation.

The only part which gives me pause is the udev rule SUBSYSTEM=="uio", MODE="0666". As default permissions go, this is a bit promiscuous. At minimum it needs to be restricted to this kernel module (cf. mrmShared/linux/README). Does Fedora/RHEL have any groups which might be suitable? eg. Debian has plugdev (for removable disks) which I might "abuse".

KERNEL=="uio*", ATTR{name}=="mrf-pci", GROUP="plugdev", MODE="0660"

Alternately, omit this file and leave permissions to a sysadmin.

Araneidae commented 3 years ago

I also have reservations about the promiscuous permissions. We do have a suitable group, but it's not a standard one: dcs (for Diamond ControlS). I could make the group configurable ... but then where am I going to put the configuration file to read? We don't have a top level CONFIG file for this kind of setting.

Didn't know about the ATTR{name} attribute (been a while since I wrote udev rules), that's definitely an improvement, will do that and repush. Will probably add the configurable group name as well.

Where should the README go? The instruction is:

  1. Run make in the dkms directory
  2. Install the RPM on your target system

I'd love build recipes for other targets (DEB in particular), but I don't know how to build those.

mdavidsaver commented 3 years ago

ATTR{name} may be old. These matches vary with kernel and udev versions. This is part of the reason I've only provided an example in mrmShared/linux/README so far. FYI. The recipe I use when formulating udev rules:

udevdump () 
{ 
    [ -z "$1" -o ! -e "$1" ] && echo "udevdump </dev/somefile>" && return 0;
    udevadm info -a -p $(udevadm info -q path -n "$1") --attribute-walk
}

Use like:

$ udevdump /dev/video0

Will print out all the possible matches for this device file.

Araneidae commented 3 years ago

Well, the first stanza of what I get is:

looking at device '/devices/pci0000:00/0000:00:01.1/0000:02:00.0/0000:03:08.0/0000:08:00.0/uio/uio0':
    KERNEL=="uio0"
    SUBSYSTEM=="uio"
    DRIVER==""
    ATTR{name}=="mrf-pci"
    ATTR{event}=="0"
    ATTR{version}==""

so think I'll go for SUBSYSTEM=="uio", ATTR{name}=="mrf-pci".

Araneidae commented 3 years ago

Perhaps a more configurable way of defining the udev rules would help, but I hope this can be a sensible starting point.

Araneidae commented 3 years ago

I've renumbered the .rules file. I'm not going to split the rpm at the moment, I imagine that can be a separate merge if it happens. Anything else to do?

mdavidsaver commented 3 years ago

Fair enough, I guess we can wait until someone complains about the udev rule failing :)