[PATCH v2 1/2] powerpc/nvdimm: Use HCALL error as the return value

[PATCH v2 1/2] powerpc/nvdimm: Use HCALL error as the return value

[Aneesh Kumar K.V]

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);
+	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;
-- 
2.21.0

[PATCH v2 2/2] powerpc/nvdimm: use H_SCM_QUERY hcall on H_OVERLAP error

[Aneesh Kumar K.V]

Right now we force an unbind of SCM memory at drcindex on H_OVERLAP error.
This really slows down operations like kexec where we get the H_OVERLAP
error because we don't go through a full hypervisor re init.

H_OVERLAP error for a H_SCM_BIND_MEM hcall indicates that SCM memory at
drc index is already bound. Since we don't specify a logical memory
address for bind hcall, we can use the H_SCM_QUERY hcall to query
the already bound logical address.

Boot time difference with and without patch is:

[    5.583617] IOMMU table initialized, virtual merging enabled
[    5.603041] papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Retrying bind after unbinding
[  301.514221] papr_scm ibm,persistent-memory:ibm,pmemory@44108001: Retrying bind after unbinding
[  340.057238] hv-24x7: read 1530 catalog entries, created 537 event attrs (0 failures), 275 descs

after fix

[    5.101572] IOMMU table initialized, virtual merging enabled
[    5.116984] papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Querying SCM details
[    5.117223] papr_scm ibm,persistent-memory:ibm,pmemory@44108001: Querying SCM details
[    5.120530] hv-24x7: read 1530 catalog entries, created 537 event attrs (0 failures), 275 descs

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
Changes from V1:
* Use the first block and last block to query the logical bind memory
* If we fail to query, ubind and retry the bind.


 arch/powerpc/platforms/pseries/papr_scm.c | 48 +++++++++++++++++++----
 1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 3bef4d298ac6..61883291defc 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -65,10 +65,8 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
 		cond_resched();
 	} while (rc == H_BUSY);
 
-	if (rc) {
-		dev_err(&p->pdev->dev, "bind err: %lld\n", rc);
+	if (rc)
 		return rc;
-	}
 
 	p->bound_addr = saved;
 	dev_dbg(&p->pdev->dev, "bound drc 0x%x to %pR\n", p->drc_index, &p->res);
@@ -110,6 +108,42 @@ static void drc_pmem_unbind(struct papr_scm_priv *p)
 	return;
 }
 
+static int drc_pmem_query_n_bind(struct papr_scm_priv *p)
+{
+	unsigned long start_addr;
+	unsigned long end_addr;
+	unsigned long ret[PLPAR_HCALL_BUFSIZE];
+	int64_t rc;
+
+
+	rc = plpar_hcall(H_SCM_QUERY_BLOCK_MEM_BINDING, ret,
+			 p->drc_index, 0);
+	if (rc)
+		goto err_out;
+	start_addr = ret[0];
+
+	/* Make sure the full region is bound. */
+	rc = plpar_hcall(H_SCM_QUERY_BLOCK_MEM_BINDING, ret,
+			 p->drc_index, p->blocks - 1);
+	if (rc)
+		goto err_out;
+	end_addr = ret[0];
+
+	if ((end_addr - start_addr) != ((p->blocks - 1) * p->block_size))
+		goto err_out;
+
+	p->bound_addr = start_addr;
+	dev_dbg(&p->pdev->dev, "bound drc 0x%x to %pR\n", p->drc_index, &p->res);
+	return rc;
+
+err_out:
+	dev_info(&p->pdev->dev,
+		 "Failed to query, trying an unbind followed by bind");
+	drc_pmem_unbind(p);
+	return drc_pmem_bind(p);
+}
+
+
 static int papr_scm_meta_get(struct papr_scm_priv *p,
 			     struct nd_cmd_get_config_data_hdr *hdr)
 {
@@ -430,13 +464,11 @@ 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 == H_OVERLAP) {
-		dev_warn(&pdev->dev, "Retrying bind after unbinding\n");
-		drc_pmem_unbind(p);
-		rc = drc_pmem_bind(p);
-	}
+	if (rc == H_OVERLAP)
+		rc = drc_pmem_query_n_bind(p);
 
 	if (rc != H_SUCCESS) {
+		dev_err(&p->pdev->dev, "bind err: %d\n", rc);
 		rc = -ENXIO;
 		goto err;
 	}
-- 
2.21.0

Re: [PATCH v2 2/2] powerpc/nvdimm: use H_SCM_QUERY hcall on H_OVERLAP error

[Vaibhav Jain]

Hi Aneesh,

Thanks for the patch. A minor suggestion below:

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

> Right now we force an unbind of SCM memory at drcindex on H_OVERLAP error.
> This really slows down operations like kexec where we get the H_OVERLAP
> error because we don't go through a full hypervisor re init.
>
> H_OVERLAP error for a H_SCM_BIND_MEM hcall indicates that SCM memory at
> drc index is already bound. Since we don't specify a logical memory
> address for bind hcall, we can use the H_SCM_QUERY hcall to query
> the already bound logical address.
>
> Boot time difference with and without patch is:
>
> [    5.583617] IOMMU table initialized, virtual merging enabled
> [    5.603041] papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Retrying bind after unbinding
> [  301.514221] papr_scm ibm,persistent-memory:ibm,pmemory@44108001: Retrying bind after unbinding
> [  340.057238] hv-24x7: read 1530 catalog entries, created 537 event attrs (0 failures), 275 descs
>
> after fix
>
> [    5.101572] IOMMU table initialized, virtual merging enabled
> [    5.116984] papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Querying SCM details
> [    5.117223] papr_scm ibm,persistent-memory:ibm,pmemory@44108001: Querying SCM details
> [    5.120530] hv-24x7: read 1530 catalog entries, created 537 event attrs (0 failures), 275 descs
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> Changes from V1:
> * Use the first block and last block to query the logical bind memory
> * If we fail to query, ubind and retry the bind.
>
>
>  arch/powerpc/platforms/pseries/papr_scm.c | 48 +++++++++++++++++++----
>  1 file changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 3bef4d298ac6..61883291defc 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -65,10 +65,8 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
>  		cond_resched();
>  	} while (rc == H_BUSY);
>  
> -	if (rc) {
> -		dev_err(&p->pdev->dev, "bind err: %lld\n", rc);
> +	if (rc)
>  		return rc;
> -	}
>  
>  	p->bound_addr = saved;
>  	dev_dbg(&p->pdev->dev, "bound drc 0x%x to %pR\n", p->drc_index, &p->res);
> @@ -110,6 +108,42 @@ static void drc_pmem_unbind(struct papr_scm_priv *p)
>  	return;
>  }
>  
> +static int drc_pmem_query_n_bind(struct papr_scm_priv *p)
> +{
> +	unsigned long start_addr;
> +	unsigned long end_addr;
> +	unsigned long ret[PLPAR_HCALL_BUFSIZE];
> +	int64_t rc;
> +
> +
> +	rc = plpar_hcall(H_SCM_QUERY_BLOCK_MEM_BINDING, ret,
> +			 p->drc_index, 0);
> +	if (rc)
> +		goto err_out;
> +	start_addr = ret[0];
> +
> +	/* Make sure the full region is bound. */
> +	rc = plpar_hcall(H_SCM_QUERY_BLOCK_MEM_BINDING, ret,
> +			 p->drc_index, p->blocks - 1);
> +	if (rc)
> +		goto err_out;
> +	end_addr = ret[0];
> +
> +	if ((end_addr - start_addr) != ((p->blocks - 1) * p->block_size))
> +		goto err_out;
> +
> +	p->bound_addr = start_addr;
> +	dev_dbg(&p->pdev->dev, "bound drc 0x%x to %pR\n", p->drc_index, &p->res);
> +	return rc;
> +

> +err_out:
> +	dev_info(&p->pdev->dev,
> +		 "Failed to query, trying an unbind followed by bind");
> +	drc_pmem_unbind(p);
> +	return drc_pmem_bind(p);
> +}
Would have preferred error handling for bind failure to be done at
single location i.e in papr_scm_probe() rather than in
drc_pmem_query_n_bind().

> +
> +
>  static int papr_scm_meta_get(struct papr_scm_priv *p,
>  			     struct nd_cmd_get_config_data_hdr *hdr)
>  {
> @@ -430,13 +464,11 @@ 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 == H_OVERLAP) {
> -		dev_warn(&pdev->dev, "Retrying bind after unbinding\n");
> -		drc_pmem_unbind(p);
> -		rc = drc_pmem_bind(p);
> -	}
> +	if (rc == H_OVERLAP)
> +		rc = drc_pmem_query_n_bind(p);
>  
>  	if (rc != H_SUCCESS) {
> +		dev_err(&p->pdev->dev, "bind err: %d\n", rc);
>  		rc = -ENXIO;
>  		goto err;
>  	}
> -- 
> 2.21.0
>

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

Re: [PATCH v2 2/2] powerpc/nvdimm: use H_SCM_QUERY hcall on H_OVERLAP error

[Aneesh Kumar K.V]

Vaibhav Jain <vaibhav@linux.ibm.com> writes:

>> +err_out:
>> +	dev_info(&p->pdev->dev,
>> +		 "Failed to query, trying an unbind followed by bind");
>> +	drc_pmem_unbind(p);
>> +	return drc_pmem_bind(p);
>> +}
> Would have preferred error handling for bind failure to be done at
> single location i.e in papr_scm_probe() rather than in
> drc_pmem_query_n_bind().
>

IMHO the final code looks simpler.

	/* request the hypervisor to bind this region to somewhere in memory */
	rc = drc_pmem_bind(p);

	/* If phyp says drc memory still bound then force unbound and retry */
	if (rc == H_OVERLAP)
		rc = drc_pmem_query_n_bind(p);

	if (rc != H_SUCCESS) {
		dev_err(&p->pdev->dev, "bind err: %d\n", rc);
		rc = -ENXIO;
		goto err;
	}


-aneesh

Re: [PATCH v2 1/2] powerpc/nvdimm: Use HCALL error as the return value

[Vaibhav Jain]

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>
Linux Technology Center, IBM India Pvt. Ltd.

Re: [PATCH v2 1/2] powerpc/nvdimm: Use HCALL error as the return value

[Aneesh Kumar K.V]

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

Re: [PATCH v2 1/2] powerpc/nvdimm: Use HCALL error as the return value

[Michael Ellerman]

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