diff mbox series

[v2,1/3] perf/x86/core: Use sysfs_emit() in show() callback function

Message ID 719558b8a435a6f942b2cc3f5c0431212be76bc6.1676987821.git.drv@mailo.com
State New
Headers
Series perf/x86: Use sysfs_emit() in show() callback function |

Commit Message

Deepak R Varma Feb. 21, 2023, 2:06 p.m. UTC 
  As per C99 standard, snprintf() returns the number of bytes that would
be encoded in the destination buffer when it is sufficiently large. This
return value may be different from what the caller is expecting and hence
may lead to potential errors in the program.
Kernel release 2.6.2 introduced scnprintf() & vscnprintf() which precisely
return the actual bytes encoded into the destination buffer.

For the sysfs attribute show() callback functions, which returns the number
of bytes to the user space, a more recent recommendation is to use
sysfs_emit() or sysfs_emit_at() instead of sprintf() family of functions.
This is recorded in the Documentation/filesystems/sysfs.rst Kernel
documentation file.

Issue identified using the coccinelle device_attr_show.cocci script.

Signed-off-by: Deepak R Varma <drv@mailo.com>
---
Changes in v2:
   - Revise patch log message to include details on the potential issues with
     current implementation and how the proposal is a better solution.
     Feedback provided by Peter Zijlstra <peterz@infradead.org>

 arch/x86/events/core.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Peter Zijlstra Feb. 22, 2023, 8:35 p.m. UTC | #1 
On Tue, Feb 21, 2023 at 07:36:12PM +0530, Deepak R Varma wrote:
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 85a63a41c471..27c03e6dcb5d 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -1896,9 +1896,7 @@ ssize_t events_hybrid_sysfs_show(struct device *dev,
>  		if (x86_pmu.hybrid_pmu[i].cpu_type & pmu->cpu_type) {
>  			next_str = strchr(str, ';');
>  			if (next_str)
> -				return snprintf(page, next_str - str + 1, "%s", str);
> -			else
> -				return sprintf(page, "%s", str);
> +				return sysfs_emit(page, "%s", str);
>  		}
>  		str = strchr(str, ';');
>  		str++;

How is this correct ?!?
Deepak R Varma Feb. 23, 2023, 4:55 a.m. UTC | #2 
On Wed, Feb 22, 2023 at 09:35:53PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 21, 2023 at 07:36:12PM +0530, Deepak R Varma wrote:
> > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> > index 85a63a41c471..27c03e6dcb5d 100644
> > --- a/arch/x86/events/core.c
> > +++ b/arch/x86/events/core.c
> > @@ -1896,9 +1896,7 @@ ssize_t events_hybrid_sysfs_show(struct device *dev,
> >  		if (x86_pmu.hybrid_pmu[i].cpu_type & pmu->cpu_type) {
> >  			next_str = strchr(str, ';');
> >  			if (next_str)
> > -				return snprintf(page, next_str - str + 1, "%s", str);
> > -			else
> > -				return sprintf(page, "%s", str);
> > +				return sysfs_emit(page, "%s", str);
> >  		}
> >  		str = strchr(str, ';');
> >  		str++;
> 
> How is this correct ?!?

oops.. that is bad on my part. My apologies for the wrong code.
I will correct it and send in v3.

Thank you Peter.

Regards,
./drv
Deepak R Varma Feb. 26, 2023, 5:36 p.m. UTC | #3 
On Thu, Feb 23, 2023 at 10:25:45AM +0530, Deepak R Varma wrote:
> On Wed, Feb 22, 2023 at 09:35:53PM +0100, Peter Zijlstra wrote:
> > On Tue, Feb 21, 2023 at 07:36:12PM +0530, Deepak R Varma wrote:
> > > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> > > index 85a63a41c471..27c03e6dcb5d 100644
> > > --- a/arch/x86/events/core.c
> > > +++ b/arch/x86/events/core.c
> > > @@ -1896,9 +1896,7 @@ ssize_t events_hybrid_sysfs_show(struct device *dev,
> > >  		if (x86_pmu.hybrid_pmu[i].cpu_type & pmu->cpu_type) {
> > >  			next_str = strchr(str, ';');
> > >  			if (next_str)
> > > -				return snprintf(page, next_str - str + 1, "%s", str);
> > > -			else
> > > -				return sprintf(page, "%s", str);
> > > +				return sysfs_emit(page, "%s", str);
> > >  		}
> > >  		str = strchr(str, ';');
> > >  		str++;
> > 
> > How is this correct ?!?
> 
> oops.. that is bad on my part. My apologies for the wrong code.
> I will correct it and send in v3.

Hello Peter,
I reviewed the code more closely and concluded that the current implementation
is better as is. I sent in a v3 with necessary correction for your review.

I do have another observation from this area that I will send in as a separate
patch soon.

Thank you again.
./drv


> 
> Thank you Peter.
> 
> Regards,
> ./drv
diff mbox series

Patch

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 85a63a41c471..27c03e6dcb5d 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1896,9 +1896,7 @@  ssize_t events_hybrid_sysfs_show(struct device *dev,
 		if (x86_pmu.hybrid_pmu[i].cpu_type & pmu->cpu_type) {
 			next_str = strchr(str, ';');
 			if (next_str)
-				return snprintf(page, next_str - str + 1, "%s", str);
-			else
-				return sprintf(page, "%s", str);
+				return sysfs_emit(page, "%s", str);
 		}
 		str = strchr(str, ';');
 		str++;
@@ -2544,7 +2542,7 @@  static ssize_t get_attr_rdpmc(struct device *cdev,
 			      struct device_attribute *attr,
 			      char *buf)
 {
-	return snprintf(buf, 40, "%d\n", x86_pmu.attr_rdpmc);
+	return sysfs_emit(buf, "%d\n", x86_pmu.attr_rdpmc);
 }
 
 static ssize_t set_attr_rdpmc(struct device *cdev,
@@ -2602,7 +2600,7 @@  static ssize_t max_precise_show(struct device *cdev,
 				  struct device_attribute *attr,
 				  char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%d\n", x86_pmu_max_precise());
+	return sysfs_emit(buf, "%d\n", x86_pmu_max_precise());
 }
 
 static DEVICE_ATTR_RO(max_precise);