[PATCH 0/3] ARS rescanning triggered by latent errors or userspace

[PATCH 0/3] ARS rescanning triggered by latent errors or userspace

[Vishal Verma]

This series adds on-demand ARS scanning on both, discovery of
latent media errors, and a sysfs trigger from userspace.

The rescanning part is easy to test using the nfit_test framework
- create a namespace (this will by default have bad sectors in
the middle), clear the bad sectors by writing to them, trigger
the rescan through sysfs, and the bad sectors will reappear in
/sys/block/<pmemX>/badblocks.

For the mce handling, I've tested the notifier chain callback
being called with a mock struct mce (called via another sysfs
trigger - this isn't included in the patch obviously), which
has the address field set to a known address in a SPA range,
and the status field with the MCACOD flag set.

What I haven't easily been able to test is the same callback
path with a 'real world' mce, being called as part of the
x86_mce_decoder_chain notifier. I'd therefore appreciate a
closer look at the initial filtering done in nfit_handle_mce
(patch 3/3) from Tony or anyone more familiar with mce handling.

The series is based on v4.7-rc7, and a tree is available at
https://git.kernel.org/cgit/linux/kernel/git/vishal/nvdimm.git/log/?h=ars-ondemand


Vishal Verma (3):
  pmem: clarify a debug print in pmem_clear_poison
  nfit, libnvdimm: allow an ARS rescan to be triggered on demand
  nfit: do an ARS rescan on hitting a latent media error

 drivers/acpi/nfit.c       | 138 ++++++++++++++++++++++++++++++++++++++++++++--
 drivers/acpi/nfit.h       |   2 +
 drivers/nvdimm/core.c     |  17 ++++++
 drivers/nvdimm/pmem.c     |   2 +-
 include/linux/libnvdimm.h |   1 +
 5 files changed, 155 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH 1/3] pmem: clarify a debug print in pmem_clear_poison

[Vishal Verma]

Prefix the sector number being cleared with a '0x' to make it clear that
this is a hex value.

Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/nvdimm/pmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 608fc44..29ab25b 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -57,7 +57,7 @@ static void pmem_clear_poison(struct pmem_device *pmem, phys_addr_t offset,
 	cleared = nvdimm_clear_poison(dev, pmem->phys_addr + offset, len);
 
 	if (cleared > 0 && cleared / 512) {
-		dev_dbg(dev, "%s: %llx clear %ld sector%s\n",
+		dev_dbg(dev, "%s: 0x%llx clear %ld sector%s\n",
 				__func__, (unsigned long long) sector,
 				cleared / 512, cleared / 512 > 1 ? "s" : "");
 		badblocks_clear(&pmem->bb, sector, cleared / 512);
-- 
2.7.4

Re: [PATCH 1/3] pmem: clarify a debug print in pmem_clear_poison

[Dan Williams]

On Mon, Jul 18, 2016 at 5:44 PM, Vishal Verma <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> Prefix the sector number being cleared with a '0x' to make it clear that
> this is a hex value.
>
> Signed-off-by: Vishal Verma <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/nvdimm/pmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 608fc44..29ab25b 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -57,7 +57,7 @@ static void pmem_clear_poison(struct pmem_device *pmem, phys_addr_t offset,
>         cleared = nvdimm_clear_poison(dev, pmem->phys_addr + offset, len);
>
>         if (cleared > 0 && cleared / 512) {
> -               dev_dbg(dev, "%s: %llx clear %ld sector%s\n",
> +               dev_dbg(dev, "%s: 0x%llx clear %ld sector%s\n",

%#llx is a bit smaller / more readable.  Shall I just fix this up on applying?

Re: [PATCH 1/3] pmem: clarify a debug print in pmem_clear_poison

[Verma, Vishal L]

On Tue, 2016-07-19 at 08:53 -0700, Dan Williams wrote:
> On Mon, Jul 18, 2016 at 5:44 PM, Vishal Verma <vishal.l.verma@intel.co
> m> wrote:
> > Prefix the sector number being cleared with a '0x' to make it clear
> > that
> > this is a hex value.
> > 
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  drivers/nvdimm/pmem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> > index 608fc44..29ab25b 100644
> > --- a/drivers/nvdimm/pmem.c
> > +++ b/drivers/nvdimm/pmem.c
> > @@ -57,7 +57,7 @@ static void pmem_clear_poison(struct pmem_device
> > *pmem, phys_addr_t offset,
> >         cleared = nvdimm_clear_poison(dev, pmem->phys_addr + offset,
> > len);
> > 
> >         if (cleared > 0 && cleared / 512) {
> > -               dev_dbg(dev, "%s: %llx clear %ld sector%s\n",
> > +               dev_dbg(dev, "%s: 0x%llx clear %ld sector%s\n",
> 
> %#llx is a bit smaller / more readable.  Shall I just fix this up on
> applying?

TIL :) Yes you can do the fixup, thanks!

Re: [PATCH 1/3] pmem: clarify a debug print in pmem_clear_poison

[Vishal Verma]

On 07/19, Verma, Vishal L wrote:
> On Tue, 2016-07-19 at 08:53 -0700, Dan Williams wrote:
> > On Mon, Jul 18, 2016 at 5:44 PM, Vishal Verma <vishal.l.verma@intel.co
> > m> wrote:
> > > Prefix the sector number being cleared with a '0x' to make it clear
> > > that
> > > this is a hex value.
> > > 
> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > > ---
> > >  drivers/nvdimm/pmem.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> > > index 608fc44..29ab25b 100644
> > > --- a/drivers/nvdimm/pmem.c
> > > +++ b/drivers/nvdimm/pmem.c
> > > @@ -57,7 +57,7 @@ static void pmem_clear_poison(struct pmem_device
> > > *pmem, phys_addr_t offset,
> > >         cleared = nvdimm_clear_poison(dev, pmem->phys_addr + offset,
> > > len);
> > > 
> > >         if (cleared > 0 && cleared / 512) {
> > > -               dev_dbg(dev, "%s: %llx clear %ld sector%s\n",
> > > +               dev_dbg(dev, "%s: 0x%llx clear %ld sector%s\n",
> > 
> > %#llx is a bit smaller / more readable.  Shall I just fix this up on
> > applying?
> 
> TIL :) Yes you can do the fixup, thanks!

Or since I'm reworking the other two, I'lll fix this up too.

> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Vishal Verma]

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 rescan.

Cc: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: <linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Cc: <linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Vishal Verma <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/acpi/nfit.c       | 36 ++++++++++++++++++++++++++++++++----
 drivers/acpi/nfit.h       |  1 +
 drivers/nvdimm/core.c     | 17 +++++++++++++++++
 include/linux/libnvdimm.h |  1 +
 4 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index ac6ddcc0..def9505 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -2138,8 +2138,9 @@ 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)
-		return;
+	if (!nfit_spa->ars_rescan)
+		if (nfit_spa->ars_done || !nfit_spa->nd_region)
+			return;
 
 	rc = ars_start(acpi_desc, nfit_spa);
 	/*
@@ -2227,7 +2228,9 @@ static void acpi_nfit_scrub(struct work_struct *work)
 	 * firmware initiated scrubs to complete and then we go search for the
 	 * affected spa regions to mark them scanned.  In the second phase we
 	 * initiate a directed scrub for every range that was not scrubbed in
-	 * phase 1.
+	 * phase 1. If we're called for a 'rescan', we harmlessly pass through
+	 * the first phase, but really only care about running phase 2, where
+	 * regions can be notified of new poison.
 	 */
 
 	/* process platform firmware initiated scrubs */
@@ -2336,8 +2339,10 @@ static void acpi_nfit_scrub(struct work_struct *work)
 			acpi_nfit_register_region(acpi_desc, nfit_spa);
 	}
 
-	list_for_each_entry(nfit_spa, &acpi_desc->spas, list)
+	list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
 		acpi_nfit_async_scrub(acpi_desc, nfit_spa);
+		nfit_spa->ars_rescan = 0;
+	}
 	mutex_unlock(&acpi_desc->init_mutex);
 }
 
@@ -2495,6 +2500,28 @@ static int acpi_nfit_clear_to_send(struct nvdimm_bus_descriptor *nd_desc,
 	return 0;
 }
 
+static int acpi_nfit_ars_rescan(struct nvdimm_bus_descriptor *nd_desc)
+{
+	struct acpi_nfit_desc *acpi_desc = to_acpi_nfit_desc(nd_desc);
+	struct device *dev = acpi_desc->dev;
+	struct nfit_spa *nfit_spa;
+
+	if (work_busy(&acpi_desc->work))
+		return -EBUSY;
+
+	list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
+		struct acpi_nfit_system_address *spa = nfit_spa->spa;
+
+		if (nfit_spa_type(spa) != NFIT_SPA_PM)
+			continue;
+
+		nfit_spa->ars_rescan = 1;
+	}
+	queue_work(nfit_wq, &acpi_desc->work);
+	dev_info(dev, "%s: ars_rescan triggered\n", __func__);
+	return 0;
+}
+
 void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev)
 {
 	struct nvdimm_bus_descriptor *nd_desc;
@@ -2507,6 +2534,7 @@ void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev)
 	nd_desc->ndctl = acpi_nfit_ctl;
 	nd_desc->flush_probe = acpi_nfit_flush_probe;
 	nd_desc->clear_to_send = acpi_nfit_clear_to_send;
+	nd_desc->ars_rescan = acpi_nfit_ars_rescan;
 	nd_desc->attr_groups = acpi_nfit_attribute_groups;
 
 	INIT_LIST_HEAD(&acpi_desc->spa_maps);
diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
index 02b9ea1..db95c5d 100644
--- a/drivers/acpi/nfit.h
+++ b/drivers/acpi/nfit.h
@@ -78,6 +78,7 @@ struct nfit_spa {
 	struct list_head list;
 	struct nd_region *nd_region;
 	unsigned int ars_done:1;
+	unsigned int ars_rescan:1;
 	u32 clear_err_unit;
 	u32 max_ars;
 };
diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
index be89764..54f6fd5 100644
--- a/drivers/nvdimm/core.c
+++ b/drivers/nvdimm/core.c
@@ -313,10 +313,27 @@ static ssize_t wait_probe_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(wait_probe);
 
+static ssize_t ars_rescan_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct nvdimm_bus *nvdimm_bus = to_nvdimm_bus(dev);
+	struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
+	int rc;
+
+	if (nd_desc->ars_rescan) {
+		rc = nd_desc->ars_rescan(nd_desc);
+		if (rc)
+			return rc;
+	}
+	return size;
+}
+static DEVICE_ATTR_WO(ars_rescan);
+
 static struct attribute *nvdimm_bus_attributes[] = {
 	&dev_attr_commands.attr,
 	&dev_attr_wait_probe.attr,
 	&dev_attr_provider.attr,
+	&dev_attr_ars_rescan.attr,
 	NULL,
 };
 
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 0c3c30c..1c6867a 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -74,6 +74,7 @@ struct nvdimm_bus_descriptor {
 	int (*flush_probe)(struct nvdimm_bus_descriptor *nd_desc);
 	int (*clear_to_send)(struct nvdimm_bus_descriptor *nd_desc,
 			struct nvdimm *nvdimm, unsigned int cmd);
+	int (*ars_rescan)(struct nvdimm_bus_descriptor *nd_desc);
 };
 
 struct nd_cmd_desc {
-- 
2.7.4

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

[Dan Williams]

On Mon, Jul 18, 2016 at 5:44 PM, Vishal Verma <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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 rescan.
>
> Cc: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: <linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Cc: <linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Vishal Verma <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/acpi/nfit.c       | 36 ++++++++++++++++++++++++++++++++----
>  drivers/acpi/nfit.h       |  1 +
>  drivers/nvdimm/core.c     | 17 +++++++++++++++++
>  include/linux/libnvdimm.h |  1 +
>  4 files changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index ac6ddcc0..def9505 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -2138,8 +2138,9 @@ 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)
> -               return;
> +       if (!nfit_spa->ars_rescan)
> +               if (nfit_spa->ars_done || !nfit_spa->nd_region)
> +                       return;

Do we need a new flag? Why not just clear ->ars_done?

>
>         rc = ars_start(acpi_desc, nfit_spa);
>         /*
> @@ -2227,7 +2228,9 @@ static void acpi_nfit_scrub(struct work_struct *work)
>          * firmware initiated scrubs to complete and then we go search for the
>          * affected spa regions to mark them scanned.  In the second phase we
>          * initiate a directed scrub for every range that was not scrubbed in
> -        * phase 1.
> +        * phase 1. If we're called for a 'rescan', we harmlessly pass through
> +        * the first phase, but really only care about running phase 2, where
> +        * regions can be notified of new poison.
>          */

I don't think we need to distinguish the initial scan case from the
re-scan case in acpi_nfit_scrub().  Whether it's a scan or a re-scan
doesn't matter to acpi_nfit_scrub().

>
>         /* process platform firmware initiated scrubs */
> @@ -2336,8 +2339,10 @@ static void acpi_nfit_scrub(struct work_struct *work)
>                         acpi_nfit_register_region(acpi_desc, nfit_spa);
>         }
>
> -       list_for_each_entry(nfit_spa, &acpi_desc->spas, list)
> +       list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
>                 acpi_nfit_async_scrub(acpi_desc, nfit_spa);
> +               nfit_spa->ars_rescan = 0;
> +       }
>         mutex_unlock(&acpi_desc->init_mutex);
>  }
>
> @@ -2495,6 +2500,28 @@ static int acpi_nfit_clear_to_send(struct nvdimm_bus_descriptor *nd_desc,
>         return 0;
>  }
>
> +static int acpi_nfit_ars_rescan(struct nvdimm_bus_descriptor *nd_desc)
> +{
> +       struct acpi_nfit_desc *acpi_desc = to_acpi_nfit_desc(nd_desc);
> +       struct device *dev = acpi_desc->dev;
> +       struct nfit_spa *nfit_spa;
> +
> +       if (work_busy(&acpi_desc->work))
> +               return -EBUSY;

How does userspace figure out when the queue is not busy?  See below
in the notes about the ars_rescan attribute.

> +
> +       list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
> +               struct acpi_nfit_system_address *spa = nfit_spa->spa;
> +
> +               if (nfit_spa_type(spa) != NFIT_SPA_PM)
> +                       continue;
> +
> +               nfit_spa->ars_rescan = 1;
> +       }
> +       queue_work(nfit_wq, &acpi_desc->work);
> +       dev_info(dev, "%s: ars_rescan triggered\n", __func__);
> +       return 0;
> +}
> +
>  void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev)
>  {
>         struct nvdimm_bus_descriptor *nd_desc;
> @@ -2507,6 +2534,7 @@ void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev)
>         nd_desc->ndctl = acpi_nfit_ctl;
>         nd_desc->flush_probe = acpi_nfit_flush_probe;
>         nd_desc->clear_to_send = acpi_nfit_clear_to_send;
> +       nd_desc->ars_rescan = acpi_nfit_ars_rescan;
>         nd_desc->attr_groups = acpi_nfit_attribute_groups;
>
>         INIT_LIST_HEAD(&acpi_desc->spa_maps);
> diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
> index 02b9ea1..db95c5d 100644
> --- a/drivers/acpi/nfit.h
> +++ b/drivers/acpi/nfit.h
> @@ -78,6 +78,7 @@ struct nfit_spa {
>         struct list_head list;
>         struct nd_region *nd_region;
>         unsigned int ars_done:1;
> +       unsigned int ars_rescan:1;
>         u32 clear_err_unit;
>         u32 max_ars;
>  };
> diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
> index be89764..54f6fd5 100644
> --- a/drivers/nvdimm/core.c
> +++ b/drivers/nvdimm/core.c
> @@ -313,10 +313,27 @@ static ssize_t wait_probe_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(wait_probe);
>
> +static ssize_t ars_rescan_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t size)
> +{
> +       struct nvdimm_bus *nvdimm_bus = to_nvdimm_bus(dev);
> +       struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
> +       int rc;
> +
> +       if (nd_desc->ars_rescan) {
> +               rc = nd_desc->ars_rescan(nd_desc);
> +               if (rc)
> +                       return rc;
> +       }
> +       return size;
> +}
> +static DEVICE_ATTR_WO(ars_rescan);

A few notes:

1/ ARS is unique to the nfit driver so let's make this nfit specific,
i.e. add it to acpi_nfit_attribute_group.

2/ Let's just call the attribute scrub and not distinguish it as "re-"

3/ Userspace may want to know when scanning is complete so let's make
this attribute read/write and on read return a count of the number of
completed scans since the driver was loaded.  For notification of last
completion use sysfs_notify_dirent_safe() to make this scrub attribute
select()/poll() capable.

> +
>  static struct attribute *nvdimm_bus_attributes[] = {
>         &dev_attr_commands.attr,
>         &dev_attr_wait_probe.attr,
>         &dev_attr_provider.attr,
> +       &dev_attr_ars_rescan.attr,
>         NULL,
>  };
>
> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> index 0c3c30c..1c6867a 100644
> --- a/include/linux/libnvdimm.h
> +++ b/include/linux/libnvdimm.h
> @@ -74,6 +74,7 @@ struct nvdimm_bus_descriptor {
>         int (*flush_probe)(struct nvdimm_bus_descriptor *nd_desc);
>         int (*clear_to_send)(struct nvdimm_bus_descriptor *nd_desc,
>                         struct nvdimm *nvdimm, unsigned int cmd);
> +       int (*ars_rescan)(struct nvdimm_bus_descriptor *nd_desc);
>  };
>
>  struct nd_cmd_desc {
> --
> 2.7.4
>

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

[Vishal Verma]

On 07/19, Dan Williams wrote:
> On Mon, Jul 18, 2016 at 5:44 PM, Vishal Verma <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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 rescan.
> >
> > Cc: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: <linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> > Cc: <linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org>
> > Signed-off-by: Vishal Verma <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/acpi/nfit.c       | 36 ++++++++++++++++++++++++++++++++----
> >  drivers/acpi/nfit.h       |  1 +
> >  drivers/nvdimm/core.c     | 17 +++++++++++++++++
> >  include/linux/libnvdimm.h |  1 +
> >  4 files changed, 51 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> > index ac6ddcc0..def9505 100644
> > --- a/drivers/acpi/nfit.c
> > +++ b/drivers/acpi/nfit.c
> > @@ -2138,8 +2138,9 @@ 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)
> > -               return;
> > +       if (!nfit_spa->ars_rescan)
> > +               if (nfit_spa->ars_done || !nfit_spa->nd_region)
> > +                       return;
> 
> Do we need a new flag? Why not just clear ->ars_done?

This is what I had started out with - clearing the done flag, but the
done flag gets set at the end of acpi_nfit_scrub if a region has been
registered for that SPA. In the rescan case, we'll almost always have
our regions registered, so the done flag will get set here, and
acpi_nfit_async_scrub won't look at it at all..

> 
> >
> >         rc = ars_start(acpi_desc, nfit_spa);
> >         /*
> > @@ -2227,7 +2228,9 @@ static void acpi_nfit_scrub(struct work_struct *work)
> >          * firmware initiated scrubs to complete and then we go search for the
> >          * affected spa regions to mark them scanned.  In the second phase we
> >          * initiate a directed scrub for every range that was not scrubbed in
> > -        * phase 1.
> > +        * phase 1. If we're called for a 'rescan', we harmlessly pass through
> > +        * the first phase, but really only care about running phase 2, where
> > +        * regions can be notified of new poison.
> >          */
> 
> I don't think we need to distinguish the initial scan case from the
> re-scan case in acpi_nfit_scrub().  Whether it's a scan or a re-scan
> doesn't matter to acpi_nfit_scrub().

Right, other than the above flag, we don't really distinguish betweent
the two. The comment was just a clarification/note that nothing
meaningful happens in this function for the rescan case.
> 
> >
> >         /* process platform firmware initiated scrubs */
> > @@ -2336,8 +2339,10 @@ static void acpi_nfit_scrub(struct work_struct *work)
> >                         acpi_nfit_register_region(acpi_desc, nfit_spa);
> >         }
> >
> > -       list_for_each_entry(nfit_spa, &acpi_desc->spas, list)
> > +       list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
> >                 acpi_nfit_async_scrub(acpi_desc, nfit_spa);
> > +               nfit_spa->ars_rescan = 0;
> > +       }
> >         mutex_unlock(&acpi_desc->init_mutex);
> >  }
> >
> > @@ -2495,6 +2500,28 @@ static int acpi_nfit_clear_to_send(struct nvdimm_bus_descriptor *nd_desc,
> >         return 0;
> >  }
> >
> > +static int acpi_nfit_ars_rescan(struct nvdimm_bus_descriptor *nd_desc)
> > +{
> > +       struct acpi_nfit_desc *acpi_desc = to_acpi_nfit_desc(nd_desc);
> > +       struct device *dev = acpi_desc->dev;
> > +       struct nfit_spa *nfit_spa;
> > +
> > +       if (work_busy(&acpi_desc->work))
> > +               return -EBUSY;
> 
> How does userspace figure out when the queue is not busy?  See below
> in the notes about the ars_rescan attribute.
> 
> > +
> > +       list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
> > +               struct acpi_nfit_system_address *spa = nfit_spa->spa;
> > +
> > +               if (nfit_spa_type(spa) != NFIT_SPA_PM)
> > +                       continue;
> > +
> > +               nfit_spa->ars_rescan = 1;
> > +       }
> > +       queue_work(nfit_wq, &acpi_desc->work);
> > +       dev_info(dev, "%s: ars_rescan triggered\n", __func__);
> > +       return 0;
> > +}
> > +
> >  void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev)
> >  {
> >         struct nvdimm_bus_descriptor *nd_desc;
> > @@ -2507,6 +2534,7 @@ void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev)
> >         nd_desc->ndctl = acpi_nfit_ctl;
> >         nd_desc->flush_probe = acpi_nfit_flush_probe;
> >         nd_desc->clear_to_send = acpi_nfit_clear_to_send;
> > +       nd_desc->ars_rescan = acpi_nfit_ars_rescan;
> >         nd_desc->attr_groups = acpi_nfit_attribute_groups;
> >
> >         INIT_LIST_HEAD(&acpi_desc->spa_maps);
> > diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
> > index 02b9ea1..db95c5d 100644
> > --- a/drivers/acpi/nfit.h
> > +++ b/drivers/acpi/nfit.h
> > @@ -78,6 +78,7 @@ struct nfit_spa {
> >         struct list_head list;
> >         struct nd_region *nd_region;
> >         unsigned int ars_done:1;
> > +       unsigned int ars_rescan:1;
> >         u32 clear_err_unit;
> >         u32 max_ars;
> >  };
> > diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
> > index be89764..54f6fd5 100644
> > --- a/drivers/nvdimm/core.c
> > +++ b/drivers/nvdimm/core.c
> > @@ -313,10 +313,27 @@ static ssize_t wait_probe_show(struct device *dev,
> >  }
> >  static DEVICE_ATTR_RO(wait_probe);
> >
> > +static ssize_t ars_rescan_store(struct device *dev,
> > +               struct device_attribute *attr, const char *buf, size_t size)
> > +{
> > +       struct nvdimm_bus *nvdimm_bus = to_nvdimm_bus(dev);
> > +       struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
> > +       int rc;
> > +
> > +       if (nd_desc->ars_rescan) {
> > +               rc = nd_desc->ars_rescan(nd_desc);
> > +               if (rc)
> > +                       return rc;
> > +       }
> > +       return size;
> > +}
> > +static DEVICE_ATTR_WO(ars_rescan);
> 
> A few notes:
> 
> 1/ ARS is unique to the nfit driver so let's make this nfit specific,
> i.e. add it to acpi_nfit_attribute_group.
> 
> 2/ Let's just call the attribute scrub and not distinguish it as "re-"
> 
> 3/ Userspace may want to know when scanning is complete so let's make
> this attribute read/write and on read return a count of the number of
> completed scans since the driver was loaded.  For notification of last
> completion use sysfs_notify_dirent_safe() to make this scrub attribute
> select()/poll() capable.

Ok, sounds reasonable.

> 
> > +
> >  static struct attribute *nvdimm_bus_attributes[] = {
> >         &dev_attr_commands.attr,
> >         &dev_attr_wait_probe.attr,
> >         &dev_attr_provider.attr,
> > +       &dev_attr_ars_rescan.attr,
> >         NULL,
> >  };
> >
> > diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> > index 0c3c30c..1c6867a 100644
> > --- a/include/linux/libnvdimm.h
> > +++ b/include/linux/libnvdimm.h
> > @@ -74,6 +74,7 @@ struct nvdimm_bus_descriptor {
> >         int (*flush_probe)(struct nvdimm_bus_descriptor *nd_desc);
> >         int (*clear_to_send)(struct nvdimm_bus_descriptor *nd_desc,
> >                         struct nvdimm *nvdimm, unsigned int cmd);
> > +       int (*ars_rescan)(struct nvdimm_bus_descriptor *nd_desc);
> >  };
> >
> >  struct nd_cmd_desc {
> > --
> > 2.7.4
> >

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

[Dan Williams]

On Tue, Jul 19, 2016 at 10:45 AM, Vishal Verma <vishal.l.verma@intel.com> wrote:
> On 07/19, Dan Williams wrote:
>> On Mon, Jul 18, 2016 at 5:44 PM, Vishal Verma <vishal.l.verma@intel.com> 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 rescan.
>> >
>> > Cc: Dan Williams <dan.j.williams@intel.com>
>> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> > Cc: <linux-acpi@vger.kernel.org>
>> > Cc: <linux-nvdimm@lists.01.org>
>> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
>> > ---
>> >  drivers/acpi/nfit.c       | 36 ++++++++++++++++++++++++++++++++----
>> >  drivers/acpi/nfit.h       |  1 +
>> >  drivers/nvdimm/core.c     | 17 +++++++++++++++++
>> >  include/linux/libnvdimm.h |  1 +
>> >  4 files changed, 51 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
>> > index ac6ddcc0..def9505 100644
>> > --- a/drivers/acpi/nfit.c
>> > +++ b/drivers/acpi/nfit.c
>> > @@ -2138,8 +2138,9 @@ 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)
>> > -               return;
>> > +       if (!nfit_spa->ars_rescan)
>> > +               if (nfit_spa->ars_done || !nfit_spa->nd_region)
>> > +                       return;
>>
>> Do we need a new flag? Why not just clear ->ars_done?
>
> This is what I had started out with - clearing the done flag, but the
> done flag gets set at the end of acpi_nfit_scrub if a region has been
> registered for that SPA. In the rescan case, we'll almost always have
> our regions registered, so the done flag will get set here, and
> acpi_nfit_async_scrub won't look at it at all..

Maybe just flip the polarity of ->ars_done to ->ars_do?  Seems we
should be able to get away with not adding a new flag.

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

[Vishal Verma]

On 07/19, Dan Williams wrote:
> On Tue, Jul 19, 2016 at 10:45 AM, Vishal Verma <vishal.l.verma@intel.com> wrote:
> > On 07/19, Dan Williams wrote:
> >> On Mon, Jul 18, 2016 at 5:44 PM, Vishal Verma <vishal.l.verma@intel.com> 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 rescan.
> >> >
> >> > Cc: Dan Williams <dan.j.williams@intel.com>
> >> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> > Cc: <linux-acpi@vger.kernel.org>
> >> > Cc: <linux-nvdimm@lists.01.org>
> >> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> >> > ---
> >> >  drivers/acpi/nfit.c       | 36 ++++++++++++++++++++++++++++++++----
> >> >  drivers/acpi/nfit.h       |  1 +
> >> >  drivers/nvdimm/core.c     | 17 +++++++++++++++++
> >> >  include/linux/libnvdimm.h |  1 +
> >> >  4 files changed, 51 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> >> > index ac6ddcc0..def9505 100644
> >> > --- a/drivers/acpi/nfit.c
> >> > +++ b/drivers/acpi/nfit.c
> >> > @@ -2138,8 +2138,9 @@ 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)
> >> > -               return;
> >> > +       if (!nfit_spa->ars_rescan)
> >> > +               if (nfit_spa->ars_done || !nfit_spa->nd_region)
> >> > +                       return;
> >>
> >> Do we need a new flag? Why not just clear ->ars_done?
> >
> > This is what I had started out with - clearing the done flag, but the
> > done flag gets set at the end of acpi_nfit_scrub if a region has been
> > registered for that SPA. In the rescan case, we'll almost always have
> > our regions registered, so the done flag will get set here, and
> > acpi_nfit_async_scrub won't look at it at all..
> 
> Maybe just flip the polarity of ->ars_done to ->ars_do?  Seems we
> should be able to get away with not adding a new flag.

Will it make a difference? We essentially use the do/done flag to decide
which SPAs need further ARS work, i.e. ars_nfit_async_scrub. When we're
running at init time, the SPAs that need to go into _async_scrub can be
a subset of all SPAs because for some of them, the scrub may be complete,
and we may have valid results. For the rescan case, _all_ SPAs need to be
_async_scrub 'ed.

Unless we change the initilization case to also start ARS for all SPAs
irrespective of whether we found good results or not, I'm not sure I see
how we can reuse the same flag?

[PATCH 3/3] nfit: do an ARS rescan on hitting a latent media error

[Vishal Verma]

When a latent (unknown to 'badblocks') error is encountered, it will
trigger a machine check exception. On a system with machine check
recovery, this will only SIGBUS the process(es) which had the bad page
mapped (as opposed to a kernel panic on platforms without machine
check recovery features). In the former case, we want to trigger a full
rescan of that nvdimm bus. This will allow any additional, new errors
to be captured in the block devices' badblocks lists, and offending
operations on them can be trapped early, avoiding machine checks.

This is done by registering a callback function with the
x86_mce_decoder_chain and calling the new ars_rescan functionality with
the address in the mce notificatiion.

Cc: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Tony Luck <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: <linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Cc: <linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Vishal Verma <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/acpi/nfit.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/nfit.h |   1 +
 2 files changed, 103 insertions(+)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index def9505..0d2d7a3 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -12,6 +12,7 @@
  */
 #include <linux/list_sort.h>
 #include <linux/libnvdimm.h>
+#include <linux/notifier.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/ndctl.h>
@@ -23,6 +24,7 @@
 #include <linux/io.h>
 #include <linux/nd.h>
 #include <asm/cacheflush.h>
+#include <asm/mce.h>
 #include "nfit.h"
 
 /*
@@ -50,6 +52,9 @@ module_param(disable_vendor_specific, bool, S_IRUGO);
 MODULE_PARM_DESC(disable_vendor_specific,
 		"Limit commands to the publicly specified set\n");
 
+static LIST_HEAD(acpi_descs);
+static DEFINE_MUTEX(acpi_desc_lock);
+
 static struct workqueue_struct *nfit_wq;
 
 struct nfit_table_prev {
@@ -2382,6 +2387,7 @@ static int acpi_nfit_check_deletions(struct acpi_nfit_desc *acpi_desc,
 
 int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
 {
+	struct acpi_nfit_desc *acpi_desc_entry;
 	struct device *dev = acpi_desc->dev;
 	struct nfit_table_prev prev;
 	const void *end;
@@ -2439,6 +2445,25 @@ int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
 
 	rc = acpi_nfit_register_regions(acpi_desc);
 
+	/*
+	 * We may get here due to an update of the nfit via _FIT.
+	 * Check if the acpi_desc we're (re)initializing is already
+	 * present in the list, and if so, don't re-add it
+	 */
+	mutex_lock(&acpi_desc_lock);
+	if (list_empty(&acpi_descs))
+		list_add_tail(&acpi_desc->list, &acpi_descs);
+	else {
+		int found = 0;
+
+		list_for_each_entry(acpi_desc_entry, &acpi_descs, list)
+			if (acpi_desc_entry == acpi_desc)
+				found = 1;
+		if (found == 0)
+			list_add_tail(&acpi_desc->list, &acpi_descs);
+	}
+	mutex_unlock(&acpi_desc_lock);
+
  out_unlock:
 	mutex_unlock(&acpi_desc->init_mutex);
 	return rc;
@@ -2522,6 +2547,69 @@ static int acpi_nfit_ars_rescan(struct nvdimm_bus_descriptor *nd_desc)
 	return 0;
 }
 
+static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
+			void *data)
+{
+	struct mce *mce = (struct mce *)data;
+	struct acpi_nfit_desc *acpi_desc;
+	struct nfit_spa *nfit_spa;
+
+	/* We only care about memory errors */
+	if (!(mce->status & MCACOD))
+		return NOTIFY_DONE;
+
+	/*
+	 * mce->addr contains the physical addr accessed that caused the
+	 * machine check. We need to walk through the list of NFITs, and see
+	 * if any of them matches that address, and only then start a scrub.
+	 */
+	mutex_lock(&acpi_desc_lock);
+	if (list_empty(&acpi_descs))
+		goto out;
+
+	list_for_each_entry(acpi_desc, &acpi_descs, list) {
+		struct device *dev = acpi_desc->dev;
+		int found_match = 0;
+
+		list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
+			struct acpi_nfit_system_address *spa = nfit_spa->spa;
+
+			if (nfit_spa_type(spa) != NFIT_SPA_PM)
+				continue;
+			/* find the spa that covers the mce addr */
+			if (spa->address > mce->addr)
+				continue;
+			if ((spa->address + spa->length - 1) < mce->addr)
+				continue;
+			found_match = 1;
+			dev_dbg(dev, "%s: addr in SPA %d (0x%llx, 0x%llx)\n",
+				__func__, spa->range_index, spa->address,
+				spa->length);
+			/*
+			 * We can break at the first match because we're going
+			 * to rescan all the SPA ranges. There shouldn't be any
+			 * aliasing anyway.
+			 */
+			break;
+		}
+
+		/*
+		 * We can ignore an -EBUSY here because if an ARS is already
+		 * in progress, just let that be the last authoritative one
+		 */
+		if (found_match)
+			acpi_nfit_ars_rescan(&acpi_desc->nd_desc);
+	}
+
+ out:
+	mutex_unlock(&acpi_desc_lock);
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block nfit_mce_dec = {
+	.notifier_call	= nfit_handle_mce,
+};
+
 void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev)
 {
 	struct nvdimm_bus_descriptor *nd_desc;
@@ -2616,6 +2704,9 @@ static int acpi_nfit_remove(struct acpi_device *adev)
 	acpi_desc->cancel = 1;
 	flush_workqueue(nfit_wq);
 	nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
+	mutex_lock(&acpi_desc_lock);
+	list_del(&acpi_desc->list);
+	mutex_unlock(&acpi_desc_lock);
 	return 0;
 }
 
@@ -2725,13 +2816,24 @@ static __init int nfit_init(void)
 	if (!nfit_wq)
 		return -ENOMEM;
 
+	INIT_LIST_HEAD(&acpi_descs);
+	mce_register_decode_chain(&nfit_mce_dec);
+
 	return acpi_bus_register_driver(&acpi_nfit_driver);
 }
 
 static __exit void nfit_exit(void)
 {
+	struct acpi_nfit_desc *acpi_desc, *next;
+
+	mce_unregister_decode_chain(&nfit_mce_dec);
 	acpi_bus_unregister_driver(&acpi_nfit_driver);
 	destroy_workqueue(nfit_wq);
+	mutex_lock(&acpi_desc_lock);
+	if (list_empty(&acpi_descs))
+		list_for_each_entry_safe(acpi_desc, next, &acpi_descs, list)
+			list_del(&acpi_desc->list);
+	mutex_unlock(&acpi_desc_lock);
 }
 
 module_init(nfit_init);
diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
index db95c5d..cf4d42d 100644
--- a/drivers/acpi/nfit.h
+++ b/drivers/acpi/nfit.h
@@ -147,6 +147,7 @@ struct acpi_nfit_desc {
 	struct nd_cmd_ars_status *ars_status;
 	size_t ars_status_size;
 	struct work_struct work;
+	struct list_head list;
 	unsigned int cancel:1;
 	unsigned long dimm_cmd_force_en;
 	unsigned long bus_cmd_force_en;
-- 
2.7.4

Re: [PATCH 3/3] nfit: do an ARS rescan on hitting a latent media error

[Dan Williams]

On Mon, Jul 18, 2016 at 5:45 PM, Vishal Verma <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> When a latent (unknown to 'badblocks') error is encountered, it will
> trigger a machine check exception. On a system with machine check
> recovery, this will only SIGBUS the process(es) which had the bad page
> mapped (as opposed to a kernel panic on platforms without machine
> check recovery features). In the former case, we want to trigger a full
> rescan of that nvdimm bus. This will allow any additional, new errors
> to be captured in the block devices' badblocks lists, and offending
> operations on them can be trapped early, avoiding machine checks.
>
> This is done by registering a callback function with the
> x86_mce_decoder_chain and calling the new ars_rescan functionality with
> the address in the mce notificatiion.
>
> Cc: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Tony Luck <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: <linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Cc: <linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Vishal Verma <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/acpi/nfit.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/acpi/nfit.h |   1 +
>  2 files changed, 103 insertions(+)
>
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index def9505..0d2d7a3 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -12,6 +12,7 @@
>   */
>  #include <linux/list_sort.h>
>  #include <linux/libnvdimm.h>
> +#include <linux/notifier.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/ndctl.h>
> @@ -23,6 +24,7 @@
>  #include <linux/io.h>
>  #include <linux/nd.h>
>  #include <asm/cacheflush.h>
> +#include <asm/mce.h>
>  #include "nfit.h"
>
>  /*
> @@ -50,6 +52,9 @@ module_param(disable_vendor_specific, bool, S_IRUGO);
>  MODULE_PARM_DESC(disable_vendor_specific,
>                 "Limit commands to the publicly specified set\n");
>
> +static LIST_HEAD(acpi_descs);
> +static DEFINE_MUTEX(acpi_desc_lock);
> +
>  static struct workqueue_struct *nfit_wq;
>
>  struct nfit_table_prev {
> @@ -2382,6 +2387,7 @@ static int acpi_nfit_check_deletions(struct acpi_nfit_desc *acpi_desc,
>
>  int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
>  {
> +       struct acpi_nfit_desc *acpi_desc_entry;
>         struct device *dev = acpi_desc->dev;
>         struct nfit_table_prev prev;
>         const void *end;
> @@ -2439,6 +2445,25 @@ int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
>
>         rc = acpi_nfit_register_regions(acpi_desc);
>
> +       /*
> +        * We may get here due to an update of the nfit via _FIT.
> +        * Check if the acpi_desc we're (re)initializing is already
> +        * present in the list, and if so, don't re-add it
> +        */
> +       mutex_lock(&acpi_desc_lock);
> +       if (list_empty(&acpi_descs))
> +               list_add_tail(&acpi_desc->list, &acpi_descs);

No need to special case list_empty(), it's covered below and this
isn't a fast path.

> +       else {
> +               int found = 0;
> +
> +               list_for_each_entry(acpi_desc_entry, &acpi_descs, list)
> +                       if (acpi_desc_entry == acpi_desc)
> +                               found = 1;
> +               if (found == 0)
> +                       list_add_tail(&acpi_desc->list, &acpi_descs);
> +       }
> +       mutex_unlock(&acpi_desc_lock);
> +
>   out_unlock:
>         mutex_unlock(&acpi_desc->init_mutex);
>         return rc;
> @@ -2522,6 +2547,69 @@ static int acpi_nfit_ars_rescan(struct nvdimm_bus_descriptor *nd_desc)
>         return 0;
>  }
>
> +static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
> +                       void *data)
> +{
> +       struct mce *mce = (struct mce *)data;
> +       struct acpi_nfit_desc *acpi_desc;
> +       struct nfit_spa *nfit_spa;
> +
> +       /* We only care about memory errors */
> +       if (!(mce->status & MCACOD))
> +               return NOTIFY_DONE;
> +
> +       /*
> +        * mce->addr contains the physical addr accessed that caused the
> +        * machine check. We need to walk through the list of NFITs, and see
> +        * if any of them matches that address, and only then start a scrub.
> +        */
> +       mutex_lock(&acpi_desc_lock);
> +       if (list_empty(&acpi_descs))
> +               goto out;

Again, no need to check for empty, list_for_each_entry() already does that...

> +
> +       list_for_each_entry(acpi_desc, &acpi_descs, list) {
> +               struct device *dev = acpi_desc->dev;
> +               int found_match = 0;
> +
> +               list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
> +                       struct acpi_nfit_system_address *spa = nfit_spa->spa;
> +
> +                       if (nfit_spa_type(spa) != NFIT_SPA_PM)
> +                               continue;
> +                       /* find the spa that covers the mce addr */
> +                       if (spa->address > mce->addr)
> +                               continue;
> +                       if ((spa->address + spa->length - 1) < mce->addr)
> +                               continue;
> +                       found_match = 1;
> +                       dev_dbg(dev, "%s: addr in SPA %d (0x%llx, 0x%llx)\n",
> +                               __func__, spa->range_index, spa->address,
> +                               spa->length);
> +                       /*
> +                        * We can break at the first match because we're going
> +                        * to rescan all the SPA ranges. There shouldn't be any
> +                        * aliasing anyway.
> +                        */
> +                       break;
> +               }
> +
> +               /*
> +                * We can ignore an -EBUSY here because if an ARS is already
> +                * in progress, just let that be the last authoritative one
> +                */
> +               if (found_match)
> +                       acpi_nfit_ars_rescan(&acpi_desc->nd_desc);
> +       }
> +
> + out:
> +       mutex_unlock(&acpi_desc_lock);
> +       return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block nfit_mce_dec = {
> +       .notifier_call  = nfit_handle_mce,
> +};
> +
>  void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev)
>  {
>         struct nvdimm_bus_descriptor *nd_desc;
> @@ -2616,6 +2704,9 @@ static int acpi_nfit_remove(struct acpi_device *adev)
>         acpi_desc->cancel = 1;
>         flush_workqueue(nfit_wq);
>         nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
> +       mutex_lock(&acpi_desc_lock);
> +       list_del(&acpi_desc->list);
> +       mutex_unlock(&acpi_desc_lock);
>         return 0;
>  }
>
> @@ -2725,13 +2816,24 @@ static __init int nfit_init(void)
>         if (!nfit_wq)
>                 return -ENOMEM;
>
> +       INIT_LIST_HEAD(&acpi_descs);
> +       mce_register_decode_chain(&nfit_mce_dec);
> +
>         return acpi_bus_register_driver(&acpi_nfit_driver);
>  }
>
>  static __exit void nfit_exit(void)
>  {
> +       struct acpi_nfit_desc *acpi_desc, *next;
> +
> +       mce_unregister_decode_chain(&nfit_mce_dec);
>         acpi_bus_unregister_driver(&acpi_nfit_driver);
>         destroy_workqueue(nfit_wq);
> +       mutex_lock(&acpi_desc_lock);
> +       if (list_empty(&acpi_descs))
> +               list_for_each_entry_safe(acpi_desc, next, &acpi_descs, list)
> +                       list_del(&acpi_desc->list);

We should WARN here, since there should be no way, outside of a bug,
that 'acpi_descs' is still populated after
acpi_bus_unregister_driver().

> +       mutex_unlock(&acpi_desc_lock);
>  }
>
>  module_init(nfit_init);
> diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
> index db95c5d..cf4d42d 100644
> --- a/drivers/acpi/nfit.h
> +++ b/drivers/acpi/nfit.h
> @@ -147,6 +147,7 @@ struct acpi_nfit_desc {
>         struct nd_cmd_ars_status *ars_status;
>         size_t ars_status_size;
>         struct work_struct work;
> +       struct list_head list;
>         unsigned int cancel:1;
>         unsigned long dimm_cmd_force_en;
>         unsigned long bus_cmd_force_en;

Outside of the minor comments above, this looks good to me.

Re: [PATCH 3/3] nfit: do an ARS rescan on hitting a latent media error

[Vishal Verma]

On 07/19, Dan Williams wrote:
> On Mon, Jul 18, 2016 at 5:45 PM, Vishal Verma <vishal.l.verma@intel.com> wrote:
> > When a latent (unknown to 'badblocks') error is encountered, it will
> > trigger a machine check exception. On a system with machine check
> > recovery, this will only SIGBUS the process(es) which had the bad page
> > mapped (as opposed to a kernel panic on platforms without machine
> > check recovery features). In the former case, we want to trigger a full
> > rescan of that nvdimm bus. This will allow any additional, new errors
> > to be captured in the block devices' badblocks lists, and offending
> > operations on them can be trapped early, avoiding machine checks.
> >
> > This is done by registering a callback function with the
> > x86_mce_decoder_chain and calling the new ars_rescan functionality with
> > the address in the mce notificatiion.
> >
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: <linux-acpi@vger.kernel.org>
> > Cc: <linux-nvdimm@lists.01.org>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  drivers/acpi/nfit.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/acpi/nfit.h |   1 +
> >  2 files changed, 103 insertions(+)
> >
> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> > index def9505..0d2d7a3 100644
> > --- a/drivers/acpi/nfit.c
> > +++ b/drivers/acpi/nfit.c
> > @@ -12,6 +12,7 @@
> >   */
> >  #include <linux/list_sort.h>
> >  #include <linux/libnvdimm.h>
> > +#include <linux/notifier.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> >  #include <linux/ndctl.h>
> > @@ -23,6 +24,7 @@
> >  #include <linux/io.h>
> >  #include <linux/nd.h>
> >  #include <asm/cacheflush.h>
> > +#include <asm/mce.h>
> >  #include "nfit.h"
> >
> >  /*
> > @@ -50,6 +52,9 @@ module_param(disable_vendor_specific, bool, S_IRUGO);
> >  MODULE_PARM_DESC(disable_vendor_specific,
> >                 "Limit commands to the publicly specified set\n");
> >
> > +static LIST_HEAD(acpi_descs);
> > +static DEFINE_MUTEX(acpi_desc_lock);
> > +
> >  static struct workqueue_struct *nfit_wq;
> >
> >  struct nfit_table_prev {
> > @@ -2382,6 +2387,7 @@ static int acpi_nfit_check_deletions(struct acpi_nfit_desc *acpi_desc,
> >
> >  int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
> >  {
> > +       struct acpi_nfit_desc *acpi_desc_entry;
> >         struct device *dev = acpi_desc->dev;
> >         struct nfit_table_prev prev;
> >         const void *end;
> > @@ -2439,6 +2445,25 @@ int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
> >
> >         rc = acpi_nfit_register_regions(acpi_desc);
> >
> > +       /*
> > +        * We may get here due to an update of the nfit via _FIT.
> > +        * Check if the acpi_desc we're (re)initializing is already
> > +        * present in the list, and if so, don't re-add it
> > +        */
> > +       mutex_lock(&acpi_desc_lock);
> > +       if (list_empty(&acpi_descs))
> > +               list_add_tail(&acpi_desc->list, &acpi_descs);
> 
> No need to special case list_empty(), it's covered below and this
> isn't a fast path.
> 
> > +       else {
> > +               int found = 0;
> > +
> > +               list_for_each_entry(acpi_desc_entry, &acpi_descs, list)
> > +                       if (acpi_desc_entry == acpi_desc)
> > +                               found = 1;
> > +               if (found == 0)
> > +                       list_add_tail(&acpi_desc->list, &acpi_descs);
> > +       }
> > +       mutex_unlock(&acpi_desc_lock);
> > +
> >   out_unlock:
> >         mutex_unlock(&acpi_desc->init_mutex);
> >         return rc;
> > @@ -2522,6 +2547,69 @@ static int acpi_nfit_ars_rescan(struct nvdimm_bus_descriptor *nd_desc)
> >         return 0;
> >  }
> >
> > +static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
> > +                       void *data)
> > +{
> > +       struct mce *mce = (struct mce *)data;
> > +       struct acpi_nfit_desc *acpi_desc;
> > +       struct nfit_spa *nfit_spa;
> > +
> > +       /* We only care about memory errors */
> > +       if (!(mce->status & MCACOD))
> > +               return NOTIFY_DONE;
> > +
> > +       /*
> > +        * mce->addr contains the physical addr accessed that caused the
> > +        * machine check. We need to walk through the list of NFITs, and see
> > +        * if any of them matches that address, and only then start a scrub.
> > +        */
> > +       mutex_lock(&acpi_desc_lock);
> > +       if (list_empty(&acpi_descs))
> > +               goto out;
> 
> Again, no need to check for empty, list_for_each_entry() already does that...
> 
> > +
> > +       list_for_each_entry(acpi_desc, &acpi_descs, list) {
> > +               struct device *dev = acpi_desc->dev;
> > +               int found_match = 0;
> > +
> > +               list_for_each_entry(nfit_spa, &acpi_desc->spas, list) {
> > +                       struct acpi_nfit_system_address *spa = nfit_spa->spa;
> > +
> > +                       if (nfit_spa_type(spa) != NFIT_SPA_PM)
> > +                               continue;
> > +                       /* find the spa that covers the mce addr */
> > +                       if (spa->address > mce->addr)
> > +                               continue;
> > +                       if ((spa->address + spa->length - 1) < mce->addr)
> > +                               continue;
> > +                       found_match = 1;
> > +                       dev_dbg(dev, "%s: addr in SPA %d (0x%llx, 0x%llx)\n",
> > +                               __func__, spa->range_index, spa->address,
> > +                               spa->length);
> > +                       /*
> > +                        * We can break at the first match because we're going
> > +                        * to rescan all the SPA ranges. There shouldn't be any
> > +                        * aliasing anyway.
> > +                        */
> > +                       break;
> > +               }
> > +
> > +               /*
> > +                * We can ignore an -EBUSY here because if an ARS is already
> > +                * in progress, just let that be the last authoritative one
> > +                */
> > +               if (found_match)
> > +                       acpi_nfit_ars_rescan(&acpi_desc->nd_desc);
> > +       }
> > +
> > + out:
> > +       mutex_unlock(&acpi_desc_lock);
> > +       return NOTIFY_DONE;
> > +}
> > +
> > +static struct notifier_block nfit_mce_dec = {
> > +       .notifier_call  = nfit_handle_mce,
> > +};
> > +
> >  void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev)
> >  {
> >         struct nvdimm_bus_descriptor *nd_desc;
> > @@ -2616,6 +2704,9 @@ static int acpi_nfit_remove(struct acpi_device *adev)
> >         acpi_desc->cancel = 1;
> >         flush_workqueue(nfit_wq);
> >         nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
> > +       mutex_lock(&acpi_desc_lock);
> > +       list_del(&acpi_desc->list);
> > +       mutex_unlock(&acpi_desc_lock);
> >         return 0;
> >  }
> >
> > @@ -2725,13 +2816,24 @@ static __init int nfit_init(void)
> >         if (!nfit_wq)
> >                 return -ENOMEM;
> >
> > +       INIT_LIST_HEAD(&acpi_descs);
> > +       mce_register_decode_chain(&nfit_mce_dec);
> > +
> >         return acpi_bus_register_driver(&acpi_nfit_driver);
> >  }
> >
> >  static __exit void nfit_exit(void)
> >  {
> > +       struct acpi_nfit_desc *acpi_desc, *next;
> > +
> > +       mce_unregister_decode_chain(&nfit_mce_dec);
> >         acpi_bus_unregister_driver(&acpi_nfit_driver);
> >         destroy_workqueue(nfit_wq);
> > +       mutex_lock(&acpi_desc_lock);
> > +       if (list_empty(&acpi_descs))
> > +               list_for_each_entry_safe(acpi_desc, next, &acpi_descs, list)
> > +                       list_del(&acpi_desc->list);
> 
> We should WARN here, since there should be no way, outside of a bug,
> that 'acpi_descs' is still populated after
> acpi_bus_unregister_driver().

Agreed, also just spotted another bug - it should've been
if (!list_empty()) ...

> 
> > +       mutex_unlock(&acpi_desc_lock);
> >  }
> >
> >  module_init(nfit_init);
> > diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
> > index db95c5d..cf4d42d 100644
> > --- a/drivers/acpi/nfit.h
> > +++ b/drivers/acpi/nfit.h
> > @@ -147,6 +147,7 @@ struct acpi_nfit_desc {
> >         struct nd_cmd_ars_status *ars_status;
> >         size_t ars_status_size;
> >         struct work_struct work;
> > +       struct list_head list;
> >         unsigned int cancel:1;
> >         unsigned long dimm_cmd_force_en;
> >         unsigned long bus_cmd_force_en;
> 
> Outside of the minor comments above, this looks good to me.

Ok, I'll fix these up and resend. Thanks!