Re: [PATCH v2 2/3] nfit, libnvdimm: allow an ARS scrub to be triggered on demand

[Dan Williams]

On Wed, Jul 20, 2016 at 6:50 PM, Vishal Verma <vishal.l.verma@xxxxxxxxx> wrote:
> Normally, an ARS (Address Range Scrub) only happens at
> boot/initialization time. There can however arise situations where a
> bus-wide rescan is needed - notably, in the case of discovering a latent
> media error, we should do a full rescan to figure out what other sectors
> are bad, and thus potentially avoid triggering an mce on them in the
> future. Also provide a sysfs trigger to start a bus-wide scrub.
>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Cc: <linux-acpi@xxxxxxxxxxxxxxx>
> Cc: <linux-nvdimm@xxxxxxxxxxxx>
> Signed-off-by: Vishal Verma <vishal.l.verma@xxxxxxxxx>
> ---
>  drivers/acpi/nfit.c              | 123 +++++++++++++++++++++++++++++++++------
>  drivers/acpi/nfit.h              |   4 +-
>  drivers/nvdimm/core.c            |   7 +++
>  include/linux/libnvdimm.h        |   1 +
>  tools/testing/nvdimm/test/nfit.c |  16 +++++
>  5 files changed, 131 insertions(+), 20 deletions(-)
>

Looks good, just a couple nits:

[..]
> @@ -2138,7 +2172,7 @@ static void acpi_nfit_async_scrub(struct acpi_nfit_desc *acpi_desc,
>         unsigned int tmo = scrub_timeout;
>         int rc;
>
> -       if (nfit_spa->ars_done || !nfit_spa->nd_region)
> +       if (!(nfit_spa->ars_required && nfit_spa->nd_region))
>                 return;

Why is nd_region part of this check?  Can't this just be:

    if (!nfit_spa->ars_requested)
        return;

[..]
>
> +static struct acpi_nfit_desc *acpi_nfit_desc_alloc_register(struct device *dev)
> +{
> +       struct acpi_nfit_desc *acpi_desc;
> +       struct kernfs_node *nfit;
> +       struct device *bus_dev;
> +
> +       acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL);
> +       if (!acpi_desc)
> +               return ERR_PTR(-ENOMEM);
> +
> +       acpi_nfit_desc_init(acpi_desc, dev);
> +
> +       acpi_desc->nvdimm_bus = nvdimm_bus_register(dev, &acpi_desc->nd_desc);
> +       if (!acpi_desc->nvdimm_bus)
> +               return ERR_PTR(-ENOMEM);
> +
> +       bus_dev = to_nvdimm_bus_dev(acpi_desc->nvdimm_bus);
> +       nfit = sysfs_get_dirent(bus_dev->kobj.sd, "nfit");
> +       if (!nfit) {
> +               dev_err(dev, "sysfs_get_dirent 'nfit' failed\n");
> +               return ERR_PTR(-ENODEV);
> +       }
> +       acpi_desc->scrub_count_state = sysfs_get_dirent(nfit, "scrub");

Missing sysfs_put(nfit) here?

> +       if (!acpi_desc->scrub_count_state) {
> +               dev_err(dev, "sysfs_get_dirent 'scrub' failed\n");
> +               return ERR_PTR(-ENODEV);
> +       }