Re: [PATCH v3 2/3] powerpc/papr_scm: Update drc_pmem_unbind() to use H_SCM_UNBIND_ALL

[Vaibhav Jain <vaibhav@linux.ibm.com>]

Thanks for reviewing this patch Aneesh,

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:

> On 6/26/19 7:34 PM, Vaibhav Jain wrote:
>> The new hcall named H_SCM_UNBIND_ALL has been introduce that can
>> unbind all or specific scm memory assigned to an lpar. This is
>> more efficient than using H_SCM_UNBIND_MEM as currently we don't
>> support partial unbind of scm memory.
>> 
>> Hence this patch proposes following changes to drc_pmem_unbind():
>> 
>>      * Update drc_pmem_unbind() to replace hcall H_SCM_UNBIND_MEM to
>>        H_SCM_UNBIND_ALL.
>> 
>>      * Update drc_pmem_unbind() to handles cases when PHYP asks the guest
>>        kernel to wait for specific amount of time before retrying the
>>        hcall via the 'LONG_BUSY' return value.
>> 
>>      * Ensure appropriate error code is returned back from the function
>>        in case of an error.
>> 
>> Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>> Change-log:
>> 
>> v3:
>> * Fixed a build warning reported by kbuild-robot.
>> * Updated patch description to put emphasis on 'scm memory' instead of
>>    'scm drc memory blocks' as for phyp there is a stark difference
>>    between how drc are managed for scm memory v/s regular memory. [Oliver]
>> 
>> v2:
>> * Added a dev_dbg when unbind operation succeeds [Oliver]
>> * Changed type of variable 'rc' to int64_t [Oliver]
>> * Removed the code that was logging a warning in case bind operation
>>    takes >1-seconds [Oliver]
>> * Spinned off changes to hvcall.h as a separate patch. [Oliver]
>> ---
>>   arch/powerpc/platforms/pseries/papr_scm.c | 29 +++++++++++++++++------
>>   1 file changed, 22 insertions(+), 7 deletions(-)
>> 
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 96c53b23e58f..c01a03fd3ee7 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/sched.h>
>>   #include <linux/libnvdimm.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/delay.h>
>>   
>>   #include <asm/plpar_wrappers.h>
>>   
>> @@ -77,22 +78,36 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
>>   static int drc_pmem_unbind(struct papr_scm_priv *p)
>>   {
>>   	unsigned long ret[PLPAR_HCALL_BUFSIZE];
>> -	uint64_t rc, token;
>> +	uint64_t token = 0;
>> +	int64_t rc;
>>   
>> -	token = 0;
>> +	dev_dbg(&p->pdev->dev, "unbind drc %x\n", p->drc_index);
>>   
>> -	/* NB: unbind has the same retry requirements mentioned above */
>> +	/* NB: unbind has the same retry requirements as drc_pmem_bind() */
>>   	do {
>> -		rc = plpar_hcall(H_SCM_UNBIND_MEM, ret, p->drc_index,
>> -				p->bound_addr, p->blocks, token);
>> +
>> +		/* Unbind of all SCM resources associated with drcIndex */
>> +		rc = plpar_hcall(H_SCM_UNBIND_ALL, ret, H_UNBIND_SCOPE_DRC,
>> +				 p->drc_index, token);
>>   		token = ret[0];
>> -		cond_resched();
>> +
>> +		/* Check if we are stalled for some time */
>> +		if (H_IS_LONG_BUSY(rc)) {
>> +			msleep(get_longbusy_msecs(rc));
>> +			rc = H_BUSY;
>> +		} else if (rc == H_BUSY) {
>> +			cond_resched();
>> +		}
>> +
>>   	} while (rc == H_BUSY);
>>   
>>   	if (rc)
>>   		dev_err(&p->pdev->dev, "unbind error: %lld\n", rc);
>> +	else
>> +		dev_dbg(&p->pdev->dev, "unbind drc %x complete\n",
>> +			p->drc_index);
>>   
> Can we add p->drc_index as part of these messages? Also s/%x/0x%x ?
the scm platform device has the name of the form
"ibm,persistent-memory:ibm,pmemory@44100002" which also contains the
drc_index. So i think printing it again in this message would be
redundant.

>
>
>> -	return !!rc;
>> +	return rc == H_SUCCESS ? 0 : -ENXIO;
>>   }
>>   
> The error -ENXIO is confusing. Can we keep the HCALL error as return for 
> this? We don't check error from this. If we can't take any action based 
> on the return. Then may be make it  void?
>
Wanted to keep the behaviour of this function symmetric to
drc_pmem_bind() which when updated in the next patch returns a kernel
error code instead of hcall error.

Agree that we arent using the return
value of this function right now but this may change in the future.

>
>>   static int papr_scm_meta_get(struct papr_scm_priv *p,
>> 
>
>
> -aneesh

-- 
Vaibhav Jain <vaibhav@linux.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.