[PATCH] fbcon: introduce for_each_registered_fb() helper

[PATCH] fbcon: introduce for_each_registered_fb() helper

[Yisheng Xie]

Following pattern is often used:

 for (i = 0; i < FB_MAX; i++) {
        if (registered_fb[i]) {
                ...
        }
 }

Therefore, as Andy's suggestion, for_each_registered_fb() helper can
be introduced to make the code easier to read and write by reducing
indentation level. It also saves few lines of code in each occurrence.

This patch convert all part here at the same time.

Signed-off-by: Yisheng Xie <ysxie@foxmail.com>
---
 drivers/video/fbdev/core/fbcon.c | 25 +++++++++----------------
 drivers/video/fbdev/core/fbmem.c |  4 +---
 include/linux/fb.h               |  4 ++++
 3 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index c910e74..610853d 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2220,8 +2220,8 @@ static int fbcon_switch(struct vc_data *vc)
 	 *
 	 * info->currcon = vc->vc_num;
 	 */
-	for (i = 0; i < FB_MAX; i++) {
-		if (registered_fb[i] != NULL && registered_fb[i]->fbcon_par) {
+	for_each_registered_fb(i) {
+		if (registered_fb[i]->fbcon_par) {
 			struct fbcon_ops *o = registered_fb[i]->fbcon_par;
 
 			o->currcon = vc->vc_num;
@@ -3103,11 +3103,9 @@ static int fbcon_fb_unregistered(struct fb_info *info)
 	if (idx = info_idx) {
 		info_idx = -1;
 
-		for (i = 0; i < FB_MAX; i++) {
-			if (registered_fb[i] != NULL) {
-				info_idx = i;
-				break;
-			}
+		for_each_registered_fb(i) {
+			info_idx = i;
+			break;
 		}
 	}
 
@@ -3562,11 +3560,9 @@ static void fbcon_start(void)
 
 		console_lock();
 
-		for (i = 0; i < FB_MAX; i++) {
-			if (registered_fb[i] != NULL) {
-				info_idx = i;
-				break;
-			}
+		for_each_registered_fb(i) {
+			info_idx = i;
+			break;
 		}
 
 		do_fbcon_takeover(0);
@@ -3586,15 +3582,12 @@ static void fbcon_exit(void)
 	kfree((void *)softback_buf);
 	softback_buf = 0UL;
 
-	for (i = 0; i < FB_MAX; i++) {
+	for_each_registered_fb(i) {
 		int pending = 0;
 
 		mapped = 0;
 		info = registered_fb[i];
 
-		if (info = NULL)
-			continue;
-
 		if (info->queue.func)
 			pending = cancel_work_sync(&info->queue);
 		DPRINTK("fbcon: %s pending work\n", (pending ? "canceled" :
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 609438d..645c6ac 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1593,10 +1593,8 @@ static int do_remove_conflicting_framebuffers(struct apertures_struct *a,
 	int i, ret;
 
 	/* check all firmware fbs and kick off if the base addr overlaps */
-	for (i = 0 ; i < FB_MAX; i++) {
+	for_each_registered_fb(i) {
 		struct apertures_struct *gen_aper;
-		if (!registered_fb[i])
-			continue;
 
 		if (!(registered_fb[i]->flags & FBINFO_MISC_FIRMWARE))
 			continue;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index aa74a22..3e13b95 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -650,6 +650,10 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var,
 extern int num_registered_fb;
 extern struct class *fb_class;
 
+#define for_each_registered_fb(i)		\
+	for (i = 0; i < FB_MAX; i++)		\
+		if (registered_fb[i])
+
 extern int lock_fb_info(struct fb_info *info);
 
 static inline void unlock_fb_info(struct fb_info *info)
-- 
1.9.1

Re: [PATCH] fbcon: introduce for_each_registered_fb() helper

[Hans de Goede]

Hi,

Looks good, but you should probably do a new version based on:

https://github.com/bzolnier/linux/tree/fbdev-for-next

This has one more loop to replace, in the fbcon_output_notifier()
function which was introduced by:
https://github.com/bzolnier/linux/commit/83d83bebf40132e2d55ec58af666713cc76f9764

Regards,

Hans



On 28-06-18 18:20, Yisheng Xie wrote:
> Following pattern is often used:
> 
>   for (i = 0; i < FB_MAX; i++) {
>          if (registered_fb[i]) {
>                  ...
>          }
>   }
> 
> Therefore, as Andy's suggestion, for_each_registered_fb() helper can
> be introduced to make the code easier to read and write by reducing
> indentation level. It also saves few lines of code in each occurrence.
> 
> This patch convert all part here at the same time.
> 
> Signed-off-by: Yisheng Xie <ysxie@foxmail.com>
> ---
>   drivers/video/fbdev/core/fbcon.c | 25 +++++++++----------------
>   drivers/video/fbdev/core/fbmem.c |  4 +---
>   include/linux/fb.h               |  4 ++++
>   3 files changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index c910e74..610853d 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2220,8 +2220,8 @@ static int fbcon_switch(struct vc_data *vc)
>   	 *
>   	 * info->currcon = vc->vc_num;
>   	 */
> -	for (i = 0; i < FB_MAX; i++) {
> -		if (registered_fb[i] != NULL && registered_fb[i]->fbcon_par) {
> +	for_each_registered_fb(i) {
> +		if (registered_fb[i]->fbcon_par) {
>   			struct fbcon_ops *o = registered_fb[i]->fbcon_par;
>   
>   			o->currcon = vc->vc_num;
> @@ -3103,11 +3103,9 @@ static int fbcon_fb_unregistered(struct fb_info *info)
>   	if (idx = info_idx) {
>   		info_idx = -1;
>   
> -		for (i = 0; i < FB_MAX; i++) {
> -			if (registered_fb[i] != NULL) {
> -				info_idx = i;
> -				break;
> -			}
> +		for_each_registered_fb(i) {
> +			info_idx = i;
> +			break;
>   		}
>   	}
>   
> @@ -3562,11 +3560,9 @@ static void fbcon_start(void)
>   
>   		console_lock();
>   
> -		for (i = 0; i < FB_MAX; i++) {
> -			if (registered_fb[i] != NULL) {
> -				info_idx = i;
> -				break;
> -			}
> +		for_each_registered_fb(i) {
> +			info_idx = i;
> +			break;
>   		}
>   
>   		do_fbcon_takeover(0);
> @@ -3586,15 +3582,12 @@ static void fbcon_exit(void)
>   	kfree((void *)softback_buf);
>   	softback_buf = 0UL;
>   
> -	for (i = 0; i < FB_MAX; i++) {
> +	for_each_registered_fb(i) {
>   		int pending = 0;
>   
>   		mapped = 0;
>   		info = registered_fb[i];
>   
> -		if (info = NULL)
> -			continue;
> -
>   		if (info->queue.func)
>   			pending = cancel_work_sync(&info->queue);
>   		DPRINTK("fbcon: %s pending work\n", (pending ? "canceled" :
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 609438d..645c6ac 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1593,10 +1593,8 @@ static int do_remove_conflicting_framebuffers(struct apertures_struct *a,
>   	int i, ret;
>   
>   	/* check all firmware fbs and kick off if the base addr overlaps */
> -	for (i = 0 ; i < FB_MAX; i++) {
> +	for_each_registered_fb(i) {
>   		struct apertures_struct *gen_aper;
> -		if (!registered_fb[i])
> -			continue;
>   
>   		if (!(registered_fb[i]->flags & FBINFO_MISC_FIRMWARE))
>   			continue;
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index aa74a22..3e13b95 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -650,6 +650,10 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var,
>   extern int num_registered_fb;
>   extern struct class *fb_class;
>   
> +#define for_each_registered_fb(i)		\
> +	for (i = 0; i < FB_MAX; i++)		\
> +		if (registered_fb[i])
> +
>   extern int lock_fb_info(struct fb_info *info);
>   
>   static inline void unlock_fb_info(struct fb_info *info)
> 

Re: [PATCH] fbcon: introduce for_each_registered_fb() helper

[Andy Shevchenko]

On Fri, 2018-06-29 at 00:20 +0800, Yisheng Xie wrote:
> Following pattern is often used:
> 
>  for (i = 0; i < FB_MAX; i++) {
>         if (registered_fb[i]) {
>                 ...
>         }
>  }
> 
> Therefore, as Andy's suggestion, for_each_registered_fb() helper can

Suggested-by then ?

> be introduced to make the code easier to read and write by reducing
> indentation level. It also saves few lines of code in each occurrence.
> 
> This patch convert all part here at the same time.

LGTM except macro implementation. That's why I have mentioned
for_each_pci_bridge() to look at.

> +#define for_each_registered_fb(i)		\
> +	for (i = 0; i < FB_MAX; i++)		\
> +		if (registered_fb[i])
> +

This needs to be protected against nested conditionals.
Otherwise compiler issues a warning and even may generate wrong code.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] fbcon: introduce for_each_registered_fb() helper

[Daniel Vetter]

On Fri, Jun 29, 2018 at 07:20:13PM +0300, Andy Shevchenko wrote:
> On Fri, 2018-06-29 at 00:20 +0800, Yisheng Xie wrote:
> > Following pattern is often used:
> > 
> >  for (i = 0; i < FB_MAX; i++) {
> >         if (registered_fb[i]) {
> >                 ...
> >         }
> >  }
> > 
> > Therefore, as Andy's suggestion, for_each_registered_fb() helper can
> 
> Suggested-by then ?
> 
> > be introduced to make the code easier to read and write by reducing
> > indentation level. It also saves few lines of code in each occurrence.
> > 
> > This patch convert all part here at the same time.
> 
> LGTM except macro implementation. That's why I have mentioned
> for_each_pci_bridge() to look at.
> 
> > +#define for_each_registered_fb(i)		\
> > +	for (i = 0; i < FB_MAX; i++)		\
> > +		if (registered_fb[i])
> > +
> 
> This needs to be protected against nested conditionals.
> Otherwise compiler issues a warning and even may generate wrong code.

See for_each_if() in include/drm/drmP.h ... we should probably lift that
into a general header. The for_each_if() is used all over drm in iterator
macros, exactly to avoid surprises.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] fbcon: introduce for_each_registered_fb() helper

[Bernd Petrovitsch]

Hi all!

On Fri, 2018-06-29 at 00:20 +0800, Yisheng Xie wrote:
[...]
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index aa74a22..3e13b95 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -650,6 +650,10 @@ extern int fb_get_color_depth(struct
> fb_var_screeninfo *var,
>  extern int num_registered_fb;
>  extern struct class *fb_class;
>  
> +#define for_each_registered_fb(i)		\
> +	for (i = 0; i < FB_MAX; i++)		\
> +		if (registered_fb[i])
> +

That leaves the possibility of a dangling-else.
----  snip  ----
#define for_each_registered_fb(i)		\
	for (i = 0; i < FB_MAX; i++)		\
		if (!registered_fb[i])		\
			continue;		\
		else
----  snip  ----
avoids that.

Kind regards,
	Bernd
-- 
Bernd Petrovitsch                  Email : bernd@petrovitsch.priv.at
                     LUGA : http://www.luga.at

Re: [PATCH] fbcon: introduce for_each_registered_fb() helper

[Andy Shevchenko]

On Mon, 2018-07-02 at 09:30 +0200, Daniel Vetter wrote:
> On Fri, Jun 29, 2018 at 07:20:13PM +0300, Andy Shevchenko wrote:
> > On Fri, 2018-06-29 at 00:20 +0800, Yisheng Xie wrote:

> > LGTM except macro implementation. That's why I have mentioned
> > for_each_pci_bridge() to look at.
> > 
> > > +#define for_each_registered_fb(i)		\
> > > +	for (i = 0; i < FB_MAX; i++)		\
> > > +		if (registered_fb[i])
> > > +
> > 
> > This needs to be protected against nested conditionals.
> > Otherwise compiler issues a warning and even may generate wrong
> > code.
> 
> See for_each_if() in include/drm/drmP.h ... we should probably lift
> that
> into a general header. The for_each_if() is used all over drm in
> iterator
> macros, exactly to avoid surprises.

Wow, didn't know we have a such. It's a good idea to forelift it for
wider use.

Yisheng, it seems you may use it in your patch directly.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] fbcon: introduce for_each_registered_fb() helper

[Andy Shevchenko]

On Mon, 2018-07-02 at 09:36 +0200, Bernd Petrovitsch wrote:


> > +#define for_each_registered_fb(i)		\
> > +	for (i = 0; i < FB_MAX; i++)		\
> > +		if (registered_fb[i])
> > +
> 
> That leaves the possibility of a dangling-else.
> ----  snip  ----
> #define for_each_registered_fb(i)		\
> 	for (i = 0; i < FB_MAX; i++)		\
> 		if (!registered_fb[i])		\
> 			continue;		\
> 		else
> ----  snip  ----
> avoids that.

Yes, you not alone :-)

AFAIU there is a v2 which fixes that, though Daniel pointed out that DRM
has a specific macro to make life easier.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy