| Message ID | 148883236278.49239.9457895421265109054.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > > Signed-off-by: Dave Jiang <dave.jiang@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? > + > out: > ACPI_FREE(out_obj); > > > _______________________________________________ > Linux-nvdimm mailing list > Linux-nvdimm@lists.01.org > https://lists.01.org/mailman/listinfo/linux-nvdimm >
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. > >> >> Signed-off-by: Dave Jiang <dave.jiang@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. 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 >> Linux-nvdimm@lists.01.org >> https://lists.01.org/mailman/listinfo/linux-nvdimm >> >
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.jiang@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? > 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 >>> Linux-nvdimm@lists.01.org >>> https://lists.01.org/mailman/listinfo/linux-nvdimm >>> >>
On Wed, Mar 8, 2017 at 8:51 AM, Linda Knippers <linda.knippers@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.jiang@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.
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; + out: ACPI_FREE(out_obj);
Make sure that xlat_status is unconditionally called. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- drivers/acpi/nfit/core.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)