On Fri, Aug 09, 2024 at 05:09:28PM +0200, Christophe JAILLET wrote:
> Le 09/08/2024 à 16:42, Dan Carpenter a écrit :
> > On Thu, Aug 08, 2024 at 11:46:11AM +0200, Christophe JAILLET wrote:
> > > 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 <christophe.jaillet@xxxxxxxxxx>
> > > ---
> > > .../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
