[PATCH] Input: atkbd - factor out atkbd_recreate_device() helper

[PATCH] Input: atkbd - factor out atkbd_recreate_device() helper

[Dmitry Torokhov]

The sysfs attribute setters atkbd_set_extra() and atkbd_set_set() contain
identical logic for unregistering the old input device, allocating a new
one, updating state, and registering the new device.

Factor out this common logic into an atkbd_recreate_device() helper to
reduce code duplication.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/atkbd.c | 109 ++++++++++++++-------------------
 1 file changed, 46 insertions(+), 63 deletions(-)

diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 5736f4bc5a50..3ff7e9dea9fe 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -1444,6 +1444,47 @@ static ssize_t atkbd_attr_set_helper(struct device *dev, const char *buf, size_t
 	return -EINTR;
 }
 
+static int atkbd_recreate_device(struct atkbd *atkbd,
+				 unsigned char new_set, bool new_extra)
+{
+	struct input_dev *new_dev, *old_dev = atkbd->dev;
+	bool old_extra = atkbd->extra;
+	u8 old_set = atkbd->set;
+	int err;
+
+	/*
+	 * Since device's properties will change we need to unregister
+	 * the old device. But allocate and register the new one first
+	 * to make sure we have it.
+	 */
+	new_dev = input_allocate_device();
+	if (!new_dev)
+		return -ENOMEM;
+
+	atkbd->dev = new_dev;
+	atkbd->set = atkbd_select_set(atkbd, new_set, new_extra);
+	atkbd_reset_state(atkbd);
+	atkbd_activate(atkbd);
+	atkbd_set_keycode_table(atkbd);
+	atkbd_set_device_attrs(atkbd);
+
+	err = input_register_device(atkbd->dev);
+	if (err) {
+		input_free_device(new_dev);
+
+		atkbd->dev = old_dev;
+		atkbd->set = atkbd_select_set(atkbd, old_set, old_extra);
+		atkbd_set_keycode_table(atkbd);
+		atkbd_set_device_attrs(atkbd);
+
+		return err;
+	}
+
+	input_unregister_device(old_dev);
+
+	return 0;
+}
+
 static ssize_t atkbd_show_extra(struct atkbd *atkbd, char *buf)
 {
 	return sprintf(buf, "%d\n", atkbd->extra ? 1 : 0);
@@ -1451,11 +1492,8 @@ static ssize_t atkbd_show_extra(struct atkbd *atkbd, char *buf)
 
 static ssize_t atkbd_set_extra(struct atkbd *atkbd, const char *buf, size_t count)
 {
-	struct input_dev *old_dev, *new_dev;
 	unsigned int value;
 	int err;
-	bool old_extra;
-	u8 old_set;
 
 	if (!atkbd->write)
 		return -EIO;
@@ -1468,38 +1506,9 @@ static ssize_t atkbd_set_extra(struct atkbd *atkbd, const char *buf, size_t coun
 		return -EINVAL;
 
 	if (atkbd->extra != value) {
-		/*
-		 * Since device's properties will change we need to
-		 * unregister old device. But allocate and register
-		 * new one first to make sure we have it.
-		 */
-		old_dev = atkbd->dev;
-		old_extra = atkbd->extra;
-		old_set = atkbd->set;
-
-		new_dev = input_allocate_device();
-		if (!new_dev)
-			return -ENOMEM;
-
-		atkbd->dev = new_dev;
-		atkbd->set = atkbd_select_set(atkbd, atkbd->set, value);
-		atkbd_reset_state(atkbd);
-		atkbd_activate(atkbd);
-		atkbd_set_keycode_table(atkbd);
-		atkbd_set_device_attrs(atkbd);
-
-		err = input_register_device(atkbd->dev);
-		if (err) {
-			input_free_device(new_dev);
-
-			atkbd->dev = old_dev;
-			atkbd->set = atkbd_select_set(atkbd, old_set, old_extra);
-			atkbd_set_keycode_table(atkbd);
-			atkbd_set_device_attrs(atkbd);
-
+		err = atkbd_recreate_device(atkbd, atkbd->set, value);
+		if (err)
 			return err;
-		}
-		input_unregister_device(old_dev);
 	}
 
 	return count;
@@ -1586,11 +1595,8 @@ static ssize_t atkbd_show_set(struct atkbd *atkbd, char *buf)
 
 static ssize_t atkbd_set_set(struct atkbd *atkbd, const char *buf, size_t count)
 {
-	struct input_dev *old_dev, *new_dev;
 	unsigned int value;
 	int err;
-	u8 old_set;
-	bool old_extra;
 
 	if (!atkbd->write)
 		return -EIO;
@@ -1603,34 +1609,11 @@ static ssize_t atkbd_set_set(struct atkbd *atkbd, const char *buf, size_t count)
 		return -EINVAL;
 
 	if (atkbd->set != value) {
-		old_dev = atkbd->dev;
-		old_extra = atkbd->extra;
-		old_set = atkbd->set;
-
-		new_dev = input_allocate_device();
-		if (!new_dev)
-			return -ENOMEM;
-
-		atkbd->dev = new_dev;
-		atkbd->set = atkbd_select_set(atkbd, value, atkbd->extra);
-		atkbd_reset_state(atkbd);
-		atkbd_activate(atkbd);
-		atkbd_set_keycode_table(atkbd);
-		atkbd_set_device_attrs(atkbd);
-
-		err = input_register_device(atkbd->dev);
-		if (err) {
-			input_free_device(new_dev);
-
-			atkbd->dev = old_dev;
-			atkbd->set = atkbd_select_set(atkbd, old_set, old_extra);
-			atkbd_set_keycode_table(atkbd);
-			atkbd_set_device_attrs(atkbd);
-
+		err = atkbd_recreate_device(atkbd, value, atkbd->extra);
+		if (err)
 			return err;
-		}
-		input_unregister_device(old_dev);
 	}
+
 	return count;
 }
 
-- 
2.55.0.679.g6767b8d81c-goog


-- 
Dmitry

Re: [PATCH] Input: atkbd - factor out atkbd_recreate_device() helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The error recovery path in atkbd_recreate_device() corrupts the active input device by clearing its EV_SYN capability, rendering the keyboard completely unresponsive.
--

--- Patch [1]: [PATCH] Input: atkbd - factor out atkbd_recreate_device() helper ---
commit 90c8b25166b08db625d55de3789ce58cd2cfc65c
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: atkbd - factor out atkbd_recreate_device() helper

This commit factors out the common logic for unregistering the old input
device, allocating a new one, updating state, and registering the new device
into a new atkbd_recreate_device() helper to reduce code duplication.

> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> index 8cb4dc6fb1658..cc061bd0761f4 100644
> --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -1452,6 +1452,47 @@ static ssize_t atkbd_attr_set_helper(struct device *dev, const char *buf, size_t

[ ... ]

> +static int atkbd_recreate_device(struct atkbd *atkbd,
> +				 unsigned char new_set, bool new_extra)
> +{
> +	struct input_dev *new_dev, *old_dev = atkbd->dev;
> +	bool old_extra = atkbd->extra;
> +	u8 old_set = atkbd->set;
> +	int err;

[ ... ]

> +	err = input_register_device(atkbd->dev);
> +	if (err) {
> +		input_free_device(new_dev);
> +
> +		atkbd->dev = old_dev;
> +		atkbd->set = atkbd_select_set(atkbd, old_set, old_extra);
> +		atkbd_set_keycode_table(atkbd);
> +		atkbd_set_device_attrs(atkbd);

[Severity: High]
This is a pre-existing issue, but does calling atkbd_set_device_attrs() on
the already-registered old_dev corrupt its capabilities during this error
recovery path?

Looking at atkbd_set_device_attrs(), it initializes capabilities using a
direct assignment:

    input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP) |
        BIT_MASK(EV_MSC);

This unconditionally clears bit 0 (EV_SYN). For a newly allocated device
this is fine, as input_register_device() adds EV_SYN later.

However, when called on the active old_dev after input_register_device()
fails, it strips the EV_SYN capability. Without EV_SYN, does the input core
drop all synchronization events (SYN_REPORT), leaving key events stuck in
client buffers and rendering the keyboard unresponsive?

> +
> +		return err;
> +	}
> +
> +	input_unregister_device(old_dev);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/anQg6dtUwQ1Fx4hM@google.com?part=1