fbcon: introduce for_each_registered_fb() helper

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 <redacted>
---
 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

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:

75 more quoted lines

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.

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

-- 
Andy Shevchenko [off-list ref]
Intel Finland Oy

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:

quoted

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 ?

quoted

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.

quoted

+#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

Hi all!

On Fri, 2018-06-29 at 00:20 +0800, Yisheng Xie wrote:
[...]

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

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:

quoted

On Fri, 2018-06-29 at 00:20 +0800, Yisheng Xie wrote:

quoted

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

quoted

+#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 [off-list ref]
Intel Finland Oy

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

quoted

+#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 [off-list ref]
Intel Finland Oy