On 12/5/2016 5:16 PM, Dan Williams wrote: > On Mon, Dec 5, 2016 at 1:54 PM, Linda Knippers <linda.knipp...@hpe.com> wrote: >> On 12/05/2016 04:43 PM, Verma, Vishal L wrote: >>> On Mon, 2016-12-05 at 13:37 -0800, Dan Williams wrote: >>>> On Mon, Dec 5, 2016 at 1:27 PM, Vishal Verma <vishal.l.ve...@intel.com >>>>> wrote: >>>>> >>>>> ACPI DSMs can have an 'extended' status which can be non-zero to >>>>> convey >>>>> additional information about the command. In the xlat_status >>>>> routine, >>>>> where we translate the command statuses, we were returning an error >>>>> for >>>>> a non-zero extended status, even if the primary status indicated >>>>> success. >>>>> >>>>> Cc: Dan Williams <dan.j.willi...@intel.com> >>>>> Signed-off-by: Vishal Verma <vishal.l.ve...@intel.com> >>>>> --- >>>>> drivers/acpi/nfit/core.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c >>>>> index 71a7d07..d14f09b 100644 >>>>> --- a/drivers/acpi/nfit/core.c >>>>> +++ b/drivers/acpi/nfit/core.c >>>>> @@ -169,7 +169,7 @@ static int xlat_status(void *buf, unsigned int >>>>> cmd, u32 status) >>>>> } >>>>> >>>>> /* all other non-zero status results in an error */ >>>>> - if (status) >>>>> + if (status & 0xffff) >>>>> return -EIO; >>>> >>>> I don't think this is right, because we have no idea at this point >>>> whether extended status is fatal or not. >>>> >>>> Each 'case' statement in that 'switch' should be returning 0 if it >>>> does not see any errors. Because that's the only part of the function >>>> with per-command knowledge of extended being benign / informational vs >>>> fatal. >>> >>> Good point - I was wondering just that.. I'll resend. >> >> But can't that function be called with the status for DSMs that aren't in >> switch >> statement? >> > > Yes, but keep in mind the only consumer of that "cmd_rc" result is > in-kernel callers. > >> All the DSM specs I've seen separate the status into status and extended or >> function-specific >> status, which is either defined or reserved. If the 2 bytes of status don't >> indicate >> a failure, I don't think you should return EIO just because there may be >> something set in a reserved bit. > > The kernel will only consume that status for ARS and label commands. > In the case of ND_CMD_CALL, and other DSMs that the kernel never > consumes internally, the translation to -EIO is benign.
Actually, it looks like -EIO won't be returned because fw_status is still 0 when xlat_status is called so there's nothing to translate. Am I reading that right? If so, you could probably avoid the call. -- ljk _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
