Hi Jason and thanks for your comments..
On 14.3.2022 20.24, Jason Gunthorpe wrote:
On Fri, Mar 11, 2022 at 05:30:50AM +0200, mpenttil@xxxxxxxxxx wrote:
From: Mika Penttilä <mpenttil@xxxxxxxxxx>
HMM selftests use an in-kernel pseudo device to emulate device private
memory. The pseudo device registers a major device range for two pseudo
device instances. User space has a script that reads /proc/devices in
order to find the assigned major number, and sends that to mknod(1),
once for each node.
This duplicates a fair amount of boilerplate that misc device can do
instead.
Change this to use misc device, which makes the device node names appear
for us. This also enables udev-like processing if desired.
This is borderline the wrong way to use misc devices, they should
never be embedded into other structs like this. It works out here
because they are eventually only placed in a static array, but still
it is a generally bad pattern to see.
Could you elaborate on this one? We have many in-tree usages of the same
pattern, like:
drivers/video/fbdev/pxa3xx-gcu.c
drivers/platform/goldfish/goldfish_pipe.c
drivers/virt/vboxguest/vboxguest_linux.c
drivers/virt/nitro_enclaves/ne_pci_dev.c
drivers/watchdog/cpwd.c
drivers/platform/x86/toshiba_acpi.c
drivers/platform/x86/wmi.c
drivers/platform/surface/surface_dtx.c
drivers/bus/fsl-mc/fsl-mc-uapi.c
drivers/misc/dw-xdata-pcie.c
drivers/input/serio/serio_raw.c
fs/dlm/user.c
lib/test_kmod.c
You mention "placed in a static array", are you seeing a potential
lifetime issue or what? Many of the examples above embed miscdevice in a
dynamically allocated object also.
The file object's private_data holds a pointer to the miscdevice, and
fops_get() pins the module. So freeing the objects miscdevice is
embedded in at module_exit time should be fine. But, as you said, in
this case the miscdevices are statically allocated, so that shouldn't be
an issue either. But maybe I'm missing something?
Delete the /proc/devices parsing from the user-space test script, now
that it is unnecessary.
This is all because the cdev is being used wrong - it get all this
stuff it should be done via cdev_device_add() and a dmirror_device
needs a struct device to hold the sysfs.
I think using cdev_add ends up in the same results in device_* api
sense. miscdevice acting like a mux at a higher abstraction level
simplifies the code.
Jason
Thanks,
Mika
