Message-ID: <CAPZ3m_gDqVeyY9x838tUHBuEjXZrANLgqiJ=+Cn8DQYeCfSnGg@mail.gmail.com>
Date: Mon, 30 Jun 2025 21:35:25 -0300
From: Marcelo Moreira <marcelomoreira1905@...il.com>
To: Greg KH <greg@...ah.com>
Cc: Andrzej Hajda <andrzej.hajda@...el.com>, skhan@...uxfoundation.org, 
	linux-kernel-mentees@...ts.linuxfoundation.org, ~lkcamp/patches@...ts.sr.ht, 
	Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>, 
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>, Jonas Karlman <jonas@...boo.se>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/bridge: it6505: replace scnprintf with sysfs_emit_at
 in debugfs show

Em seg., 30 de jun. de 2025 às 02:09, Greg KH <greg@...ah.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@...il.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

Powered by blists - more mailing lists