Re: [RFC v5 1/7] testing/nvdimm: Add test module for non-nfit platforms

[Dan Williams <dan.j.williams@intel.com>]

On Mon, Dec 14, 2020 at 2:39 AM Santosh Sivaraj <santosh@fossix.org> wrote:
>
> The current test module cannot be used for testing platforms (make check)
> that do not have support for NFIT. In order to get the ndctl tests working,
> we need a module which can emulate NVDIMM devices without relying on
> ACPI/NFIT.
>
> The aim of this proposed module is to implement a similar functionality to
> the existing module but without the ACPI dependencies.
>
> This RFC series is split into reviewable and compilable chunks.
>
> This patch adds a new driver and registers two nvdimm bus needed for ndctl
> make check.
>
> Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
> ---
>  tools/testing/nvdimm/config_check.c |   3 +-
>  tools/testing/nvdimm/test/Kbuild    |   6 +-
>  tools/testing/nvdimm/test/ndtest.c  | 206 ++++++++++++++++++++++++++++
>  tools/testing/nvdimm/test/ndtest.h  |  16 +++
>  4 files changed, 229 insertions(+), 2 deletions(-)
>  create mode 100644 tools/testing/nvdimm/test/ndtest.c
>  create mode 100644 tools/testing/nvdimm/test/ndtest.h
>
> diff --git a/tools/testing/nvdimm/config_check.c b/tools/testing/nvdimm/config_check.c
> index cac891028cd1..3e3a5f518864 100644
> --- a/tools/testing/nvdimm/config_check.c
> +++ b/tools/testing/nvdimm/config_check.c
> @@ -12,7 +12,8 @@ void check(void)
>         BUILD_BUG_ON(!IS_MODULE(CONFIG_ND_BTT));
>         BUILD_BUG_ON(!IS_MODULE(CONFIG_ND_PFN));
>         BUILD_BUG_ON(!IS_MODULE(CONFIG_ND_BLK));
> -       BUILD_BUG_ON(!IS_MODULE(CONFIG_ACPI_NFIT));
> +       if (IS_ENABLED(CONFIG_ACPI_NFIT))
> +               BUILD_BUG_ON(!IS_MODULE(CONFIG_ACPI_NFIT));
>         BUILD_BUG_ON(!IS_MODULE(CONFIG_DEV_DAX));
>         BUILD_BUG_ON(!IS_MODULE(CONFIG_DEV_DAX_PMEM));
>  }
> diff --git a/tools/testing/nvdimm/test/Kbuild b/tools/testing/nvdimm/test/Kbuild
> index 75baebf8f4ba..197bcb2b7f35 100644
> --- a/tools/testing/nvdimm/test/Kbuild
> +++ b/tools/testing/nvdimm/test/Kbuild
> @@ -5,5 +5,9 @@ ccflags-y += -I$(srctree)/drivers/acpi/nfit/
>  obj-m += nfit_test.o
>  obj-m += nfit_test_iomap.o
>

> -nfit_test-y := nfit.o
> +ifeq  ($(CONFIG_ACPI_NFIT),m)
> +       nfit_test-y := nfit.o
> +else
> +       nfit_test-y := ndtest.o
> +endif

So it took me a moment to figure out what happened to my
ARCH_SUPPORTS_ACPI suggestion and that you're using the "nfit_test"
name to both claim a level of compatibility, but also prevent ndtest.o
from colliding with the nfit.o implementation (i.e. they both can't be
loaded at the same time).

I think that's a reasonable change, but it sorely needs to be
documented. Especially given that this may silently swap out
nfit_test.ko with a version that passes fewer tests it needs a mention
in the ndctl README.md that there are 2 modes of the test module
possible. This lets developers of other archs that come along discover
their options.

A mechanism for how to tell which flavor of nfit_test is installed is
needed so that when someone files a github issue about a test failure
there is an unambiguous way to validate what mode is being run.
Perhaps:

MODULE_DESCRIPTION("nfit_test: compat")

MODULE_DESCRIPTION("nfit_test: native")

...in the 2 different builds? Bonus points if the ndctl tests that are
known to fail with the compat version just skip instead of fail when
detecting compat.



>  nfit_test_iomap-y := iomap.o
> diff --git a/tools/testing/nvdimm/test/ndtest.c b/tools/testing/nvdimm/test/ndtest.c
> new file mode 100644
> index 000000000000..f89d74fdcdee
> --- /dev/null
> +++ b/tools/testing/nvdimm/test/ndtest.c
> @@ -0,0 +1,206 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/platform_device.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/genalloc.h>
> +#include <linux/vmalloc.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/list_sort.h>
> +#include <linux/libnvdimm.h>
> +#include <linux/ndctl.h>
> +#include <nd-core.h>
> +#include <linux/printk.h>
> +#include <linux/seq_buf.h>
> +
> +#include "../watermark.h"
> +#include "nfit_test.h"
> +#include "ndtest.h"
> +
> +enum {
> +       DIMM_SIZE = SZ_32M,
> +       LABEL_SIZE = SZ_128K,
> +       NUM_INSTANCES = 2,
> +       NUM_DCR = 4,
> +};
> +
> +static struct ndtest_priv *instances[NUM_INSTANCES];
> +static struct class *ndtest_dimm_class;
> +
> +static inline struct ndtest_priv *to_ndtest_priv(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +
> +       return container_of(pdev, struct ndtest_priv, pdev);
> +}
> +
> +static int ndtest_ctl(struct nvdimm_bus_descriptor *nd_desc,
> +                     struct nvdimm *nvdimm, unsigned int cmd, void *buf,
> +                     unsigned int buf_len, int *cmd_rc)
> +{
> +       struct ndtest_dimm *dimm;
> +       int _cmd_rc;
> +
> +       if (!cmd_rc)
> +               cmd_rc = &_cmd_rc;
> +
> +       *cmd_rc = 0;
> +
> +       if (!nvdimm)
> +               return -EINVAL;
> +
> +       dimm = nvdimm_provider_data(nvdimm);
> +       if (!dimm)
> +               return -EINVAL;
> +
> +       switch (cmd) {
> +       case ND_CMD_GET_CONFIG_SIZE:
> +       case ND_CMD_GET_CONFIG_DATA:
> +       case ND_CMD_SET_CONFIG_DATA:
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int ndtest_bus_register(struct ndtest_priv *p)
> +{
> +       p->bus_desc.ndctl = ndtest_ctl;
> +       p->bus_desc.module = THIS_MODULE;
> +       p->bus_desc.provider_name = NULL;
> +
> +       p->bus = nvdimm_bus_register(&p->pdev.dev, &p->bus_desc);
> +       if (!p->bus) {
> +               dev_err(&p->pdev.dev, "Error creating nvdimm bus %pOF\n", p->dn);
> +               return -ENOMEM;
> +       }
> +
> +       return 0;
> +}
> +
> +static int ndtest_remove(struct platform_device *pdev)
> +{
> +       struct ndtest_priv *p = to_ndtest_priv(&pdev->dev);
> +
> +       nvdimm_bus_unregister(p->bus);
> +       return 0;
> +}
> +
> +static int ndtest_probe(struct platform_device *pdev)
> +{
> +       struct ndtest_priv *p;
> +
> +       p = to_ndtest_priv(&pdev->dev);
> +       if (ndtest_bus_register(p))
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, p);
> +
> +       return 0;
> +}
> +
> +static const struct platform_device_id ndtest_id[] = {
> +       { KBUILD_MODNAME },
> +       { },
> +};
> +
> +static struct platform_driver ndtest_driver = {
> +       .probe = ndtest_probe,
> +       .remove = ndtest_remove,
> +       .driver = {
> +               .name = KBUILD_MODNAME,
> +       },
> +       .id_table = ndtest_id,
> +};
> +
> +static void ndtest_release(struct device *dev)
> +{
> +       struct ndtest_priv *p = to_ndtest_priv(dev);
> +
> +       kfree(p);
> +}
> +
> +static void cleanup_devices(void)
> +{
> +       int i;
> +
> +       for (i = 0; i < NUM_INSTANCES; i++)
> +               if (instances[i])
> +                       platform_device_unregister(&instances[i]->pdev);
> +
> +       nfit_test_teardown();
> +
> +       if (ndtest_dimm_class)
> +               class_destroy(ndtest_dimm_class);
> +}
> +
> +static __init int ndtest_init(void)
> +{
> +       int rc, i;
> +
> +       pmem_test();
> +       libnvdimm_test();
> +       device_dax_test();
> +       dax_pmem_test();
> +       dax_pmem_core_test();
> +#ifdef CONFIG_DEV_DAX_PMEM_COMPAT
> +       dax_pmem_compat_test();
> +#endif
> +
> +       ndtest_dimm_class = class_create(THIS_MODULE, "nfit_test_dimm");
> +       if (IS_ERR(ndtest_dimm_class)) {
> +               rc = PTR_ERR(ndtest_dimm_class);
> +               goto err_register;
> +       }
> +
> +       /* Each instance can be taken as a bus, which can have multiple dimms */
> +       for (i = 0; i < NUM_INSTANCES; i++) {
> +               struct ndtest_priv *priv;
> +               struct platform_device *pdev;
> +
> +               priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +               if (!priv) {
> +                       rc = -ENOMEM;
> +                       goto err_register;
> +               }
> +
> +               INIT_LIST_HEAD(&priv->resources);
> +               pdev = &priv->pdev;
> +               pdev->name = KBUILD_MODNAME;
> +               pdev->id = i;
> +               pdev->dev.release = ndtest_release;
> +               rc = platform_device_register(pdev);
> +               if (rc) {
> +                       put_device(&pdev->dev);
> +                       goto err_register;
> +               }
> +               get_device(&pdev->dev);
> +
> +               instances[i] = priv;
> +       }
> +
> +       rc = platform_driver_register(&ndtest_driver);
> +       if (rc)
> +               goto err_register;
> +
> +       return 0;
> +
> +err_register:
> +       pr_err("Error registering platform device\n");
> +       cleanup_devices();
> +
> +       return rc;
> +}
> +
> +static __exit void ndtest_exit(void)
> +{
> +       cleanup_devices();
> +       platform_driver_unregister(&ndtest_driver);
> +}
> +
> +module_init(ndtest_init);
> +module_exit(ndtest_exit);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("IBM Corporation");
> diff --git a/tools/testing/nvdimm/test/ndtest.h b/tools/testing/nvdimm/test/ndtest.h
> new file mode 100644
> index 000000000000..831ac5c65f50
> --- /dev/null
> +++ b/tools/testing/nvdimm/test/ndtest.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef NDTEST_H
> +#define NDTEST_H
> +
> +#include <linux/platform_device.h>
> +#include <linux/libnvdimm.h>
> +
> +struct ndtest_priv {
> +       struct platform_device pdev;
> +       struct device_node *dn;
> +       struct list_head resources;
> +       struct nvdimm_bus_descriptor bus_desc;
> +       struct nvdimm_bus *bus;
> +};
> +
> +#endif /* NDTEST_H */
> --
> 2.26.2
>
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org