On Wed, Mar 8, 2017 at 8:51 AM, Linda Knippers <linda.knipp...@hpe.com> wrote:
> On 03/08/2017 11:29 AM, Dave Jiang wrote:
>> On 03/07/2017 05:20 PM, Linda Knippers wrote:
>>> On 03/06/2017 03:32 PM, Dave Jiang wrote:
>>>> Make sure that xlat_status is unconditionally called.
>>>
>>> Is this just code cleanup or is it fixing something?
>>
>> This is code clean up requested by Dan.
>
> Ok, then it seems like the commit message above should match
> the subject. As it is, it makes it sound like it's important
> to call xlat_status unconditionally, but I still have a question
> about that below.
>
>>
>>>
>>>>
>>>> Signed-off-by: Dave Jiang <dave.ji...@intel.com>
>>>> ---
>>>> drivers/acpi/nfit/core.c | 15 +++++++--------
>>>> 1 file changed, 7 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>>>> index 7361d00..9d4f461 100644
>>>> --- a/drivers/acpi/nfit/core.c
>>>> +++ b/drivers/acpi/nfit/core.c
>>>> @@ -199,7 +199,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor
>>>> *nd_desc, struct nvdimm *nvdimm,
>>>> acpi_handle handle;
>>>> unsigned int func;
>>>> const u8 *uuid;
>>>> - int rc, i;
>>>> + int rc = 0, xlat_rc, i;
>>>>
>>>> func = cmd;
>>>> if (cmd == ND_CMD_CALL) {
>>>> @@ -343,21 +343,20 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor
>>>> *nd_desc, struct nvdimm *nvdimm,
>>>> * unfilled in the output buffer
>>>> */
>>>> rc = buf_len - offset - in_buf.buffer.length;
>>>> - if (cmd_rc)
>>>> - *cmd_rc = xlat_status(nvdimm, buf, cmd,
>>>> - fw_status);
>>>> } else {
>>>> dev_err(dev, "%s:%s underrun cmd: %s buf_len: %d
>>>> out_len: %d\n",
>>>> __func__, dimm_name, cmd_name, buf_len,
>>>> offset);
>>>> rc = -ENXIO;
>>>> + goto out;
>>>> }
>>>> - } else {
>>>> - rc = 0;
>>>> - if (cmd_rc)
>>>> - *cmd_rc = xlat_status(nvdimm, buf, cmd, fw_status);
>>>> }
>>>>
>>>> + xlat_rc = xlat_status(nvdimm, buf, cmd, fw_status);
>>>> +
>>>> + if (cmd_rc)
>>>> + *cmd_rc = xlat_rc;
>>>
>>> Is there some benefit of calling xlat_status and then throwing away the
>>> result?
>>
>> It's not exactly thrown away. I tried to preserve the original code path
>> as much as possible.
>
> The original code only calls xlat_status if cmd_rc is set, but you call it,
> then don't use the result if cmd_rc isn't set. Why call it if you know
> ahead of time you're not going to use the result? That's what I don't
> understand. Why isn't the call made only 'if (cmd_rc)', like the original
> code?
>
In a following patch we use the xlat_rc result even in cases when
cmd_rc is NULL.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
