| Message ID | 20190903123452.28620-1-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4111cdef0e871c0f7c8874b19ee68a3671f7d63e |
| Headers | show |
| Series | [v2,1/2] powerpc/nvdimm: Use HCALL error as the return value | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (c317052c95bef1f977b023158e5aa929215f443d) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 63 lines checked |
Hi Aneesh, Thanks for the patch. Minor review comments below: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes: > This simplifies the error handling and also enable us to switch to > H_SCM_QUERY hcall in a later patch on H_OVERLAP error. > > We also do some kernel print formatting fixup in this patch. > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > arch/powerpc/platforms/pseries/papr_scm.c | 26 ++++++++++------------- > 1 file changed, 11 insertions(+), 15 deletions(-) > > diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c > index a5ac371a3f06..3bef4d298ac6 100644 > --- a/arch/powerpc/platforms/pseries/papr_scm.c > +++ b/arch/powerpc/platforms/pseries/papr_scm.c > @@ -66,28 +66,22 @@ static int drc_pmem_bind(struct papr_scm_priv *p) > } while (rc == H_BUSY); > > if (rc) { > - /* H_OVERLAP needs a separate error path */ > - if (rc == H_OVERLAP) > - return -EBUSY; > - > dev_err(&p->pdev->dev, "bind err: %lld\n", rc); > - return -ENXIO; > + return rc; > } > > p->bound_addr = saved; > - > - dev_dbg(&p->pdev->dev, "bound drc %x to %pR\n", p->drc_index, &p->res); > - > - return 0; > + dev_dbg(&p->pdev->dev, "bound drc 0x%x to %pR\n", p->drc_index, &p->res); s/0x%x/%#x/ > + return rc; rc == 0 always at this point hence 'return 0' should still work. > } > > -static int drc_pmem_unbind(struct papr_scm_priv *p) > +static void drc_pmem_unbind(struct papr_scm_priv *p) > { > unsigned long ret[PLPAR_HCALL_BUFSIZE]; > uint64_t token = 0; > int64_t rc; > > - dev_dbg(&p->pdev->dev, "unbind drc %x\n", p->drc_index); > + dev_dbg(&p->pdev->dev, "unbind drc 0x%x\n", p->drc_index); > > /* NB: unbind has the same retry requirements as drc_pmem_bind() */ > do { > @@ -110,10 +104,10 @@ static int drc_pmem_unbind(struct papr_scm_priv *p) > if (rc) > dev_err(&p->pdev->dev, "unbind error: %lld\n", rc); > else > - dev_dbg(&p->pdev->dev, "unbind drc %x complete\n", > + dev_dbg(&p->pdev->dev, "unbind drc 0x%x complete\n", > p->drc_index); > > - return rc == H_SUCCESS ? 0 : -ENXIO; > + return; I would prefer drc_pmem_unbind() to still return error from the HCALL. The caller can descide if it wants to ignore the error or not. > } > > static int papr_scm_meta_get(struct papr_scm_priv *p, > @@ -436,14 +430,16 @@ static int papr_scm_probe(struct platform_device *pdev) > rc = drc_pmem_bind(p); > > /* If phyp says drc memory still bound then force unbound and retry */ > - if (rc == -EBUSY) { > + if (rc == H_OVERLAP) { > dev_warn(&pdev->dev, "Retrying bind after unbinding\n"); > drc_pmem_unbind(p); > rc = drc_pmem_bind(p); > } > > - if (rc) > + if (rc != H_SUCCESS) { > + rc = -ENXIO; > goto err; > + } > > /* setup the resource for the newly bound range */ > p->res.start = p->bound_addr; > -- > 2.21.0 >
Vaibhav Jain <vaibhav@linux.ibm.com> writes: > Hi Aneesh, > > Thanks for the patch. Minor review comments below: > > "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes: > >> This simplifies the error handling and also enable us to switch to >> H_SCM_QUERY hcall in a later patch on H_OVERLAP error. >> >> We also do some kernel print formatting fixup in this patch. >> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> --- >> arch/powerpc/platforms/pseries/papr_scm.c | 26 ++++++++++------------- >> 1 file changed, 11 insertions(+), 15 deletions(-) >> >> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c >> index a5ac371a3f06..3bef4d298ac6 100644 >> --- a/arch/powerpc/platforms/pseries/papr_scm.c >> +++ b/arch/powerpc/platforms/pseries/papr_scm.c >> @@ -66,28 +66,22 @@ static int drc_pmem_bind(struct papr_scm_priv *p) >> } while (rc == H_BUSY); >> >> if (rc) { >> - /* H_OVERLAP needs a separate error path */ >> - if (rc == H_OVERLAP) >> - return -EBUSY; >> - >> dev_err(&p->pdev->dev, "bind err: %lld\n", rc); >> - return -ENXIO; >> + return rc; >> } >> >> p->bound_addr = saved; >> - >> - dev_dbg(&p->pdev->dev, "bound drc %x to %pR\n", p->drc_index, &p->res); >> - >> - return 0; > >> + dev_dbg(&p->pdev->dev, "bound drc 0x%x to %pR\n", p->drc_index, &p->res); > s/0x%x/%#x/ I never used #x, I guess both gives similar output? Any specific reason one is prefered over the other? >> + return rc; > rc == 0 always at this point hence 'return 0' should still work. > >> } >> >> -static int drc_pmem_unbind(struct papr_scm_priv *p) >> +static void drc_pmem_unbind(struct papr_scm_priv *p) >> { >> unsigned long ret[PLPAR_HCALL_BUFSIZE]; >> uint64_t token = 0; >> int64_t rc; >> >> - dev_dbg(&p->pdev->dev, "unbind drc %x\n", p->drc_index); >> + dev_dbg(&p->pdev->dev, "unbind drc 0x%x\n", p->drc_index); >> >> /* NB: unbind has the same retry requirements as drc_pmem_bind() */ >> do { >> @@ -110,10 +104,10 @@ static int drc_pmem_unbind(struct papr_scm_priv *p) >> if (rc) >> dev_err(&p->pdev->dev, "unbind error: %lld\n", rc); >> else >> - dev_dbg(&p->pdev->dev, "unbind drc %x complete\n", >> + dev_dbg(&p->pdev->dev, "unbind drc 0x%x complete\n", >> p->drc_index); >> >> - return rc == H_SUCCESS ? 0 : -ENXIO; >> + return; > I would prefer drc_pmem_unbind() to still return error from the > HCALL. The caller can descide if it wants to ignore the error or not. We should do that when we know what we should do with unbind errors. Currently we ignore the error and if we are ignoring why bother to return? > >> } >> >> static int papr_scm_meta_get(struct papr_scm_priv *p, >> @@ -436,14 +430,16 @@ static int papr_scm_probe(struct platform_device *pdev) >> rc = drc_pmem_bind(p); >> >> /* If phyp says drc memory still bound then force unbound and retry */ >> - if (rc == -EBUSY) { >> + if (rc == H_OVERLAP) { >> dev_warn(&pdev->dev, "Retrying bind after unbinding\n"); >> drc_pmem_unbind(p); >> rc = drc_pmem_bind(p); >> } >> >> - if (rc) >> + if (rc != H_SUCCESS) { >> + rc = -ENXIO; >> goto err; >> + } >> >> /* setup the resource for the newly bound range */ >> p->res.start = p->bound_addr; >> -- >> 2.21.0 >> -aneesh
On Tue, 2019-09-03 at 12:34:51 UTC, "Aneesh Kumar K.V" wrote: > This simplifies the error handling and also enable us to switch to > H_SCM_QUERY hcall in a later patch on H_OVERLAP error. > > We also do some kernel print formatting fixup in this patch. > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> Series applied to powerpc fixes, thanks. https://git.kernel.org/powerpc/c/4111cdef0e871c0f7c8874b19ee68a3671f7d63e cheers
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c index a5ac371a3f06..3bef4d298ac6 100644 --- a/arch/powerpc/platforms/pseries/papr_scm.c +++ b/arch/powerpc/platforms/pseries/papr_scm.c @@ -66,28 +66,22 @@ static int drc_pmem_bind(struct papr_scm_priv *p) } while (rc == H_BUSY); if (rc) { - /* H_OVERLAP needs a separate error path */ - if (rc == H_OVERLAP) - return -EBUSY; - dev_err(&p->pdev->dev, "bind err: %lld\n", rc); - return -ENXIO; + return rc; } p->bound_addr = saved; - - dev_dbg(&p->pdev->dev, "bound drc %x to %pR\n", p->drc_index, &p->res); - - return 0; + dev_dbg(&p->pdev->dev, "bound drc 0x%x to %pR\n", p->drc_index, &p->res); + return rc; } -static int drc_pmem_unbind(struct papr_scm_priv *p) +static void drc_pmem_unbind(struct papr_scm_priv *p) { unsigned long ret[PLPAR_HCALL_BUFSIZE]; uint64_t token = 0; int64_t rc; - dev_dbg(&p->pdev->dev, "unbind drc %x\n", p->drc_index); + dev_dbg(&p->pdev->dev, "unbind drc 0x%x\n", p->drc_index); /* NB: unbind has the same retry requirements as drc_pmem_bind() */ do { @@ -110,10 +104,10 @@ static int drc_pmem_unbind(struct papr_scm_priv *p) if (rc) dev_err(&p->pdev->dev, "unbind error: %lld\n", rc); else - dev_dbg(&p->pdev->dev, "unbind drc %x complete\n", + dev_dbg(&p->pdev->dev, "unbind drc 0x%x complete\n", p->drc_index); - return rc == H_SUCCESS ? 0 : -ENXIO; + return; } static int papr_scm_meta_get(struct papr_scm_priv *p, @@ -436,14 +430,16 @@ static int papr_scm_probe(struct platform_device *pdev) rc = drc_pmem_bind(p); /* If phyp says drc memory still bound then force unbound and retry */ - if (rc == -EBUSY) { + if (rc == H_OVERLAP) { dev_warn(&pdev->dev, "Retrying bind after unbinding\n"); drc_pmem_unbind(p); rc = drc_pmem_bind(p); } - if (rc) + if (rc != H_SUCCESS) { + rc = -ENXIO; goto err; + } /* setup the resource for the newly bound range */ p->res.start = p->bound_addr;
This simplifies the error handling and also enable us to switch to H_SCM_QUERY hcall in a later patch on H_OVERLAP error. We also do some kernel print formatting fixup in this patch. Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- arch/powerpc/platforms/pseries/papr_scm.c | 26 ++++++++++------------- 1 file changed, 11 insertions(+), 15 deletions(-)