Thanks for reviewing this patch Ira,
My responses below:
Ira Weiny <ira.we...@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-nvd...@lists.01.org
>> To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
--
Cheers
~ Vaibhav
