Re: [PATCH 3/5] acpi: cleanup acpi_nfit_ctl calling xlat_status

From: Linda Knippers <linda.knippers@hpe.com>
To: Dave Jiang <dave.jiang@intel.com>, dan.j.williams@intel.com
Cc: linux-nvdimm@lists.01.org
Subject: Re: [PATCH 3/5] acpi: cleanup acpi_nfit_ctl calling xlat_status
Date: Wed, 8 Mar 2017 11:51:40 -0500	[thread overview]
Message-ID: <58C0369C.9060100@hpe.com> (raw)
In-Reply-To: <e565aec9-b400-6712-25b2-904a64b87dc4@intel.com>

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
>>>
>>

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm