| Message ID | 1480973246-32078-1-git-send-email-vishal.l.verma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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 >
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.
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
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; }
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(-)