[PATCH] Add support of NVDIMM memory error notification in ACPI 6.2

[PATCH] Add support of NVDIMM memory error notification in ACPI 6.2

[Toshi Kani]

ACPI 6.2 defines a new ACPI notification value to NVDIMM Root Device
in Table 5-169.

 0x81 Unconsumed Uncorrectable Memory Error Detected
      Used to pro-actively notify OSPM of uncorrectable memory errors
      detected (for example a memory scrubbing engine that continuously
      scans the NVDIMMs memory). This is an optional notification. Only
      locations that were mapped in to SPA by the platform will generate
      a notification.

Add support of this notification value by initiating an ARS scan. This
will find new error locations and add their badblocks information.

Link: http://www.uefi.org/sites/default/files/resources/ACPI_6_2.pdf
Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/acpi/nfit/core.c |   28 ++++++++++++++++++++++------
 drivers/acpi/nfit/nfit.h |    1 +
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 656acb5..cc22778 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -2967,7 +2967,7 @@ static int acpi_nfit_remove(struct acpi_device *adev)
 	return 0;
 }
 
-void __acpi_nfit_notify(struct device *dev, acpi_handle handle, u32 event)
+static void acpi_nfit_update_notify(struct device *dev, acpi_handle handle)
 {
 	struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(dev);
 	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -2975,11 +2975,6 @@ void __acpi_nfit_notify(struct device *dev, acpi_handle handle, u32 event)
 	acpi_status status;
 	int ret;
 
-	dev_dbg(dev, "%s: event: %d\n", __func__, event);
-
-	if (event != NFIT_NOTIFY_UPDATE)
-		return;
-
 	if (!dev->driver) {
 		/* dev->driver may be null if we're being removed */
 		dev_dbg(dev, "%s: no driver found for dev\n", __func__);
@@ -3016,6 +3011,27 @@ void __acpi_nfit_notify(struct device *dev, acpi_handle handle, u32 event)
 		dev_err(dev, "Invalid _FIT\n");
 	kfree(buf.pointer);
 }
+
+static void acpi_nfit_uc_error_notify(struct device *dev, acpi_handle handle)
+{
+	struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(dev);
+
+	acpi_nfit_ars_rescan(acpi_desc);
+}
+
+void __acpi_nfit_notify(struct device *dev, acpi_handle handle, u32 event)
+{
+	dev_dbg(dev, "%s: event: 0x%x\n", __func__, event);
+
+	switch (event) {
+	case NFIT_NOTIFY_UPDATE:
+		return acpi_nfit_update_notify(dev, handle);
+	case NFIT_NOTIFY_UC_MEMORY_ERROR:
+		return acpi_nfit_uc_error_notify(dev, handle);
+	default:
+		return;
+	}
+}
 EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
 
 static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index 58fb7d6..6cf9d21 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -80,6 +80,7 @@ enum {
 
 enum nfit_root_notifiers {
 	NFIT_NOTIFY_UPDATE = 0x80,
+	NFIT_NOTIFY_UC_MEMORY_ERROR = 0x81,
 };
 
 enum nfit_dimm_notifiers {

Re: [PATCH] Add support of NVDIMM memory error notification in ACPI 6.2

[Dan Williams]

On Wed, Jun 7, 2017 at 11:49 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
> ACPI 6.2 defines a new ACPI notification value to NVDIMM Root Device
> in Table 5-169.
>
>  0x81 Unconsumed Uncorrectable Memory Error Detected
>       Used to pro-actively notify OSPM of uncorrectable memory errors
>       detected (for example a memory scrubbing engine that continuously
>       scans the NVDIMMs memory). This is an optional notification. Only
>       locations that were mapped in to SPA by the platform will generate
>       a notification.
>
> Add support of this notification value by initiating an ARS scan. This
> will find new error locations and add their badblocks information.
>
> Link: http://www.uefi.org/sites/default/files/resources/ACPI_6_2.pdf
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  drivers/acpi/nfit/core.c |   28 ++++++++++++++++++++++------
>  drivers/acpi/nfit/nfit.h |    1 +
>  2 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 656acb5..cc22778 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -2967,7 +2967,7 @@ static int acpi_nfit_remove(struct acpi_device *adev)
>         return 0;
>  }
>
> -void __acpi_nfit_notify(struct device *dev, acpi_handle handle, u32 event)
> +static void acpi_nfit_update_notify(struct device *dev, acpi_handle handle)
>  {
>         struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(dev);
>         struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> @@ -2975,11 +2975,6 @@ void __acpi_nfit_notify(struct device *dev, acpi_handle handle, u32 event)
>         acpi_status status;
>         int ret;
>
> -       dev_dbg(dev, "%s: event: %d\n", __func__, event);
> -
> -       if (event != NFIT_NOTIFY_UPDATE)
> -               return;
> -
>         if (!dev->driver) {
>                 /* dev->driver may be null if we're being removed */
>                 dev_dbg(dev, "%s: no driver found for dev\n", __func__);
> @@ -3016,6 +3011,27 @@ void __acpi_nfit_notify(struct device *dev, acpi_handle handle, u32 event)
>                 dev_err(dev, "Invalid _FIT\n");
>         kfree(buf.pointer);
>  }
> +
> +static void acpi_nfit_uc_error_notify(struct device *dev, acpi_handle handle)
> +{
> +       struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(dev);
> +
> +       acpi_nfit_ars_rescan(acpi_desc);

I wonder if we should gate re-scanning with a similar:

    if (acpi_desc->scrub_mode == HW_ERROR_SCRUB_ON)

...check that we do in the mce notification case? Maybe not since we
don't get an indication of where the error is without a rescan.
However, at a minimum I think we need support for the new Start ARS
flag ("If set to 1 the firmware shall return data from a previous
scrub, if any, without starting a new scrub") and use that for this
case.

Another thing that seems to be missing in both this and the mce case
is a notification to userspace that something changed. We have calls
to sysfs_notify_dirent() to notify scrub completion events and DIMM
health status change events, I think we need a similar notifier
mechanism for new un-correctable errors.

Re: [PATCH] Add support of NVDIMM memory error notification in ACPI 6.2

[Kani, Toshimitsu]

On Wed, 2017-06-07 at 12:09 -0700, Dan Williams wrote:
> On Wed, Jun 7, 2017 at 11:49 AM, Toshi Kani <toshi.kani@hpe.com>
> wrote:
 :
> > +
> > +static void acpi_nfit_uc_error_notify(struct device *dev,
> > acpi_handle handle)
> > +{
> > +       struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(dev);
> > +
> > +       acpi_nfit_ars_rescan(acpi_desc);
> 
> I wonder if we should gate re-scanning with a similar:
> 
>     if (acpi_desc->scrub_mode == HW_ERROR_SCRUB_ON)
> 
> ...check that we do in the mce notification case? Maybe not since we
> don't get an indication of where the error is without a rescan.

I think this mce case is different since the MCE handler already knows
where the new poison location is and can update badblocks information
for it.  Starting ARS is an optional precaution.

> However, at a minimum I think we need support for the new Start ARS
> flag ("If set to 1 the firmware shall return data from a previous
> scrub, if any, without starting a new scrub") and use that for this
> case.

That's an interesting idea.  But I wonder how users know if it is OK to
set this flag as it relies on BIOS implementation that is not described
in ACPI...

> Another thing that seems to be missing in both this and the mce case
> is a notification to userspace that something changed. We have calls
> to sysfs_notify_dirent() to notify scrub completion events and DIMM
> health status change events, I think we need a similar notifier
> mechanism for new un-correctable errors.

Good point.  I think this can be a badblocks population event, which
gets generated when badblocks information is updated at boot-time and
run-time via this notification and MCE.

Thanks,
-Toshi

Re: [PATCH] Add support of NVDIMM memory error notification in ACPI 6.2

[Dan Williams]

On Wed, Jun 7, 2017 at 1:57 PM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote:
> On Wed, 2017-06-07 at 12:09 -0700, Dan Williams wrote:
>> On Wed, Jun 7, 2017 at 11:49 AM, Toshi Kani <toshi.kani@hpe.com>
>> wrote:
>  :
>> > +
>> > +static void acpi_nfit_uc_error_notify(struct device *dev,
>> > acpi_handle handle)
>> > +{
>> > +       struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(dev);
>> > +
>> > +       acpi_nfit_ars_rescan(acpi_desc);
>>
>> I wonder if we should gate re-scanning with a similar:
>>
>>     if (acpi_desc->scrub_mode == HW_ERROR_SCRUB_ON)
>>
>> ...check that we do in the mce notification case? Maybe not since we
>> don't get an indication of where the error is without a rescan.
>
> I think this mce case is different since the MCE handler already knows
> where the new poison location is and can update badblocks information
> for it.  Starting ARS is an optional precaution.
>
>> However, at a minimum I think we need support for the new Start ARS
>> flag ("If set to 1 the firmware shall return data from a previous
>> scrub, if any, without starting a new scrub") and use that for this
>> case.
>
> That's an interesting idea.  But I wonder how users know if it is OK to
> set this flag as it relies on BIOS implementation that is not described
> in ACPI...

Ugh, you're right. We might need a revision-id=2 version of Start ARS
so software knows that the BIOS is aware of the new flag.

Re: [PATCH] Add support of NVDIMM memory error notification in ACPI 6.2

[Kani, Toshimitsu]

On Wed, 2017-06-07 at 14:06 -0700, Dan Williams wrote:
> On Wed, Jun 7, 2017 at 1:57 PM, Kani, Toshimitsu <toshi.kani@hpe.com>
> wrote:
> > On Wed, 2017-06-07 at 12:09 -0700, Dan Williams wrote:
> > > On Wed, Jun 7, 2017 at 11:49 AM, Toshi Kani <toshi.kani@hpe.com>
> > > wrote:
> > 
> >  :
> > > > +
> > > > +static void acpi_nfit_uc_error_notify(struct device *dev,
> > > > acpi_handle handle)
> > > > +{
> > > > +       struct acpi_nfit_desc *acpi_desc =
> > > > dev_get_drvdata(dev);
> > > > +
> > > > +       acpi_nfit_ars_rescan(acpi_desc);
> > > 
> > > I wonder if we should gate re-scanning with a similar:
> > > 
> > >     if (acpi_desc->scrub_mode == HW_ERROR_SCRUB_ON)
> > > 
> > > ...check that we do in the mce notification case? Maybe not since
> > > we
> > > don't get an indication of where the error is without a rescan.
> > 
> > I think this mce case is different since the MCE handler already
> > knows where the new poison location is and can update badblocks
> > information for it.  Starting ARS is an optional precaution.
> > 
> > > However, at a minimum I think we need support for the new Start
> > > ARS flag ("If set to 1 the firmware shall return data from a
> > > previous scrub, if any, without starting a new scrub") and use
> > > that for this case.
> > 
> > That's an interesting idea.  But I wonder how users know if it is
> > OK to set this flag as it relies on BIOS implementation that is not
> > described in ACPI...
> 
> Ugh, you're right. We might need a revision-id=2 version of Start ARS
> so software knows that the BIOS is aware of the new flag.

My bad.  Looking at ACPI 6.2, it actually defines what you described. 
Start ARS now defines bit[1] in Flags which can be set to avoid
scanning for this notification.  I will update the patch to set this
flag when HW_ERROR_SCRUB_ON is not set.

Thanks,
-Toshi

Re: [PATCH] Add support of NVDIMM memory error notification in ACPI 6.2

[Linda Knippers]

On 6/7/2017 5:33 PM, Kani, Toshimitsu wrote:
> On Wed, 2017-06-07 at 14:06 -0700, Dan Williams wrote:
>> On Wed, Jun 7, 2017 at 1:57 PM, Kani, Toshimitsu <toshi.kani@hpe.com>
>> wrote:
>>> On Wed, 2017-06-07 at 12:09 -0700, Dan Williams wrote:
>>>> On Wed, Jun 7, 2017 at 11:49 AM, Toshi Kani <toshi.kani@hpe.com>
>>>> wrote:
>>>
>>>  :
>>>>> +
>>>>> +static void acpi_nfit_uc_error_notify(struct device *dev,
>>>>> acpi_handle handle)
>>>>> +{
>>>>> +       struct acpi_nfit_desc *acpi_desc =
>>>>> dev_get_drvdata(dev);
>>>>> +
>>>>> +       acpi_nfit_ars_rescan(acpi_desc);
>>>>
>>>> I wonder if we should gate re-scanning with a similar:
>>>>
>>>>     if (acpi_desc->scrub_mode == HW_ERROR_SCRUB_ON)
>>>>
>>>> ...check that we do in the mce notification case? Maybe not since
>>>> we
>>>> don't get an indication of where the error is without a rescan.
>>>
>>> I think this mce case is different since the MCE handler already
>>> knows where the new poison location is and can update badblocks
>>> information for it.  Starting ARS is an optional precaution.
>>>
>>>> However, at a minimum I think we need support for the new Start
>>>> ARS flag ("If set to 1 the firmware shall return data from a
>>>> previous scrub, if any, without starting a new scrub") and use
>>>> that for this case.
>>>
>>> That's an interesting idea.  But I wonder how users know if it is
>>> OK to set this flag as it relies on BIOS implementation that is not
>>> described in ACPI...
>>
>> Ugh, you're right. We might need a revision-id=2 version of Start ARS
>> so software knows that the BIOS is aware of the new flag.
>
> My bad.  Looking at ACPI 6.2, it actually defines what you described.
> Start ARS now defines bit[1] in Flags which can be set to avoid
> scanning for this notification.  I will update the patch to set this
> flag when HW_ERROR_SCRUB_ON is not set.

Wasn't Dan concerned about how the OS can know whether the FW supports
that bit in the Start ARS?

The Query ARS Capabilities DSM has a bit that tells the OS whether the
platform supports the notification and the point of the notification was
to tell the OS it could do a Start ARS with bit 1 set.  Of course, if
you get the notification then that means the platform has the capability
to deliver it, but it might not hurt to check the flag from the Query
Capabilities bit.

If the spec isn't clear about the relationship between the bits, we could
clarify that in the spec without bumping the revision number for the call.

We could also check to see what a platform does if it gets a flag it
doesn't know about.  Would that be an Invalid Input Parameter?

-- ljk
>
> Thanks,
> -Toshi\x13��칻\x1c�&�~�&�\x18��+-��ݶ\x17��w��˛���m�b��Zr����^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階�ݢj"��\x1a�^[m�����z�ޖ���f���h���~�m�
>

Re: [PATCH] Add support of NVDIMM memory error notification in ACPI 6.2

[Dan Williams]

On Thu, Jun 8, 2017 at 10:30 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
[..]
> Wasn't Dan concerned about how the OS can know whether the FW supports
> that bit in the Start ARS?
>
> The Query ARS Capabilities DSM has a bit that tells the OS whether the
> platform supports the notification and the point of the notification was
> to tell the OS it could do a Start ARS with bit 1 set.  Of course, if
> you get the notification then that means the platform has the capability
> to deliver it, but it might not hurt to check the flag from the Query
> Capabilities bit.

Good point, yes, I think it is safe to assume that a BIOS that claims
to support un-correctable error notification also supports this Start
ARS flag.

Re: [PATCH] Add support of NVDIMM memory error notification in ACPI 6.2

[Kani, Toshimitsu]

On Thu, 2017-06-08 at 10:34 -0700, Dan Williams wrote:
> On Thu, Jun 8, 2017 at 10:30 AM, Linda Knippers <linda.knippers@hpe.c
> om> wrote:
> [..]
> > Wasn't Dan concerned about how the OS can know whether the FW
> > supports that bit in the Start ARS?
> > 
> > The Query ARS Capabilities DSM has a bit that tells the OS whether
> > the platform supports the notification and the point of the
> > notification was to tell the OS it could do a Start ARS with bit 1
> > set.  Of course, if you get the notification then that means the
> > platform has the capability to deliver it, but it might not hurt to
> > check the flag from the Query Capabilities bit.
> 
> Good point, yes, I think it is safe to assume that a BIOS that claims
> to support un-correctable error notification also supports this Start
> ARS flag.

Yes, ACPI 6.2, section 9.20.7.2, defines that:

      Upon receiving the notification, the OSPM may decide to issue
      a Start ARS with Flags Bit [1] set to prepare for the retrieval
      of existing records and issue the Query ARS Status function to
      retrieve the records.

So, I believe it is safe to assume that BIOS supporting 0x81 also
supports flags Big [1].  Sorry, this is what I should have said in my
previous email...

Thanks,
-Toshi

Re: [PATCH] Add support of NVDIMM memory error notification in ACPI 6.2

[Kani, Toshimitsu]

On Thu, 2017-06-08 at 11:37 -0600, Toshi Kani wrote:
> On Thu, 2017-06-08 at 10:34 -0700, Dan Williams wrote:
> > On Thu, Jun 8, 2017 at 10:30 AM, Linda Knippers <linda.knippers@hpe
> > .c
> > om> wrote:
> > [..]
> > > Wasn't Dan concerned about how the OS can know whether the FW
> > > supports that bit in the Start ARS?
> > > 
> > > The Query ARS Capabilities DSM has a bit that tells the OS
> > > whether the platform supports the notification and the point of
> > > the notification was to tell the OS it could do a Start ARS with
> > > bit 1 set.  Of course, if you get the notification then that
> > > means the platform has the capability to deliver it, but it might
> > > not hurt to check the flag from the Query Capabilities bit.
> > 
> > Good point, yes, I think it is safe to assume that a BIOS that
> > claims to support un-correctable error notification also supports
> > this Start ARS flag.
> 
> Yes, ACPI 6.2, section 9.20.7.2, defines that:
> 
>       Upon receiving the notification, the OSPM may decide to issue
>       a Start ARS with Flags Bit [1] set to prepare for the retrieval
>       of existing records and issue the Query ARS Status function to
>       retrieve the records.
> 
> So, I believe it is safe to assume that BIOS supporting 0x81 also
> supports flags Bit [1].  Sorry, this is what I should have said in my
> previous email...

To reiterate my thinking, I believe the statement above clarifies that
the OS can assume BIOS support of Flags Bit[1] upon receiving a 0x81
notification.  Since BIOS may also support Flags Bit[1] without
supporting this 0x81 (in which case I do not know how to detect it, but
BIOS should simply ignore this bit when not supporting it), I am not
going to add a check/restriction that 0x81 support is necessary to set
Bit[1] in the scan function.

Thanks,
-Toshi