[PATCH v3 01/10] media: synopsys: hdmirx: free the driver data when the last user is gone

[Sascha Hauer]

The driver data is allocated with devm_kzalloc() and contains the
video_device, the vb2 queue and the control handler. devres frees it when
the device is unbound, but an open file descriptor on the video device
node keeps the video_device referenced beyond that point:
video_unregister_device() only drops the reference taken at registration,
and the core's release of the device node is deferred until the last
close. When that close comes, the core calls the vb2 release fop on the
freed queue and v4l2_device_release() reads the minor, the cdev and the
release callback from the freed video_device. A file that subscribed to
control events additionally has the core look the control up in the
freed handler.

Documentation/driver-api/media/v4l2-dev.rst asks drivers that embed the
video_device to free the containing structure from a release callback,
when the last user of the device node is gone. Do that, using the
v4l2_device release callback since that refcount is taken per registered
device node. Allocate the driver data with kzalloc(), free it together
with the control handler from the release callback, and have remove() and
the probe error paths drop the reference instead of relying on devres.
Error paths before v4l2_device_register() free the allocation directly,
no reference exists yet.

With the controls alive until the last user is gone, hdmirx_disable() can
run before the control handler is freed on both remove and the probe
error paths. The NULL checks in hdmirx_plugout() and the pointer clearing
in hdmirx_remove() that worked around the old order go away. The
hdl->error path in probe also no longer leaks the handler.

Fixes: 7b59b132ad439 ("media: platform: synopsys: Add support for HDMI input driver")
Cc: stable@xxxxxxxxxxxxxxx
Assisted-by: LLM
Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
---
 .../media/platform/synopsys/hdmirx/snps_hdmirx.c   | 56 +++++++++++++---------
 1 file changed, 34 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
index 25f8ca0d6d946..bc541431638e7 100644
--- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
+++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
@@ -637,13 +637,9 @@ static void hdmirx_plugout(struct snps_hdmirx_dev *hdmirx_dev)
 	hdmirx_writel(hdmirx_dev, PHYCREG_CONFIG0, 0x0);
 	cancel_delayed_work(&hdmirx_dev->delayed_work_res_change);
 
-	/* will be NULL on driver removal */
-	if (hdmirx_dev->rgb_range)
-		v4l2_ctrl_s_ctrl(hdmirx_dev->rgb_range, V4L2_DV_RGB_RANGE_AUTO);
-
-	if (hdmirx_dev->content_type)
-		v4l2_ctrl_s_ctrl(hdmirx_dev->content_type,
-				 V4L2_DV_IT_CONTENT_TYPE_NO_ITC);
+	v4l2_ctrl_s_ctrl(hdmirx_dev->rgb_range, V4L2_DV_RGB_RANGE_AUTO);
+	v4l2_ctrl_s_ctrl(hdmirx_dev->content_type,
+			 V4L2_DV_IT_CONTENT_TYPE_NO_ITC);
 
 	hdmirx_dev->plugged = false;
 }
@@ -2646,6 +2642,16 @@ static int hdmirx_register_cec(struct snps_hdmirx_dev *hdmirx_dev,
 	return 0;
 }
 
+/* Runs when the last user of a device node is gone, possibly after remove() */
+static void hdmirx_v4l2_release(struct v4l2_device *v4l2_dev)
+{
+	struct snps_hdmirx_dev *hdmirx_dev =
+		container_of(v4l2_dev, struct snps_hdmirx_dev, v4l2_dev);
+
+	v4l2_ctrl_handler_free(&hdmirx_dev->hdl);
+	kfree(hdmirx_dev);
+}
+
 static int hdmirx_probe(struct platform_device *pdev)
 {
 	struct snps_hdmirx_dev *hdmirx_dev;
@@ -2655,7 +2661,7 @@ static int hdmirx_probe(struct platform_device *pdev)
 	struct v4l2_device *v4l2_dev;
 	int ret;
 
-	hdmirx_dev = devm_kzalloc(dev, sizeof(*hdmirx_dev), GFP_KERNEL);
+	hdmirx_dev = kzalloc_obj(*hdmirx_dev);
 	if (!hdmirx_dev)
 		return -ENOMEM;
 
@@ -2665,23 +2671,25 @@ static int hdmirx_probe(struct platform_device *pdev)
 	 */
 	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
 	if (ret)
-		return ret;
+		goto err_free;
 
 	hdmirx_dev->dev = dev;
 	dev_set_drvdata(dev, hdmirx_dev);
 
 	ret = hdmirx_parse_dt(hdmirx_dev);
 	if (ret)
-		return ret;
+		goto err_free;
 
 	ret = hdmirx_setup_irq(hdmirx_dev, pdev);
 	if (ret)
-		return ret;
+		goto err_free;
 
 	hdmirx_dev->regs = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(hdmirx_dev->regs))
-		return dev_err_probe(dev, PTR_ERR(hdmirx_dev->regs),
-				     "failed to remap regs resource\n");
+	if (IS_ERR(hdmirx_dev->regs)) {
+		ret = dev_err_probe(dev, PTR_ERR(hdmirx_dev->regs),
+				    "failed to remap regs resource\n");
+		goto err_free;
+	}
 
 	mutex_init(&hdmirx_dev->phy_rw_lock);
 	mutex_init(&hdmirx_dev->stream_lock);
@@ -2735,11 +2743,12 @@ static int hdmirx_probe(struct platform_device *pdev)
 		goto err_pm;
 	}
 	hdmirx_dev->v4l2_dev.ctrl_handler = hdl;
+	hdmirx_dev->v4l2_dev.release = hdmirx_v4l2_release;
 
 	ret = v4l2_device_register(dev, &hdmirx_dev->v4l2_dev);
 	if (ret < 0) {
 		dev_err_probe(dev, ret, "v4l2 device registration failed\n");
-		goto err_hdl;
+		goto err_pm;
 	}
 
 	stream = &hdmirx_dev->stream;
@@ -2771,10 +2780,16 @@ static int hdmirx_probe(struct platform_device *pdev)
 	vb2_video_unregister_device(&hdmirx_dev->stream.vdev);
 err_unreg_v4l2_dev:
 	v4l2_device_unregister(&hdmirx_dev->v4l2_dev);
-err_hdl:
-	v4l2_ctrl_handler_free(&hdmirx_dev->hdl);
+	hdmirx_disable(dev);
+	v4l2_device_put(&hdmirx_dev->v4l2_dev);
+
+	return ret;
+
 err_pm:
 	hdmirx_disable(dev);
+	v4l2_ctrl_handler_free(&hdmirx_dev->hdl);
+err_free:
+	kfree(hdmirx_dev);
 
 	return ret;
 }
@@ -2792,16 +2807,13 @@ static void hdmirx_remove(struct platform_device *pdev)
 	hdmirx_disable_irq(dev);
 
 	vb2_video_unregister_device(&hdmirx_dev->stream.vdev);
-	v4l2_ctrl_handler_free(&hdmirx_dev->hdl);
 	v4l2_device_unregister(&hdmirx_dev->v4l2_dev);
 
-	/* touched by hdmirx_disable()->hdmirx_plugout() */
-	hdmirx_dev->rgb_range = NULL;
-	hdmirx_dev->content_type = NULL;
-
 	hdmirx_disable(dev);
 
 	reset_control_bulk_assert(HDMIRX_NUM_RST, hdmirx_dev->resets);
+
+	v4l2_device_put(&hdmirx_dev->v4l2_dev);
 }
 
 static const struct of_device_id hdmirx_id[] = {

-- 
2.47.3