RE: [PATCH v2 2/2] acpi/nfit: Issue Start ARS to retrieve existing records
> On Fri, Jun 16, 2017 at 5:01 PM, Kani, Toshimitsu wrote:
> >> > --- a/drivers/acpi/nfit/core.c
> >> > +++ b/drivers/acpi/nfit/core.c
> >> > @@ -1031,7 +1031,7 @@ static ssize_t scrub_store(struct device *dev,
> >> > if (nd_desc) {
> >> > struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
> >> >
> >> > - rc = acpi_nfit_ars_rescan(acpi_desc);
> >> > + rc = acpi_nfit_ars_rescan(acpi_desc, 0);
> >> > }
> >> > device_unlock(dev);
> >> > if (rc)
> >> > @@ -2057,6 +2057,10 @@ static int ars_start(struct acpi_nfit_desc
> >> *acpi_desc, struct nfit_spa *nfit_spa
> >> > ars_start.type = ND_ARS_VOLATILE;
> >> > else
> >> > return -ENOTTY;
> >> > + if (nfit_spa->ars_prev_data) {
> >> > + ars_start.flags |= ND_ARS_RETURN_PREV_DATA;
> >> > + nfit_spa->ars_prev_data = 0;
> >> > + }
> >>
> >> I'd rather you plumb a new 'flags' parameter all the way through from
> >> acpi_nfit_ars_rescan() to ars_start() rather than carrying this as a
> >> property of nfit_spa.
> >
> > Yes, I wanted to carry 'flags' all the way, but since acpi_nfit_ars_rescan()
> > calls acpi_nfit_scrub() via acpi_desc->work, all info needs to be marshalled
> > into struct acpi_nfit_desc. Using nfit_spa allows a request to be carried
> > as
> > per-spa basis...
>
> Ah ok, but I still think it does not belong to a spa. This is control
> / context information for the workqueue and that belongs with
> acpi_nfit_desc. It think it's fine if all spas get re-scrubbed with
> the "prev_data" flag in the the notification case.
Sounds good. I will add the flags info to acpi_nfit_desc and update
this patch.
Thanks!
-Toshi
Re: [PATCH v2 2/2] acpi/nfit: Issue Start ARS to retrieve existing records
On Fri, Jun 16, 2017 at 5:01 PM, Kani, Toshimitsu wrote:
>> > --- a/drivers/acpi/nfit/core.c
>> > +++ b/drivers/acpi/nfit/core.c
>> > @@ -1031,7 +1031,7 @@ static ssize_t scrub_store(struct device *dev,
>> > if (nd_desc) {
>> > struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
>> >
>> > - rc = acpi_nfit_ars_rescan(acpi_desc);
>> > + rc = acpi_nfit_ars_rescan(acpi_desc, 0);
>> > }
>> > device_unlock(dev);
>> > if (rc)
>> > @@ -2057,6 +2057,10 @@ static int ars_start(struct acpi_nfit_desc
>> *acpi_desc, struct nfit_spa *nfit_spa
>> > ars_start.type = ND_ARS_VOLATILE;
>> > else
>> > return -ENOTTY;
>> > + if (nfit_spa->ars_prev_data) {
>> > + ars_start.flags |= ND_ARS_RETURN_PREV_DATA;
>> > + nfit_spa->ars_prev_data = 0;
>> > + }
>>
>> I'd rather you plumb a new 'flags' parameter all the way through from
>> acpi_nfit_ars_rescan() to ars_start() rather than carrying this as a
>> property of nfit_spa.
>
> Yes, I wanted to carry 'flags' all the way, but since acpi_nfit_ars_rescan()
> calls acpi_nfit_scrub() via acpi_desc->work, all info needs to be marshalled
> into struct acpi_nfit_desc. Using nfit_spa allows a request to be carried as
> per-spa basis...
Ah ok, but I still think it does not belong to a spa. This is control
/ context information for the workqueue and that belongs with
acpi_nfit_desc. It think it's fine if all spas get re-scrubbed with
the "prev_data" flag in the the notification case.
RE: [PATCH v2 2/2] acpi/nfit: Issue Start ARS to retrieve existing records
> > --- a/drivers/acpi/nfit/core.c
> > +++ b/drivers/acpi/nfit/core.c
> > @@ -1031,7 +1031,7 @@ static ssize_t scrub_store(struct device *dev,
> > if (nd_desc) {
> > struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
> >
> > - rc = acpi_nfit_ars_rescan(acpi_desc);
> > + rc = acpi_nfit_ars_rescan(acpi_desc, 0);
> > }
> > device_unlock(dev);
> > if (rc)
> > @@ -2057,6 +2057,10 @@ static int ars_start(struct acpi_nfit_desc
> *acpi_desc, struct nfit_spa *nfit_spa
> > ars_start.type = ND_ARS_VOLATILE;
> > else
> > return -ENOTTY;
> > + if (nfit_spa->ars_prev_data) {
> > + ars_start.flags |= ND_ARS_RETURN_PREV_DATA;
> > + nfit_spa->ars_prev_data = 0;
> > + }
>
> I'd rather you plumb a new 'flags' parameter all the way through from
> acpi_nfit_ars_rescan() to ars_start() rather than carrying this as a
> property of nfit_spa.
Yes, I wanted to carry 'flags' all the way, but since acpi_nfit_ars_rescan()
calls acpi_nfit_scrub() via acpi_desc->work, all info needs to be marshalled
into struct acpi_nfit_desc. Using nfit_spa allows a request to be carried as
per-spa basis...
Thanks,
-Toshi
Re: [PATCH v2 2/2] acpi/nfit: Issue Start ARS to retrieve existing records
Thanks for this. I think it's going in the right direction, but one
comment below.
On Thu, Jun 8, 2017 at 11:36 AM, Toshi Kani wrote:
> ACPI 6.2 defines in section 9.20.7.2 that the OSPM may call a Start
> ARS with Flags Bit [1] set upon receiving the 0x81 notification.
>
> 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.
>
> Add support to call a Start ARS from acpi_nfit_uc_error_notify()
> with ND_ARS_RETURN_PREV_DATA set when HW_ERROR_SCRUB_ON is not set.
>
> Link: http://www.uefi.org/sites/default/files/resources/ACPI_6_2.pdf
> Signed-off-by: Toshi Kani
> Cc: Dan Williams
> Cc: Rafael J. Wysocki
> Cc: Vishal Verma
> Cc: Linda Knippers
> ---
> drivers/acpi/nfit/core.c | 14 +++---
> drivers/acpi/nfit/mce.c|2 +-
> drivers/acpi/nfit/nfit.h |3 ++-
> include/uapi/linux/ndctl.h |1 +
> 4 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index cc22778..d820d72 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -1031,7 +1031,7 @@ static ssize_t scrub_store(struct device *dev,
> if (nd_desc) {
> struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
>
> - rc = acpi_nfit_ars_rescan(acpi_desc);
> + rc = acpi_nfit_ars_rescan(acpi_desc, 0);
> }
> device_unlock(dev);
> if (rc)
> @@ -2057,6 +2057,10 @@ static int ars_start(struct acpi_nfit_desc *acpi_desc,
> struct nfit_spa *nfit_spa
> ars_start.type = ND_ARS_VOLATILE;
> else
> return -ENOTTY;
> + if (nfit_spa->ars_prev_data) {
> + ars_start.flags |= ND_ARS_RETURN_PREV_DATA;
> + nfit_spa->ars_prev_data = 0;
> + }
I'd rather you plumb a new 'flags' parameter all the way through from
acpi_nfit_ars_rescan() to ars_start() rather than carrying this as a
property of nfit_spa.
