Re: [PATCH v10 4/7] input: keyboard: Add driver for ASUS Transformer dock multimedia keys

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Concurrent lockless access to the shared state variable `priv->special_key_pressed` across different execution contexts without `READ_ONCE()` / `WRITE_ONCE()` annotations.
--

--- Patch [4]: [PATCH v10 4/7] input: keyboard: Add driver for ASUS Transformer dock multimedia keys ---
commit 258dea9f255ed0dea28a80d3eb88521b79b225d6
Author: Michał Mirosław <[email protected]>

input: keyboard: Add driver for ASUS Transformer dock multimedia keys

This patch adds support for the multimedia top button row of the ASUS
Transformer's Mobile Dock keyboard. It enables function keys by default
and allows switching to multimedia keys by pressing ScreenLock + AltGr.

> diff --git a/drivers/input/keyboard/asus-transformer-ec-keys.c b/drivers/input/keyboard/asus-transformer-ec-keys.c
> new file mode 100644
> index 0000000000000..44235e4a7f548
> --- /dev/null
> +++ b/drivers/input/keyboard/asus-transformer-ec-keys.c

[ ... ]

> +static void asus_ec_input_event(struct input_handle *handle,
> +				unsigned int event_type,
> +				unsigned int event_code, int value)
> +{
> +	struct asus_ec_keys_data *priv = handle->handler->private;
> +
> +	/* Store special key state */
> +	if (event_type == EV_KEY && event_code == KEY_RIGHTALT)
> +		priv->special_key_pressed = !!value;

[Severity: Medium]
Could this lockless write cause data races when accessed concurrently?

This executes in the input core's event delivery context, while the
EC notification path reads it. Should this use WRITE_ONCE() to prevent
potential KCSAN warnings about unmarked data races?

> +}

[ ... ]

> +static int asus_ec_keys_process_key(struct input_dev *dev, u8 code)
> +{
> +	struct asus_ec_keys_data *priv = dev_get_drvdata(dev->dev.parent);
> +	unsigned int key = 0;
> +
> +	if (code == 0)
> +		return NOTIFY_DONE;
> +
> +	/* Flip special key mode state when pressing SCREEN LOCK + R ALT */
> +	if (priv->special_key_pressed && code == 1) {

[Severity: Medium]
Is it possible to read a stale value here due to concurrent modification?

Since priv->special_key_pressed is updated in the input event path and
read here in the blocking notifier thread without synchronization, should
this use READ_ONCE() to ensure the compiler doesn't perform unexpected
optimizations?

> +		priv->special_key_mode = !priv->special_key_mode;
> +		return NOTIFY_DONE;
> +	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4