Re: [PATCH v2 6/7] vgacon: Implement struct consw.con_unblank

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] console: IRQ state leak in vgacon_unblank() during panic
- [Medium] vgacon: failure to blank screen during graphics mode switch
--

commit 1fe83e2b75ae124b5285efec89c3a22f914e107d
Author: Thomas Zimmermann <tzimmermann@suse.de>

vgacon: Implement struct consw.con_unblank

This commit implements the con_unblank callback from struct consw by
extracting the VESA_NO_BLANKING logic from vgacon_blank() into a new
function named vgacon_unblank(). It also updates how the global
vga_is_gfx mode tracking is handled during console transitions.

> diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
> index 37bd18730fe0d..761c8da10449b 100644
> --- a/drivers/video/console/vgacon.c
> +++ b/drivers/video/console/vgacon.c
> @@ -800,39 +800,47 @@ static void vga_pal_blank(struct vgastate *state)
>  static bool vgacon_blank(struct vc_data *c, enum vesa_blank_mode blank,
>  			 bool mode_switch)
>  {
> +	if (mode_switch)
> +		vga_is_gfx = true;

[Severity: Medium]
Does setting vga_is_gfx to true before calling vgacon_set_origin() prevent
the screen from blanking correctly?

If mode_switch is true, vga_is_gfx is set to true immediately. Later in this
same path under VESA_NO_BLANKING, vgacon_set_origin() is called:

static bool vgacon_set_origin(struct vc_data *c)
{
	if (vga_is_gfx ||	/* We don't play origin tricks in graphic modes */
	    (console_blanked && !vga_palette_blanked))	/* Nor we write to blanked screens */
		return false;
        ...
}

Because vga_is_gfx is already set to true, vgacon_set_origin() will
short-circuit and return false without reprogramming the hardware mem_top.
Will this result in a visible flash of uncleared text before the graphics
driver takes over?

> +
>  	switch (blank) {

[ ... ]

> +static bool vgacon_unblank(struct vc_data *c)
> +{
> +	bool mode_switch = vga_is_gfx && c->vc_mode == KD_TEXT;
> +
> +	if (mode_switch)
> +		vga_is_gfx = false;
> +
> +	if (vga_vesa_blanked) {
> +		vga_vesa_unblank(&vgastate);

[Severity: High]
Can this call safely execute in a panic context where interrupts must
remain disabled?

When panic() executes, it explicitly disables local interrupts via
local_irq_disable() before calling console_unblank().

The console unblanking subsystem will invoke this new .con_unblank callback,
which calls vga_vesa_unblank(). However, vga_vesa_unblank() uses
raw_spin_unlock_irq():

static void vga_vesa_unblank(struct vgastate *state)
{
	/* restore original values of VGA controller registers */
	raw_spin_lock_irq(&vga_lock);
        ...
	raw_spin_unlock_irq(&vga_lock);
}

This unconditionally re-enables hardware interrupts, which destroys the safe
context set up by panic(). If a hardware interrupt fires during the remainder
of the panic sequence, could it lead to secondary oopses or deadlocks that
hide the panic logs from the user?

Should the vga_lock acquisition use irqsave/irqrestore variants instead
to preserve the caller's IRQ state?

> +		vga_vesa_blanked = VESA_NO_BLANKING;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016093357.33294-1-tzimmermann@suse.de?part=6