[PATCH v2 0/7] HID: sensor: custom: use-after-freec after remove

[PATCH v2 0/7] HID: sensor: custom: use-after-freec after remove

[Srinivas Pandruvada]

This is v2 for patch " [PATCH] HID: sensor-custom: Synchronize
misc-device teardown with file lifetime".

This patch series is essentially the same version that was sent to
Семён for testing.

Following a feedback review conducted with various AI models, it was
suggested to break each change into individual, separate patches. This
makes perfect sense, which is why this series now contains more patches.

Additionally, these reviews identified issues in the adjacent probe and
removal code when active sensor instances are present. I have addressed
those issues in this version as well.

Srinivas Pandruvada (7):
  HID: sensor: hub: Synchronize callback removal with raw events
  HID: sensor: custom: Set owner for misc device
  HID: sensor: custom: Disable sensor before hub close on removal
  HID: sensor: custom: Fix state lifetime during removal
  HID: sensor: custom: Serialize misc device reads
  HID: sensor: custom: Register enable attribute after misc device
  HID: sensor: hub: Remove clients before stopping hardware

 drivers/hid/hid-sensor-custom.c | 121 +++++++++++++++++++++++---------
 drivers/hid/hid-sensor-hub.c    |  27 +++++--
 2 files changed, 110 insertions(+), 38 deletions(-)

-- 
2.56.0

[PATCH v2 1/7] HID: sensor: hub: Synchronize callback removal with raw events

[Srinivas Pandruvada]

Raw-event processing holds pdata->lock across callback lookup and
invocation, and takes dyn_callback_lock during lookup. Removal must
prevent both active callbacks and future lookups before client state can
be released.

Take pdata->lock before dyn_callback_lock, matching raw-event order,
and unlink the callback while holding both locks. This ensures active
raw-event callbacks have finished and prevents future lookups.

Fixes: 401ca24fb34a ("HID: sensors: introduce sensor framework")
Reported-by: Семён Марьясин <simeon@maryasin.name>
Closes: https://lore.kernel.org/all/CAM2dg7ewfVJSP4rFyhkCB1Rmd1+X56sAg=eUdbPthG2xrLqwQQ@mail.gmail.com/
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/hid-sensor-hub.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 6470a290ebfc..0c4884c5821f 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -173,7 +173,9 @@ int sensor_hub_remove_callback(struct hid_sensor_hub_device *hsdev,
 	struct sensor_hub_data *pdata = hid_get_drvdata(hsdev->hdev);
 	unsigned long flags;
 
-	spin_lock_irqsave(&pdata->dyn_callback_lock, flags);
+	/* Serialize callback removal with raw-event callback execution. */
+	spin_lock_irqsave(&pdata->lock, flags);
+	spin_lock(&pdata->dyn_callback_lock);
 	list_for_each_entry(callback, &pdata->dyn_callback_list, list)
 		if (callback->usage_id == usage_id &&
 						callback->hsdev == hsdev) {
@@ -181,7 +183,8 @@ int sensor_hub_remove_callback(struct hid_sensor_hub_device *hsdev,
 			kfree(callback);
 			break;
 		}
-	spin_unlock_irqrestore(&pdata->dyn_callback_lock, flags);
+	spin_unlock(&pdata->dyn_callback_lock);
+	spin_unlock_irqrestore(&pdata->lock, flags);
 
 	return 0;
 }
-- 
2.56.0

[PATCH v2 2/7] HID: sensor: custom: Set owner for misc device

[Srinivas Pandruvada]

Set .owner in the misc-device file operations so open file descriptors
keep the module loaded until their release callbacks have completed.

Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
Reported-by: Семён Марьясин <simeon@maryasin.name>
Closes: https://lore.kernel.org/all/CAM2dg7ewfVJSP4rFyhkCB1Rmd1+X56sAg=eUdbPthG2xrLqwQQ@mail.gmail.com/
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/hid-sensor-custom.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index c2b425afd951..72e5639cb1d9 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -719,6 +719,7 @@ static __poll_t hid_sensor_custom_poll(struct file *file,
 }
 
 static const struct file_operations hid_sensor_custom_fops = {
+	.owner = THIS_MODULE,
 	.open =  hid_sensor_custom_open,
 	.read =  hid_sensor_custom_read,
 	.release = hid_sensor_custom_release,
-- 
2.56.0

[PATCH v2 3/7] HID: sensor: custom: Disable sensor before hub close on removal

[Srinivas Pandruvada]

When removing an enabled sensor, set its power and reporting states to
off before closing the hub. Keep the field descriptors alive until the
state update has completed, and release the hub's open reference.

Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
Reported-by: Семён Марьясин <simeon@maryasin.name>
Closes: https://lore.kernel.org/all/CAM2dg7ewfVJSP4rFyhkCB1Rmd1+X56sAg=eUdbPthG2xrLqwQQ@mail.gmail.com/
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/hid-sensor-custom.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index 72e5639cb1d9..8375697b459d 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -1053,6 +1053,13 @@ static void hid_sensor_custom_remove(struct platform_device *pdev)
 	/* Remove enable_sensor first as it uses fields via power_state/report_state. */
 	sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
 			   &enable_sensor_attr_group);
+	mutex_lock(&sensor_inst->mutex);
+	if (sensor_inst->enable) {
+		set_power_report_state(sensor_inst, false);
+		sensor_hub_device_close(hsdev);
+		sensor_inst->enable = false;
+	}
+	mutex_unlock(&sensor_inst->mutex);
 	hid_sensor_custom_remove_attributes(sensor_inst);
 	sensor_hub_remove_callback(hsdev, hsdev->usage);
 }
-- 
2.56.0

[PATCH v2 4/7] HID: sensor: custom: Fix state lifetime during removal

[Srinivas Pandruvada]

Open misc device files can outlive platform-device removal. Since the
devm-allocated state can be freed while a reader is queued on its wait
queue, subsequent file operations may access freed memory.

Allocate the state independently of devres and give every open
file a reference to keep the FIFO and state alive after removal.

On removal, mark the device as disconnected and wake readers; make reads
return -ENODEV and polls report hangup and error. Free the FIFO and state
when the final reference is released. Unregister the sensor hub callback
before tearing down the interface so in-flight raw-event processing
finishes before the sensor state can be freed.

Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
Reported-by: Семён Марьясин <simeon@maryasin.name>
Closes: https://lore.kernel.org/all/CAM2dg7ewfVJSP4rFyhkCB1Rmd1+X56sAg=eUdbPthG2xrLqwQQ@mail.gmail.com/
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/hid-sensor-custom.c | 82 ++++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 26 deletions(-)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index 8375697b459d..2a3b502821c7 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -11,6 +11,7 @@
 #include <linux/init.h>
 #include <linux/miscdevice.h>
 #include <linux/kfifo.h>
+#include <linux/kref.h>
 #include <linux/sched.h>
 #include <linux/wait.h>
 #include <linux/poll.h>
@@ -37,6 +38,8 @@ struct hid_sensor_custom_field {
 
 struct hid_sensor_custom {
 	struct mutex mutex;
+	struct kref ref;
+	bool removed;
 	struct platform_device *pdev;
 	struct hid_sensor_hub_device *hsdev;
 	struct hid_sensor_hub_callbacks callbacks;
@@ -643,6 +646,15 @@ static void hid_sensor_custom_remove_attributes(struct hid_sensor_custom *
 	kfree(sensor_inst->fields);
 }
 
+static void hid_sensor_custom_free(struct kref *ref)
+{
+	struct hid_sensor_custom *sensor_inst =
+		container_of(ref, struct hid_sensor_custom, ref);
+
+	kfifo_free(&sensor_inst->data_fifo);
+	kfree(sensor_inst);
+}
+
 static ssize_t hid_sensor_custom_read(struct file *file, char __user *buf,
 				      size_t count, loff_t *f_ps)
 {
@@ -656,22 +668,27 @@ static ssize_t hid_sensor_custom_read(struct file *file, char __user *buf,
 	if (count < sizeof(struct hid_sensor_sample))
 		return -EINVAL;
 
-	do {
-		if (kfifo_is_empty(&sensor_inst->data_fifo)) {
-			if (file->f_flags & O_NONBLOCK)
-				return -EAGAIN;
+	for (;;) {
+		if (READ_ONCE(sensor_inst->removed))
+			return -ENODEV;
 
-			ret = wait_event_interruptible(sensor_inst->wait,
-				!kfifo_is_empty(&sensor_inst->data_fifo));
-			if (ret)
-				return ret;
-		}
 		ret = kfifo_to_user(&sensor_inst->data_fifo, buf, count,
 				    &copied);
+		if (ret || copied)
+			break;
+
+		if (file->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		ret = wait_event_interruptible(sensor_inst->wait,
+			READ_ONCE(sensor_inst->removed) ||
+			!kfifo_is_empty(&sensor_inst->data_fifo));
 		if (ret)
 			return ret;
+	}
 
-	} while (copied == 0);
+	if (ret)
+		return ret;
 
 	return copied;
 }
@@ -684,6 +701,7 @@ static int hid_sensor_custom_release(struct inode *inode, struct file *file)
 				   struct hid_sensor_custom, custom_dev);
 
 	clear_bit(0, &sensor_inst->misc_opened);
+	kref_put(&sensor_inst->ref, hid_sensor_custom_free);
 
 	return 0;
 }
@@ -691,14 +709,25 @@ static int hid_sensor_custom_release(struct inode *inode, struct file *file)
 static int hid_sensor_custom_open(struct inode *inode, struct file *file)
 {
 	struct hid_sensor_custom *sensor_inst;
+	int ret;
 
 	sensor_inst = container_of(file->private_data,
 				   struct hid_sensor_custom, custom_dev);
+	if (READ_ONCE(sensor_inst->removed))
+		return -ENODEV;
+
 	/* We essentially have single reader and writer */
 	if (test_and_set_bit(0, &sensor_inst->misc_opened))
 		return -EBUSY;
 
-	return stream_open(inode, file);
+	kref_get(&sensor_inst->ref);
+	ret = stream_open(inode, file);
+	if (ret) {
+		clear_bit(0, &sensor_inst->misc_opened);
+		kref_put(&sensor_inst->ref, hid_sensor_custom_free);
+	}
+
+	return ret;
 }
 
 static __poll_t hid_sensor_custom_poll(struct file *file,
@@ -712,6 +741,9 @@ static __poll_t hid_sensor_custom_poll(struct file *file,
 
 	poll_wait(file, &sensor_inst->wait, wait);
 
+	if (READ_ONCE(sensor_inst->removed))
+		return EPOLLHUP | EPOLLERR;
+
 	if (!kfifo_is_empty(&sensor_inst->data_fifo))
 		mask = EPOLLIN | EPOLLRDNORM;
 
@@ -741,21 +773,15 @@ static int hid_sensor_custom_dev_if_add(struct hid_sensor_custom *sensor_inst)
 	sensor_inst->custom_dev.minor = MISC_DYNAMIC_MINOR;
 	sensor_inst->custom_dev.name = dev_name(&sensor_inst->pdev->dev);
 	sensor_inst->custom_dev.fops = &hid_sensor_custom_fops;
-	ret = misc_register(&sensor_inst->custom_dev);
-	if (ret) {
-		kfifo_free(&sensor_inst->data_fifo);
-		return ret;
-	}
-	return 0;
+	return misc_register(&sensor_inst->custom_dev);
 }
 
 static void hid_sensor_custom_dev_if_remove(struct hid_sensor_custom
 								*sensor_inst)
 {
-	wake_up(&sensor_inst->wait);
+	WRITE_ONCE(sensor_inst->removed, true);
+	wake_up_interruptible_all(&sensor_inst->wait);
 	misc_deregister(&sensor_inst->custom_dev);
-	kfifo_free(&sensor_inst->data_fifo);
-
 }
 
 /*
@@ -978,8 +1004,7 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 	int ret;
 	const struct hid_sensor_custom_match *match;
 
-	sensor_inst = devm_kzalloc(&pdev->dev, sizeof(*sensor_inst),
-				   GFP_KERNEL);
+	sensor_inst = kzalloc_obj(*sensor_inst);
 	if (!sensor_inst)
 		return -ENOMEM;
 
@@ -989,6 +1014,7 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 	sensor_inst->hsdev = hsdev;
 	sensor_inst->pdev = pdev;
 	mutex_init(&sensor_inst->mutex);
+	kref_init(&sensor_inst->ref);
 	platform_set_drvdata(pdev, sensor_inst);
 
 	ret = hid_sensor_custom_get_known(hsdev, &match);
@@ -1000,7 +1026,7 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 		if (ret) {
 			dev_err(&pdev->dev,
 				"register_platform_device failed\n");
-			return ret;
+			goto err_free;
 		}
 
 		return 0;
@@ -1010,7 +1036,7 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 					   &sensor_inst->callbacks);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "callback reg failed\n");
-		return ret;
+		goto err_free;
 	}
 
 	ret = hid_sensor_custom_add_attributes(sensor_inst);
@@ -1035,6 +1061,8 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 	hid_sensor_custom_remove_attributes(sensor_inst);
 err_remove_callback:
 	sensor_hub_remove_callback(hsdev, hsdev->usage);
+err_free:
+	kref_put(&sensor_inst->ref, hid_sensor_custom_free);
 
 	return ret;
 }
@@ -1046,9 +1074,10 @@ static void hid_sensor_custom_remove(struct platform_device *pdev)
 
 	if (sensor_inst->custom_pdev) {
 		platform_device_unregister(sensor_inst->custom_pdev);
-		return;
+		goto out;
 	}
 
+	sensor_hub_remove_callback(hsdev, hsdev->usage);
 	hid_sensor_custom_dev_if_remove(sensor_inst);
 	/* Remove enable_sensor first as it uses fields via power_state/report_state. */
 	sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
@@ -1061,7 +1090,8 @@ static void hid_sensor_custom_remove(struct platform_device *pdev)
 	}
 	mutex_unlock(&sensor_inst->mutex);
 	hid_sensor_custom_remove_attributes(sensor_inst);
-	sensor_hub_remove_callback(hsdev, hsdev->usage);
+out:
+	kref_put(&sensor_inst->ref, hid_sensor_custom_free);
 }
 
 static const struct platform_device_id hid_sensor_custom_ids[] = {
-- 
2.56.0

[PATCH v2 5/7] HID: sensor: custom: Serialize misc device reads

[Srinivas Pandruvada]

The misc device permits only one open file, but threads sharing that
file can still read concurrently. Serialize readers to preserve the
single-reader assumption of the FIFO while keeping removal independent
of a stalled user copy.

Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
Reported-by: Семён Марьясин <simeon@maryasin.name>
Closes: https://lore.kernel.org/all/CAM2dg7ewfVJSP4rFyhkCB1Rmd1+X56sAg=eUdbPthG2xrLqwQQ@mail.gmail.com/
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/hid-sensor-custom.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index 2a3b502821c7..4f824cadd370 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -38,6 +38,7 @@ struct hid_sensor_custom_field {
 
 struct hid_sensor_custom {
 	struct mutex mutex;
+	struct mutex read_mutex;
 	struct kref ref;
 	bool removed;
 	struct platform_device *pdev;
@@ -668,25 +669,39 @@ static ssize_t hid_sensor_custom_read(struct file *file, char __user *buf,
 	if (count < sizeof(struct hid_sensor_sample))
 		return -EINVAL;
 
+	if (file->f_flags & O_NONBLOCK) {
+		if (!mutex_trylock(&sensor_inst->read_mutex))
+			return -EAGAIN;
+	} else {
+		ret = mutex_lock_interruptible(&sensor_inst->read_mutex);
+		if (ret)
+			return ret;
+	}
+
 	for (;;) {
-		if (READ_ONCE(sensor_inst->removed))
-			return -ENODEV;
+		if (READ_ONCE(sensor_inst->removed)) {
+			ret = -ENODEV;
+			break;
+		}
 
 		ret = kfifo_to_user(&sensor_inst->data_fifo, buf, count,
 				    &copied);
 		if (ret || copied)
 			break;
 
-		if (file->f_flags & O_NONBLOCK)
-			return -EAGAIN;
+		if (file->f_flags & O_NONBLOCK) {
+			ret = -EAGAIN;
+			break;
+		}
 
 		ret = wait_event_interruptible(sensor_inst->wait,
 			READ_ONCE(sensor_inst->removed) ||
 			!kfifo_is_empty(&sensor_inst->data_fifo));
 		if (ret)
-			return ret;
+			break;
 	}
 
+	mutex_unlock(&sensor_inst->read_mutex);
 	if (ret)
 		return ret;
 
@@ -1014,6 +1029,7 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 	sensor_inst->hsdev = hsdev;
 	sensor_inst->pdev = pdev;
 	mutex_init(&sensor_inst->mutex);
+	mutex_init(&sensor_inst->read_mutex);
 	kref_init(&sensor_inst->ref);
 	platform_set_drvdata(pdev, sensor_inst);
 
-- 
2.56.0

[PATCH v2 6/7] HID: sensor: custom: Register enable attribute after misc device

[Srinivas Pandruvada]

Register the enable_sensor sysfs group only after misc-device setup
succeeds. This prevents userspace from opening the sensor hub while
probe can still fail, which would otherwise leave the hub open reference
unbalanced on unwind.

Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
Reported-by: Семён Марьясин <simeon@maryasin.name>
Closes: https://lore.kernel.org/all/CAM2dg7ewfVJSP4rFyhkCB1Rmd1+X56sAg=eUdbPthG2xrLqwQQ@mail.gmail.com/
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/hid-sensor-custom.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index 4f824cadd370..990e653995e1 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -1059,20 +1059,21 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_remove_callback;
 
-	ret = sysfs_create_group(&sensor_inst->pdev->dev.kobj,
-				 &enable_sensor_attr_group);
+	ret = hid_sensor_custom_dev_if_add(sensor_inst);
 	if (ret)
 		goto err_remove_attributes;
 
-	ret = hid_sensor_custom_dev_if_add(sensor_inst);
+	ret = sysfs_create_group(&sensor_inst->pdev->dev.kobj,
+				 &enable_sensor_attr_group);
 	if (ret)
-		goto err_remove_group;
+		goto err_remove_dev_if;
 
 	return 0;
 
-err_remove_group:
-	sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
-			   &enable_sensor_attr_group);
+err_remove_dev_if:
+	sensor_hub_remove_callback(hsdev, hsdev->usage);
+	hid_sensor_custom_dev_if_remove(sensor_inst);
+	goto err_remove_attributes;
 err_remove_attributes:
 	hid_sensor_custom_remove_attributes(sensor_inst);
 err_remove_callback:
-- 
2.56.0

[PATCH v2 7/7] HID: sensor: hub: Remove clients before stopping hardware

[Srinivas Pandruvada]

Remove MFD clients before stopping the HID hardware so their close calls
remain valid. Runtime-PM teardown may cancel a pending autosuspend without
running the client suspend callback, leaving hub references after child
removal.

Clients share one HID open while ref_cnt is nonzero. After removing the
children, drain any remaining references and call hid_hw_close() once if
the count is nonzero. Then stop the HID hardware. This avoids both a
leaked open and the underflow caused by an unconditional close.

The pending-request walk runs before hid_hw_stop() disconnects HID-core
children such as hidraw. Skip non-platform children and platform children
without sensor-hub data before checking pending request state.

Fixes: 1df3a401154a ("HID: Delay opening HID device")
Reported-by: Семён Марьясин <simeon@maryasin.name>
Closes: https://lore.kernel.org/all/CAM2dg7ewfVJSP4rFyhkCB1Rmd1+X56sAg=eUdbPthG2xrLqwQQ@mail.gmail.com/
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/hid-sensor-hub.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 0c4884c5821f..e9d6eb3b3831 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -10,6 +10,7 @@
 #include <linux/slab.h>
 #include <linux/mfd/core.h>
 #include <linux/list.h>
+#include <linux/platform_device.h>
 #include <linux/hid-sensor-ids.h>
 #include <linux/hid-sensor-hub.h>
 #include "hid-ids.h"
@@ -806,7 +807,14 @@ static int sensor_hub_probe(struct hid_device *hdev,
 
 static int sensor_hub_finalize_pending_fn(struct device *dev, void *data)
 {
-	struct hid_sensor_hub_device *hsdev = dev->platform_data;
+	struct hid_sensor_hub_device *hsdev;
+
+	if (!dev_is_platform(dev))
+		return 0;
+
+	hsdev = to_platform_device(dev)->dev.platform_data;
+	if (!hsdev)
+		return 0;
 
 	if (hsdev->pending.status)
 		complete(&hsdev->pending.ready);
@@ -820,8 +828,6 @@ static void sensor_hub_remove(struct hid_device *hdev)
 	unsigned long flags;
 
 	hid_dbg(hdev, " hardware removed\n");
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 
 	spin_lock_irqsave(&data->lock, flags);
 	device_for_each_child(&hdev->dev, NULL,
@@ -829,6 +835,14 @@ static void sensor_hub_remove(struct hid_device *hdev)
 	spin_unlock_irqrestore(&data->lock, flags);
 
 	mfd_remove_devices(&hdev->dev);
+	/* Clients share one HID open; release it if references remain. */
+	mutex_lock(&data->mutex);
+	if (data->ref_cnt) {
+		data->ref_cnt = 0;
+		hid_hw_close(hdev);
+	}
+	mutex_unlock(&data->mutex);
+	hid_hw_stop(hdev);
 	mutex_destroy(&data->mutex);
 }
 
-- 
2.56.0

Re: [PATCH v2 6/7] HID: sensor: custom: Register enable attribute after misc device

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] HID: sensor: custom: duplicate callback removal in hid_sensor_custom_probe()
--

commit 3c6fa7005fd38bc50c4da9b69a1182ec5457915f
Author: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

HID: sensor: custom: Register enable attribute after misc device

This commit reorders hid_sensor_custom_probe() so enable_sensor_attr_group is
registered after hid_sensor_custom_dev_if_add() succeeds, preventing userspace
from enabling the sensor hub while probe can still fail.

> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index 4f824cadd370a..990e653995e15 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -1059,20 +1059,21 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_remove_callback;
>  
> -	ret = sysfs_create_group(&sensor_inst->pdev->dev.kobj,
> -				 &enable_sensor_attr_group);
> +	ret = hid_sensor_custom_dev_if_add(sensor_inst);
>  	if (ret)
>  		goto err_remove_attributes;
>  
> -	ret = hid_sensor_custom_dev_if_add(sensor_inst);
> +	ret = sysfs_create_group(&sensor_inst->pdev->dev.kobj,
> +				 &enable_sensor_attr_group);
>  	if (ret)
> -		goto err_remove_group;
> +		goto err_remove_dev_if;
>  
>  	return 0;
>  
> -err_remove_group:
> -	sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
> -			   &enable_sensor_attr_group);
> +err_remove_dev_if:
> +	sensor_hub_remove_callback(hsdev, hsdev->usage);
> +	hid_sensor_custom_dev_if_remove(sensor_inst);
> +	goto err_remove_attributes;
>  err_remove_attributes:
>  	hid_sensor_custom_remove_attributes(sensor_inst);
>  err_remove_callback:

[Severity: Low]
In hid_sensor_custom_probe() in drivers/hid/hid-sensor-custom.c, when
sysfs_create_group() for enable_sensor_attr_group fails and jumps to
err_remove_dev_if, sensor_hub_remove_callback(hsdev, hsdev->usage) is called
before hid_sensor_custom_remove_attributes(sensor_inst), followed by a goto to
the immediately following err_remove_attributes label and a fallthrough to
err_remove_callback:

err_remove_dev_if:
	sensor_hub_remove_callback(hsdev, hsdev->usage);
	hid_sensor_custom_dev_if_remove(sensor_inst);
	goto err_remove_attributes;
err_remove_attributes:
	hid_sensor_custom_remove_attributes(sensor_inst);
err_remove_callback:
	sensor_hub_remove_callback(hsdev, hsdev->usage);

Because hid_sensor_custom_probe() still holds the initial reference on
sensor_inst until err_free (where kref_put() calls hid_sensor_custom_free() to
free sensor_inst->data_fifo and sensor_inst), removing the callback at
err_remove_dev_if unregisters the callback while the field sysfs attributes are
still exposed, and then calls sensor_hub_remove_callback() a second time at
err_remove_callback.

Should err_remove_dev_if just call hid_sensor_custom_dev_if_remove(sensor_inst)
and fall through to err_remove_attributes and err_remove_callback?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104201403.2039921-1-srinivas.pandruvada@linux.intel.com?part=6

Re: [PATCH v2 6/7] HID: sensor: custom: Register enable attribute after misc device

[srinivas pandruvada]

On Wed, 2026-11-04 at 20:25 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [Low] HID: sensor: custom: duplicate callback removal in
> hid_sensor_custom_probe()
> --
> 
> commit 3c6fa7005fd38bc50c4da9b69a1182ec5457915f
> Author: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> 
> HID: sensor: custom: Register enable attribute after misc device
> 
> This commit reorders hid_sensor_custom_probe() so
> enable_sensor_attr_group is
> registered after hid_sensor_custom_dev_if_add() succeeds, preventing
> userspace
> from enabling the sensor hub while probe can still fail.
> 

Will fix in next rev as this a minor issue to readjust code.

Thanks,
Srinivas

> > diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-
> > sensor-custom.c
> > index 4f824cadd370a..990e653995e15 100644
> > --- a/drivers/hid/hid-sensor-custom.c
> > +++ b/drivers/hid/hid-sensor-custom.c
> > @@ -1059,20 +1059,21 @@ static int hid_sensor_custom_probe(struct
> > platform_device *pdev)
> >  	if (ret)
> >  		goto err_remove_callback;
> >  
> > -	ret = sysfs_create_group(&sensor_inst->pdev->dev.kobj,
> > -				 &enable_sensor_attr_group);
> > +	ret = hid_sensor_custom_dev_if_add(sensor_inst);
> >  	if (ret)
> >  		goto err_remove_attributes;
> >  
> > -	ret = hid_sensor_custom_dev_if_add(sensor_inst);
> > +	ret = sysfs_create_group(&sensor_inst->pdev->dev.kobj,
> > +				 &enable_sensor_attr_group);
> >  	if (ret)
> > -		goto err_remove_group;
> > +		goto err_remove_dev_if;
> >  
> >  	return 0;
> >  
> > -err_remove_group:
> > -	sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
> > -			   &enable_sensor_attr_group);
> > +err_remove_dev_if:
> > +	sensor_hub_remove_callback(hsdev, hsdev->usage);
> > +	hid_sensor_custom_dev_if_remove(sensor_inst);
> > +	goto err_remove_attributes;
> >  err_remove_attributes:
> >  	hid_sensor_custom_remove_attributes(sensor_inst);
> >  err_remove_callback:
> 
> [Severity: Low]
> In hid_sensor_custom_probe() in drivers/hid/hid-sensor-custom.c, when
> sysfs_create_group() for enable_sensor_attr_group fails and jumps to
> err_remove_dev_if, sensor_hub_remove_callback(hsdev, hsdev->usage) is
> called
> before hid_sensor_custom_remove_attributes(sensor_inst), followed by
> a goto to
> the immediately following err_remove_attributes label and a
> fallthrough to
> err_remove_callback:
> 
> err_remove_dev_if:
> 	sensor_hub_remove_callback(hsdev, hsdev->usage);
> 	hid_sensor_custom_dev_if_remove(sensor_inst);
> 	goto err_remove_attributes;
> err_remove_attributes:
> 	hid_sensor_custom_remove_attributes(sensor_inst);
> err_remove_callback:
> 	sensor_hub_remove_callback(hsdev, hsdev->usage);
> 
> Because hid_sensor_custom_probe() still holds the initial reference
> on
> sensor_inst until err_free (where kref_put() calls
> hid_sensor_custom_free() to
> free sensor_inst->data_fifo and sensor_inst), removing the callback
> at
> err_remove_dev_if unregisters the callback while the field sysfs
> attributes are
> still exposed, and then calls sensor_hub_remove_callback() a second
> time at
> err_remove_callback.
> 
> Should err_remove_dev_if just call
> hid_sensor_custom_dev_if_remove(sensor_inst)
> and fall through to err_remove_attributes and err_remove_callback?