1. Patchew
  2. linux
  3. View series

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

Christophe JAILLET posted 1 patch 1 year, 6 months ago 
.../omap2/omapfb/displays/panel-sony-acx565akm.c  | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
[PATCH] fbdev: omapfb: panel-sony-acx565akm: Simplify show_cabc_available_modes()
Posted by Christophe JAILLET 1 year, 6 months ago 
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@wanadoo.fr>
---
 .../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';
 
-	return len < PAGE_SIZE ? len : PAGE_SIZE - 1;
+	return len;
 }
 
 static DEVICE_ATTR(cabc_mode, S_IRUGO | S_IWUSR,
-- 
2.46.0
Re: [PATCH] fbdev: omapfb: panel-sony-acx565akm: Simplify show_cabc_available_modes()
Posted by Helge Deller 1 year, 5 months ago 
On 8/8/24 11:46, 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@wanadoo.fr>

applied.

Thanks!
Helge
Re: [PATCH] fbdev: omapfb: panel-sony-acx565akm: Simplify show_cabc_available_modes()
Posted by Dan Carpenter 1 year, 6 months ago 
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@wanadoo.fr>
> ---
>  .../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:

	for (i = 0; i < ARRAY_SIZE(cabc_modes); i++)
		len += sysfs_emit_at(buf, len, "%s%s", cabc_modes[i],
				     i == ARRAY_SIZE(cabc_modes) - 1 ? "\n" : "");

regards,
dan carpenter
Re: [PATCH] fbdev: omapfb: panel-sony-acx565akm: Simplify show_cabc_available_modes()
Posted by Christophe JAILLET 1 year, 6 months ago 
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@wanadoo.fr>
>> ---
>>   .../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.

So, as len != 0, len-1 is in [0..PAGE_SIZE-2].

How do you think an overflow could happen?


Also, all this code does is buildind:
	"off, ui, still-image, moving-image\n"

So in any case, an overflow is impossible, and really unlikely in the 
future as well.


 From my PoV, my proposed patch is both much more readable and still 
correct in all cases.

CJ

> 	for (i = 0; i < ARRAY_SIZE(cabc_modes); i++)
> 		len += sysfs_emit_at(buf, len, "%s%s", cabc_modes[i],
> 				     i == ARRAY_SIZE(cabc_modes) - 1 ? "\n" : "");
> 
> regards,
> dan carpenter
> 
> 
> 
>
Re: [PATCH] fbdev: omapfb: panel-sony-acx565akm: Simplify show_cabc_available_modes()
Posted by Dan Carpenter 1 year, 6 months ago 
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@wanadoo.fr>
> > > ---
> > >   .../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
Re: [PATCH] fbdev: omapfb: panel-sony-acx565akm: Simplify show_cabc_available_modes()
Posted by Dan Carpenter 1 year, 6 months ago 
On Fri, Aug 09, 2024 at 05:42:32PM +0300, Dan Carpenter wrote:
> 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@wanadoo.fr>
> > ---
> >  .../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:
> 
> 	for (i = 0; i < ARRAY_SIZE(cabc_modes); i++)
> 		len += sysfs_emit_at(buf, len, "%s%s", cabc_modes[i],
> 				     i == ARRAY_SIZE(cabc_modes) - 1 ? "\n" : "");

Or you could change it to:

	if (len)
		sysfs_emit_at(buf, len - 1, "\n");

But that feels weird.

regards,
dan carpenter
Re: [PATCH] fbdev: omapfb: panel-sony-acx565akm: Simplify show_cabc_available_modes()
Posted by Andi Shyti 1 year, 6 months ago 
Hi Christophe,

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@wanadoo.fr>

neat!

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Andi

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.