Re: acpi_nfit_query_poison() broken on non-ARS systems

[Vishal Verma <vishal@kernel.org>]

On Fri, 2016-04-29 at 17:04 -0700, Dan Williams wrote:
> On Fri, Apr 29, 2016 at 5:03 PM, Vishal Verma <vishal@kernel.org>
> wrote:
> > 
> > On Fri, 2016-04-29 at 19:36 -0400, Linda Knippers wrote:
> > 
> > [snip]
> > > 
> > > 
> > > > 
> > > > static int acpi_nfit_query_poison(struct acpi_nfit_desc
> > > > *acpi_desc,
> > > >                 struct nfit_spa *nfit_spa)
> > > > {
> > > >         struct acpi_nfit_system_address *spa = nfit_spa->spa;
> > > >         int rc;
> > > > 
> > > >         if (!nfit_spa->max_ars) {
> > > On a platform that doesn't support ARS, max_ars will always be
> > > zero
> > > so you'll keep executing this code when it seems like you're
> > > trying
> > > to avoid that.
> > > 
> > > > 
> > > > 
> > > >                 struct nd_cmd_ars_cap ars_cap;
> > > > 
> > > >                 memset(&ars_cap, 0, sizeof(ars_cap));
> > > >                 rc = ars_get_cap(acpi_desc, &ars_cap,
> > > > nfit_spa);
> > > >                 if (rc < 0)
> > > >                         return rc;
> > > The call succeeds so this return isn't taken, but then the code
> > > assumes
> > > everything is good.  It should check ars_cap.status so see if the
> > > function
> > > is supported or if there was an error and return something
> > > appropriate.
> > > In previous version of the code, that's what
> > > acpi_nfit_find_poison()
> > > did.
> > > Instead, this function continues, using data that's not right and
> > > making
> > > more calls that also aren't supported.
> > Good point - I think we should add a check here and make sure ARS
> > is
> > supported using the status field. I can work on a patch. Thanks for
> > the
> > report!
> ...but we do, or are supposed to, fail on any non-zero status:
> 
>                 /* Command failed */
>                 if (ars_cap->status & 0xffff)
>                         return -EIO;

This doesn't check the extended status, does it..
I think we'd also want something like:

		/* ARS not supported */>
		if (ars_cap->status & 0xffff0000 == 0)
			return -EOPNOTSUPP;

From ACPI 6.1:
Extended Status (Len 2) (Off 2)
Bit[0] – If set to 1, indicates Volatile Memory scrub is supported
Bit[1] – If set to 1, indicates Persistent Memory Scrub is supported
Bits[15:2] – Reserved

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm