* [PATCH] nfit: Fix extended status translations for ACPI DSMs
@ 2016-12-05 21:27 Vishal Verma
2016-12-05 21:37 ` Dan Williams
0 siblings, 1 reply; 6+ messages in thread
From: Vishal Verma @ 2016-12-05 21:27 UTC (permalink / raw)
To: linux-nvdimm
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.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@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;
return 0;
}
--
2.7.4
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] nfit: Fix extended status translations for ACPI DSMs
2016-12-05 21:27 [PATCH] nfit: Fix extended status translations for ACPI DSMs Vishal Verma
@ 2016-12-05 21:37 ` Dan Williams
2016-12-05 21:43 ` Verma, Vishal L
0 siblings, 1 reply; 6+ messages in thread
From: Dan Williams @ 2016-12-05 21:37 UTC (permalink / raw)
To: Vishal Verma; +Cc: linux-nvdimm@lists.01.org
On Mon, Dec 5, 2016 at 1:27 PM, Vishal Verma <vishal.l.verma@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.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nfit: Fix extended status translations for ACPI DSMs
2016-12-05 21:37 ` Dan Williams
@ 2016-12-05 21:43 ` Verma, Vishal L
2016-12-05 21:54 ` Linda Knippers
0 siblings, 1 reply; 6+ messages in thread
From: Verma, Vishal L @ 2016-12-05 21:43 UTC (permalink / raw)
To: Williams, Dan J; +Cc: linux-nvdimm@lists.01.org
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.verma@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.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nfit: Fix extended status translations for ACPI DSMs
2016-12-05 21:43 ` Verma, Vishal L
@ 2016-12-05 21:54 ` Linda Knippers
2016-12-05 22:16 ` Dan Williams
0 siblings, 1 reply; 6+ messages in thread
From: Linda Knippers @ 2016-12-05 21:54 UTC (permalink / raw)
To: Verma, Vishal L, Williams, Dan J; +Cc: linux-nvdimm@lists.01.org
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.verma@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.williams@intel.com>
>>> Signed-off-by: Vishal Verma <vishal.l.verma@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?
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.
-- ljk
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nfit: Fix extended status translations for ACPI DSMs
2016-12-05 21:54 ` Linda Knippers
@ 2016-12-05 22:16 ` Dan Williams
2016-12-05 22:26 ` Linda Knippers
0 siblings, 1 reply; 6+ messages in thread
From: Dan Williams @ 2016-12-05 22:16 UTC (permalink / raw)
To: Linda Knippers; +Cc: linux-nvdimm@lists.01.org
On Mon, Dec 5, 2016 at 1:54 PM, Linda Knippers <linda.knippers@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.verma@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.williams@intel.com>
>>>> Signed-off-by: Vishal Verma <vishal.l.verma@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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nfit: Fix extended status translations for ACPI DSMs
2016-12-05 22:16 ` Dan Williams
@ 2016-12-05 22:26 ` Linda Knippers
0 siblings, 0 replies; 6+ messages in thread
From: Linda Knippers @ 2016-12-05 22:26 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-nvdimm@lists.01.org
On 12/5/2016 5:16 PM, Dan Williams wrote:
> On Mon, Dec 5, 2016 at 1:54 PM, Linda Knippers <linda.knippers@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.verma@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.williams@intel.com>
>>>>> Signed-off-by: Vishal Verma <vishal.l.verma@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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-12-05 22:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-05 21:27 [PATCH] nfit: Fix extended status translations for ACPI DSMs Vishal Verma
2016-12-05 21:37 ` Dan Williams
2016-12-05 21:43 ` Verma, Vishal L
2016-12-05 21:54 ` Linda Knippers
2016-12-05 22:16 ` Dan Williams
2016-12-05 22:26 ` Linda Knippers
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.