| Message ID | 1530202809-24105-1-git-send-email-ysxie@foxmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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
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
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.
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.
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)
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(-)