Re: [PATCH 1/6] vt: keyboard: publish shift_state with release semantics

[sashiko-bot@kernel.org]

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

commit 97a82ad73b6718204026d341ed2e3831f7cbd52f
Author: Jaidev Shastri <jaidevshastri@vt.edu>

vt: keyboard: publish shift_state with release semantics

This commit replaces plain reads and writes of shift_state with
smp_load_acquire() and smp_store_release() to specify the relation
between the counters and the summary word.

> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index c41d850b29c6e..089f3b048beee 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c

[ ... ]

> @@ -889,9 +890,11 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
>  		shift_down[value]++;
>  
>  	if (shift_down[value])
> -		shift_state |= BIT(value);
> +		state = shift_state | BIT(value);
>  	else
> -		shift_state &= ~BIT(value);
> +		state = shift_state & ~BIT(value);
> +	/* Pairs with the smp_load_acquire() in vt_get_shift_state(). */
> +	smp_store_release(&shift_state, state);

[Severity: Low]
Are acquire/release semantics strictly necessary here?

The only lockless reader is vt_get_shift_state(), which simply returns
shift_state directly to userspace when it calls the TIOCL_GETSHIFTSTATE ioctl.
It does not read any dependent memory like the shift_down[] counters.

Without dependent memory reads to sequence after the flag read, the memory
ordering guarantees provided by acquire/release semantics seem unused,
adding memory barrier overhead to every shift key press and release.

Would a simple WRITE_ONCE() here and READ_ONCE() in vt_get_shift_state()
be sufficient to prevent compiler tearing and reordering for this single
scalar value?

>  
>  	/* kludge */
>  	if (up_flag && shift_state != old_state && npadch_active) {

[ ... ]

> @@ -2212,8 +2215,11 @@ void vt_reset_unicode(unsigned int console)
>   */
>  int vt_get_shift_state(void)
>  {
> -        /* Don't lock as this is a transient report */
> -        return shift_state;
> +	/*
> +	 * Don't lock as this is a transient report. Pairs with the
> +	 * smp_store_release() in k_shift().
> +	 */
> +	return smp_load_acquire(&shift_state);

[Severity: Low]
Could this use a plain READ_ONCE() instead of smp_load_acquire() since
it does not read any dependent memory?

>  }
>  
>  /**

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