Re: acpi_nfit_query_poison() broken on non-ARS systems

[Vishal Verma <vishal@kernel.org>]

On Fri, 2016-04-29 at 17:15 -0700, Dan Williams wrote:
> On Fri, Apr 29, 2016 at 5:15 PM, Vishal Verma <vishal@kernel.org>
> wrote:
> > 
> > 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
> See xlat_status():
> 
>                 /* No supported scan types for this range */
>                 flags = ND_ARS_PERSISTENT | ND_ARS_VOLATILE;
>                 if ((ars_cap->status >> 16 & flags) == 0)
>                         return -ENOTTY;\

Ah missed that - thanks. Yep in that case something is weird :)

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