[PATCH] nfit: Fix extended status translations for ACPI DSMs

[PATCH] nfit: Fix extended status translations for ACPI DSMs

[Vishal Verma]

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

Re: [PATCH] nfit: Fix extended status translations for ACPI DSMs

[Dan Williams]

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

Re: [PATCH] nfit: Fix extended status translations for ACPI DSMs

[Verma, Vishal L]

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

Re: [PATCH] nfit: Fix extended status translations for ACPI DSMs

[Linda Knippers]

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

Re: [PATCH] nfit: Fix extended status translations for ACPI DSMs

[Dan Williams]

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

Re: [PATCH] nfit: Fix extended status translations for ACPI DSMs

[Linda Knippers]

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