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

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

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