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 <[email protected]> >>> --- >>> 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? > So in the current case, if xlat_status() fails, > then *cmd_rc gets it like in the original code. If it's succeeds, then > the forget_poison provides the status instead. > >> >>> + >>> out: >>> ACPI_FREE(out_obj); >>> >>> >>> _______________________________________________ >>> Linux-nvdimm mailing list >>> [email protected] >>> https://lists.01.org/mailman/listinfo/linux-nvdimm >>> >> _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
