Re: acpi_nfit_query_poison() broken on non-ARS systems

From: Linda Knippers <linda.knippers@hpe.com>
To: Vishal Verma <vishal@kernel.org>,
	Dan Williams <dan.j.williams@intel.com>
Cc: "linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>
Subject: Re: acpi_nfit_query_poison() broken on non-ARS systems
Date: Sun, 1 May 2016 13:27:33 -0400	[thread overview]
Message-ID: <57263C85.4070405@hpe.com> (raw)
In-Reply-To: <1461975534.11211.12.camel@kernel.org>

On 4/29/2016 8:18 PM, Vishal Verma wrote:
> 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 :)

The problem is that xlat_status() isn't called when you're initiating the
sequence from acpi_nfit_query_poison().  I think it's only called when
the DSM is initiated from user space.

If you go back to my original message, you'll see that.

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