Messages in this thread | | | Date | Tue, 15 Mar 2022 05:22:15 +0200 | Subject | Re: [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev | From | Mika Penttilä <> |
| |
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@redhat.com wrote: >> From: Mika Penttilä <mpenttil@redhat.com> >> >> 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
| |