Re: [PATCH] fbdev: omapfb: panel-sony-acx565akm: Simplify show_cabc_available_modes()

On Fri, Aug 09, 2024 at 05:09:28PM +0200, Christophe JAILLET wrote:

Le 09/08/2024 à 16:42, Dan Carpenter a écrit :

quoted

On Thu, Aug 08, 2024 at 11:46:11AM +0200, Christophe JAILLET wrote:

quoted

Use sysfs_emit_at() instead of snprintf() + custom logic.
Using sysfs_emit_at() is much more simple.

Also, sysfs_emit() is already used in this function, so using
sysfs_emit_at() is more consistent.

Also simplify the logic:
   - always add a space after an entry
   - change the last space into a '\n'

Finally it is easy to see that, given the size of cabc_modes, PAGE_SIZE
can not be reached.
So better keep everything simple (and correct).

Signed-off-by: Christophe JAILLET <redacted>
---
  .../omap2/omapfb/displays/panel-sony-acx565akm.c  | 15 ++++++++-------
  1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/displays/panel-sony-acx565akm.c b/drivers/video/fbdev/omap2/omapfb/displays/panel-sony-acx565akm.c
index 71d2e015960c..fc975615d5c9 100644
--- a/drivers/video/fbdev/omap2/omapfb/displays/panel-sony-acx565akm.c
+++ b/drivers/video/fbdev/omap2/omapfb/displays/panel-sony-acx565akm.c

@@ -466,19 +466,20 @@ static ssize_t show_cabc_available_modes(struct device *dev,
  		char *buf)
  {
  	struct panel_drv_data *ddata = dev_get_drvdata(dev);
-	int len;
+	int len = 0;
  	int i;
  	if (!ddata->has_cabc)
  		return sysfs_emit(buf, "%s\n", cabc_modes[0]);
-	for (i = 0, len = 0;
-	     len < PAGE_SIZE && i < ARRAY_SIZE(cabc_modes); i++)
-		len += snprintf(&buf[len], PAGE_SIZE - len, "%s%s%s",
-			i ? " " : "", cabc_modes[i],
-			i == ARRAY_SIZE(cabc_modes) - 1 ? "\n" : "");
+	for (i = 0; i < ARRAY_SIZE(cabc_modes); i++)
+		len += sysfs_emit_at(buf, len, "%s ", cabc_modes[i]);
+
+	/* Remove the trailing space */
+	if (len)
+		buf[len - 1] = '\n';

I'm uncomfortable with this line.  It assumes we don't overflow PAGE_SIZE where
the original code was careful about checking.  Probably easiest to do what the
original code did and say:

Hi Dan,

I don't follow you. AFAIK, it does not assume anything.

Thanks to sysfs_emit_at(), len can only be in [0..PAGE_SIZE-1] because the
trailing \0 is not counted.

Ah, you're right.  Sorry for the noise.

regards,
dan carpenter