Re: [PATCH 6/6] vt: keyboard: publish the shift_down[] counters with release semantics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] tty: vt: unnecessary memory barriers on shift_down
--

commit 00b20e2546808f30870c343c8e766b47b9bd3b39
Author: Jaidev Shastri <jaidevshastri@vt.edu>

vt: keyboard: publish the shift_down[] counters with release semantics
    
The commit updates accesses to the shift_down[] array by adding
smp_store_release() and smp_load_acquire() semantics. This change was based on
a static analyzer report.

> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index 87970415a043c..e6112e0db1020 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -823,7 +823,8 @@ static void k_pad(struct vc_data *vc, unsigned char value, char up_flag)
>  		return;		/* no action, if this is a key release */
>  
>  	/* kludge... shift forces cursor/number keys */
> -	if (vc_kbd_mode(kbd, VC_APPLIC) && !shift_down[KG_SHIFT]) {
> +	/* Pairs with the smp_store_release() in k_shift(). */
> +	if (vc_kbd_mode(kbd, VC_APPLIC) && !smp_load_acquire(&shift_down[KG_SHIFT])) {

[Severity: Low]
Are these acquire semantics necessary?

A key press or release triggers kbd_event(), acquiring the kbd_event_lock
spinlock. This calls kbd_keycode() which invokes k_shift() and k_pad() via the
k_handler array.

All functions accessing shift_down[] (k_shift(), k_pad(), and
do_compute_shiftstate()) appear to be strictly executed under this spinlock,
meaning there are no lockless readers for shift_down[] in the kernel. Did
the static analyzer produce a false positive by failing to trace the
spinlock across the function pointer invocations in k_handler[]?

>  		applkey(vc, app_map[value], 1);
>  		return;
>  	}

[ ... ]

> @@ -890,15 +892,18 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
>  			clr_vc_kbd_led(kbd, VC_CAPSLOCK);
>  	}
>  
> +	cnt = shift_down[value];
>  	if (up_flag) {
>  		/*
>  		 * handle the case that two shift or control
>  		 * keys are depressed simultaneously
>  		 */
> -		if (shift_down[value])
> -			shift_down[value]--;
> +		if (cnt)
> +			cnt--;
>  	} else
> -		shift_down[value]++;
> +		cnt++;
> +	/* Pairs with the smp_load_acquire() in k_pad(). */
> +	smp_store_release(&shift_down[value], cnt);

[Severity: Low]
Are these release semantics necessary?

As mentioned above, since shift_down[] is already protected by the
kbd_event_lock spinlock, are there any lockless readers that require these
memory barriers? Also, as there are plain reads adjacent to the release
store (e.g. cnt = shift_down[value] just above), the implementation would
be flawed if lockless concurrency were actually present.

>  
>  	if (shift_down[value])
>  		state = shift_state | BIT(value);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-mb-keyboard-v1-0-d170228b80c0@vt.edu?part=6