[PATCH] media: i2c: hi556: fix power leak on remove while runtime-active

[PATCH] media: i2c: hi556: fix power leak on remove while runtime-active

[Cong Nguyen]

hi556_remove() calls pm_runtime_disable() but never hi556_suspend().
pm_runtime_disable() only blocks future auto suspend/resume, it
doesn't force a final one -- if unbound while hi556_set_stream() is
holding an outstanding PM reference (removed while still streaming),
the device stays genuinely active with regulators/clock enabled
forever after.

probe()'s own error path already shows the right pairing (calls
hi556_suspend() when powered on). Do the same in remove(), guarded by
pm_runtime_status_suspended() so the common already-suspended case
isn't suspended twice -- same guard sibling imx412.c's remove() uses.

Moved remove() after suspend()/resume()'s definitions since it now
calls them directly.

Fixes: e62138403a84 ("media: hi556: Add support for Hi-556 sensor")
Assisted-by: LLM
Signed-off-by: Cong Nguyen <congnt264@gmail.com>
---
 drivers/media/i2c/hi556.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/hi556.c b/drivers/media/i2c/hi556.c
index de573cee4451..45d1366c6d5a 100644
--- a/drivers/media/i2c/hi556.c
+++ b/drivers/media/i2c/hi556.c
@@ -1264,18 +1264,6 @@ static int hi556_check_hwcfg(struct device *dev)
 	return ret;
 }
 
-static void hi556_remove(struct i2c_client *client)
-{
-	struct v4l2_subdev *sd = i2c_get_clientdata(client);
-	struct hi556 *hi556 = to_hi556(sd);
-
-	v4l2_async_unregister_subdev(sd);
-	media_entity_cleanup(&sd->entity);
-	v4l2_ctrl_handler_free(sd->ctrl_handler);
-	pm_runtime_disable(hi556->dev);
-	mutex_destroy(&hi556->mutex);
-}
-
 static int hi556_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
@@ -1316,6 +1304,20 @@ static int hi556_resume(struct device *dev)
 	return 0;
 }
 
+static void hi556_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct hi556 *hi556 = to_hi556(sd);
+
+	v4l2_async_unregister_subdev(sd);
+	media_entity_cleanup(&sd->entity);
+	v4l2_ctrl_handler_free(sd->ctrl_handler);
+	pm_runtime_disable(hi556->dev);
+	if (!pm_runtime_status_suspended(hi556->dev))
+		hi556_suspend(hi556->dev);
+	mutex_destroy(&hi556->mutex);
+}
+
 static int hi556_probe(struct i2c_client *client)
 {
 	struct hi556 *hi556;
-- 
2.25.1

Re: [PATCH] media: i2c: hi556: fix power leak on remove while runtime-active

[Sakari Ailus]

Hi Cong,

Thanks for the patches.

On Mon, Sep 28, 2026 at 08:51:40PM +0700, Cong Nguyen wrote:
> hi556_remove() calls pm_runtime_disable() but never hi556_suspend().
> pm_runtime_disable() only blocks future auto suspend/resume, it
> doesn't force a final one -- if unbound while hi556_set_stream() is
> holding an outstanding PM reference (removed while still streaming),
> the device stays genuinely active with regulators/clock enabled
> forever after.
> 
> probe()'s own error path already shows the right pairing (calls
> hi556_suspend() when powered on). Do the same in remove(), guarded by
> pm_runtime_status_suspended() so the common already-suspended case
> isn't suspended twice -- same guard sibling imx412.c's remove() uses.
> 
> Moved remove() after suspend()/resume()'s definitions since it now
> calls them directly.

A few notes on this as the rest:

- The quality of this commit message needs to be improved.

- When you're sending a bunch of similar patches, please send them in a
  single patchset.

- When moving functions around and then making changes to them, please do
  this in two separate patches.

- If the device's PM runtime status isn't SUSPENDED in remove(), it needs
  to be set SUSPENDED using pm_runtime_set_suspended(). Please do this
  while making other PM runtime changes to the remove functions.

I'm marking these as "changes requested".

-- 
Kind regards,

Sakari Ailus