Re: [PATCH 2/2] powerpc/papr_scm: Add support for fetching nvdimm 'fuel-gauge' metric

[Vaibhav Jain <vaibhav@linux.ibm.com>]

Thanks for reviewing this patch Ira,

My responses below:

Ira Weiny <ira.weiny@intel.com> writes:

[snip]
>> +static int papr_pdsm_fuel_gauge(struct papr_scm_priv *p,
>> +				union nd_pdsm_payload *payload)
>> +{
>> +	int rc, size;
>> +	struct papr_scm_perf_stat *stat;
>> +	struct papr_scm_perf_stats *stats;
>> +
>> +	/* Silently fail if fetching performance metrics isn't  supported */
>> +	if (!p->len_stat_buffer)
>> +		return 0;
>> +
>> +	/* Allocate request buffer enough to hold single performance stat */
>> +	size = sizeof(struct papr_scm_perf_stats) +
>> +		sizeof(struct papr_scm_perf_stat);
>> +
>> +	stats = kzalloc(size, GFP_KERNEL);
>> +	if (!stats)
>> +		return -ENOMEM;
>> +
>> +	stat = &stats->scm_statistic[0];
>> +	memcpy(&stat->statistic_id, "MemLife ", sizeof(stat->statistic_id));
>> +	stat->statistic_value = 0;
>> +
>> +	/* Fetch the fuel gauge and populate it in payload */
>> +	rc = drc_pmem_query_stats(p, stats, size, 1, NULL);
>> +	if (!rc) {
>
> Always best to except the error case...
>
> 	if (rc) {
> 		... print debuging from below...
> 		goto free_stats;
> 	}
>
Sure, I don't feel strongly about it. Will update this in v2.

>> +		dev_dbg(&p->pdev->dev,
>> +			"Fetched fuel-gauge %llu", stat->statistic_value);
>> +		payload->health.extension_flags |=
>> +			PDSM_DIMM_HEALTH_RUN_GAUGE_VALID;
>> +		payload->health.dimm_fuel_gauge = stat->statistic_value;
>> +
>> +		rc = sizeof(struct nd_papr_pdsm_health);
>> +	}
>> +
>
> free_stats:
>
>> +	kfree(stats);
>> +	return rc;
>> +}
>> +
>>  /* Fetch the DIMM health info and populate it in provided package. */
>>  static int papr_pdsm_health(struct papr_scm_priv *p,
>>  			    union nd_pdsm_payload *payload)
>> @@ -546,6 +585,14 @@ static int papr_pdsm_health(struct papr_scm_priv *p,
>>  
>>  	/* struct populated hence can release the mutex now */
>>  	mutex_unlock(&p->health_mutex);
>> +
>> +	/* Populate the fuel gauge meter in the payload */
>> +	rc = papr_pdsm_fuel_gauge(p, payload);
>> +
>> +	/* Error fetching fuel gauge is not fatal */
>> +	if (rc < 0)
>> +		dev_dbg(&p->pdev->dev, "Err(%d) fetching fuel gauge\n", rc);
>
> Why even return an error?  Just have *_fuel_guage() the print the debugging and
> return void.
>
papr_pdsm_fuel_gauge uses the same signature as other PDSM service
functions as described in pdsm_cmd_desc.service callback. Hence designed
the function signature as such.

>> +
>>  	rc = sizeof(struct nd_papr_pdsm_health);
>
> You just override rc here anyway...
>
> Ira
>
>>  
>>  out:
>> -- 
>> 2.26.2
>> _______________________________________________
>> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
>> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

-- 
Cheers
~ Vaibhav