Thanks for reviewing this patch Aneesh, "Aneesh Kumar K.V" <aneesh.ku...@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 <ooh...@gmail.com> >> Signed-off-by: Vaibhav Jain <vaib...@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 <vaib...@linux.ibm.com> Linux Technology Center, IBM India Pvt. Ltd.