Message-ID: <87bllyhdk0.fsf@linux.ibm.com>
Date:   Fri, 05 Jun 2020 00:19:03 +0530
From:   Vaibhav Jain <vaibhav@...ux.ibm.com>
To:     Ira Weiny <ira.weiny@...el.com>
Cc:     Santosh Sivaraj <santosh@...six.org>, linux-nvdimm@...ts.01.org,
        linux-kernel@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>,
        "Oliver O'Halloran" <oohall@...il.com>,
        "Aneesh Kumar K . V" <aneesh.kumar@...ux.ibm.com>,
        Dan Williams <dan.j.williams@...el.com>,
        linuxppc-dev@...ts.ozlabs.org
Subject: Re: [RESEND PATCH v9 5/5] powerpc/papr_scm: Implement support for PAPR_PDSM_HEALTH

Hi Ira,

Thanks again for looking into patch. My responses below:

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

> On Thu, Jun 04, 2020 at 12:34:04AM +0530, Vaibhav Jain wrote:
>> Hi Ira,
>> 
>> Thanks for reviewing this patch. My responses below:
>> 
>> Ira Weiny <ira.weiny@...el.com> writes:
>> 
>> > On Tue, Jun 02, 2020 at 03:44:38PM +0530, Vaibhav Jain wrote:
>> >> This patch implements support for PDSM request 'PAPR_PDSM_HEALTH'
>> >> that returns a newly introduced 'struct nd_papr_pdsm_health' instance
>> >> containing dimm health information back to user space in response to
>> >> ND_CMD_CALL. This functionality is implemented in newly introduced
>> >> papr_pdsm_health() that queries the nvdimm health information and
>> >> then copies this information to the package payload whose layout is
>> >> defined by 'struct nd_papr_pdsm_health'.
>> >> 
>> >> The patch also introduces a new member 'struct papr_scm_priv.health'
>> >> thats an instance of 'struct nd_papr_pdsm_health' to cache the health
>> >> information of a nvdimm. As a result functions drc_pmem_query_health()
>> >> and flags_show() are updated to populate and use this new struct
>> >> instead of a u64 integer that was earlier used.
>> >> 
>> >> Cc: "Aneesh Kumar K . V" <aneesh.kumar@...ux.ibm.com>
>> >> Cc: Dan Williams <dan.j.williams@...el.com>
>> >> Cc: Michael Ellerman <mpe@...erman.id.au>
>> >> Cc: Ira Weiny <ira.weiny@...el.com>
>> >> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@...ux.ibm.com>
>> >> Signed-off-by: Vaibhav Jain <vaibhav@...ux.ibm.com>
>> >> ---
>> >> Changelog:
>> >> 
>> >> Resend:
>> >> * Added ack from Aneesh.
>> >> 
>> >> v8..v9:
>> >> * s/PAPR_SCM_PDSM_HEALTH/PAPR_PDSM_HEALTH/g  [ Dan , Aneesh ]
>> >> * s/PAPR_SCM_PSDM_DIMM_*/PAPR_PDSM_DIMM_*/g
>> >> * Renamed papr_scm_get_health() to papr_psdm_health()
>> >> * Updated patch description to replace papr-scm dimm with nvdimm.
>> >> 
>> >> v7..v8:
>> >> * None
>> >> 
>> >> Resend:
>> >> * None
>> >> 
>> >> v6..v7:
>> >> * Updated flags_show() to use seq_buf_printf(). [Mpe]
>> >> * Updated papr_scm_get_health() to use newly introduced
>> >>   __drc_pmem_query_health() bypassing the cache [Mpe].
>> >> 
>> >> v5..v6:
>> >> * Added attribute '__packed' to 'struct nd_papr_pdsm_health_v1' to
>> >>   gaurd against possibility of different compilers adding different
>> >>   paddings to the struct [ Dan Williams ]
>> >> 
>> >> * Updated 'struct nd_papr_pdsm_health_v1' to use __u8 instead of
>> >>   'bool' and also updated drc_pmem_query_health() to take this into
>> >>   account. [ Dan Williams ]
>> >> 
>> >> v4..v5:
>> >> * None
>> >> 
>> >> v3..v4:
>> >> * Call the DSM_PAPR_SCM_HEALTH service function from
>> >>   papr_scm_service_dsm() instead of papr_scm_ndctl(). [Aneesh]
>> >> 
>> >> v2..v3:
>> >> * Updated struct nd_papr_scm_dimm_health_stat_v1 to use '__xx' types
>> >>   as its exported to the userspace [Aneesh]
>> >> * Changed the constants DSM_PAPR_SCM_DIMM_XX indicating dimm health
>> >>   from enum to #defines [Aneesh]
>> >> 
>> >> v1..v2:
>> >> * New patch in the series
>> >> ---
>> >>  arch/powerpc/include/uapi/asm/papr_pdsm.h |  39 +++++++
>> >>  arch/powerpc/platforms/pseries/papr_scm.c | 125 +++++++++++++++++++---
>> >>  2 files changed, 147 insertions(+), 17 deletions(-)
>> >> 
>> >> diff --git a/arch/powerpc/include/uapi/asm/papr_pdsm.h b/arch/powerpc/include/uapi/asm/papr_pdsm.h
>> >> index 6407fefcc007..411725a91591 100644
>> >> --- a/arch/powerpc/include/uapi/asm/papr_pdsm.h
>> >> +++ b/arch/powerpc/include/uapi/asm/papr_pdsm.h
>> >> @@ -115,6 +115,7 @@ struct nd_pdsm_cmd_pkg {
>> >>   */
>> >>  enum papr_pdsm {
>> >>  	PAPR_PDSM_MIN = 0x0,
>> >> +	PAPR_PDSM_HEALTH,
>> >>  	PAPR_PDSM_MAX,
>> >>  };
>> >>  
>> >> @@ -133,4 +134,42 @@ static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
>> >>  		return (void *)(pcmd->payload);
>> >>  }
>> >>  
>> >> +/* Various nvdimm health indicators */
>> >> +#define PAPR_PDSM_DIMM_HEALTHY       0
>> >> +#define PAPR_PDSM_DIMM_UNHEALTHY     1
>> >> +#define PAPR_PDSM_DIMM_CRITICAL      2
>> >> +#define PAPR_PDSM_DIMM_FATAL         3
>> >> +
>> >> +/*
>> >> + * Struct exchanged between kernel & ndctl in for PAPR_PDSM_HEALTH
>> >> + * Various flags indicate the health status of the dimm.
>> >> + *
>> >> + * dimm_unarmed		: Dimm not armed. So contents wont persist.
>> >> + * dimm_bad_shutdown	: Previous shutdown did not persist contents.
>> >> + * dimm_bad_restore	: Contents from previous shutdown werent restored.
>> >> + * dimm_scrubbed	: Contents of the dimm have been scrubbed.
>> >> + * dimm_locked		: Contents of the dimm cant be modified until CEC reboot
>> >> + * dimm_encrypted	: Contents of dimm are encrypted.
>> >> + * dimm_health		: Dimm health indicator. One of PAPR_PDSM_DIMM_XXXX
>> >> + */
>> >> +struct nd_papr_pdsm_health_v1 {
>> >> +	__u8 dimm_unarmed;
>> >> +	__u8 dimm_bad_shutdown;
>> >> +	__u8 dimm_bad_restore;
>> >> +	__u8 dimm_scrubbed;
>> >> +	__u8 dimm_locked;
>> >> +	__u8 dimm_encrypted;
>> >> +	__u16 dimm_health;
>> >> +} __packed;
>> >> +
>> >> +/*
>> >> + * Typedef the current struct for dimm_health so that any application
>> >> + * or kernel recompiled after introducing a new version automatically
>> >> + * supports the new version.
>> >> + */
>> >> +#define nd_papr_pdsm_health nd_papr_pdsm_health_v1
>> >> +
>> >> +/* Current version number for the dimm health struct */
>> >
>> > This can't be the 'current' version.  You will need a list of versions you
>> > support.  Because if the user passes in an old version you need to be able to
>> > respond with that old version.  Also if you plan to support 'return X for a Y
>> > query' then the user will need both X and Y defined to interpret X.
>> Yes, and that change will be introduced with addition of version-2 of
>> nd_papr_pdsm_health. Earlier version of the patchset[1] had such a table
>> implemented. But to simplify the patchset, as we are only dealing with
>> version-1 of the structs right now, it was dropped.
>> 
>> [1] :
>> https://lore.kernel.org/linuxppc-dev/20200220095805.197229-9-vaibhav@linux.ibm.com/
>
> I'm not sure I follow that comment.
>
> I feel like there is some confusion about what firmware can return vs the UAPI
> structure.  You have already marshaled the data between the 2.  We can define
> whatever we want for the UAPI structures throwing away data the kernel does not
> understand from the firmware.
>
>> 
>> >
>> >> +#define ND_PAPR_PDSM_HEALTH_VERSION 1
>> >> +
>> >>  #endif /* _UAPI_ASM_POWERPC_PAPR_PDSM_H_ */
>> >> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> >> index 5e2237e7ec08..c0606c0c659c 100644
>> >> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> >> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> >> @@ -88,7 +88,7 @@ struct papr_scm_priv {
>> >>  	unsigned long lasthealth_jiffies;
>> >>  
>> >>  	/* Health information for the dimm */
>> >> -	u64 health_bitmap;
>> >> +	struct nd_papr_pdsm_health health;
>> >
>> > ok so we are throwing away all the #defs from patch 1?  Are they still valid?
>> >
>> > I'm confused that patch 3 added this and we are throwing it away
>> > here...
>> The #defines are still valid, only the usage moved to a __drc_pmem_query_health().
>> 
>> >
>> >>  };
>> >>  
>> >>  static int drc_pmem_bind(struct papr_scm_priv *p)
>> >> @@ -201,6 +201,7 @@ static int drc_pmem_query_n_bind(struct papr_scm_priv *p)
>> >>  static int __drc_pmem_query_health(struct papr_scm_priv *p)
>> >>  {
>> >>  	unsigned long ret[PLPAR_HCALL_BUFSIZE];
>> >> +	u64 health;
>> >>  	long rc;
>> >>  
>> >>  	/* issue the hcall */
>> >> @@ -208,18 +209,46 @@ static int __drc_pmem_query_health(struct papr_scm_priv *p)
>> >>  	if (rc != H_SUCCESS) {
>> >>  		dev_err(&p->pdev->dev,
>> >>  			 "Failed to query health information, Err:%ld\n", rc);
>> >> -		rc = -ENXIO;
>> >> -		goto out;
>> >> +		return -ENXIO;
>> >
>> > I missed this...  probably did not need the goto in the first patch?
>> Yes, will get rid of the goto from patch-1.
>
> Cool.
>
>> 
>> >
>> >>  	}
>> >>  
>> >>  	p->lasthealth_jiffies = jiffies;
>> >> -	p->health_bitmap = ret[0] & ret[1];
>> >> +	health = 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;
>> >> +
>> >> +	memset(&p->health, 0, sizeof(p->health));
>> >> +
>> >> +	/* Check for various masks in bitmap and set the buffer */
>> >> +	if (health & PAPR_PMEM_UNARMED_MASK)
>> >
>> > Oh ok...  odd.  (don't add code then just take it away in a series)
>> > You could have lead with the user structure and put this code in patch
>> > 3.
>> The struct nd_papr_pdsm_health in only introduced this patch in header
>> 'papr_pdsm.h' as means of exchanging nvdimm health information with
>> userspace. Introducing this struct without introducing the necessary
>> scafolding in 'papr_pdsm.h' would have been very counter-intutive.
>
> I respectfully disagree.  You intended to use a copy of this structure in
> kernel to store the data.  Just do that.
Have addressed this in v10 that doesnt resort to removing the
functionality that was introduced in an earlier patch.

>
>> 
>> >
>> > Why does the user need u8 to represent a single bit?  Does this help protect
>> > against endian issues?
>> This was 'bool' earlier but since type 'bool' isnt suitable for ioctl abi
>> and I wanted to avoid bit fields here as not sure if their packing may
>> differ across compilers hence replaced with u8.
>> 
>
> ok works for me...
>
>> >
>> >> +		p->health.dimm_unarmed = 1;
>> >> +
>> >> +	if (health & PAPR_PMEM_BAD_SHUTDOWN_MASK)
>> >> +		p->health.dimm_bad_shutdown = 1;
>> >> +
>> >> +	if (health & PAPR_PMEM_BAD_RESTORE_MASK)
>> >> +		p->health.dimm_bad_restore = 1;
>> >> +
>> >> +	if (health & PAPR_PMEM_ENCRYPTED)
>> >> +		p->health.dimm_encrypted = 1;
>> >> +
>> >> +	if (health & PAPR_PMEM_SCRUBBED_AND_LOCKED) {
>> >> +		p->health.dimm_locked = 1;
>> >> +		p->health.dimm_scrubbed = 1;
>> >> +	}
>> >> +
>> >> +	if (health & PAPR_PMEM_HEALTH_UNHEALTHY)
>> >> +		p->health.dimm_health = PAPR_PDSM_DIMM_UNHEALTHY;
>> >> +
>> >> +	if (health & PAPR_PMEM_HEALTH_CRITICAL)
>> >> +		p->health.dimm_health = PAPR_PDSM_DIMM_CRITICAL;
>> >> +
>> >> +	if (health & PAPR_PMEM_HEALTH_FATAL)
>> >> +		p->health.dimm_health = PAPR_PDSM_DIMM_FATAL;
>> >> +
>> >> +	return 0;
>> >>  }
>> >>  
>> >>  /* Min interval in seconds for assuming stable dimm health */
>> >> @@ -403,6 +432,58 @@ static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
>> >>  	return 0;
>> >>  }
>> >>  
>> >> +/* Fetch the DIMM health info and populate it in provided package. */
>> >> +static int papr_pdsm_health(struct papr_scm_priv *p,
>> >> +			       struct nd_pdsm_cmd_pkg *pkg)
>> >> +{
>> >> +	int rc;
>> >> +	size_t copysize = sizeof(p->health);
>> >> +
>> >> +	/* Ensure dimm health mutex is taken preventing concurrent access */
>> >> +	rc = mutex_lock_interruptible(&p->health_mutex);
>> >> +	if (rc)
>> >> +		goto out;
>> >> +
>> >> +	/* Always fetch upto date dimm health data ignoring cached values */
>> >> +	rc = __drc_pmem_query_health(p);
>> >> +	if (rc)
>> >> +		goto out_unlock;
>> >> +	/*
>> >> +	 * If the requested payload version is greater than one we know
>> >> +	 * about, return the payload version we know about and let
>> >> +	 * caller/userspace handle.
>> >> +	 */
>> >> +	if (pkg->payload_version > ND_PAPR_PDSM_HEALTH_VERSION)
>> >> +		pkg->payload_version = ND_PAPR_PDSM_HEALTH_VERSION;
>> >
>> > I know this seems easy now but I do think you will run into trouble later.
>> 
>> I did addressed this in an earlier iteration of this patchset[1] and
>> dropped it in favour of simplicity.
>> 
>> [1] :
>> https://lore.kernel.org/linuxppc-dev/20200220095805.197229-9-vaibhav@linux.ibm.com/
>  
> I don't see how that addresses this?  See my other email.
>
> Ira
>
>> 
>> > Ira
>> >
>> >> +
>> >> +	if (pkg->hdr.nd_size_out < copysize) {
>> >> +		dev_dbg(&p->pdev->dev, "Truncated payload (%u). Expected (%lu)",
>> >> +			pkg->hdr.nd_size_out, copysize);
>> >> +		rc = -ENOSPC;
>> >> +		goto out_unlock;
>> >> +	}
>> >> +
>> >> +	dev_dbg(&p->pdev->dev, "Copying payload size=%lu version=0x%x\n",
>> >> +		copysize, pkg->payload_version);
>> >> +
>> >> +	/* Copy the health struct to the payload */
>> >> +	memcpy(pdsm_cmd_to_payload(pkg), &p->health, copysize);
>> >> +	pkg->hdr.nd_fw_size = copysize;
>> >> +
>> >> +out_unlock:
>> >> +	mutex_unlock(&p->health_mutex);
>> >> +
>> >> +out:
>> >> +	/*
>> >> +	 * Put the error in out package and return success from function
>> >> +	 * so that errors if any are propogated back to userspace.
>> >> +	 */
>> >> +	pkg->cmd_status = rc;
>> >> +	dev_dbg(&p->pdev->dev, "completion code = %d\n", rc);
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >>  static int papr_scm_service_pdsm(struct papr_scm_priv *p,
>> >>  				struct nd_pdsm_cmd_pkg *call_pkg)
>> >>  {
>> >> @@ -417,6 +498,9 @@ static int papr_scm_service_pdsm(struct papr_scm_priv *p,
>> >>  
>> >>  	/* Depending on the DSM command call appropriate service routine */
>> >>  	switch (call_pkg->hdr.nd_command) {
>> >> +	case PAPR_PDSM_HEALTH:
>> >> +		return papr_pdsm_health(p, call_pkg);
>> >> +
>> >>  	default:
>> >>  		dev_dbg(&p->pdev->dev, "Unsupported PDSM request 0x%llx\n",
>> >>  			call_pkg->hdr.nd_command);
>> >> @@ -485,34 +569,41 @@ static ssize_t flags_show(struct device *dev,
>> >>  	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);
>> >>  	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_PMEM_UNARMED_MASK)
>> >> +
>> >> +	/* Protect concurrent modifications to papr_scm_priv */
>> >> +	rc = mutex_lock_interruptible(&p->health_mutex);
>> >> +	if (rc)
>> >> +		return rc;
>> >> +
>> >> +	if (p->health.dimm_unarmed)
>> >>  		seq_buf_printf(&s, "not_armed ");
>> >>  
>> >> -	if (health & PAPR_PMEM_BAD_SHUTDOWN_MASK)
>> >> +	if (p->health.dimm_bad_shutdown)
>> >>  		seq_buf_printf(&s, "flush_fail ");
>> >>  
>> >> -	if (health & PAPR_PMEM_BAD_RESTORE_MASK)
>> >> +	if (p->health.dimm_bad_restore)
>> >>  		seq_buf_printf(&s, "restore_fail ");
>> >>  
>> >> -	if (health & PAPR_PMEM_ENCRYPTED)
>> >> +	if (p->health.dimm_encrypted)
>> >>  		seq_buf_printf(&s, "encrypted ");
>> >>  
>> >> -	if (health & PAPR_PMEM_SMART_EVENT_MASK)
>> >> +	if (p->health.dimm_health)
>> >>  		seq_buf_printf(&s, "smart_notify ");
>> >>  
>> >> -	if (health & PAPR_PMEM_SCRUBBED_AND_LOCKED)
>> >> -		seq_buf_printf(&s, "scrubbed locked ");
>> >> +	if (p->health.dimm_scrubbed)
>> >> +		seq_buf_printf(&s, "scrubbed ");
>> >> +
>> >> +	if (p->health.dimm_locked)
>> >> +		seq_buf_printf(&s, "locked ");
>> >> +
>> >> +	mutex_unlock(&p->health_mutex);
>> >>  
>> >>  	if (seq_buf_used(&s))
>> >>  		seq_buf_printf(&s, "\n");
>> >> -- 
>> >> 2.26.2
>> >> 
>> 
>> -- 
>> Cheers
>> ~ Vaibhav

-- 
Cheers
~ Vaibhav

Powered by blists - more mailing lists