| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ceb1f1f1-ba8c-58c1-b33e-31324424f608@nvidia.com>
Date: Mon, 21 Mar 2022 18:45:49 -0700
From: John Hubbard <jhubbard@...dia.com>
To: mpenttil@...hat.com, linux-kernel@...r.kernel.org,
linux-mm@...ck.org
Cc: apopple@...dia.com, rcampbell@...dia.com, jgg@...pe.ca,
vbabka@...e.cz
Subject: Re: [PATCH v4] mm/hmm/test: use char dev with struct device to get
device node
On 3/20/22 7:54 PM, mpenttil@...hat.com wrote:
> From: Mika Penttilä <mpenttil@...hat.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.
>
> Change this to use proper use of cdev APIs and struct device.
Maybe, "Change this to properly use cdev and struct device APIs".
>
> Delete the /proc/devices parsing from the user-space test script, now
> that it is unnecessary.
>
> Signed-off-by: Mika Penttilä <mpenttil@...hat.com>
> Cc: Alistair Popple <apopple@...dia.com>
> Cc: John Hubbard <jhubbard@...dia.com>
> Cc: Ralph Campbell <rcampbell@...dia.com>
> Cc: Jason Gunthorpe <jgg@...pe.ca>
> Cc: Vlastimil Babka <vbabka@...e.cz>
> ---
> v4:
> - fix commit log
> v3:
> - use cdev_device_add() instead of miscdevice
> v2:
> - Cleanups per review comments from John Hubbard
> - Added Tested-by and Ccs
>
> lib/test_hmm.c | 25 ++++++++++++++++++-------
> tools/testing/selftests/vm/test_hmm.sh | 6 ------
> 2 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/lib/test_hmm.c b/lib/test_hmm.c
> index 767538089a62..d247e9c0fe94 100644
> --- a/lib/test_hmm.c
> +++ b/lib/test_hmm.c
> @@ -29,11 +29,17 @@
>
> #include "test_hmm_uapi.h"
>
> -#define DMIRROR_NDEVICES 2
> #define DMIRROR_RANGE_FAULT_TIMEOUT 1000
> #define DEVMEM_CHUNK_SIZE (256 * 1024 * 1024U)
> #define DEVMEM_CHUNKS_RESERVE 16
>
> +static const char *dmirror_device_names[] = {
> + "hmm_dmirror0",
> + "hmm_dmirror1"
> +};
> +
> +#define DMIRROR_NDEVICES ARRAY_SIZE(dmirror_device_names)
> +
> static const struct dev_pagemap_ops dmirror_devmem_ops;
> static const struct mmu_interval_notifier_ops dmirror_min_ops;
> static dev_t dmirror_dev;
> @@ -74,7 +80,7 @@ struct dmirror {
> * ZONE_DEVICE pages for migration and simulating device memory.
> */
> struct dmirror_chunk {
> - struct dev_pagemap pagemap;
> + struct dev_pagemap pagemap;
Unnecessary (likely unintended?) whitespace change. I notice that this
file uses tabs to line up the names of fields and local variables, let's
not disturb that unless there is some reason to do so, as it merely adds
diffs that we don't really need, cluttering up the real point of the
patch.
> struct dmirror_device *mdevice;
> };
>
> @@ -82,8 +88,9 @@ struct dmirror_chunk {
> * Per device data.
> */
> struct dmirror_device {
> - struct cdev cdevice;
> - struct hmm_devmem *devmem;
> + struct cdev cdevice;
> + struct device device;
> + struct hmm_devmem *devmem;
Same thing here: in this case, you only added one line, and the other
changes are due to whitespace that should really just be left
undisturbed.
However...I just noticed that "struct hmm_devmem *devmem;" is
entirely unused! Yikes. Probably best to delete it and add a short note
in the commit description to mention that. Something like, "Also,
deleted an unused field in struct dmirror_device: devmem."
>
> unsigned int devmem_capacity;
> unsigned int devmem_count;
> @@ -132,7 +139,7 @@ static int dmirror_fops_open(struct inode *inode, struct file *filp)
> xa_init(&dmirror->pt);
>
> ret = mmu_interval_notifier_insert(&dmirror->notifier, current->mm,
> - 0, ULONG_MAX & PAGE_MASK, &dmirror_min_ops);
> + 0, ULONG_MAX & PAGE_MASK, &dmirror_min_ops);
Another inadvertent whitespace change.
> if (ret) {
> kfree(dmirror);
> return ret;
> @@ -1225,7 +1232,11 @@ static int dmirror_device_init(struct dmirror_device *mdevice, int id)
>
> cdev_init(&mdevice->cdevice, &dmirror_fops);
> mdevice->cdevice.owner = THIS_MODULE;
> - ret = cdev_add(&mdevice->cdevice, dev, 1);
> + device_initialize(&mdevice->device);
> + dev_set_name(&mdevice->device, "%s", dmirror_device_names[id]);
> + mdevice->device.devt = dev;
> +
> + ret = cdev_device_add(&mdevice->cdevice, &mdevice->device);
looks good...
> if (ret)
> return ret;
>
> @@ -1252,7 +1263,7 @@ static void dmirror_device_remove(struct dmirror_device *mdevice)
> kfree(mdevice->devmem_chunks);
> }
>
> - cdev_del(&mdevice->cdevice);
> + cdev_device_del(&mdevice->cdevice, &mdevice->device);
> }
>
> static int __init hmm_dmirror_init(void)
> diff --git a/tools/testing/selftests/vm/test_hmm.sh b/tools/testing/selftests/vm/test_hmm.sh
> index 0647b525a625..69f5889f8575 100755
> --- a/tools/testing/selftests/vm/test_hmm.sh
> +++ b/tools/testing/selftests/vm/test_hmm.sh
> @@ -41,17 +41,11 @@ check_test_requirements()
> load_driver()
> {
> modprobe $DRIVER > /dev/null 2>&1
> - if [ $? == 0 ]; then
> - major=$(awk "\$2==\"HMM_DMIRROR\" {print \$1}" /proc/devices)
> - mknod /dev/hmm_dmirror0 c $major 0
> - mknod /dev/hmm_dmirror1 c $major 1
> - fi
> }
>
> unload_driver()
> {
> modprobe -r $DRIVER > /dev/null 2>&1
> - rm -f /dev/hmm_dmirror?
> }
>
> run_smoke()
The diffs look functionally correct (although I have not actually tested
them), so with the above minor cleanup applied (with, or without also
deleting that .devmem field--up to you), please feel free to add:
Reviewed-by: John Hubbard <jhubbard@...dia.com>
thanks,
--
John Hubbard
NVIDIA
Powered by blists - more mailing lists