Re: [PATCH v3 3/5] vgacon: Use vt_cursor_{start,end}()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vgacon: CUR_DEF bypasses pre-VGA hardware cursor hiding workaround
--

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

vgacon: Use vt_cursor_{start,end}()
    
Replace vgacon's custom cursor metrics with the shared helpers
vc_cursor_start() and vc_font_cursor_end(). Note that the _end()
function returns the index of the scanline below the cursor, while
VGA hardware expects the cursor's final scanline. Hence vgacon
subtracts 1 from the end value.
    
The cursor shapes remain mostly unchanged, except that non-underline
cursors now also use the glyph's top-most and bottom-most scanline.
This follows the style used by fbcon.
    
v2:
- restore use of vc_cell_height (Sashiko)
    
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Helge Deller <deller@gmx.de>

> diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
> index 536e7fe4d1426..b1408bb5e2656 100644
> --- a/drivers/video/console/vgacon.c
> +++ b/drivers/video/console/vgacon.c
[ ... ]
> @@ -513,10 +514,11 @@ static void vgacon_cursor(struct vc_data *c, bool enable)
>  	vgacon_restore_screen(c);
>  
>  	c_height = c->vc_cell_height;
> +	c_size = CUR_SIZE(c->vc_cursor_type);
>  
>  	write_vga(14, (c->vc_pos - vga_vram_base) / 2);
>  
> -	if (!enable) {
> +	if (!enable || c_size == CUR_NONE) {

[Severity: Medium]
Does this check fail to intercept CUR_DEF if the default cursor is
configured as CUR_NONE?

If cur_default is set to CUR_NONE and an unprivileged user triggers a cursor
reset (\e[?0c), c_size will equal CUR_DEF. Since CUR_DEF does not equal
CUR_NONE, this bypasses the hardware-specific hide sequence below.

>  	        if (vga_video_type >= VIDEO_TYPE_VGAC)
>  			vgacon_set_cursor_size(31, 30);
>  		else
> @@ -524,33 +526,8 @@ static void vgacon_cursor(struct vc_data *c, bool enable)
>  		return;
>  	}
>  
[ ... ]
> -	default:
> -		vgacon_set_cursor_size(1, c_height);
> -		break;
> -	}
> +	vgacon_set_cursor_size(vc_cursor_start(c_height, c_size),
> +			       vc_cursor_end(c_height, c_size) - 1);

[Severity: Medium]
If c_size is CUR_DEF and the default cursor is CUR_NONE, could this
cause cursor wrapping on EGA/MDA hardware?

When vc_cursor_start() and vc_cursor_end() resolve the CUR_DEF to CUR_NONE
internally, they both return c_height. This results in
vgacon_set_cursor_size() being called with start = c_height and
end = c_height - 1.

Pre-VGA hardware (EGA/MDA/CGA 6845 CRTCs) do not support start > end for
hiding cursors, which leads to a massive wrapping block cursor instead of
hiding it.

>  }

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