drm/bridge: it6505: replace scnprintf with sysfs_emit_at in debugfs show

[PATCH] drm/bridge: it6505: replace scnprintf with sysfs_emit_at in debugfs show

Posted by Marcelo Moreira 3 months, 1 week ago

Update the receive_timing_debugfs_show() function to utilize
sysfs_emit_at() for formatting output to the debugfs buffer.
This change adheres to the recommendation outlined
in Documentation/filesystems/sysfs.rst.

This modification aligns with current sysfs guidelines.

Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
---
 drivers/gpu/drm/bridge/ite-it6505.c | 46 ++++++++++++++---------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 1383d1e21afe..98bea08a14e4 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -3427,37 +3427,35 @@ static ssize_t receive_timing_debugfs_show(struct file *file, char __user *buf,
 	struct it6505 *it6505 = file->private_data;
 	struct drm_display_mode *vid;
 	u8 read_buf[READ_BUFFER_SIZE];
-	u8 *str = read_buf, *end = read_buf + READ_BUFFER_SIZE;
-	ssize_t ret, count;
+	ssize_t ret;
+	ssize_t count = 0;
 
 	if (!it6505)
 		return -ENODEV;
 
 	it6505_calc_video_info(it6505);
 	vid = &it6505->video_info;
-	str += scnprintf(str, end - str, "---video timing---\n");
-	str += scnprintf(str, end - str, "PCLK:%d.%03dMHz\n",
-			 vid->clock / 1000, vid->clock % 1000);
-	str += scnprintf(str, end - str, "HTotal:%d\n", vid->htotal);
-	str += scnprintf(str, end - str, "HActive:%d\n", vid->hdisplay);
-	str += scnprintf(str, end - str, "HFrontPorch:%d\n",
-			 vid->hsync_start - vid->hdisplay);
-	str += scnprintf(str, end - str, "HSyncWidth:%d\n",
-			 vid->hsync_end - vid->hsync_start);
-	str += scnprintf(str, end - str, "HBackPorch:%d\n",
-			 vid->htotal - vid->hsync_end);
-	str += scnprintf(str, end - str, "VTotal:%d\n", vid->vtotal);
-	str += scnprintf(str, end - str, "VActive:%d\n", vid->vdisplay);
-	str += scnprintf(str, end - str, "VFrontPorch:%d\n",
-			 vid->vsync_start - vid->vdisplay);
-	str += scnprintf(str, end - str, "VSyncWidth:%d\n",
-			 vid->vsync_end - vid->vsync_start);
-	str += scnprintf(str, end - str, "VBackPorch:%d\n",
-			 vid->vtotal - vid->vsync_end);
-
-	count = str - read_buf;
+	count += sysfs_emit_at(read_buf, count, "---video timing---\n");
+	count += sysfs_emit_at(read_buf, count, "PCLK:%d.%03dMHz\n",
+			vid->clock / 1000, vid->clock % 1000);
+	count += sysfs_emit_at(read_buf, count, "HTotal:%d\n", vid->htotal);
+	count += sysfs_emit_at(read_buf, count, "HActive:%d\n", vid->hdisplay);
+	count += sysfs_emit_at(read_buf, count, "HFrontPorch:%d\n",
+			vid->hsync_start - vid->hdisplay);
+	count += sysfs_emit_at(read_buf, count, "HSyncWidth:%d\n",
+			vid->hsync_end - vid->hsync_start);
+	count += sysfs_emit_at(read_buf, count, "HBackPorch:%d\n",
+			vid->htotal - vid->hsync_end);
+	count += sysfs_emit_at(read_buf, count, "VTotal:%d\n", vid->vtotal);
+	count += sysfs_emit_at(read_buf, count, "VActive:%d\n", vid->vdisplay);
+	count += sysfs_emit_at(read_buf, count, "VFrontPorch:%d\n",
+			vid->vsync_start - vid->vdisplay);
+	count += sysfs_emit_at(read_buf, count, "VSyncWidth:%d\n",
+			vid->vsync_end - vid->vsync_start);
+	count += sysfs_emit_at(read_buf, count, "VBackPorch:%d\n",
+			vid->vtotal - vid->vsync_end);
+	
 	ret = simple_read_from_buffer(buf, len, ppos, read_buf, count);
-
 	return ret;
 }
 
-- 
2.50.0

Re: [PATCH] drm/bridge: it6505: replace scnprintf with sysfs_emit_at in debugfs show

Posted by Greg KH 3 months, 1 week ago

On Sun, Jun 29, 2025 at 08:35:09PM -0300, Marcelo Moreira wrote:
> Update the receive_timing_debugfs_show() function to utilize
> sysfs_emit_at() for formatting output to the debugfs buffer.
> This change adheres to the recommendation outlined
> in Documentation/filesystems/sysfs.rst.
> 
> This modification aligns with current sysfs guidelines.

But this isn't a sysfs file, it's a debugfs file, so why are you calling
sysfs_emit_at()?

> 
> Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
> ---
>  drivers/gpu/drm/bridge/ite-it6505.c | 46 ++++++++++++++---------------
>  1 file changed, 22 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 1383d1e21afe..98bea08a14e4 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -3427,37 +3427,35 @@ static ssize_t receive_timing_debugfs_show(struct file *file, char __user *buf,
>  	struct it6505 *it6505 = file->private_data;
>  	struct drm_display_mode *vid;
>  	u8 read_buf[READ_BUFFER_SIZE];
> -	u8 *str = read_buf, *end = read_buf + READ_BUFFER_SIZE;
> -	ssize_t ret, count;
> +	ssize_t ret;
> +	ssize_t count = 0;
>  
>  	if (!it6505)
>  		return -ENODEV;
>  
>  	it6505_calc_video_info(it6505);
>  	vid = &it6505->video_info;
> -	str += scnprintf(str, end - str, "---video timing---\n");
> -	str += scnprintf(str, end - str, "PCLK:%d.%03dMHz\n",
> -			 vid->clock / 1000, vid->clock % 1000);
> -	str += scnprintf(str, end - str, "HTotal:%d\n", vid->htotal);
> -	str += scnprintf(str, end - str, "HActive:%d\n", vid->hdisplay);
> -	str += scnprintf(str, end - str, "HFrontPorch:%d\n",
> -			 vid->hsync_start - vid->hdisplay);
> -	str += scnprintf(str, end - str, "HSyncWidth:%d\n",
> -			 vid->hsync_end - vid->hsync_start);
> -	str += scnprintf(str, end - str, "HBackPorch:%d\n",
> -			 vid->htotal - vid->hsync_end);
> -	str += scnprintf(str, end - str, "VTotal:%d\n", vid->vtotal);
> -	str += scnprintf(str, end - str, "VActive:%d\n", vid->vdisplay);
> -	str += scnprintf(str, end - str, "VFrontPorch:%d\n",
> -			 vid->vsync_start - vid->vdisplay);
> -	str += scnprintf(str, end - str, "VSyncWidth:%d\n",
> -			 vid->vsync_end - vid->vsync_start);
> -	str += scnprintf(str, end - str, "VBackPorch:%d\n",
> -			 vid->vtotal - vid->vsync_end);
> -
> -	count = str - read_buf;
> +	count += sysfs_emit_at(read_buf, count, "---video timing---\n");
> +	count += sysfs_emit_at(read_buf, count, "PCLK:%d.%03dMHz\n",
> +			vid->clock / 1000, vid->clock % 1000);
> +	count += sysfs_emit_at(read_buf, count, "HTotal:%d\n", vid->htotal);
> +	count += sysfs_emit_at(read_buf, count, "HActive:%d\n", vid->hdisplay);
> +	count += sysfs_emit_at(read_buf, count, "HFrontPorch:%d\n",
> +			vid->hsync_start - vid->hdisplay);
> +	count += sysfs_emit_at(read_buf, count, "HSyncWidth:%d\n",
> +			vid->hsync_end - vid->hsync_start);
> +	count += sysfs_emit_at(read_buf, count, "HBackPorch:%d\n",
> +			vid->htotal - vid->hsync_end);
> +	count += sysfs_emit_at(read_buf, count, "VTotal:%d\n", vid->vtotal);
> +	count += sysfs_emit_at(read_buf, count, "VActive:%d\n", vid->vdisplay);
> +	count += sysfs_emit_at(read_buf, count, "VFrontPorch:%d\n",
> +			vid->vsync_start - vid->vdisplay);
> +	count += sysfs_emit_at(read_buf, count, "VSyncWidth:%d\n",
> +			vid->vsync_end - vid->vsync_start);
> +	count += sysfs_emit_at(read_buf, count, "VBackPorch:%d\n",
> +			vid->vtotal - vid->vsync_end);
> +	
>  	ret = simple_read_from_buffer(buf, len, ppos, read_buf, count);
> -

Shouldn't this all be using seq_print() instead?

Again, don't use sysfs_emit*() functions for non-sysfs files, as you do
NOT know the size of the buffer here (hint, it's not the same).

And, your patch added trailing whitespace, did you forget to run it
through checkpatch.pl before sending it?

thanks,

greg k-h

Re: [PATCH] drm/bridge: it6505: replace scnprintf with sysfs_emit_at in debugfs show

Posted by Marcelo Moreira 3 months, 1 week ago

Em seg., 30 de jun. de 2025 às 02:09, Greg KH <greg@kroah.com> escreveu:
>
> On Sun, Jun 29, 2025 at 08:35:09PM -0300, Marcelo Moreira wrote:
> > Update the receive_timing_debugfs_show() function to utilize
> > sysfs_emit_at() for formatting output to the debugfs buffer.
> > This change adheres to the recommendation outlined
> > in Documentation/filesystems/sysfs.rst.
> >
> > This modification aligns with current sysfs guidelines.
>
> But this isn't a sysfs file, it's a debugfs file, so why are you calling
> sysfs_emit_at()?
>

You're right, thanks Greg.

> >
> > Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
> > ---
> >  drivers/gpu/drm/bridge/ite-it6505.c | 46 ++++++++++++++---------------
> >  1 file changed, 22 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> > index 1383d1e21afe..98bea08a14e4 100644
> > --- a/drivers/gpu/drm/bridge/ite-it6505.c
> > +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> > @@ -3427,37 +3427,35 @@ static ssize_t receive_timing_debugfs_show(struct file *file, char __user *buf,
> >       struct it6505 *it6505 = file->private_data;
> >       struct drm_display_mode *vid;
> >       u8 read_buf[READ_BUFFER_SIZE];
> > -     u8 *str = read_buf, *end = read_buf + READ_BUFFER_SIZE;
> > -     ssize_t ret, count;
> > +     ssize_t ret;
> > +     ssize_t count = 0;
> >
> >       if (!it6505)
> >               return -ENODEV;
> >
> >       it6505_calc_video_info(it6505);
> >       vid = &it6505->video_info;
> > -     str += scnprintf(str, end - str, "---video timing---\n");
> > -     str += scnprintf(str, end - str, "PCLK:%d.%03dMHz\n",
> > -                      vid->clock / 1000, vid->clock % 1000);
> > -     str += scnprintf(str, end - str, "HTotal:%d\n", vid->htotal);
> > -     str += scnprintf(str, end - str, "HActive:%d\n", vid->hdisplay);
> > -     str += scnprintf(str, end - str, "HFrontPorch:%d\n",
> > -                      vid->hsync_start - vid->hdisplay);
> > -     str += scnprintf(str, end - str, "HSyncWidth:%d\n",
> > -                      vid->hsync_end - vid->hsync_start);
> > -     str += scnprintf(str, end - str, "HBackPorch:%d\n",
> > -                      vid->htotal - vid->hsync_end);
> > -     str += scnprintf(str, end - str, "VTotal:%d\n", vid->vtotal);
> > -     str += scnprintf(str, end - str, "VActive:%d\n", vid->vdisplay);
> > -     str += scnprintf(str, end - str, "VFrontPorch:%d\n",
> > -                      vid->vsync_start - vid->vdisplay);
> > -     str += scnprintf(str, end - str, "VSyncWidth:%d\n",
> > -                      vid->vsync_end - vid->vsync_start);
> > -     str += scnprintf(str, end - str, "VBackPorch:%d\n",
> > -                      vid->vtotal - vid->vsync_end);
> > -
> > -     count = str - read_buf;
> > +     count += sysfs_emit_at(read_buf, count, "---video timing---\n");
> > +     count += sysfs_emit_at(read_buf, count, "PCLK:%d.%03dMHz\n",
> > +                     vid->clock / 1000, vid->clock % 1000);
> > +     count += sysfs_emit_at(read_buf, count, "HTotal:%d\n", vid->htotal);
> > +     count += sysfs_emit_at(read_buf, count, "HActive:%d\n", vid->hdisplay);
> > +     count += sysfs_emit_at(read_buf, count, "HFrontPorch:%d\n",
> > +                     vid->hsync_start - vid->hdisplay);
> > +     count += sysfs_emit_at(read_buf, count, "HSyncWidth:%d\n",
> > +                     vid->hsync_end - vid->hsync_start);
> > +     count += sysfs_emit_at(read_buf, count, "HBackPorch:%d\n",
> > +                     vid->htotal - vid->hsync_end);
> > +     count += sysfs_emit_at(read_buf, count, "VTotal:%d\n", vid->vtotal);
> > +     count += sysfs_emit_at(read_buf, count, "VActive:%d\n", vid->vdisplay);
> > +     count += sysfs_emit_at(read_buf, count, "VFrontPorch:%d\n",
> > +                     vid->vsync_start - vid->vdisplay);
> > +     count += sysfs_emit_at(read_buf, count, "VSyncWidth:%d\n",
> > +                     vid->vsync_end - vid->vsync_start);
> > +     count += sysfs_emit_at(read_buf, count, "VBackPorch:%d\n",
> > +                     vid->vtotal - vid->vsync_end);
> > +
> >       ret = simple_read_from_buffer(buf, len, ppos, read_buf, count);
> > -
>
> Shouldn't this all be using seq_print() instead?
>
> Again, don't use sysfs_emit*() functions for non-sysfs files, as you do
> NOT know the size of the buffer here (hint, it's not the same).
>
> And, your patch added trailing whitespace, did you forget to run it
> through checkpatch.pl before sending it?


Thanks again for the clarification, I'll be more attentive for future
submissions.

Specifically for this patch I forgot to run checkpatch, sorry. Sorry
for all the inconvenience.

Thanks Greg!

-- 
Cheers,
Marcelo Moreira