[PATCH 0/8] Convert input core to use new cleanup facilities

[PATCH 0/8] Convert input core to use new cleanup facilities

[Dmitry Torokhov]

Hi,

This series converts input core (but not input handlers such as evdev,
joydev, etc) to use new __free() and guard() cleanup facilities that
simplify the code and ensure that all resources are released
appropriately.

Input handlers will be converted separately later.

Thanks!

Dmitry Torokhov (8):
  Input: ff-core - convert locking to guard notation
  Input: ff-core - make use of __free() cleanup facility
  Input: ff-memless - convert locking to guard notation
  Input: ff-memless - make use of __free() cleanup facility
  Input: mt - convert locking to guard notation
  Input: mt - make use of __free() cleanup facility
  Input: poller - convert locking to guard notation
  Input: use guard notation in input core

 drivers/input/ff-core.c      |  90 ++++------
 drivers/input/ff-memless.c   |  17 +-
 drivers/input/input-mt.c     |  34 ++--
 drivers/input/input-poller.c |   4 +-
 drivers/input/input.c        | 339 ++++++++++++++---------------------
 5 files changed, 184 insertions(+), 300 deletions(-)

-- 
Dmitry

[PATCH 1/8] Input: ff-core - convert locking to guard notation

[Dmitry Torokhov]

Use guard() and scoped_guard() notation instead of explicitly acquiring
and releasing spinlocks and mutexes to simplify the code and ensure that
all locks are released properly.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/ff-core.c | 71 +++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 42 deletions(-)

diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
index 609a5f01761b..eb01bcb69d00 100644
--- a/drivers/input/ff-core.c
+++ b/drivers/input/ff-core.c
@@ -93,7 +93,7 @@ int input_ff_upload(struct input_dev *dev, struct ff_effect *effect,
 {
 	struct ff_device *ff = dev->ff;
 	struct ff_effect *old;
-	int ret = 0;
+	int error;
 	int id;
 
 	if (!test_bit(EV_FF, dev->evbit))
@@ -114,22 +114,20 @@ int input_ff_upload(struct input_dev *dev, struct ff_effect *effect,
 	}
 
 	if (!test_bit(effect->type, ff->ffbit)) {
-		ret = compat_effect(ff, effect);
-		if (ret)
-			return ret;
+		error = compat_effect(ff, effect);
+		if (error)
+			return error;
 	}
 
-	mutex_lock(&ff->mutex);
+	guard(mutex)(&ff->mutex);
 
 	if (effect->id == -1) {
 		for (id = 0; id < ff->max_effects; id++)
 			if (!ff->effect_owners[id])
 				break;
 
-		if (id >= ff->max_effects) {
-			ret = -ENOSPC;
-			goto out;
-		}
+		if (id >= ff->max_effects)
+			return -ENOSPC;
 
 		effect->id = id;
 		old = NULL;
@@ -137,30 +135,26 @@ int input_ff_upload(struct input_dev *dev, struct ff_effect *effect,
 	} else {
 		id = effect->id;
 
-		ret = check_effect_access(ff, id, file);
-		if (ret)
-			goto out;
+		error = check_effect_access(ff, id, file);
+		if (error)
+			return error;
 
 		old = &ff->effects[id];
 
-		if (!check_effects_compatible(effect, old)) {
-			ret = -EINVAL;
-			goto out;
-		}
+		if (!check_effects_compatible(effect, old))
+			return -EINVAL;
 	}
 
-	ret = ff->upload(dev, effect, old);
-	if (ret)
-		goto out;
+	error = ff->upload(dev, effect, old);
+	if (error)
+		return error;
 
-	spin_lock_irq(&dev->event_lock);
-	ff->effects[id] = *effect;
-	ff->effect_owners[id] = file;
-	spin_unlock_irq(&dev->event_lock);
+	scoped_guard(spinlock_irq, &dev->event_lock) {
+		ff->effects[id] = *effect;
+		ff->effect_owners[id] = file;
+	}
 
- out:
-	mutex_unlock(&ff->mutex);
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(input_ff_upload);
 
@@ -178,17 +172,16 @@ static int erase_effect(struct input_dev *dev, int effect_id,
 	if (error)
 		return error;
 
-	spin_lock_irq(&dev->event_lock);
-	ff->playback(dev, effect_id, 0);
-	ff->effect_owners[effect_id] = NULL;
-	spin_unlock_irq(&dev->event_lock);
+	scoped_guard(spinlock_irq, &dev->event_lock) {
+		ff->playback(dev, effect_id, 0);
+		ff->effect_owners[effect_id] = NULL;
+	}
 
 	if (ff->erase) {
 		error = ff->erase(dev, effect_id);
 		if (error) {
-			spin_lock_irq(&dev->event_lock);
-			ff->effect_owners[effect_id] = file;
-			spin_unlock_irq(&dev->event_lock);
+			scoped_guard(spinlock_irq, &dev->event_lock)
+				ff->effect_owners[effect_id] = file;
 
 			return error;
 		}
@@ -210,16 +203,12 @@ static int erase_effect(struct input_dev *dev, int effect_id,
 int input_ff_erase(struct input_dev *dev, int effect_id, struct file *file)
 {
 	struct ff_device *ff = dev->ff;
-	int ret;
 
 	if (!test_bit(EV_FF, dev->evbit))
 		return -ENOSYS;
 
-	mutex_lock(&ff->mutex);
-	ret = erase_effect(dev, effect_id, file);
-	mutex_unlock(&ff->mutex);
-
-	return ret;
+	guard(mutex)(&ff->mutex);
+	return erase_effect(dev, effect_id, file);
 }
 EXPORT_SYMBOL_GPL(input_ff_erase);
 
@@ -239,13 +228,11 @@ int input_ff_flush(struct input_dev *dev, struct file *file)
 
 	dev_dbg(&dev->dev, "flushing now\n");
 
-	mutex_lock(&ff->mutex);
+	guard(mutex)(&ff->mutex);
 
 	for (i = 0; i < ff->max_effects; i++)
 		erase_effect(dev, i, file);
 
-	mutex_unlock(&ff->mutex);
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(input_ff_flush);
-- 
2.47.0.277.g8800431eea-goog

[PATCH 2/8] Input: ff-core - make use of __free() cleanup facility

[Dmitry Torokhov]

Annotate allocated memory with __free(kfree) to simplify the code and
make sure memory is released appropriately.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/ff-core.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
index eb01bcb69d00..a235d2eb6b31 100644
--- a/drivers/input/ff-core.c
+++ b/drivers/input/ff-core.c
@@ -290,8 +290,6 @@ EXPORT_SYMBOL_GPL(input_ff_event);
  */
 int input_ff_create(struct input_dev *dev, unsigned int max_effects)
 {
-	struct ff_device *ff;
-	size_t ff_dev_size;
 	int i;
 
 	if (!max_effects) {
@@ -304,25 +302,20 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects)
 		return -EINVAL;
 	}
 
-	ff_dev_size = struct_size(ff, effect_owners, max_effects);
-	if (ff_dev_size == SIZE_MAX) /* overflow */
-		return -EINVAL;
-
-	ff = kzalloc(ff_dev_size, GFP_KERNEL);
+	struct ff_device *ff __free(kfree) =
+		kzalloc(struct_size(ff, effect_owners, max_effects),
+			GFP_KERNEL);
 	if (!ff)
 		return -ENOMEM;
 
-	ff->effects = kcalloc(max_effects, sizeof(struct ff_effect),
-			      GFP_KERNEL);
-	if (!ff->effects) {
-		kfree(ff);
+	ff->effects = kcalloc(max_effects, sizeof(*ff->effects), GFP_KERNEL);
+	if (!ff->effects)
 		return -ENOMEM;
-	}
 
 	ff->max_effects = max_effects;
 	mutex_init(&ff->mutex);
 
-	dev->ff = ff;
+	dev->ff = no_free_ptr(ff);
 	dev->flush = input_ff_flush;
 	dev->event = input_ff_event;
 	__set_bit(EV_FF, dev->evbit);
-- 
2.47.0.277.g8800431eea-goog

[PATCH 3/8] Input: ff-memless - convert locking to guard notation

[Dmitry Torokhov]

Use guard() notation instead of explicitly acquiring and releasing
spinlocks to simplify the code and ensure that all locks are released.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/ff-memless.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
index c321cdabd214..ec99c070a97c 100644
--- a/drivers/input/ff-memless.c
+++ b/drivers/input/ff-memless.c
@@ -401,13 +401,11 @@ static void ml_effect_timer(struct timer_list *t)
 {
 	struct ml_device *ml = from_timer(ml, t, timer);
 	struct input_dev *dev = ml->dev;
-	unsigned long flags;
 
 	pr_debug("timer: updating effects\n");
 
-	spin_lock_irqsave(&dev->event_lock, flags);
+	guard(spinlock_irqsave)(&dev->event_lock);
 	ml_play_effects(ml);
-	spin_unlock_irqrestore(&dev->event_lock, flags);
 }
 
 /*
@@ -465,7 +463,7 @@ static int ml_ff_upload(struct input_dev *dev,
 	struct ml_device *ml = dev->ff->private;
 	struct ml_effect_state *state = &ml->states[effect->id];
 
-	spin_lock_irq(&dev->event_lock);
+	guard(spinlock_irq)(&dev->event_lock);
 
 	if (test_bit(FF_EFFECT_STARTED, &state->flags)) {
 		__clear_bit(FF_EFFECT_PLAYING, &state->flags);
@@ -477,8 +475,6 @@ static int ml_ff_upload(struct input_dev *dev,
 		ml_schedule_timer(ml);
 	}
 
-	spin_unlock_irq(&dev->event_lock);
-
 	return 0;
 }
 
-- 
2.47.0.277.g8800431eea-goog

[PATCH 4/8] Input: ff-memless - make use of __free() cleanup facility

[Dmitry Torokhov]

Annotate allocated memory with __free(kfree) to simplify the code and
make sure memory is released appropriately.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/ff-memless.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
index ec99c070a97c..0bbeceb35545 100644
--- a/drivers/input/ff-memless.c
+++ b/drivers/input/ff-memless.c
@@ -503,12 +503,11 @@ static void ml_ff_destroy(struct ff_device *ff)
 int input_ff_create_memless(struct input_dev *dev, void *data,
 		int (*play_effect)(struct input_dev *, void *, struct ff_effect *))
 {
-	struct ml_device *ml;
 	struct ff_device *ff;
 	int error;
 	int i;
 
-	ml = kzalloc(sizeof(struct ml_device), GFP_KERNEL);
+	struct ml_device *ml __free(kfree) = kzalloc(sizeof(*ml), GFP_KERNEL);
 	if (!ml)
 		return -ENOMEM;
 
@@ -521,13 +520,11 @@ int input_ff_create_memless(struct input_dev *dev, void *data,
 	set_bit(FF_GAIN, dev->ffbit);
 
 	error = input_ff_create(dev, FF_MEMLESS_EFFECTS);
-	if (error) {
-		kfree(ml);
+	if (error)
 		return error;
-	}
 
 	ff = dev->ff;
-	ff->private = ml;
+	ff->private = no_free_ptr(ml);
 	ff->upload = ml_ff_upload;
 	ff->playback = ml_ff_playback;
 	ff->set_gain = ml_ff_set_gain;
-- 
2.47.0.277.g8800431eea-goog

[PATCH 5/8] Input: mt - convert locking to guard notation

[Dmitry Torokhov]

Use guard() notation instead of explicitly acquiring and releasing
spinlocks to simplify the code and ensure that all locks are released.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/input-mt.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index 6b04a674f832..45e41fc9059c 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -285,14 +285,10 @@ void input_mt_drop_unused(struct input_dev *dev)
 	struct input_mt *mt = dev->mt;
 
 	if (mt) {
-		unsigned long flags;
-
-		spin_lock_irqsave(&dev->event_lock, flags);
+		guard(spinlock_irqsave)(&dev->event_lock);
 
 		__input_mt_drop_unused(dev, mt);
 		mt->frame++;
-
-		spin_unlock_irqrestore(&dev->event_lock, flags);
 	}
 }
 EXPORT_SYMBOL(input_mt_drop_unused);
@@ -339,11 +335,8 @@ void input_mt_sync_frame(struct input_dev *dev)
 		return;
 
 	if (mt->flags & INPUT_MT_DROP_UNUSED) {
-		unsigned long flags;
-
-		spin_lock_irqsave(&dev->event_lock, flags);
+		guard(spinlock_irqsave)(&dev->event_lock);
 		__input_mt_drop_unused(dev, mt);
-		spin_unlock_irqrestore(&dev->event_lock, flags);
 	}
 
 	if ((mt->flags & INPUT_MT_POINTER) && !(mt->flags & INPUT_MT_SEMI_MT))
-- 
2.47.0.277.g8800431eea-goog

[PATCH 6/8] Input: mt - make use of __free() cleanup facility

[Dmitry Torokhov]

Annotate allocated memory with __free(kfree) to simplify the code and
make sure memory is released appropriately.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/input-mt.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index 45e41fc9059c..337006dd9dcf 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -39,20 +39,20 @@ static void copy_abs(struct input_dev *dev, unsigned int dst, unsigned int src)
 int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,
 			unsigned int flags)
 {
-	struct input_mt *mt = dev->mt;
-	int i;
-
 	if (!num_slots)
 		return 0;
-	if (mt)
-		return mt->num_slots != num_slots ? -EINVAL : 0;
+
+	if (dev->mt)
+		return dev->mt->num_slots != num_slots ? -EINVAL : 0;
+
 	/* Arbitrary limit for avoiding too large memory allocation. */
 	if (num_slots > 1024)
 		return -EINVAL;
 
-	mt = kzalloc(struct_size(mt, slots, num_slots), GFP_KERNEL);
+	struct input_mt *mt __free(kfree) =
+			kzalloc(struct_size(mt, slots, num_slots), GFP_KERNEL);
 	if (!mt)
-		goto err_mem;
+		return -ENOMEM;
 
 	mt->num_slots = num_slots;
 	mt->flags = flags;
@@ -86,21 +86,18 @@ int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,
 		unsigned int n2 = num_slots * num_slots;
 		mt->red = kcalloc(n2, sizeof(*mt->red), GFP_KERNEL);
 		if (!mt->red)
-			goto err_mem;
+			return -ENOMEM;
 	}
 
 	/* Mark slots as 'inactive' */
-	for (i = 0; i < num_slots; i++)
+	for (unsigned int i = 0; i < num_slots; i++)
 		input_mt_set_value(&mt->slots[i], ABS_MT_TRACKING_ID, -1);
 
 	/* Mark slots as 'unused' */
 	mt->frame = 1;
 
-	dev->mt = mt;
+	dev->mt = no_free_ptr(mt);
 	return 0;
-err_mem:
-	kfree(mt);
-	return -ENOMEM;
 }
 EXPORT_SYMBOL(input_mt_init_slots);
 
-- 
2.47.0.277.g8800431eea-goog

[PATCH 7/8] Input: poller - convert locking to guard notation

[Dmitry Torokhov]

Use guard() notation instead of explicitly acquiring and releasing
mutex to simplify the code and ensure that it is released.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/input-poller.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/input/input-poller.c b/drivers/input/input-poller.c
index 688e3cb1c2a0..9c57713a6151 100644
--- a/drivers/input/input-poller.c
+++ b/drivers/input/input-poller.c
@@ -162,7 +162,7 @@ static ssize_t input_dev_set_poll_interval(struct device *dev,
 	if (interval > poller->poll_interval_max)
 		return -EINVAL;
 
-	mutex_lock(&input->mutex);
+	guard(mutex)(&input->mutex);
 
 	poller->poll_interval = interval;
 
@@ -172,8 +172,6 @@ static ssize_t input_dev_set_poll_interval(struct device *dev,
 			input_dev_poller_queue_work(poller);
 	}
 
-	mutex_unlock(&input->mutex);
-
 	return count;
 }
 
-- 
2.47.0.277.g8800431eea-goog

[PATCH 8/8] Input: use guard notation in input core

[Dmitry Torokhov]

Switch input core to use "guard" notation when acquiring spinlocks and
mutexes to simplify the code and ensure that locks are automatically
released when control leaves critical section.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/input.c | 339 ++++++++++++++++--------------------------
 1 file changed, 131 insertions(+), 208 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index fd9d40286b75..d73231e695f8 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -115,23 +115,23 @@ static void input_pass_values(struct input_dev *dev,
 
 	lockdep_assert_held(&dev->event_lock);
 
-	rcu_read_lock();
+	scoped_guard(rcu) {
+		handle = rcu_dereference(dev->grab);
+		if (handle) {
+			count = handle->handle_events(handle, vals, count);
+			break;
+		}
 
-	handle = rcu_dereference(dev->grab);
-	if (handle) {
-		count = handle->handle_events(handle, vals, count);
-	} else {
-		list_for_each_entry_rcu(handle, &dev->h_list, d_node)
+		list_for_each_entry_rcu(handle, &dev->h_list, d_node) {
 			if (handle->open) {
 				count = handle->handle_events(handle, vals,
 							      count);
 				if (!count)
 					break;
 			}
+		}
 	}
 
-	rcu_read_unlock();
-
 	/* trigger auto repeat for key events */
 	if (test_bit(EV_REP, dev->evbit) && test_bit(EV_KEY, dev->evbit)) {
 		for (v = vals; v != vals + count; v++) {
@@ -390,13 +390,9 @@ void input_handle_event(struct input_dev *dev,
 void input_event(struct input_dev *dev,
 		 unsigned int type, unsigned int code, int value)
 {
-	unsigned long flags;
-
 	if (is_event_supported(type, dev->evbit, EV_MAX)) {
-
-		spin_lock_irqsave(&dev->event_lock, flags);
+		guard(spinlock_irqsave)(&dev->event_lock);
 		input_handle_event(dev, type, code, value);
-		spin_unlock_irqrestore(&dev->event_lock, flags);
 	}
 }
 EXPORT_SYMBOL(input_event);
@@ -417,18 +413,15 @@ void input_inject_event(struct input_handle *handle,
 {
 	struct input_dev *dev = handle->dev;
 	struct input_handle *grab;
-	unsigned long flags;
 
 	if (is_event_supported(type, dev->evbit, EV_MAX)) {
-		spin_lock_irqsave(&dev->event_lock, flags);
+		guard(spinlock_irqsave)(&dev->event_lock);
+		guard(rcu)();
 
-		rcu_read_lock();
 		grab = rcu_dereference(dev->grab);
 		if (!grab || grab == handle)
 			input_handle_event(dev, type, code, value);
-		rcu_read_unlock();
 
-		spin_unlock_irqrestore(&dev->event_lock, flags);
 	}
 }
 EXPORT_SYMBOL(input_inject_event);
@@ -526,22 +519,15 @@ EXPORT_SYMBOL(input_copy_abs);
 int input_grab_device(struct input_handle *handle)
 {
 	struct input_dev *dev = handle->dev;
-	int retval;
 
-	retval = mutex_lock_interruptible(&dev->mutex);
-	if (retval)
-		return retval;
+	scoped_cond_guard(mutex_intr, return -EINTR, &dev->mutex) {
+		if (dev->grab)
+			return -EBUSY;
 
-	if (dev->grab) {
-		retval = -EBUSY;
-		goto out;
+		rcu_assign_pointer(dev->grab, handle);
 	}
 
-	rcu_assign_pointer(dev->grab, handle);
-
- out:
-	mutex_unlock(&dev->mutex);
-	return retval;
+	return 0;
 }
 EXPORT_SYMBOL(input_grab_device);
 
@@ -576,9 +562,8 @@ void input_release_device(struct input_handle *handle)
 {
 	struct input_dev *dev = handle->dev;
 
-	mutex_lock(&dev->mutex);
+	guard(mutex)(&dev->mutex);
 	__input_release_device(handle);
-	mutex_unlock(&dev->mutex);
 }
 EXPORT_SYMBOL(input_release_device);
 
@@ -592,67 +577,57 @@ EXPORT_SYMBOL(input_release_device);
 int input_open_device(struct input_handle *handle)
 {
 	struct input_dev *dev = handle->dev;
-	int retval;
-
-	retval = mutex_lock_interruptible(&dev->mutex);
-	if (retval)
-		return retval;
-
-	if (dev->going_away) {
-		retval = -ENODEV;
-		goto out;
-	}
+	int error;
 
-	handle->open++;
+	scoped_cond_guard(mutex_intr, return -EINTR, &dev->mutex) {
+		if (dev->going_away)
+			return -ENODEV;
 
-	if (handle->handler->passive_observer)
-		goto out;
+		handle->open++;
 
-	if (dev->users++ || dev->inhibited) {
-		/*
-		 * Device is already opened and/or inhibited,
-		 * so we can exit immediately and report success.
-		 */
-		goto out;
-	}
+		if (handle->handler->passive_observer)
+			return 0;
 
-	if (dev->open) {
-		retval = dev->open(dev);
-		if (retval) {
-			dev->users--;
-			handle->open--;
+		if (dev->users++ || dev->inhibited) {
 			/*
-			 * Make sure we are not delivering any more events
-			 * through this handle
+			 * Device is already opened and/or inhibited,
+			 * so we can exit immediately and report success.
 			 */
-			synchronize_rcu();
-			goto out;
+			return 0;
 		}
-	}
 
-	if (dev->poller)
-		input_dev_poller_start(dev->poller);
+		if (dev->open) {
+			error = dev->open(dev);
+			if (error) {
+				dev->users--;
+				handle->open--;
+				/*
+				 * Make sure we are not delivering any more
+				 * events through this handle.
+				 */
+				synchronize_rcu();
+				return error;
+			}
+		}
 
- out:
-	mutex_unlock(&dev->mutex);
-	return retval;
+		if (dev->poller)
+			input_dev_poller_start(dev->poller);
+	}
+
+	return 0;
 }
 EXPORT_SYMBOL(input_open_device);
 
 int input_flush_device(struct input_handle *handle, struct file *file)
 {
 	struct input_dev *dev = handle->dev;
-	int retval;
 
-	retval = mutex_lock_interruptible(&dev->mutex);
-	if (retval)
-		return retval;
-
-	if (dev->flush)
-		retval = dev->flush(dev, file);
+	scoped_cond_guard(mutex_intr, return -EINTR, &dev->mutex) {
+		if (dev->flush)
+			return dev->flush(dev, file);
+	}
 
-	mutex_unlock(&dev->mutex);
-	return retval;
+	return 0;
 }
 EXPORT_SYMBOL(input_flush_device);
 
@@ -667,7 +642,7 @@ void input_close_device(struct input_handle *handle)
 {
 	struct input_dev *dev = handle->dev;
 
-	mutex_lock(&dev->mutex);
+	guard(mutex)(&dev->mutex);
 
 	__input_release_device(handle);
 
@@ -688,8 +663,6 @@ void input_close_device(struct input_handle *handle)
 		 */
 		synchronize_rcu();
 	}
-
-	mutex_unlock(&dev->mutex);
 }
 EXPORT_SYMBOL(input_close_device);
 
@@ -726,11 +699,10 @@ static void input_disconnect_device(struct input_dev *dev)
 	 * not to protect access to dev->going_away but rather to ensure
 	 * that there are no threads in the middle of input_open_device()
 	 */
-	mutex_lock(&dev->mutex);
-	dev->going_away = true;
-	mutex_unlock(&dev->mutex);
+	scoped_guard(mutex, &dev->mutex)
+		dev->going_away = true;
 
-	spin_lock_irq(&dev->event_lock);
+	guard(spinlock_irq)(&dev->event_lock);
 
 	/*
 	 * Simulate keyup events for all pressed keys so that handlers
@@ -743,8 +715,6 @@ static void input_disconnect_device(struct input_dev *dev)
 
 	list_for_each_entry(handle, &dev->h_list, d_node)
 		handle->open = 0;
-
-	spin_unlock_irq(&dev->event_lock);
 }
 
 /**
@@ -901,14 +871,9 @@ static int input_default_setkeycode(struct input_dev *dev,
  */
 int input_get_keycode(struct input_dev *dev, struct input_keymap_entry *ke)
 {
-	unsigned long flags;
-	int retval;
+	guard(spinlock_irqsave)(&dev->event_lock);
 
-	spin_lock_irqsave(&dev->event_lock, flags);
-	retval = dev->getkeycode(dev, ke);
-	spin_unlock_irqrestore(&dev->event_lock, flags);
-
-	return retval;
+	return dev->getkeycode(dev, ke);
 }
 EXPORT_SYMBOL(input_get_keycode);
 
@@ -923,18 +888,17 @@ EXPORT_SYMBOL(input_get_keycode);
 int input_set_keycode(struct input_dev *dev,
 		      const struct input_keymap_entry *ke)
 {
-	unsigned long flags;
 	unsigned int old_keycode;
-	int retval;
+	int error;
 
 	if (ke->keycode > KEY_MAX)
 		return -EINVAL;
 
-	spin_lock_irqsave(&dev->event_lock, flags);
+	guard(spinlock_irqsave)(&dev->event_lock);
 
-	retval = dev->setkeycode(dev, ke, &old_keycode);
-	if (retval)
-		goto out;
+	error = dev->setkeycode(dev, ke, &old_keycode);
+	if (error)
+		return error;
 
 	/* Make sure KEY_RESERVED did not get enabled. */
 	__clear_bit(KEY_RESERVED, dev->keybit);
@@ -962,10 +926,7 @@ int input_set_keycode(struct input_dev *dev,
 				    EV_SYN, SYN_REPORT, 1);
 	}
 
- out:
-	spin_unlock_irqrestore(&dev->event_lock, flags);
-
-	return retval;
+	return 0;
 }
 EXPORT_SYMBOL(input_set_keycode);
 
@@ -1802,26 +1763,21 @@ static void input_dev_toggle(struct input_dev *dev, bool activate)
  */
 void input_reset_device(struct input_dev *dev)
 {
-	unsigned long flags;
-
-	mutex_lock(&dev->mutex);
-	spin_lock_irqsave(&dev->event_lock, flags);
+	guard(mutex)(&dev->mutex);
+	guard(spinlock_irqsave)(&dev->event_lock);
 
 	input_dev_toggle(dev, true);
 	if (input_dev_release_keys(dev))
 		input_handle_event(dev, EV_SYN, SYN_REPORT, 1);
-
-	spin_unlock_irqrestore(&dev->event_lock, flags);
-	mutex_unlock(&dev->mutex);
 }
 EXPORT_SYMBOL(input_reset_device);
 
 static int input_inhibit_device(struct input_dev *dev)
 {
-	mutex_lock(&dev->mutex);
+	guard(mutex)(&dev->mutex);
 
 	if (dev->inhibited)
-		goto out;
+		return 0;
 
 	if (dev->users) {
 		if (dev->close)
@@ -1830,54 +1786,50 @@ static int input_inhibit_device(struct input_dev *dev)
 			input_dev_poller_stop(dev->poller);
 	}
 
-	spin_lock_irq(&dev->event_lock);
-	input_mt_release_slots(dev);
-	input_dev_release_keys(dev);
-	input_handle_event(dev, EV_SYN, SYN_REPORT, 1);
-	input_dev_toggle(dev, false);
-	spin_unlock_irq(&dev->event_lock);
+	scoped_guard(spinlock_irq, &dev->event_lock) {
+		input_mt_release_slots(dev);
+		input_dev_release_keys(dev);
+		input_handle_event(dev, EV_SYN, SYN_REPORT, 1);
+		input_dev_toggle(dev, false);
+	}
 
 	dev->inhibited = true;
 
-out:
-	mutex_unlock(&dev->mutex);
 	return 0;
 }
 
 static int input_uninhibit_device(struct input_dev *dev)
 {
-	int ret = 0;
+	int error;
 
-	mutex_lock(&dev->mutex);
+	guard(mutex)(&dev->mutex);
 
 	if (!dev->inhibited)
-		goto out;
+		return 0;
 
 	if (dev->users) {
 		if (dev->open) {
-			ret = dev->open(dev);
-			if (ret)
-				goto out;
+			error = dev->open(dev);
+			if (error)
+				return error;
 		}
 		if (dev->poller)
 			input_dev_poller_start(dev->poller);
 	}
 
 	dev->inhibited = false;
-	spin_lock_irq(&dev->event_lock);
-	input_dev_toggle(dev, true);
-	spin_unlock_irq(&dev->event_lock);
 
-out:
-	mutex_unlock(&dev->mutex);
-	return ret;
+	scoped_guard(spinlock_irq, &dev->event_lock)
+		input_dev_toggle(dev, true);
+
+	return 0;
 }
 
 static int input_dev_suspend(struct device *dev)
 {
 	struct input_dev *input_dev = to_input_dev(dev);
 
-	spin_lock_irq(&input_dev->event_lock);
+	guard(spinlock_irq)(&input_dev->event_lock);
 
 	/*
 	 * Keys that are pressed now are unlikely to be
@@ -1889,8 +1841,6 @@ static int input_dev_suspend(struct device *dev)
 	/* Turn off LEDs and sounds, if any are active. */
 	input_dev_toggle(input_dev, false);
 
-	spin_unlock_irq(&input_dev->event_lock);
-
 	return 0;
 }
 
@@ -1898,13 +1848,11 @@ static int input_dev_resume(struct device *dev)
 {
 	struct input_dev *input_dev = to_input_dev(dev);
 
-	spin_lock_irq(&input_dev->event_lock);
+	guard(spinlock_irq)(&input_dev->event_lock);
 
 	/* Restore state of LEDs and sounds, if any were active. */
 	input_dev_toggle(input_dev, true);
 
-	spin_unlock_irq(&input_dev->event_lock);
-
 	return 0;
 }
 
@@ -1912,7 +1860,7 @@ static int input_dev_freeze(struct device *dev)
 {
 	struct input_dev *input_dev = to_input_dev(dev);
 
-	spin_lock_irq(&input_dev->event_lock);
+	guard(spinlock_irq)(&input_dev->event_lock);
 
 	/*
 	 * Keys that are pressed now are unlikely to be
@@ -1921,8 +1869,6 @@ static int input_dev_freeze(struct device *dev)
 	if (input_dev_release_keys(input_dev))
 		input_handle_event(input_dev, EV_SYN, SYN_REPORT, 1);
 
-	spin_unlock_irq(&input_dev->event_lock);
-
 	return 0;
 }
 
@@ -1930,13 +1876,11 @@ static int input_dev_poweroff(struct device *dev)
 {
 	struct input_dev *input_dev = to_input_dev(dev);
 
-	spin_lock_irq(&input_dev->event_lock);
+	guard(spinlock_irq)(&input_dev->event_lock);
 
 	/* Turn off LEDs and sounds, if any are active. */
 	input_dev_toggle(input_dev, false);
 
-	spin_unlock_irq(&input_dev->event_lock);
-
 	return 0;
 }
 
@@ -2277,18 +2221,16 @@ static void __input_unregister_device(struct input_dev *dev)
 
 	input_disconnect_device(dev);
 
-	mutex_lock(&input_mutex);
-
-	list_for_each_entry_safe(handle, next, &dev->h_list, d_node)
-		handle->handler->disconnect(handle);
-	WARN_ON(!list_empty(&dev->h_list));
+	scoped_guard(mutex, &input_mutex) {
+		list_for_each_entry_safe(handle, next, &dev->h_list, d_node)
+			handle->handler->disconnect(handle);
+		WARN_ON(!list_empty(&dev->h_list));
 
-	del_timer_sync(&dev->timer);
-	list_del_init(&dev->node);
+		del_timer_sync(&dev->timer);
+		list_del_init(&dev->node);
 
-	input_wakeup_procfs_readers();
-
-	mutex_unlock(&input_mutex);
+		input_wakeup_procfs_readers();
+	}
 
 	device_del(&dev->dev);
 }
@@ -2311,9 +2253,8 @@ static void devm_input_device_unregister(struct device *dev, void *res)
 static void input_repeat_key(struct timer_list *t)
 {
 	struct input_dev *dev = from_timer(dev, t, timer);
-	unsigned long flags;
 
-	spin_lock_irqsave(&dev->event_lock, flags);
+	guard(spinlock_irqsave)(&dev->event_lock);
 
 	if (!dev->inhibited &&
 	    test_bit(dev->repeat_key, dev->key) &&
@@ -2327,8 +2268,6 @@ static void input_repeat_key(struct timer_list *t)
 			mod_timer(&dev->timer, jiffies +
 					msecs_to_jiffies(dev->rep[REP_PERIOD]));
 	}
-
-	spin_unlock_irqrestore(&dev->event_lock, flags);
 }
 
 /**
@@ -2373,10 +2312,10 @@ static int input_device_tune_vals(struct input_dev *dev)
 	if (!vals)
 		return -ENOMEM;
 
-	spin_lock_irq(&dev->event_lock);
-	dev->max_vals = max_vals;
-	swap(dev->vals, vals);
-	spin_unlock_irq(&dev->event_lock);
+	scoped_guard(spinlock_irq, &dev->event_lock) {
+		dev->max_vals = max_vals;
+		swap(dev->vals, vals);
+	}
 
 	/* Because of swap() above, this frees the old vals memory */
 	kfree(vals);
@@ -2468,18 +2407,15 @@ int input_register_device(struct input_dev *dev)
 		path ? path : "N/A");
 	kfree(path);
 
-	error = mutex_lock_interruptible(&input_mutex);
-	if (error)
-		goto err_device_del;
+	error = -EINTR;
+	scoped_cond_guard(mutex_intr, goto err_device_del, &input_mutex) {
+		list_add_tail(&dev->node, &input_dev_list);
 
-	list_add_tail(&dev->node, &input_dev_list);
+		list_for_each_entry(handler, &input_handler_list, node)
+			input_attach_handler(dev, handler);
 
-	list_for_each_entry(handler, &input_handler_list, node)
-		input_attach_handler(dev, handler);
-
-	input_wakeup_procfs_readers();
-
-	mutex_unlock(&input_mutex);
+		input_wakeup_procfs_readers();
+	}
 
 	if (dev->devres_managed) {
 		dev_dbg(dev->dev.parent, "%s: registering %s with devres.\n",
@@ -2559,20 +2495,17 @@ int input_register_handler(struct input_handler *handler)
 	if (error)
 		return error;
 
-	INIT_LIST_HEAD(&handler->h_list);
+	scoped_cond_guard(mutex_intr, return -EINTR, &input_mutex) {
+		INIT_LIST_HEAD(&handler->h_list);
 
-	error = mutex_lock_interruptible(&input_mutex);
-	if (error)
-		return error;
-
-	list_add_tail(&handler->node, &input_handler_list);
+		list_add_tail(&handler->node, &input_handler_list);
 
-	list_for_each_entry(dev, &input_dev_list, node)
-		input_attach_handler(dev, handler);
+		list_for_each_entry(dev, &input_dev_list, node)
+			input_attach_handler(dev, handler);
 
-	input_wakeup_procfs_readers();
+		input_wakeup_procfs_readers();
+	}
 
-	mutex_unlock(&input_mutex);
 	return 0;
 }
 EXPORT_SYMBOL(input_register_handler);
@@ -2588,7 +2521,7 @@ void input_unregister_handler(struct input_handler *handler)
 {
 	struct input_handle *handle, *next;
 
-	mutex_lock(&input_mutex);
+	guard(mutex)(&input_mutex);
 
 	list_for_each_entry_safe(handle, next, &handler->h_list, h_node)
 		handler->disconnect(handle);
@@ -2597,8 +2530,6 @@ void input_unregister_handler(struct input_handler *handler)
 	list_del_init(&handler->node);
 
 	input_wakeup_procfs_readers();
-
-	mutex_unlock(&input_mutex);
 }
 EXPORT_SYMBOL(input_unregister_handler);
 
@@ -2618,19 +2549,17 @@ int input_handler_for_each_handle(struct input_handler *handler, void *data,
 				  int (*fn)(struct input_handle *, void *))
 {
 	struct input_handle *handle;
-	int retval = 0;
+	int retval;
 
-	rcu_read_lock();
+	guard(rcu)();
 
 	list_for_each_entry_rcu(handle, &handler->h_list, h_node) {
 		retval = fn(handle, data);
 		if (retval)
-			break;
+			return retval;
 	}
 
-	rcu_read_unlock();
-
-	return retval;
+	return 0;
 }
 EXPORT_SYMBOL(input_handler_for_each_handle);
 
@@ -2718,27 +2647,22 @@ int input_register_handle(struct input_handle *handle)
 {
 	struct input_handler *handler = handle->handler;
 	struct input_dev *dev = handle->dev;
-	int error;
 
 	input_handle_setup_event_handler(handle);
 	/*
 	 * We take dev->mutex here to prevent race with
 	 * input_release_device().
 	 */
-	error = mutex_lock_interruptible(&dev->mutex);
-	if (error)
-		return error;
-
-	/*
-	 * Filters go to the head of the list, normal handlers
-	 * to the tail.
-	 */
-	if (handler->filter)
-		list_add_rcu(&handle->d_node, &dev->h_list);
-	else
-		list_add_tail_rcu(&handle->d_node, &dev->h_list);
-
-	mutex_unlock(&dev->mutex);
+	scoped_cond_guard(mutex_intr, return -EINTR, &dev->mutex) {
+		/*
+		 * Filters go to the head of the list, normal handlers
+		 * to the tail.
+		 */
+		if (handler->filter)
+			list_add_rcu(&handle->d_node, &dev->h_list);
+		else
+			list_add_tail_rcu(&handle->d_node, &dev->h_list);
+	}
 
 	/*
 	 * Since we are supposed to be called from ->connect()
@@ -2774,9 +2698,8 @@ void input_unregister_handle(struct input_handle *handle)
 	/*
 	 * Take dev->mutex to prevent race with input_release_device().
 	 */
-	mutex_lock(&dev->mutex);
-	list_del_rcu(&handle->d_node);
-	mutex_unlock(&dev->mutex);
+	scoped_guard(mutex, &dev->mutex)
+		list_del_rcu(&handle->d_node);
 
 	synchronize_rcu();
 }
-- 
2.47.0.277.g8800431eea-goog

Re: [PATCH 0/8] Convert input core to use new cleanup facilities

[Dmitry Torokhov]

On Wed, Nov 06, 2024 at 11:15:27PM -0800, Dmitry Torokhov wrote:
> Hi,
> 
> This series converts input core (but not input handlers such as evdev,
> joydev, etc) to use new __free() and guard() cleanup facilities that
> simplify the code and ensure that all resources are released
> appropriately.
> 
> Input handlers will be converted separately later.

Since there were no objections applied for the 6.14 merge window.

Thanks.

-- 
Dmitry

Re: [PATCH 2/8] Input: ff-core - make use of __free() cleanup facility

[Marek Szyprowski]

On 07.11.2024 08:15, Dmitry Torokhov wrote:
> Annotate allocated memory with __free(kfree) to simplify the code and
> make sure memory is released appropriately.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>   drivers/input/ff-core.c | 19 ++++++-------------
>   1 file changed, 6 insertions(+), 13 deletions(-)

This patch landed in linux-next as commit 5203b3a18c1b ("Input: ff-core 
- make use of __free() cleanup facility"). In my tests I found that it 
causes the following kernel panic on some of my test boards. Reverting 
it, together with fd5ba0501d38 ("Input: ff-memless - make use of 
__free() cleanup facility") on top of next-20241220 fixes this panic 
issue. Here is the relevant log captured on Samsung Exynos4412 ARM 
32bit-based Trats2 board:

8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address 
00000024 when read
[00000024] *pgd=00000000
Internal error: Oops: 5 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 
6.13.0-rc3-next-20241220 #15500
Hardware name: Samsung Exynos (Flattened Device Tree)
PC is at input_ff_create+0xa0/0x13c
LR is at input_ff_create+0xb8/0x13c
pc : [<c08d7e14>]    lr : [<c08d7e2c>]    psr: 80000013
...
Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
...
Call trace:
  input_ff_create from input_ff_create_memless+0x8c/0x160
  input_ff_create_memless from max77693_haptic_probe+0x1b0/0x284
  max77693_haptic_probe from platform_probe+0x80/0xc0
  platform_probe from really_probe+0x154/0x3ac
  really_probe from __driver_probe_device+0xa0/0x1d4
  __driver_probe_device from driver_probe_device+0x30/0xd0
  driver_probe_device from __driver_attach+0x10c/0x190
  __driver_attach from bus_for_each_dev+0x60/0xb4
  bus_for_each_dev from bus_add_driver+0xe0/0x220
  bus_add_driver from driver_register+0x7c/0x118
  driver_register from do_one_initcall+0x6c/0x328
  do_one_initcall from kernel_init_freeable+0x1c8/0x218
  kernel_init_freeable from kernel_init+0x1c/0x12c
  kernel_init from ret_from_fork+0x14/0x28
Exception stack(0xf0845fb0 to 0xf0845ff8)
...
---[ end trace 0000000000000000 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
---[ end Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x0000000b ]---

> diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
> index eb01bcb69d00..a235d2eb6b31 100644
> --- a/drivers/input/ff-core.c
> +++ b/drivers/input/ff-core.c
> @@ -290,8 +290,6 @@ EXPORT_SYMBOL_GPL(input_ff_event);
>    */
>   int input_ff_create(struct input_dev *dev, unsigned int max_effects)
>   {
> -	struct ff_device *ff;
> -	size_t ff_dev_size;
>   	int i;
>   
>   	if (!max_effects) {
> @@ -304,25 +302,20 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects)
>   		return -EINVAL;
>   	}
>   
> -	ff_dev_size = struct_size(ff, effect_owners, max_effects);
> -	if (ff_dev_size == SIZE_MAX) /* overflow */
> -		return -EINVAL;
> -
> -	ff = kzalloc(ff_dev_size, GFP_KERNEL);
> +	struct ff_device *ff __free(kfree) =
> +		kzalloc(struct_size(ff, effect_owners, max_effects),
> +			GFP_KERNEL);
>   	if (!ff)
>   		return -ENOMEM;
>   
> -	ff->effects = kcalloc(max_effects, sizeof(struct ff_effect),
> -			      GFP_KERNEL);
> -	if (!ff->effects) {
> -		kfree(ff);
> +	ff->effects = kcalloc(max_effects, sizeof(*ff->effects), GFP_KERNEL);
> +	if (!ff->effects)
>   		return -ENOMEM;
> -	}
>   
>   	ff->max_effects = max_effects;
>   	mutex_init(&ff->mutex);
>   
> -	dev->ff = ff;
> +	dev->ff = no_free_ptr(ff);
>   	dev->flush = input_ff_flush;
>   	dev->event = input_ff_event;
>   	__set_bit(EV_FF, dev->evbit);

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 2/8] Input: ff-core - make use of __free() cleanup facility

[Dmitry Torokhov]

On Fri, Dec 20, 2024 at 01:36:59PM +0100, Marek Szyprowski wrote:
> On 07.11.2024 08:15, Dmitry Torokhov wrote:
> > Annotate allocated memory with __free(kfree) to simplify the code and
> > make sure memory is released appropriately.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >   drivers/input/ff-core.c | 19 ++++++-------------
> >   1 file changed, 6 insertions(+), 13 deletions(-)
> 
> This patch landed in linux-next as commit 5203b3a18c1b ("Input: ff-core 
> - make use of __free() cleanup facility"). In my tests I found that it 
> causes the following kernel panic on some of my test boards. Reverting 
> it, together with fd5ba0501d38 ("Input: ff-memless - make use of 
> __free() cleanup facility") on top of next-20241220 fixes this panic 
> issue.

Gah, I fixed this once and then undid it for some reason. I think the
following should fix the problem:


diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
index a235d2eb6b31..c25e05eeb8e5 100644
--- a/drivers/input/ff-core.c
+++ b/drivers/input/ff-core.c
@@ -315,7 +315,6 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects)
 	ff->max_effects = max_effects;
 	mutex_init(&ff->mutex);
 
-	dev->ff = no_free_ptr(ff);
 	dev->flush = input_ff_flush;
 	dev->event = input_ff_event;
 	__set_bit(EV_FF, dev->evbit);
@@ -328,6 +327,7 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects)
 	if (test_bit(FF_PERIODIC, ff->ffbit))
 		__set_bit(FF_RUMBLE, dev->ffbit);
 
+	dev->ff = no_free_ptr(ff);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(input_ff_create);
diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
index 0bbeceb35545..e9120ba6bae0 100644
--- a/drivers/input/ff-memless.c
+++ b/drivers/input/ff-memless.c
@@ -524,7 +524,6 @@ int input_ff_create_memless(struct input_dev *dev, void *data,
 		return error;
 
 	ff = dev->ff;
-	ff->private = no_free_ptr(ml);
 	ff->upload = ml_ff_upload;
 	ff->playback = ml_ff_playback;
 	ff->set_gain = ml_ff_set_gain;
@@ -541,6 +540,8 @@ int input_ff_create_memless(struct input_dev *dev, void *data,
 	for (i = 0; i < FF_MEMLESS_EFFECTS; i++)
 		ml->states[i].effect = &ff->effects[i];
 
+	ff->private = no_free_ptr(ml);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(input_ff_create_memless);


Thanks.

-- 
Dmitry

Re: [PATCH 2/8] Input: ff-core - make use of __free() cleanup facility

[Marek Szyprowski]

On 20.12.2024 18:22, Dmitry Torokhov wrote:
> On Fri, Dec 20, 2024 at 01:36:59PM +0100, Marek Szyprowski wrote:
>> On 07.11.2024 08:15, Dmitry Torokhov wrote:
>>> Annotate allocated memory with __free(kfree) to simplify the code and
>>> make sure memory is released appropriately.
>>>
>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> ---
>>>    drivers/input/ff-core.c | 19 ++++++-------------
>>>    1 file changed, 6 insertions(+), 13 deletions(-)
>> This patch landed in linux-next as commit 5203b3a18c1b ("Input: ff-core
>> - make use of __free() cleanup facility"). In my tests I found that it
>> causes the following kernel panic on some of my test boards. Reverting
>> it, together with fd5ba0501d38 ("Input: ff-memless - make use of
>> __free() cleanup facility") on top of next-20241220 fixes this panic
>> issue.
> Gah, I fixed this once and then undid it for some reason. I think the
> following should fix the problem:

Yep, this fixes the problem. Feel free to add:

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>


> diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
> index a235d2eb6b31..c25e05eeb8e5 100644
> --- a/drivers/input/ff-core.c
> +++ b/drivers/input/ff-core.c
> @@ -315,7 +315,6 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects)
>   	ff->max_effects = max_effects;
>   	mutex_init(&ff->mutex);
>   
> -	dev->ff = no_free_ptr(ff);
>   	dev->flush = input_ff_flush;
>   	dev->event = input_ff_event;
>   	__set_bit(EV_FF, dev->evbit);
> @@ -328,6 +327,7 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects)
>   	if (test_bit(FF_PERIODIC, ff->ffbit))
>   		__set_bit(FF_RUMBLE, dev->ffbit);
>   
> +	dev->ff = no_free_ptr(ff);
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(input_ff_create);
> diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
> index 0bbeceb35545..e9120ba6bae0 100644
> --- a/drivers/input/ff-memless.c
> +++ b/drivers/input/ff-memless.c
> @@ -524,7 +524,6 @@ int input_ff_create_memless(struct input_dev *dev, void *data,
>   		return error;
>   
>   	ff = dev->ff;
> -	ff->private = no_free_ptr(ml);
>   	ff->upload = ml_ff_upload;
>   	ff->playback = ml_ff_playback;
>   	ff->set_gain = ml_ff_set_gain;
> @@ -541,6 +540,8 @@ int input_ff_create_memless(struct input_dev *dev, void *data,
>   	for (i = 0; i < FF_MEMLESS_EFFECTS; i++)
>   		ml->states[i].effect = &ff->effects[i];
>   
> +	ff->private = no_free_ptr(ml);
> +
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(input_ff_create_memless);
>
>
> Thanks.
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 2/8] Input: ff-core - make use of __free() cleanup facility

[Dmitry Torokhov]

On Fri, Dec 20, 2024 at 06:38:04PM +0100, Marek Szyprowski wrote:
> On 20.12.2024 18:22, Dmitry Torokhov wrote:
> > On Fri, Dec 20, 2024 at 01:36:59PM +0100, Marek Szyprowski wrote:
> >> On 07.11.2024 08:15, Dmitry Torokhov wrote:
> >>> Annotate allocated memory with __free(kfree) to simplify the code and
> >>> make sure memory is released appropriately.
> >>>
> >>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>> ---
> >>>    drivers/input/ff-core.c | 19 ++++++-------------
> >>>    1 file changed, 6 insertions(+), 13 deletions(-)
> >> This patch landed in linux-next as commit 5203b3a18c1b ("Input: ff-core
> >> - make use of __free() cleanup facility"). In my tests I found that it
> >> causes the following kernel panic on some of my test boards. Reverting
> >> it, together with fd5ba0501d38 ("Input: ff-memless - make use of
> >> __free() cleanup facility") on top of next-20241220 fixes this panic
> >> issue.
> > Gah, I fixed this once and then undid it for some reason. I think the
> > following should fix the problem:
> 
> Yep, this fixes the problem. Feel free to add:
> 
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Thank you for the report and testing. Do you mind if I fold these fixes
into original patches?

Thanks.

-- 
Dmitry

Re: [PATCH 2/8] Input: ff-core - make use of __free() cleanup facility

[Marek Szyprowski]

On 20.12.2024 18:39, Dmitry Torokhov wrote:
> On Fri, Dec 20, 2024 at 06:38:04PM +0100, Marek Szyprowski wrote:
>> On 20.12.2024 18:22, Dmitry Torokhov wrote:
>>> On Fri, Dec 20, 2024 at 01:36:59PM +0100, Marek Szyprowski wrote:
>>>> On 07.11.2024 08:15, Dmitry Torokhov wrote:
>>>>> Annotate allocated memory with __free(kfree) to simplify the code and
>>>>> make sure memory is released appropriately.
>>>>>
>>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>>> ---
>>>>>     drivers/input/ff-core.c | 19 ++++++-------------
>>>>>     1 file changed, 6 insertions(+), 13 deletions(-)
>>>> This patch landed in linux-next as commit 5203b3a18c1b ("Input: ff-core
>>>> - make use of __free() cleanup facility"). In my tests I found that it
>>>> causes the following kernel panic on some of my test boards. Reverting
>>>> it, together with fd5ba0501d38 ("Input: ff-memless - make use of
>>>> __free() cleanup facility") on top of next-20241220 fixes this panic
>>>> issue.
>>> Gah, I fixed this once and then undid it for some reason. I think the
>>> following should fix the problem:
>> Yep, this fixes the problem. Feel free to add:
>>
>> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Thank you for the report and testing. Do you mind if I fold these fixes
> into original patches?

Go ahead, no problem for me.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland