linux-nvdimm.lists.01.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
@ 2021-04-14 12:40 Vaibhav Jain
  2021-04-14 15:36 ` Ira Weiny
  2021-04-14 21:32 ` Dan Williams
  0 siblings, 2 replies; 9+ messages in thread
From: Vaibhav Jain @ 2021-04-14 12:40 UTC (permalink / raw)
  To: linuxppc-dev, linux-nvdimm
  Cc: Vaibhav Jain, Aneesh Kumar K . V, Michael Ellerman

Currently drc_pmem_qeury_stats() generates a dev_err in case
"Enable Performance Information Collection" feature is disabled from
HMC. The error is of the form below:

papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query
	 performance stats, Err:-10

This error message confuses users as it implies a possible problem
with the nvdimm even though its due to a disabled feature.

So we fix this by explicitly handling the H_AUTHORITY error from the
H_SCM_PERFORMANCE_STATS hcall and generating a warning instead of an
error, saying that "Performance stats in-accessible".

Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/papr_scm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 835163f54244..9216424f8be3 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -277,6 +277,9 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
 		dev_err(&p->pdev->dev,
 			"Unknown performance stats, Err:0x%016lX\n", ret[0]);
 		return -ENOENT;
+	} else if (rc == H_AUTHORITY) {
+		dev_warn(&p->pdev->dev, "Performance stats in-accessible");
+		return -EPERM;
 	} else if (rc != H_SUCCESS) {
 		dev_err(&p->pdev->dev,
 			"Failed to query performance stats, Err:%lld\n", rc);
-- 
2.30.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
  2021-04-14 12:40 [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible Vaibhav Jain
@ 2021-04-14 15:36 ` Ira Weiny
  2021-04-14 16:21   ` Vaibhav Jain
  2021-04-14 21:32 ` Dan Williams
  1 sibling, 1 reply; 9+ messages in thread
From: Ira Weiny @ 2021-04-14 15:36 UTC (permalink / raw)
  To: Vaibhav Jain
  Cc: linuxppc-dev, linux-nvdimm, Aneesh Kumar K . V, Michael Ellerman

On Wed, Apr 14, 2021 at 06:10:26PM +0530, Vaibhav Jain wrote:
> Currently drc_pmem_qeury_stats() generates a dev_err in case
> "Enable Performance Information Collection" feature is disabled from
> HMC. The error is of the form below:
> 
> papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query
> 	 performance stats, Err:-10
> 
> This error message confuses users as it implies a possible problem
> with the nvdimm even though its due to a disabled feature.
> 
> So we fix this by explicitly handling the H_AUTHORITY error from the
> H_SCM_PERFORMANCE_STATS hcall and generating a warning instead of an
> error, saying that "Performance stats in-accessible".
> 
> Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/papr_scm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 835163f54244..9216424f8be3 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -277,6 +277,9 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
>  		dev_err(&p->pdev->dev,
>  			"Unknown performance stats, Err:0x%016lX\n", ret[0]);
>  		return -ENOENT;
> +	} else if (rc == H_AUTHORITY) {
> +		dev_warn(&p->pdev->dev, "Performance stats in-accessible");
> +		return -EPERM;

Is this because of a disabled feature or because of permissions?

Ira

>  	} else if (rc != H_SUCCESS) {
>  		dev_err(&p->pdev->dev,
>  			"Failed to query performance stats, Err:%lld\n", rc);
> -- 
> 2.30.2
> 
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
  2021-04-14 15:36 ` Ira Weiny
@ 2021-04-14 16:21   ` Vaibhav Jain
  2021-04-14 21:24     ` Ira Weiny
  0 siblings, 1 reply; 9+ messages in thread
From: Vaibhav Jain @ 2021-04-14 16:21 UTC (permalink / raw)
  To: Ira Weiny
  Cc: linuxppc-dev, linux-nvdimm, Aneesh Kumar K . V, Michael Ellerman

Thanks for looking into this patch Ira,

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

> On Wed, Apr 14, 2021 at 06:10:26PM +0530, Vaibhav Jain wrote:
>> Currently drc_pmem_qeury_stats() generates a dev_err in case
>> "Enable Performance Information Collection" feature is disabled from
>> HMC. The error is of the form below:
>> 
>> papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query
>> 	 performance stats, Err:-10
>> 
>> This error message confuses users as it implies a possible problem
>> with the nvdimm even though its due to a disabled feature.
>> 
>> So we fix this by explicitly handling the H_AUTHORITY error from the
>> H_SCM_PERFORMANCE_STATS hcall and generating a warning instead of an
>> error, saying that "Performance stats in-accessible".
>> 
>> Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>>  arch/powerpc/platforms/pseries/papr_scm.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 835163f54244..9216424f8be3 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -277,6 +277,9 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
>>  		dev_err(&p->pdev->dev,
>>  			"Unknown performance stats, Err:0x%016lX\n", ret[0]);
>>  		return -ENOENT;
>> +	} else if (rc == H_AUTHORITY) {
>> +		dev_warn(&p->pdev->dev, "Performance stats in-accessible");
>> +		return -EPERM;
>
> Is this because of a disabled feature or because of permissions?

Its because of a disabled feature that revokes permission for a guest to
retrieve performance statistics.

The feature is called "Enable Performance Information Collection" and
once disabled the hcall H_SCM_PERFORMANCE_STATS returns an error
H_AUTHORITY indicating that the guest doesn't have permission to retrieve
performance statistics.

>
> Ira
>
>>  	} else if (rc != H_SUCCESS) {
>>  		dev_err(&p->pdev->dev,
>>  			"Failed to query performance stats, Err:%lld\n", rc);
>> -- 
>> 2.30.2
>> 

-- 
Cheers
~ Vaibhav
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
  2021-04-14 16:21   ` Vaibhav Jain
@ 2021-04-14 21:24     ` Ira Weiny
  2021-04-15 11:48       ` Vaibhav Jain
  0 siblings, 1 reply; 9+ messages in thread
From: Ira Weiny @ 2021-04-14 21:24 UTC (permalink / raw)
  To: Vaibhav Jain
  Cc: linuxppc-dev, linux-nvdimm, Aneesh Kumar K . V, Michael Ellerman

On Wed, Apr 14, 2021 at 09:51:40PM +0530, Vaibhav Jain wrote:
> Thanks for looking into this patch Ira,
> 
> Ira Weiny <ira.weiny@intel.com> writes:
> 
> > On Wed, Apr 14, 2021 at 06:10:26PM +0530, Vaibhav Jain wrote:
> >> Currently drc_pmem_qeury_stats() generates a dev_err in case
> >> "Enable Performance Information Collection" feature is disabled from
> >> HMC. The error is of the form below:
> >> 
> >> papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query
> >> 	 performance stats, Err:-10
> >> 
> >> This error message confuses users as it implies a possible problem
> >> with the nvdimm even though its due to a disabled feature.
> >> 
> >> So we fix this by explicitly handling the H_AUTHORITY error from the
> >> H_SCM_PERFORMANCE_STATS hcall and generating a warning instead of an
> >> error, saying that "Performance stats in-accessible".
> >> 
> >> Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
> >> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> >> ---
> >>  arch/powerpc/platforms/pseries/papr_scm.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >> 
> >> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> >> index 835163f54244..9216424f8be3 100644
> >> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> >> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> >> @@ -277,6 +277,9 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
> >>  		dev_err(&p->pdev->dev,
> >>  			"Unknown performance stats, Err:0x%016lX\n", ret[0]);
> >>  		return -ENOENT;
> >> +	} else if (rc == H_AUTHORITY) {
> >> +		dev_warn(&p->pdev->dev, "Performance stats in-accessible");
> >> +		return -EPERM;
> >
> > Is this because of a disabled feature or because of permissions?
> 
> Its because of a disabled feature that revokes permission for a guest to
> retrieve performance statistics.
> 
> The feature is called "Enable Performance Information Collection" and
> once disabled the hcall H_SCM_PERFORMANCE_STATS returns an error
> H_AUTHORITY indicating that the guest doesn't have permission to retrieve
> performance statistics.

In that case would it be appropriate to have the error message indicate a
permission issue?

Something like 'permission denied'?

Ira
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
  2021-04-14 12:40 [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible Vaibhav Jain
  2021-04-14 15:36 ` Ira Weiny
@ 2021-04-14 21:32 ` Dan Williams
  2021-04-15 11:43   ` Vaibhav Jain
  1 sibling, 1 reply; 9+ messages in thread
From: Dan Williams @ 2021-04-14 21:32 UTC (permalink / raw)
  To: Vaibhav Jain
  Cc: linuxppc-dev, linux-nvdimm, Aneesh Kumar K . V, Michael Ellerman

On Wed, Apr 14, 2021 at 5:40 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote:
>
> Currently drc_pmem_qeury_stats() generates a dev_err in case
> "Enable Performance Information Collection" feature is disabled from
> HMC. The error is of the form below:
>
> papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query
>          performance stats, Err:-10
>
> This error message confuses users as it implies a possible problem
> with the nvdimm even though its due to a disabled feature.
>
> So we fix this by explicitly handling the H_AUTHORITY error from the
> H_SCM_PERFORMANCE_STATS hcall and generating a warning instead of an
> error, saying that "Performance stats in-accessible".
>
> Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/papr_scm.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 835163f54244..9216424f8be3 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -277,6 +277,9 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
>                 dev_err(&p->pdev->dev,
>                         "Unknown performance stats, Err:0x%016lX\n", ret[0]);
>                 return -ENOENT;
> +       } else if (rc == H_AUTHORITY) {
> +               dev_warn(&p->pdev->dev, "Performance stats in-accessible");
> +               return -EPERM;

So userspace can spam the kernel log? Why is kernel log message needed
at all? EPERM told the caller what happened.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
  2021-04-14 21:32 ` Dan Williams
@ 2021-04-15 11:43   ` Vaibhav Jain
  2021-04-15 17:03     ` Dan Williams
  0 siblings, 1 reply; 9+ messages in thread
From: Vaibhav Jain @ 2021-04-15 11:43 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-nvdimm, Aneesh Kumar K . V, linuxppc-dev

Thanks for looking into this Dan,

Dan Williams <dan.j.williams@intel.com> writes:

> On Wed, Apr 14, 2021 at 5:40 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote:
>>
>> Currently drc_pmem_qeury_stats() generates a dev_err in case
>> "Enable Performance Information Collection" feature is disabled from
>> HMC. The error is of the form below:
>>
>> papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query
>>          performance stats, Err:-10
>>
>> This error message confuses users as it implies a possible problem
>> with the nvdimm even though its due to a disabled feature.
>>
>> So we fix this by explicitly handling the H_AUTHORITY error from the
>> H_SCM_PERFORMANCE_STATS hcall and generating a warning instead of an
>> error, saying that "Performance stats in-accessible".
>>
>> Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>>  arch/powerpc/platforms/pseries/papr_scm.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 835163f54244..9216424f8be3 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -277,6 +277,9 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
>>                 dev_err(&p->pdev->dev,
>>                         "Unknown performance stats, Err:0x%016lX\n", ret[0]);
>>                 return -ENOENT;
>> +       } else if (rc == H_AUTHORITY) {
>> +               dev_warn(&p->pdev->dev, "Performance stats in-accessible");
>> +               return -EPERM;
>
> So userspace can spam the kernel log? Why is kernel log message needed
> at all? EPERM told the caller what happened.
Currently this error message is only reported during probe of the
nvdimm. So userspace cannot directly spam kernel log.

The callsite for this function in papr_scm_nvdimm_init() doesnt handle
specific error codes. Instead in case of an error it only reports that
"Dimm performance stats are unavailable". The log message just
preceeding that mentions the real cause of failure. Thats why just
returning -EPERM wont be usefui.

Alternatively I can update papr_scm_nvdimm_init() to report the error
code returned from drc_pmem_query_stats().

-- 
Cheers
~ Vaibhav
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
  2021-04-14 21:24     ` Ira Weiny
@ 2021-04-15 11:48       ` Vaibhav Jain
  2021-04-18  7:58         ` kajoljain
  0 siblings, 1 reply; 9+ messages in thread
From: Vaibhav Jain @ 2021-04-15 11:48 UTC (permalink / raw)
  To: Ira Weiny; +Cc: linux-nvdimm, linuxppc-dev

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

> On Wed, Apr 14, 2021 at 09:51:40PM +0530, Vaibhav Jain wrote:
>> Thanks for looking into this patch Ira,
>> 
>> Ira Weiny <ira.weiny@intel.com> writes:
>> 
>> > On Wed, Apr 14, 2021 at 06:10:26PM +0530, Vaibhav Jain wrote:
>> >> Currently drc_pmem_qeury_stats() generates a dev_err in case
>> >> "Enable Performance Information Collection" feature is disabled from
>> >> HMC. The error is of the form below:
>> >> 
>> >> papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query
>> >> 	 performance stats, Err:-10
>> >> 
>> >> This error message confuses users as it implies a possible problem
>> >> with the nvdimm even though its due to a disabled feature.
>> >> 
>> >> So we fix this by explicitly handling the H_AUTHORITY error from the
>> >> H_SCM_PERFORMANCE_STATS hcall and generating a warning instead of an
>> >> error, saying that "Performance stats in-accessible".
>> >> 
>> >> Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
>> >> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> >> ---
>> >>  arch/powerpc/platforms/pseries/papr_scm.c | 3 +++
>> >>  1 file changed, 3 insertions(+)
>> >> 
>> >> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> >> index 835163f54244..9216424f8be3 100644
>> >> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> >> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> >> @@ -277,6 +277,9 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
>> >>  		dev_err(&p->pdev->dev,
>> >>  			"Unknown performance stats, Err:0x%016lX\n", ret[0]);
>> >>  		return -ENOENT;
>> >> +	} else if (rc == H_AUTHORITY) {
>> >> +		dev_warn(&p->pdev->dev, "Performance stats in-accessible");
>> >> +		return -EPERM;
>> >
>> > Is this because of a disabled feature or because of permissions?
>> 
>> Its because of a disabled feature that revokes permission for a guest to
>> retrieve performance statistics.
>> 
>> The feature is called "Enable Performance Information Collection" and
>> once disabled the hcall H_SCM_PERFORMANCE_STATS returns an error
>> H_AUTHORITY indicating that the guest doesn't have permission to retrieve
>> performance statistics.
>
> In that case would it be appropriate to have the error message indicate a
> permission issue?
>
> Something like 'permission denied'?

Yes, Something like "Permission denied while accessing performance
stats" might be more clear and intuitive.

Will update the warn message in v2.

>
> Ira
>

-- 
Cheers
~ Vaibhav
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
  2021-04-15 11:43   ` Vaibhav Jain
@ 2021-04-15 17:03     ` Dan Williams
  0 siblings, 0 replies; 9+ messages in thread
From: Dan Williams @ 2021-04-15 17:03 UTC (permalink / raw)
  To: Vaibhav Jain; +Cc: linux-nvdimm, Aneesh Kumar K . V, linuxppc-dev

On Thu, Apr 15, 2021 at 4:44 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote:
>
> Thanks for looking into this Dan,
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
> > On Wed, Apr 14, 2021 at 5:40 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote:
> >>
> >> Currently drc_pmem_qeury_stats() generates a dev_err in case
> >> "Enable Performance Information Collection" feature is disabled from
> >> HMC. The error is of the form below:
> >>
> >> papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query
> >>          performance stats, Err:-10
> >>
> >> This error message confuses users as it implies a possible problem
> >> with the nvdimm even though its due to a disabled feature.
> >>
> >> So we fix this by explicitly handling the H_AUTHORITY error from the
> >> H_SCM_PERFORMANCE_STATS hcall and generating a warning instead of an
> >> error, saying that "Performance stats in-accessible".
> >>
> >> Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
> >> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> >> ---
> >>  arch/powerpc/platforms/pseries/papr_scm.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> >> index 835163f54244..9216424f8be3 100644
> >> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> >> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> >> @@ -277,6 +277,9 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
> >>                 dev_err(&p->pdev->dev,
> >>                         "Unknown performance stats, Err:0x%016lX\n", ret[0]);
> >>                 return -ENOENT;
> >> +       } else if (rc == H_AUTHORITY) {
> >> +               dev_warn(&p->pdev->dev, "Performance stats in-accessible");
> >> +               return -EPERM;
> >
> > So userspace can spam the kernel log? Why is kernel log message needed
> > at all? EPERM told the caller what happened.
> Currently this error message is only reported during probe of the
> nvdimm. So userspace cannot directly spam kernel log.

Oh, ok, I saw things like papr_pdsm_fuel_gauge() in the call stack and
thought this was reachable through an ioctl. Sorry for the noise.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
  2021-04-15 11:48       ` Vaibhav Jain
@ 2021-04-18  7:58         ` kajoljain
  0 siblings, 0 replies; 9+ messages in thread
From: kajoljain @ 2021-04-18  7:58 UTC (permalink / raw)
  To: Vaibhav Jain, Ira Weiny; +Cc: Aneesh Kumar K . V, linuxppc-dev, linux-nvdimm



On 4/15/21 5:18 PM, Vaibhav Jain wrote:
> Ira Weiny <ira.weiny@intel.com> writes:
> 
>> On Wed, Apr 14, 2021 at 09:51:40PM +0530, Vaibhav Jain wrote:
>>> Thanks for looking into this patch Ira,
>>>
>>> Ira Weiny <ira.weiny@intel.com> writes:
>>>
>>>> On Wed, Apr 14, 2021 at 06:10:26PM +0530, Vaibhav Jain wrote:
>>>>> Currently drc_pmem_qeury_stats() generates a dev_err in case
>>>>> "Enable Performance Information Collection" feature is disabled from
>>>>> HMC. The error is of the form below:
>>>>>
>>>>> papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query
>>>>> 	 performance stats, Err:-10
>>>>>
>>>>> This error message confuses users as it implies a possible problem
>>>>> with the nvdimm even though its due to a disabled feature.
>>>>>
>>>>> So we fix this by explicitly handling the H_AUTHORITY error from the
>>>>> H_SCM_PERFORMANCE_STATS hcall and generating a warning instead of an
>>>>> error, saying that "Performance stats in-accessible".
>>>>>
>>>>> Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
>>>>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>>>>> ---
>>>>>  arch/powerpc/platforms/pseries/papr_scm.c | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>>>>> index 835163f54244..9216424f8be3 100644
>>>>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>>>>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>>>>> @@ -277,6 +277,9 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
>>>>>  		dev_err(&p->pdev->dev,
>>>>>  			"Unknown performance stats, Err:0x%016lX\n", ret[0]);
>>>>>  		return -ENOENT;
>>>>> +	} else if (rc == H_AUTHORITY) {
>>>>> +		dev_warn(&p->pdev->dev, "Performance stats in-accessible");
>>>>> +		return -EPERM;
>>>>
>>>> Is this because of a disabled feature or because of permissions?
>>>
>>> Its because of a disabled feature that revokes permission for a guest to
>>> retrieve performance statistics.
>>>
>>> The feature is called "Enable Performance Information Collection" and
>>> once disabled the hcall H_SCM_PERFORMANCE_STATS returns an error
>>> H_AUTHORITY indicating that the guest doesn't have permission to retrieve
>>> performance statistics.
>>
>> In that case would it be appropriate to have the error message indicate a
>> permission issue?
>>
>> Something like 'permission denied'?
> 
> Yes, Something like "Permission denied while accessing performance
> stats" might be more clear and intuitive.

Hi Vaibhav,
   Thanks for the patch. I agree with Ira and above warning message with "Permission denied" looks more clear.
With that change, patch looks good to me.

Reviewed-By: Kajol Jain<kjain@linux.ibm.com>

Thanks,
Kajol Jain
> 
> Will update the warn message in v2.
> 
>>
>> Ira
>>
> 
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-04-18  7:59 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-14 12:40 [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible Vaibhav Jain
2021-04-14 15:36 ` Ira Weiny
2021-04-14 16:21   ` Vaibhav Jain
2021-04-14 21:24     ` Ira Weiny
2021-04-15 11:48       ` Vaibhav Jain
2021-04-18  7:58         ` kajoljain
2021-04-14 21:32 ` Dan Williams
2021-04-15 11:43   ` Vaibhav Jain
2021-04-15 17:03     ` Dan Williams

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).