Depending on rotation settings, fbcon sets different callback
functions in struct fbcon from within fbcon_set_bitops(). Declare
the callback functions in the new type struct fbcon_bitops. Then
only replace the single bitops pointer in struct fbcon.
Keeping callbacks in constant instances of struct fbcon_bitops
makes it harder to exploit the callbacks. Also makes the code slightly
easier to maintain.
For tile-based consoles, there's a separate instance of the bitops
structure.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/video/fbdev/core/bitblit.c | 17 ++++---
drivers/video/fbdev/core/fbcon.c | 67 +++++++++++++++-------------
drivers/video/fbdev/core/fbcon.h | 7 ++-
drivers/video/fbdev/core/fbcon_ccw.c | 18 +++++---
drivers/video/fbdev/core/fbcon_cw.c | 18 +++++---
drivers/video/fbdev/core/fbcon_ud.c | 18 +++++---
drivers/video/fbdev/core/tileblit.c | 16 ++++---
7 files changed, 94 insertions(+), 67 deletions(-)
diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
index a2202cae0691..267bd1635a41 100644
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -384,15 +384,18 @@ static int bit_update_start(struct fb_info *info)
return err;
}
+static const struct fbcon_bitops bit_fbcon_bitops = {
+ .bmove = bit_bmove,
+ .clear = bit_clear,
+ .putcs = bit_putcs,
+ .clear_margins = bit_clear_margins,
+ .cursor = bit_cursor,
+ .update_start = bit_update_start,
+};
+
void fbcon_set_bitops(struct fbcon *confb)
{
- confb->bmove = bit_bmove;
- confb->clear = bit_clear;
- confb->putcs = bit_putcs;
- confb->clear_margins = bit_clear_margins;
- confb->cursor = bit_cursor;
- confb->update_start = bit_update_start;
- confb->rotate_font = NULL;
+ confb->bitops = &bit_fbcon_bitops;
if (confb->rotate)
fbcon_set_rotate(confb);
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index ac8e897be5cb..baaed48dbb4f 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -405,9 +405,9 @@ static void fb_flashcursor(struct work_struct *work)
c = scr_readw((u16 *) vc->vc_pos);
enable = confb->cursor_flash && !confb->cursor_state.enable;
- confb->cursor(vc, info, enable,
- get_fg_color(vc, info, c),
- get_bg_color(vc, info, c));
+ confb->bitops->cursor(vc, info, enable,
+ get_fg_color(vc, info, c),
+ get_bg_color(vc, info, c));
console_unlock();
queue_delayed_work(system_power_efficient_wq, &confb->cursor_work,
@@ -1162,7 +1162,7 @@ static void fbcon_init(struct vc_data *vc, bool init)
if (logo)
fbcon_prepare_logo(vc, info, cols, rows, new_cols, new_rows);
- if (confb->rotate_font && confb->rotate_font(info, vc)) {
+ if (confb->bitops->rotate_font && confb->bitops->rotate_font(info, vc)) {
confb->rotate = FB_ROTATE_UR;
set_blitting_type(vc, info);
}
@@ -1303,10 +1303,11 @@ static void __fbcon_clear(struct vc_data *vc, unsigned int sy, unsigned int sx,
y_break = p->vrows - p->yscroll;
if (sy < y_break && sy + height - 1 >= y_break) {
u_int b = y_break - sy;
- confb->clear(vc, info, real_y(p, sy), sx, b, width, fg, bg);
- confb->clear(vc, info, real_y(p, sy + b), sx, height - b, width, fg, bg);
+ confb->bitops->clear(vc, info, real_y(p, sy), sx, b, width, fg, bg);
+ confb->bitops->clear(vc, info, real_y(p, sy + b), sx, height - b,
+ width, fg, bg);
} else
- confb->clear(vc, info, real_y(p, sy), sx, height, width, fg, bg);
+ confb->bitops->clear(vc, info, real_y(p, sy), sx, height, width, fg, bg);
}
static void fbcon_clear(struct vc_data *vc, unsigned int sy, unsigned int sx,
@@ -1323,9 +1324,9 @@ static void fbcon_putcs(struct vc_data *vc, const u16 *s, unsigned int count,
struct fbcon *confb = info->fbcon_par;
if (fbcon_is_active(vc, info))
- confb->putcs(vc, info, s, count, real_y(p, ypos), xpos,
- get_fg_color(vc, info, scr_readw(s)),
- get_bg_color(vc, info, scr_readw(s)));
+ confb->bitops->putcs(vc, info, s, count, real_y(p, ypos), xpos,
+ get_fg_color(vc, info, scr_readw(s)),
+ get_bg_color(vc, info, scr_readw(s)));
}
static void fbcon_clear_margins(struct vc_data *vc, int bottom_only)
@@ -1334,7 +1335,7 @@ static void fbcon_clear_margins(struct vc_data *vc, int bottom_only)
struct fbcon *confb = info->fbcon_par;
if (fbcon_is_active(vc, info))
- confb->clear_margins(vc, info, margin_color, bottom_only);
+ confb->bitops->clear_margins(vc, info, margin_color, bottom_only);
}
static void fbcon_cursor(struct vc_data *vc, bool enable)
@@ -1355,12 +1356,12 @@ static void fbcon_cursor(struct vc_data *vc, bool enable)
confb->cursor_flash = enable;
- if (!confb->cursor)
+ if (!confb->bitops->cursor)
return;
- confb->cursor(vc, info, enable,
- get_fg_color(vc, info, c),
- get_bg_color(vc, info, c));
+ confb->bitops->cursor(vc, info, enable,
+ get_fg_color(vc, info, c),
+ get_bg_color(vc, info, c));
}
static int scrollback_phys_max = 0;
@@ -1444,7 +1445,7 @@ static __inline__ void ywrap_up(struct vc_data *vc, int count)
confb->var.xoffset = 0;
confb->var.yoffset = p->yscroll * vc->vc_font.height;
confb->var.vmode |= FB_VMODE_YWRAP;
- confb->update_start(info);
+ confb->bitops->update_start(info);
scrollback_max += count;
if (scrollback_max > scrollback_phys_max)
scrollback_max = scrollback_phys_max;
@@ -1463,7 +1464,7 @@ static __inline__ void ywrap_down(struct vc_data *vc, int count)
confb->var.xoffset = 0;
confb->var.yoffset = p->yscroll * vc->vc_font.height;
confb->var.vmode |= FB_VMODE_YWRAP;
- confb->update_start(info);
+ confb->bitops->update_start(info);
scrollback_max -= count;
if (scrollback_max < 0)
scrollback_max = 0;
@@ -1478,15 +1479,15 @@ static __inline__ void ypan_up(struct vc_data *vc, int count)
p->yscroll += count;
if (p->yscroll > p->vrows - vc->vc_rows) {
- confb->bmove(vc, info, p->vrows - vc->vc_rows,
- 0, 0, 0, vc->vc_rows, vc->vc_cols);
+ confb->bitops->bmove(vc, info, p->vrows - vc->vc_rows,
+ 0, 0, 0, vc->vc_rows, vc->vc_cols);
p->yscroll -= p->vrows - vc->vc_rows;
}
confb->var.xoffset = 0;
confb->var.yoffset = p->yscroll * vc->vc_font.height;
confb->var.vmode &= ~FB_VMODE_YWRAP;
- confb->update_start(info);
+ confb->bitops->update_start(info);
fbcon_clear_margins(vc, 1);
scrollback_max += count;
if (scrollback_max > scrollback_phys_max)
@@ -1510,7 +1511,7 @@ static __inline__ void ypan_up_redraw(struct vc_data *vc, int t, int count)
confb->var.xoffset = 0;
confb->var.yoffset = p->yscroll * vc->vc_font.height;
confb->var.vmode &= ~FB_VMODE_YWRAP;
- confb->update_start(info);
+ confb->bitops->update_start(info);
fbcon_clear_margins(vc, 1);
scrollback_max += count;
if (scrollback_max > scrollback_phys_max)
@@ -1526,15 +1527,15 @@ static __inline__ void ypan_down(struct vc_data *vc, int count)
p->yscroll -= count;
if (p->yscroll < 0) {
- confb->bmove(vc, info, 0, 0, p->vrows - vc->vc_rows,
- 0, vc->vc_rows, vc->vc_cols);
+ confb->bitops->bmove(vc, info, 0, 0, p->vrows - vc->vc_rows,
+ 0, vc->vc_rows, vc->vc_cols);
p->yscroll += p->vrows - vc->vc_rows;
}
confb->var.xoffset = 0;
confb->var.yoffset = p->yscroll * vc->vc_font.height;
confb->var.vmode &= ~FB_VMODE_YWRAP;
- confb->update_start(info);
+ confb->bitops->update_start(info);
fbcon_clear_margins(vc, 1);
scrollback_max -= count;
if (scrollback_max < 0)
@@ -1558,7 +1559,7 @@ static __inline__ void ypan_down_redraw(struct vc_data *vc, int t, int count)
confb->var.xoffset = 0;
confb->var.yoffset = p->yscroll * vc->vc_font.height;
confb->var.vmode &= ~FB_VMODE_YWRAP;
- confb->update_start(info);
+ confb->bitops->update_start(info);
fbcon_clear_margins(vc, 1);
scrollback_max -= count;
if (scrollback_max < 0)
@@ -1620,8 +1621,8 @@ static void fbcon_redraw_blit(struct vc_data *vc, struct fb_info *info,
if (c == scr_readw(d)) {
if (s > start) {
- confb->bmove(vc, info, line + ycount, x,
- line, x, 1, s - start);
+ confb->bitops->bmove(vc, info, line + ycount, x,
+ line, x, 1, s - start);
x += s - start + 1;
start = s + 1;
} else {
@@ -1636,7 +1637,8 @@ static void fbcon_redraw_blit(struct vc_data *vc, struct fb_info *info,
d++;
} while (s < le);
if (s > start)
- confb->bmove(vc, info, line + ycount, x, line, x, 1, s - start);
+ confb->bitops->bmove(vc, info, line + ycount, x, line, x, 1,
+ s - start);
console_conditional_schedule();
if (ycount > 0)
line++;
@@ -1741,7 +1743,8 @@ static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy,
}
return;
}
- confb->bmove(vc, info, real_y(p, sy), sx, real_y(p, dy), dx, height, width);
+ confb->bitops->bmove(vc, info, real_y(p, sy), sx, real_y(p, dy), dx,
+ height, width);
}
static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
@@ -2161,7 +2164,7 @@ static bool fbcon_switch(struct vc_data *vc)
set_blitting_type(vc, info);
confb->cursor_reset = 1;
- if (confb->rotate_font && confb->rotate_font(info, vc)) {
+ if (confb->bitops->rotate_font && confb->bitops->rotate_font(info, vc)) {
confb->rotate = FB_ROTATE_UR;
set_blitting_type(vc, info);
}
@@ -2194,7 +2197,7 @@ static bool fbcon_switch(struct vc_data *vc)
if (fbcon_is_active(vc, info)) {
confb->var.xoffset = confb->var.yoffset = p->yscroll = 0;
- confb->update_start(info);
+ confb->bitops->update_start(info);
}
fbcon_set_palette(vc, color_table);
@@ -2693,7 +2696,7 @@ static void fbcon_modechanged(struct fb_info *info)
if (fbcon_is_active(vc, info)) {
confb->var.xoffset = confb->var.yoffset = p->yscroll = 0;
- confb->update_start(info);
+ confb->bitops->update_start(info);
}
fbcon_set_palette(vc, color_table);
diff --git a/drivers/video/fbdev/core/fbcon.h b/drivers/video/fbdev/core/fbcon.h
index 666ed89526da..6a4dac3fd12e 100644
--- a/drivers/video/fbdev/core/fbcon.h
+++ b/drivers/video/fbdev/core/fbcon.h
@@ -51,7 +51,7 @@ struct fbcon_display {
const struct fb_videomode *mode;
};
-struct fbcon {
+struct fbcon_bitops {
void (*bmove)(struct vc_data *vc, struct fb_info *info, int sy,
int sx, int dy, int dx, int height, int width);
void (*clear)(struct vc_data *vc, struct fb_info *info, int sy,
@@ -65,6 +65,9 @@ struct fbcon {
bool enable, int fg, int bg);
int (*update_start)(struct fb_info *info);
int (*rotate_font)(struct fb_info *info, struct vc_data *vc);
+};
+
+struct fbcon {
struct fb_var_screeninfo var; /* copy of the current fb_var_screeninfo */
struct delayed_work cursor_work; /* Cursor timer */
struct fb_cursor cursor_state;
@@ -86,6 +89,8 @@ struct fbcon {
u8 *cursor_src;
u32 cursor_size;
u32 fd_size;
+
+ const struct fbcon_bitops *bitops;
};
/*
diff --git a/drivers/video/fbdev/core/fbcon_ccw.c b/drivers/video/fbdev/core/fbcon_ccw.c
index 4c1a40864e84..4902541305dd 100644
--- a/drivers/video/fbdev/core/fbcon_ccw.c
+++ b/drivers/video/fbdev/core/fbcon_ccw.c
@@ -390,13 +390,17 @@ static int ccw_update_start(struct fb_info *info)
return err;
}
+static const struct fbcon_bitops ccw_fbcon_bitops = {
+ .bmove = ccw_bmove,
+ .clear = ccw_clear,
+ .putcs = ccw_putcs,
+ .clear_margins = ccw_clear_margins,
+ .cursor = ccw_cursor,
+ .update_start = ccw_update_start,
+ .rotate_font = fbcon_rotate_font,
+};
+
void fbcon_rotate_ccw(struct fbcon *confb)
{
- confb->bmove = ccw_bmove;
- confb->clear = ccw_clear;
- confb->putcs = ccw_putcs;
- confb->clear_margins = ccw_clear_margins;
- confb->cursor = ccw_cursor;
- confb->update_start = ccw_update_start;
- confb->rotate_font = fbcon_rotate_font;
+ confb->bitops = &ccw_fbcon_bitops;
}
diff --git a/drivers/video/fbdev/core/fbcon_cw.c b/drivers/video/fbdev/core/fbcon_cw.c
index ac3af039fe4a..0c924581e65d 100644
--- a/drivers/video/fbdev/core/fbcon_cw.c
+++ b/drivers/video/fbdev/core/fbcon_cw.c
@@ -373,13 +373,17 @@ static int cw_update_start(struct fb_info *info)
return err;
}
+static const struct fbcon_bitops cw_fbcon_bitops = {
+ .bmove = cw_bmove,
+ .clear = cw_clear,
+ .putcs = cw_putcs,
+ .clear_margins = cw_clear_margins,
+ .cursor = cw_cursor,
+ .update_start = cw_update_start,
+ .rotate_font = fbcon_rotate_font,
+};
+
void fbcon_rotate_cw(struct fbcon *confb)
{
- confb->bmove = cw_bmove;
- confb->clear = cw_clear;
- confb->putcs = cw_putcs;
- confb->clear_margins = cw_clear_margins;
- confb->cursor = cw_cursor;
- confb->update_start = cw_update_start;
- confb->rotate_font = fbcon_rotate_font;
+ confb->bitops = &cw_fbcon_bitops;
}
diff --git a/drivers/video/fbdev/core/fbcon_ud.c b/drivers/video/fbdev/core/fbcon_ud.c
index 74e1331fae33..6bc73966e1ff 100644
--- a/drivers/video/fbdev/core/fbcon_ud.c
+++ b/drivers/video/fbdev/core/fbcon_ud.c
@@ -417,13 +417,17 @@ static int ud_update_start(struct fb_info *info)
return err;
}
+static const struct fbcon_bitops ud_fbcon_bitops = {
+ .bmove = ud_bmove,
+ .clear = ud_clear,
+ .putcs = ud_putcs,
+ .clear_margins = ud_clear_margins,
+ .cursor = ud_cursor,
+ .update_start = ud_update_start,
+ .rotate_font = fbcon_rotate_font,
+};
+
void fbcon_rotate_ud(struct fbcon *confb)
{
- confb->bmove = ud_bmove;
- confb->clear = ud_clear;
- confb->putcs = ud_putcs;
- confb->clear_margins = ud_clear_margins;
- confb->cursor = ud_cursor;
- confb->update_start = ud_update_start;
- confb->rotate_font = fbcon_rotate_font;
+ confb->bitops = &ud_fbcon_bitops;
}
diff --git a/drivers/video/fbdev/core/tileblit.c b/drivers/video/fbdev/core/tileblit.c
index 1ba8987302e6..f84219b403ff 100644
--- a/drivers/video/fbdev/core/tileblit.c
+++ b/drivers/video/fbdev/core/tileblit.c
@@ -161,17 +161,21 @@ static int tile_update_start(struct fb_info *info)
return err;
}
+static const struct fbcon_bitops tile_fbcon_bitops = {
+ .bmove = tile_bmove,
+ .clear = tile_clear,
+ .putcs = tile_putcs,
+ .clear_margins = tile_clear_margins,
+ .cursor = tile_cursor,
+ .update_start = tile_update_start,
+};
+
void fbcon_set_tileops(struct vc_data *vc, struct fb_info *info)
{
struct fb_tilemap map;
struct fbcon *confb = info->fbcon_par;
- confb->bmove = tile_bmove;
- confb->clear = tile_clear;
- confb->putcs = tile_putcs;
- confb->clear_margins = tile_clear_margins;
- confb->cursor = tile_cursor;
- confb->update_start = tile_update_start;
+ confb->bitops = &tile_fbcon_bitops;
if (confb->p) {
map.width = vc->vc_font.width;
--
2.50.1Hi Thomas.
On Mon, Aug 18, 2025 at 12:36:39PM +0200, Thomas Zimmermann wrote:
> Depending on rotation settings, fbcon sets different callback
> functions in struct fbcon from within fbcon_set_bitops(). Declare
> the callback functions in the new type struct fbcon_bitops. Then
> only replace the single bitops pointer in struct fbcon.
>
> Keeping callbacks in constant instances of struct fbcon_bitops
> makes it harder to exploit the callbacks. Also makes the code slightly
> easier to maintain.
>
> For tile-based consoles, there's a separate instance of the bitops
> structure.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
> drivers/video/fbdev/core/bitblit.c | 17 ++++---
> drivers/video/fbdev/core/fbcon.c | 67 +++++++++++++++-------------
> drivers/video/fbdev/core/fbcon.h | 7 ++-
> drivers/video/fbdev/core/fbcon_ccw.c | 18 +++++---
> drivers/video/fbdev/core/fbcon_cw.c | 18 +++++---
> drivers/video/fbdev/core/fbcon_ud.c | 18 +++++---
> drivers/video/fbdev/core/tileblit.c | 16 ++++---
> 7 files changed, 94 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
> index a2202cae0691..267bd1635a41 100644
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -384,15 +384,18 @@ static int bit_update_start(struct fb_info *info)
> return err;
> }
>
> +static const struct fbcon_bitops bit_fbcon_bitops = {
> + .bmove = bit_bmove,
> + .clear = bit_clear,
> + .putcs = bit_putcs,
> + .clear_margins = bit_clear_margins,
> + .cursor = bit_cursor,
> + .update_start = bit_update_start,
> +};
> +
> void fbcon_set_bitops(struct fbcon *confb)
> {
> - confb->bmove = bit_bmove;
> - confb->clear = bit_clear;
> - confb->putcs = bit_putcs;
> - confb->clear_margins = bit_clear_margins;
> - confb->cursor = bit_cursor;
> - confb->update_start = bit_update_start;
> - confb->rotate_font = NULL;
> + confb->bitops = &bit_fbcon_bitops;
>
> if (confb->rotate)
> fbcon_set_rotate(confb);
fbcon_set_rotate() is only used to set the correct bitops.
It would be simpler to just do
if (confb->rotate)
confb->bitops = fbcon_rotate_get_ops();
And rename fbcon_set_rotate() to fbcon_rotate_get_ops() and return the
pointer to the struct.
The no need to pass the struct, and it is obvious that the bitops are
overwritten.
Sam
Hi Sam,
thanks for doing the review.
Am 05.09.25 um 20:53 schrieb Sam Ravnborg:
> Hi Thomas.
>
> On Mon, Aug 18, 2025 at 12:36:39PM +0200, Thomas Zimmermann wrote:
>> Depending on rotation settings, fbcon sets different callback
>> functions in struct fbcon from within fbcon_set_bitops(). Declare
>> the callback functions in the new type struct fbcon_bitops. Then
>> only replace the single bitops pointer in struct fbcon.
>>
>> Keeping callbacks in constant instances of struct fbcon_bitops
>> makes it harder to exploit the callbacks. Also makes the code slightly
>> easier to maintain.
>>
>> For tile-based consoles, there's a separate instance of the bitops
>> structure.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>> drivers/video/fbdev/core/bitblit.c | 17 ++++---
>> drivers/video/fbdev/core/fbcon.c | 67 +++++++++++++++-------------
>> drivers/video/fbdev/core/fbcon.h | 7 ++-
>> drivers/video/fbdev/core/fbcon_ccw.c | 18 +++++---
>> drivers/video/fbdev/core/fbcon_cw.c | 18 +++++---
>> drivers/video/fbdev/core/fbcon_ud.c | 18 +++++---
>> drivers/video/fbdev/core/tileblit.c | 16 ++++---
>> 7 files changed, 94 insertions(+), 67 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
>> index a2202cae0691..267bd1635a41 100644
>> --- a/drivers/video/fbdev/core/bitblit.c
>> +++ b/drivers/video/fbdev/core/bitblit.c
>> @@ -384,15 +384,18 @@ static int bit_update_start(struct fb_info *info)
>> return err;
>> }
>>
>> +static const struct fbcon_bitops bit_fbcon_bitops = {
>> + .bmove = bit_bmove,
>> + .clear = bit_clear,
>> + .putcs = bit_putcs,
>> + .clear_margins = bit_clear_margins,
>> + .cursor = bit_cursor,
>> + .update_start = bit_update_start,
>> +};
>> +
>> void fbcon_set_bitops(struct fbcon *confb)
>> {
>> - confb->bmove = bit_bmove;
>> - confb->clear = bit_clear;
>> - confb->putcs = bit_putcs;
>> - confb->clear_margins = bit_clear_margins;
>> - confb->cursor = bit_cursor;
>> - confb->update_start = bit_update_start;
>> - confb->rotate_font = NULL;
>> + confb->bitops = &bit_fbcon_bitops;
>>
>> if (confb->rotate)
>> fbcon_set_rotate(confb);
> fbcon_set_rotate() is only used to set the correct bitops.
>
> It would be simpler to just do
>
> if (confb->rotate)
> confb->bitops = fbcon_rotate_get_ops();
>
> And rename fbcon_set_rotate() to fbcon_rotate_get_ops() and return the
> pointer to the struct.
>
> The no need to pass the struct, and it is obvious that the bitops are
> overwritten.
I tried to keep the changes here to a minimum and avoided changing the
function interfaces too much.
But did you read patch 5 already? I think the cleanup you're looking for
is there. fbcon_set_rotate() will be gone. And the update bit-op
selection is contained in fbcon_set_bitops(). I guess this could be
renamed to fbcon_update_bitops() to make it clear that it updates from
internal state.
Best regards
Thomas
>
> Sam
>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Hi Thomas.
On Mon, Sep 08, 2025 at 03:06:46PM +0200, Thomas Zimmermann wrote:
> Hi Sam,
>
> thanks for doing the review.
>
> Am 05.09.25 um 20:53 schrieb Sam Ravnborg:
> > Hi Thomas.
> >
> > On Mon, Aug 18, 2025 at 12:36:39PM +0200, Thomas Zimmermann wrote:
> > > Depending on rotation settings, fbcon sets different callback
> > > functions in struct fbcon from within fbcon_set_bitops(). Declare
> > > the callback functions in the new type struct fbcon_bitops. Then
> > > only replace the single bitops pointer in struct fbcon.
> > >
> > > Keeping callbacks in constant instances of struct fbcon_bitops
> > > makes it harder to exploit the callbacks. Also makes the code slightly
> > > easier to maintain.
> > >
> > > For tile-based consoles, there's a separate instance of the bitops
> > > structure.
> > >
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > ---
> > > drivers/video/fbdev/core/bitblit.c | 17 ++++---
> > > drivers/video/fbdev/core/fbcon.c | 67 +++++++++++++++-------------
> > > drivers/video/fbdev/core/fbcon.h | 7 ++-
> > > drivers/video/fbdev/core/fbcon_ccw.c | 18 +++++---
> > > drivers/video/fbdev/core/fbcon_cw.c | 18 +++++---
> > > drivers/video/fbdev/core/fbcon_ud.c | 18 +++++---
> > > drivers/video/fbdev/core/tileblit.c | 16 ++++---
> > > 7 files changed, 94 insertions(+), 67 deletions(-)
> > >
> > > diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
> > > index a2202cae0691..267bd1635a41 100644
> > > --- a/drivers/video/fbdev/core/bitblit.c
> > > +++ b/drivers/video/fbdev/core/bitblit.c
> > > @@ -384,15 +384,18 @@ static int bit_update_start(struct fb_info *info)
> > > return err;
> > > }
> > > +static const struct fbcon_bitops bit_fbcon_bitops = {
> > > + .bmove = bit_bmove,
> > > + .clear = bit_clear,
> > > + .putcs = bit_putcs,
> > > + .clear_margins = bit_clear_margins,
> > > + .cursor = bit_cursor,
> > > + .update_start = bit_update_start,
> > > +};
> > > +
> > > void fbcon_set_bitops(struct fbcon *confb)
> > > {
> > > - confb->bmove = bit_bmove;
> > > - confb->clear = bit_clear;
> > > - confb->putcs = bit_putcs;
> > > - confb->clear_margins = bit_clear_margins;
> > > - confb->cursor = bit_cursor;
> > > - confb->update_start = bit_update_start;
> > > - confb->rotate_font = NULL;
> > > + confb->bitops = &bit_fbcon_bitops;
> > > if (confb->rotate)
> > > fbcon_set_rotate(confb);
> > fbcon_set_rotate() is only used to set the correct bitops.
> >
> > It would be simpler to just do
> >
> > if (confb->rotate)
> > confb->bitops = fbcon_rotate_get_ops();
> >
> > And rename fbcon_set_rotate() to fbcon_rotate_get_ops() and return the
> > pointer to the struct.
> >
> > The no need to pass the struct, and it is obvious that the bitops are
> > overwritten.
>
> I tried to keep the changes here to a minimum and avoided changing the
> function interfaces too much.
>
> But did you read patch 5 already? I think the cleanup you're looking for is
> there. fbcon_set_rotate() will be gone. And the update bit-op selection is
> contained in fbcon_set_bitops(). I guess this could be renamed to
> fbcon_update_bitops() to make it clear that it updates from internal state.
Patch 5 looks good, and is again a nice cleanup.
I like that the code is now more explicit in what it does and do not
do overwrites.
Returning a pointer or adding the assignment in a helper is not a big
deal.
With or without the suggested renaming both patch 4 + 5 are r-b.
That said, I am not expert in this field, but at least you had another
pair of eyes on the changes.
I look forward to see the next batches of refactoring you have planned.
Sam
Hi
Am 08.09.25 um 20:51 schrieb Sam Ravnborg:
> Hi Thomas.
>
> On Mon, Sep 08, 2025 at 03:06:46PM +0200, Thomas Zimmermann wrote:
>> Hi Sam,
>>
>> thanks for doing the review.
>>
>> Am 05.09.25 um 20:53 schrieb Sam Ravnborg:
>>> Hi Thomas.
>>>
>>> On Mon, Aug 18, 2025 at 12:36:39PM +0200, Thomas Zimmermann wrote:
>>>> Depending on rotation settings, fbcon sets different callback
>>>> functions in struct fbcon from within fbcon_set_bitops(). Declare
>>>> the callback functions in the new type struct fbcon_bitops. Then
>>>> only replace the single bitops pointer in struct fbcon.
>>>>
>>>> Keeping callbacks in constant instances of struct fbcon_bitops
>>>> makes it harder to exploit the callbacks. Also makes the code slightly
>>>> easier to maintain.
>>>>
>>>> For tile-based consoles, there's a separate instance of the bitops
>>>> structure.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>> drivers/video/fbdev/core/bitblit.c | 17 ++++---
>>>> drivers/video/fbdev/core/fbcon.c | 67 +++++++++++++++-------------
>>>> drivers/video/fbdev/core/fbcon.h | 7 ++-
>>>> drivers/video/fbdev/core/fbcon_ccw.c | 18 +++++---
>>>> drivers/video/fbdev/core/fbcon_cw.c | 18 +++++---
>>>> drivers/video/fbdev/core/fbcon_ud.c | 18 +++++---
>>>> drivers/video/fbdev/core/tileblit.c | 16 ++++---
>>>> 7 files changed, 94 insertions(+), 67 deletions(-)
>>>>
>>>> diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
>>>> index a2202cae0691..267bd1635a41 100644
>>>> --- a/drivers/video/fbdev/core/bitblit.c
>>>> +++ b/drivers/video/fbdev/core/bitblit.c
>>>> @@ -384,15 +384,18 @@ static int bit_update_start(struct fb_info *info)
>>>> return err;
>>>> }
>>>> +static const struct fbcon_bitops bit_fbcon_bitops = {
>>>> + .bmove = bit_bmove,
>>>> + .clear = bit_clear,
>>>> + .putcs = bit_putcs,
>>>> + .clear_margins = bit_clear_margins,
>>>> + .cursor = bit_cursor,
>>>> + .update_start = bit_update_start,
>>>> +};
>>>> +
>>>> void fbcon_set_bitops(struct fbcon *confb)
>>>> {
>>>> - confb->bmove = bit_bmove;
>>>> - confb->clear = bit_clear;
>>>> - confb->putcs = bit_putcs;
>>>> - confb->clear_margins = bit_clear_margins;
>>>> - confb->cursor = bit_cursor;
>>>> - confb->update_start = bit_update_start;
>>>> - confb->rotate_font = NULL;
>>>> + confb->bitops = &bit_fbcon_bitops;
>>>> if (confb->rotate)
>>>> fbcon_set_rotate(confb);
>>> fbcon_set_rotate() is only used to set the correct bitops.
>>>
>>> It would be simpler to just do
>>>
>>> if (confb->rotate)
>>> confb->bitops = fbcon_rotate_get_ops();
>>>
>>> And rename fbcon_set_rotate() to fbcon_rotate_get_ops() and return the
>>> pointer to the struct.
>>>
>>> The no need to pass the struct, and it is obvious that the bitops are
>>> overwritten.
>> I tried to keep the changes here to a minimum and avoided changing the
>> function interfaces too much.
>>
>> But did you read patch 5 already? I think the cleanup you're looking for is
>> there. fbcon_set_rotate() will be gone. And the update bit-op selection is
>> contained in fbcon_set_bitops(). I guess this could be renamed to
>> fbcon_update_bitops() to make it clear that it updates from internal state.
> Patch 5 looks good, and is again a nice cleanup.
> I like that the code is now more explicit in what it does and do not
> do overwrites.
>
> Returning a pointer or adding the assignment in a helper is not a big
> deal.
>
> With or without the suggested renaming both patch 4 + 5 are r-b.
>
> That said, I am not expert in this field, but at least you had another
> pair of eyes on the changes.
Thanks a lot. I'll update the series and keep it around for a bit, in
case anyone else wants to comment.
Best regards
Thomas
> I look forward to see the next batches of refactoring you have planned.
>
> Sam
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
© 2016 - 2025 Red Hat, Inc.