9:32 a.m.
On Wed, May 20, 2020 at 10:45:58PM +0530, Vaibhav Jain wrote:
...
> On Wed, May 20, 2020 at 12:30:56AM +0530, Vaibhav Jain wrote:
...
>> @@ -39,6 +78,15 @@ struct papr_scm_priv {
>> struct resource res;
>> struct nd_region *region;
>> struct nd_interleave_set nd_set;
>> +
>> + /* Protect dimm health data from concurrent read/writes */
>> + struct mutex health_mutex;
>> +
>> + /* Last time the health information of the dimm was updated */
>> + unsigned long lasthealth_jiffies;
>> +
>> + /* Health information for the dimm */
>> + u64 health_bitmap;
>
> I wonder if this should be typed big endian as you mention that it is in the
> commit message?
This was discussed in an earlier review of the patch series at
https://lore.kernel.org/linux-nvdimm/878sjetcis.fsf@mpe.ellerman.id.au
Even though health bitmap is returned in big endian format (For ex
value 0xC00000000000000 indicates bits 0,1 set), its value is never
used. Instead only test for specific bits being set in the register is
done.
Hence using native cpu type instead of __be64 to store this value.
ok.
>
>> };
>>
>> static int drc_pmem_bind(struct papr_scm_priv *p)
>> @@ -144,6 +192,62 @@ static int drc_pmem_query_n_bind(struct papr_scm_priv *p)
>> return drc_pmem_bind(p);
>> }
>>
>> +/*
>> + * Issue hcall to retrieve dimm health info and populate papr_scm_priv with
the
>> + * health information.
>> + */
>> +static int __drc_pmem_query_health(struct papr_scm_priv *p)
>> +{
>> + unsigned long ret[PLPAR_HCALL_BUFSIZE];
>
> Is this exclusive to 64bit? Why not u64?
Yes this is specific to 64 bit as the array holds 64 bit register values
returned from PHYP. Can u64 but here that will be a departure from existing
practice within arch/powerpc code to use an unsigned long array to fetch
returned values for PHYP.
>
>> + s64 rc;
>
> plpar_hcall() returns long and this function returns int and rc is declared
> s64?
>
> Why not have them all be long to follow plpar_hcall?
Yes 'long' type is better suited for variable 'rc' and I will get it
fixed.
But the value of variable 'rc' is never directly returned from this
function, we always return kernel error codes instead. Hence the
return type of this function is consistent.
Honestly masking the error return of plpar_hcall() seems a problem as well...
but ok.
Ira
>
> >
> >> +
> >> + /* issue the hcall */
> >> + rc = plpar_hcall(H_SCM_HEALTH, ret, p->drc_index);
> >> + if (rc != H_SUCCESS) {
> >> + dev_err(&p->pdev->dev,
> >> + "Failed to query health information, Err:%lld\n", rc);
> >> + rc = -ENXIO;
> >> + goto out;
> >> + }
> >> +
> >> + p->lasthealth_jiffies = jiffies;
> >> + p->health_bitmap = ret[0] & ret[1];
> >> +
> >> + dev_dbg(&p->pdev->dev,
> >> + "Queried dimm health info. Bitmap:0x%016lx Mask:0x%016lx\n",
> >> + ret[0], ret[1]);
> >> +out:
> >> + return rc;
> >> +}
> >> +
> >> +/* Min interval in seconds for assuming stable dimm health */
> >> +#define MIN_HEALTH_QUERY_INTERVAL 60
> >> +
> >> +/* Query cached health info and if needed call drc_pmem_query_health */
> >> +static int drc_pmem_query_health(struct papr_scm_priv *p)
> >> +{
> >> + unsigned long cache_timeout;
> >> + s64 rc;
> >> +
> >> + /* Protect concurrent modifications to papr_scm_priv */
> >> + rc = mutex_lock_interruptible(&p->health_mutex);
> >> + if (rc)
> >> + return rc;
> >> +
> >> + /* Jiffies offset for which the health data is assumed to be same */
> >> + cache_timeout = p->lasthealth_jiffies +
> >> + msecs_to_jiffies(MIN_HEALTH_QUERY_INTERVAL * 1000);
> >> +
> >> + /* Fetch new health info is its older than MIN_HEALTH_QUERY_INTERVAL */
> >> + if (time_after(jiffies, cache_timeout))
> >> + rc = __drc_pmem_query_health(p);
> >
> > And back to s64 after returning int?
> Agree, will change 's64 rc' to 'int rc'.
>
> >
> >> + else
> >> + /* Assume cached health data is valid */
> >> + rc = 0;
> >> +
> >> + mutex_unlock(&p->health_mutex);
> >> + return rc;
> >> +}
> >>
> >> static int papr_scm_meta_get(struct papr_scm_priv *p,
> >> struct nd_cmd_get_config_data_hdr *hdr)
> >> @@ -286,6 +390,64 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor
*nd_desc,
> >> return 0;
> >> }
> >>
> >> +static ssize_t flags_show(struct device *dev,
> >> + struct device_attribute *attr, char *buf)
> >> +{
> >> + struct nvdimm *dimm = to_nvdimm(dev);
> >> + struct papr_scm_priv *p = nvdimm_provider_data(dimm);
> >> + struct seq_buf s;
> >> + u64 health;
> >> + int rc;
> >> +
> >> + rc = drc_pmem_query_health(p);
> >
> > and back to int...
> >
> drc_pmem_query_health() returns an 'int' so the type of variable
'rc'
> looks correct to me.
>
> > Just make them long all through...
> I think the return type for above all functions is 'int' with
> an issue in drc_pmem_query_health() that you pointed out.
>
> With that fixed the usage of 'int' return type for functions will become
> consistent.
>
> >
> > Ira
> >
> >> + if (rc)
> >> + return rc;
> >> +
> >> + /* Copy health_bitmap locally, check masks & update out buffer */
> >> + health = READ_ONCE(p->health_bitmap);
> >> +
> >> + seq_buf_init(&s, buf, PAGE_SIZE);
> >> + if (health & PAPR_SCM_DIMM_UNARMED_MASK)
> >> + seq_buf_printf(&s, "not_armed ");
> >> +
> >> + if (health & PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK)
> >> + seq_buf_printf(&s, "flush_fail ");
> >> +
> >> + if (health & PAPR_SCM_DIMM_BAD_RESTORE_MASK)
> >> + seq_buf_printf(&s, "restore_fail ");
> >> +
> >> + if (health & PAPR_SCM_DIMM_ENCRYPTED)
> >> + seq_buf_printf(&s, "encrypted ");
> >> +
> >> + if (health & PAPR_SCM_DIMM_SMART_EVENT_MASK)
> >> + seq_buf_printf(&s, "smart_notify ");
> >> +
> >> + if (health & PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED)
> >> + seq_buf_printf(&s, "scrubbed locked ");
> >> +
> >> + if (seq_buf_used(&s))
> >> + seq_buf_printf(&s, "\n");
> >> +
> >> + return seq_buf_used(&s);
> >> +}
> >> +DEVICE_ATTR_RO(flags);
> >> +
> >> +/* papr_scm specific dimm attributes */
> >> +static struct attribute *papr_scm_nd_attributes[] = {
> >> + &dev_attr_flags.attr,
> >> + NULL,
> >> +};
> >> +
> >> +static struct attribute_group papr_scm_nd_attribute_group = {
> >> + .name = "papr",
> >> + .attrs = papr_scm_nd_attributes,
> >> +};
> >> +
> >> +static const struct attribute_group *papr_scm_dimm_attr_groups[] = {
> >> + &papr_scm_nd_attribute_group,
> >> + NULL,
> >> +};
> >> +
> >> static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
> >> {
> >> struct device *dev = &p->pdev->dev;
> >> @@ -312,8 +474,8 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv
*p)
> >> dimm_flags = 0;
> >> set_bit(NDD_LABELING, &dimm_flags);
> >>
> >> - p->nvdimm = nvdimm_create(p->bus, p, NULL, dimm_flags,
> >> - PAPR_SCM_DIMM_CMD_MASK, 0, NULL);
> >> + p->nvdimm = nvdimm_create(p->bus, p, papr_scm_dimm_attr_groups,
> >> + dimm_flags, PAPR_SCM_DIMM_CMD_MASK, 0, NULL);
> >> if (!p->nvdimm) {
> >> dev_err(dev, "Error creating DIMM object for %pOF\n",
p->dn);
> >> goto err;
> >> @@ -399,6 +561,9 @@ static int papr_scm_probe(struct platform_device *pdev)
> >> if (!p)
> >> return -ENOMEM;
> >>
> >> + /* Initialize the dimm mutex */
> >> + mutex_init(&p->health_mutex);
> >> +
> >> /* optional DT properties */
> >> of_property_read_u32(dn, "ibm,metadata-size",
&metadata_size);
> >>
> >> --
> >> 2.26.2
> >> _______________________________________________
> >> Linux-nvdimm mailing list -- linux-nvdimm(a)lists.01.org
> >> To unsubscribe send an email to linux-nvdimm-leave(a)lists.01.org
>
> --
> Cheers
> ~ Vaibhav