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

["Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>]

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 ?


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


>   static int papr_scm_meta_get(struct papr_scm_priv *p,
> 


-aneesh