[PATCH 0/5] media: verisilicon: Recover the Rockchip VPUs from faults and timeouts

[PATCH 0/5] media: verisilicon: Recover the Rockchip VPUs from faults and timeouts

[Arun Iyengar via B4 Relay]

This series makes the Hantro driver recover from a job that times out
on the Rockchip VPUs, and fixes a panic on module unload on the RK3588.

On the RK3588, a VEPU121 JPEG encoder core that reads an unmapped IOVA
faults in its IOMMU and never becomes idle again: the register-level
stop in the encoder reset operation isn't enough, the IOMMU can't be
stalled anymore ("Enable stall request timed out"), every later job
times out and unbinding the driver can lock up the system. Patches 3
and 4 pulse the reset lines of the core on a timeout and reinitialize
the IOMMU afterwards, the way rkvdec does. Patches 1 and 5 describe the
AXI/AHB reset lines of the four VEPU121 cores in the binding and in the
RK3588 device tree; the driver already handles optional resets, so
device trees without them keep working.

Patch 2 fixes a panic on module unload: the AV1 decoder resets of the
RK3588 also cover the bus interface of its IOMMU, and holding them
asserted after remove makes the IOMMU driver's runtime suspend hit
registers in reset (SError).

Patch 5 touches arch/arm64 only and can go through the Rockchip tree
independently of the driver patches; it is only useful together with
patch 3.

Tested on an Orange Pi 5 (RK3588S). Patch 2 was verified by unloading
and reloading the module with the AV1 decoder bound and a 1 s gap, which
panicked before the change and is clean with it. Patches 3 and 4 ran on
every encoder timeout since: a trace of a wedged VEPU121 job shows
hantro_watchdog() -> rockchip_vpu2_enc_reset() -> the IOMMU re-attach
with no stall or paging timeout, the next jobs on the same core succeed,
and the IOMMU runtime-suspends cleanly afterwards; later, ten JPEG
encode jobs with an unmapped input address were each reset by the
timeout and the following jobs encoded normally. The reset lines of
patches 1 and 5 are present on all four VEPU121 nodes of the live
device tree, dt_binding_check and CHECK_DTBS are clean, and the encoder
keeps working with them.

This series was developed with Claude (Anthropic) as a coding assistant:
it wrote the code and the commit messages, and I reviewed and tested
every change as described above.

Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
Arun Iyengar (5):
      dt-bindings: media: rockchip,rk3568-vepu: Add the reset lines
      media: verisilicon: Don't hold a VPU behind an IOMMU in reset after remove
      media: verisilicon: Reset the Rockchip encoder cores on timeout
      media: verisilicon: Reinitialize the IOMMU after a timeout
      arm64: dts: rockchip: Add the reset lines of the RK3588 VEPU121 cores

 .../bindings/media/rockchip,rk3568-vepu.yaml       | 10 +++
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi      |  8 +++
 drivers/media/platform/verisilicon/hantro.h        |  3 +
 drivers/media/platform/verisilicon/hantro_drv.c    | 77 +++++++++++++++++++++-
 .../media/platform/verisilicon/rockchip_vpu_hw.c   | 30 +++++++++
 5 files changed, 126 insertions(+), 2 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261110-hantro-robust-6a6c84febb06

Best regards,
--  
Arun Iyengar <aiyengar1990@gmail.com>

[PATCH 1/5] dt-bindings: media: rockchip,rk3568-vepu: Add the reset lines

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

The VEPU121 encoder cores of the RK3588 each have their own AXI and AHB
reset lines in the CRU (SRST_A_JPEG_ENCODERx and SRST_H_JPEG_ENCODERx).
They are needed to recover a core that is stuck on the bus, which its
registers can't stop.

Describe them as optional, so that existing device trees stay valid.

Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 .../devicetree/bindings/media/rockchip,rk3568-vepu.yaml        | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
index d246f5d38427..e9fda4373f0f 100644
--- a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
+++ b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
@@ -32,6 +32,16 @@ properties:
       - const: aclk
       - const: hclk
 
+  resets:
+    items:
+      - description: AXI reset line
+      - description: AHB reset line
+
+  reset-names:
+    items:
+      - const: axi
+      - const: ahb
+
   power-domains:
     maxItems: 1
 

-- 
2.55.0

[PATCH 2/5] media: verisilicon: Don't hold a VPU behind an IOMMU in reset after remove

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

On the RK3588, the resets of the AV1 decoder include the AXI and APB
bus interface resets (SRST_A_AV1_BIU, SRST_P_AV1_BIU), which also cover
the Verisilicon IOMMU of the decoder. When the driver is unbound, the
driver core runtime resumes the device and, through the runtime PM
device link, its IOMMU. hantro_remove() then asserts the resets, and
when the IOMMU autosuspends 100 ms later, vsi_iommu_suspend() writes to
its registers while they are held in reset, which raises an SError and
panics the system:

  SError Interrupt on CPU4, code 0x00000000be000411 -- SError
  Workqueue: pm pm_runtime_work
  Call trace:
   clk_disable+0x30/0x90 (P)
   clk_bulk_disable+0x44/0x90
   vsi_iommu_suspend+0x4c/0xc8 [vsi_iommu]
   pm_generic_runtime_suspend+0x38/0x80
   genpd_runtime_suspend+0xc4/0x348
   ...
  Kernel panic - not syncing: Asynchronous SError Interrupt

This was hit when reloading the module, unless the decoder happened to
be probed again within the autosuspend delay of its IOMMU.

The IOMMU outlives the VPU driver, so leave the resets of a VPU that is
behind an IOMMU deasserted on remove and on the probe error path. VPUs
without an IOMMU are still put into reset.

Fixes: ea71631b7129 ("media: hantro: add support for reset lines")
Cc: stable@vger.kernel.org
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/media/platform/verisilicon/hantro_drv.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index 26e5d5547466..702d878c7b50 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -1069,6 +1069,22 @@ static struct v4l2_m2m_dev *hantro_get_v4l2_m2m_dev(struct hantro_dev *vpu)
 	return v4l2_m2m_init(&vpu_m2m_ops);
 }
 
+/*
+ * The resets may also cover the bus interface that the IOMMU of the VPU is
+ * accessed through, e.g. on the RK3588 AV1 decoder. The IOMMU is a separate
+ * device which is runtime resumed with the VPU while it is unbound, and
+ * accesses its registers again when it runtime suspends afterwards. Holding
+ * the VPU in reset at that point causes an asynchronous external abort, so
+ * leave the resets deasserted for a VPU behind an IOMMU.
+ */
+static void hantro_reset_assert(struct hantro_dev *vpu)
+{
+	if (device_iommu_mapped(vpu->dev))
+		return;
+
+	reset_control_assert(vpu->resets);
+}
+
 static int hantro_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
@@ -1267,7 +1283,7 @@ static int hantro_probe(struct platform_device *pdev)
 err_pm_disable_assert_reset:
 	pm_runtime_dont_use_autosuspend(vpu->dev);
 	pm_runtime_disable(vpu->dev);
-	reset_control_assert(vpu->resets);
+	hantro_reset_assert(vpu);
 	return ret;
 err_pm_disable:
 	pm_runtime_dont_use_autosuspend(vpu->dev);
@@ -1290,7 +1306,7 @@ static void hantro_remove(struct platform_device *pdev)
 	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
 	pm_runtime_dont_use_autosuspend(vpu->dev);
 	pm_runtime_disable(vpu->dev);
-	reset_control_assert(vpu->resets);
+	hantro_reset_assert(vpu);
 }
 
 static int hantro_runtime_resume(struct device *dev)

-- 
2.55.0

[PATCH 3/5] media: verisilicon: Reset the Rockchip encoder cores on timeout

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

On a job timeout, the Rockchip VEPU1 and VEPU2 reset operations mask the
interrupt and clear the encode enable and AXI control registers. That
only stops a core that is still responsive. On RK3588, when a VEPU121
core reads an IOVA that isn't mapped, the IOMMU reports a page fault and
the core never becomes idle again: its AXI transactions stay
outstanding, the IOMMU can't be stalled anymore ("Enable stall request
timed out", then "Disable paging request timed out" on runtime
suspend), every later job times out as well, and unbinding the driver
can lock up the system.

The driver already gets the optional reset lines of the device and
deasserts them at probe. Pulse them in the encoder reset operations,
after stopping the core through its registers, to return it to a clean
idle state. The clocks are still enabled at that point, as the job that
timed out holds them. This is a no-op on device trees that don't
describe the resets, which is the case for all of them so far.

The Rockchip BSP MPP driver resets these cores the same way, through
their AXI and AHB reset lines.

Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 .../media/platform/verisilicon/rockchip_vpu_hw.c   | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/media/platform/verisilicon/rockchip_vpu_hw.c b/drivers/media/platform/verisilicon/rockchip_vpu_hw.c
index 02673be9878e..0dced7515833 100644
--- a/drivers/media/platform/verisilicon/rockchip_vpu_hw.c
+++ b/drivers/media/platform/verisilicon/rockchip_vpu_hw.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 
 #include "hantro.h"
 #include "hantro_jpeg.h"
@@ -468,6 +469,33 @@ static void rk3066_vpu_dec_reset(struct hantro_ctx *ctx)
 	vdpu_write(vpu, G1_REG_CONFIG_DEC_CLK_GATE_E, G1_REG_CONFIG);
 }
 
+/*
+ * Stopping the encoder through its registers isn't enough when it is stuck
+ * on the bus, for instance after a translation fault in its IOMMU: it keeps
+ * AXI transactions outstanding and never becomes idle, which also prevents
+ * the IOMMU from stalling. Pulse the reset lines of the core, if the device
+ * tree describes them, to return it to its idle state.
+ */
+static void rockchip_vpu_enc_hw_reset(struct hantro_dev *vpu)
+{
+	int ret;
+
+	if (!vpu->resets)
+		return;
+
+	ret = reset_control_assert(vpu->resets);
+	if (ret) {
+		dev_warn(vpu->dev, "Failed to assert resets: %d\n", ret);
+		return;
+	}
+
+	udelay(5);
+
+	ret = reset_control_deassert(vpu->resets);
+	if (ret)
+		dev_err(vpu->dev, "Failed to deassert resets: %d\n", ret);
+}
+
 static void rockchip_vpu1_enc_reset(struct hantro_ctx *ctx)
 {
 	struct hantro_dev *vpu = ctx->dev;
@@ -475,6 +503,7 @@ static void rockchip_vpu1_enc_reset(struct hantro_ctx *ctx)
 	vepu_write(vpu, H1_REG_INTERRUPT_DIS_BIT, H1_REG_INTERRUPT);
 	vepu_write(vpu, 0, H1_REG_ENC_CTRL);
 	vepu_write(vpu, 0, H1_REG_AXI_CTRL);
+	rockchip_vpu_enc_hw_reset(vpu);
 }
 
 static void rockchip_vpu2_dec_reset(struct hantro_ctx *ctx)
@@ -493,6 +522,7 @@ static void rockchip_vpu2_enc_reset(struct hantro_ctx *ctx)
 	vepu_write(vpu, VEPU_REG_INTERRUPT_DIS_BIT, VEPU_REG_INTERRUPT);
 	vepu_write(vpu, 0, VEPU_REG_ENCODE_START);
 	vepu_write(vpu, 0, VEPU_REG_AXI_CTRL);
+	rockchip_vpu_enc_hw_reset(vpu);
 }
 
 /*

-- 
2.55.0

[PATCH 4/5] media: verisilicon: Reinitialize the IOMMU after a timeout

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

A job that times out can leave the IOMMU of the VPU in a bad state: a
core that faulted on an unmapped IOVA leaves the IOMMU with a page fault
pending, and resetting the core can leave the IOMMU out of sync with
what its driver expects. The IOMMU is only reinitialized when it runtime
suspends and resumes, which doesn't happen between back-to-back jobs, so
the next jobs run with a broken IOMMU and time out as well.

Do like rkvdec: allocate an empty paging domain at probe, and after a
timeout attach it and detach it again, which returns the device to its
default DMA domain. The IOMMU driver disables the IOMMU and fully
reinitializes it on each domain change (on Rockchip: stall, force reset,
program the directory table and enable paging), and the default domain
keeps all the existing mappings. This is done from the watchdog work,
in process context, after the codec reset operation stopped the core
and while the job still holds the runtime PM reference, so the IOMMU is
powered.

Devices without an IOMMU, and IOMMUs that can't allocate a paging
domain, are left alone.

Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/media/platform/verisilicon/hantro.h     |  3 ++
 drivers/media/platform/verisilicon/hantro_drv.c | 57 +++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/drivers/media/platform/verisilicon/hantro.h b/drivers/media/platform/verisilicon/hantro.h
index 0353de154a1e..51bc40668c6f 100644
--- a/drivers/media/platform/verisilicon/hantro.h
+++ b/drivers/media/platform/verisilicon/hantro.h
@@ -193,6 +193,8 @@ hantro_vdev_to_func(struct video_device *vdev)
  *			shared with interrupt handlers.
  * @variant:		Hardware variant-specific parameters.
  * @watchdog_work:	Delayed work for hardware timeout handling.
+ * @empty_domain:	Empty IOMMU domain, used to reinitialize the IOMMU after
+ *			a timeout. NULL if the VPU isn't behind an IOMMU.
  */
 struct hantro_dev {
 	struct v4l2_device v4l2_dev;
@@ -213,6 +215,7 @@ struct hantro_dev {
 	spinlock_t irqlock;
 	const struct hantro_variant *variant;
 	struct delayed_work watchdog_work;
+	struct iommu_domain *empty_domain;
 };
 
 /**
diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index 702d878c7b50..c85c8f07db34 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -11,6 +11,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/iommu.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
@@ -112,6 +113,31 @@ void hantro_irq_done(struct hantro_dev *vpu,
 	}
 }
 
+/*
+ * A job that timed out can leave the IOMMU stalled, for instance on a page
+ * fault the core never recovered from, and a hardware reset of the core can
+ * leave the IOMMU state out of sync with its driver. Attach the empty domain
+ * and detach it again to return to the default DMA domain: the IOMMU driver
+ * then disables and fully reinitializes the IOMMU, which keeps all the
+ * mappings of the default domain.
+ */
+static void hantro_iommu_restore(struct hantro_dev *vpu)
+{
+	int ret;
+
+	if (!vpu->empty_domain)
+		return;
+
+	ret = iommu_attach_device(vpu->empty_domain, vpu->dev);
+	if (ret) {
+		dev_warn(vpu->dev, "Failed to attach the empty IOMMU domain: %d\n",
+			 ret);
+		return;
+	}
+
+	iommu_detach_device(vpu->empty_domain, vpu->dev);
+}
+
 void hantro_watchdog(struct work_struct *work)
 {
 	struct hantro_dev *vpu;
@@ -124,6 +150,7 @@ void hantro_watchdog(struct work_struct *work)
 		vpu_err("frame processing timed out!\n");
 		if (ctx->codec_ops->reset)
 			ctx->codec_ops->reset(ctx);
+		hantro_iommu_restore(vpu);
 		hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
 	}
 }
@@ -1085,6 +1112,32 @@ static void hantro_reset_assert(struct hantro_dev *vpu)
 	reset_control_assert(vpu->resets);
 }
 
+static void hantro_free_empty_domain(void *data)
+{
+	iommu_domain_free(data);
+}
+
+static int hantro_alloc_empty_domain(struct hantro_dev *vpu)
+{
+	struct iommu_domain *domain;
+
+	if (!iommu_get_domain_for_dev(vpu->dev))
+		return 0;
+
+	domain = iommu_paging_domain_alloc(vpu->dev);
+	if (IS_ERR(domain)) {
+		/* Not fatal, the IOMMU just won't be restored on timeouts. */
+		dev_warn(vpu->dev, "Failed to allocate an empty IOMMU domain: %pe\n",
+			 domain);
+		return 0;
+	}
+
+	vpu->empty_domain = domain;
+
+	return devm_add_action_or_reset(vpu->dev, hantro_free_empty_domain,
+					domain);
+}
+
 static int hantro_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
@@ -1146,6 +1199,10 @@ static int hantro_probe(struct platform_device *pdev)
 	if (IS_ERR(vpu->resets))
 		return PTR_ERR(vpu->resets);
 
+	ret = hantro_alloc_empty_domain(vpu);
+	if (ret)
+		return ret;
+
 	num_bases = vpu->variant->num_regs ?: 1;
 	vpu->reg_bases = devm_kcalloc(&pdev->dev, num_bases,
 				      sizeof(*vpu->reg_bases), GFP_KERNEL);

-- 
2.55.0

[PATCH 5/5] arm64: dts: rockchip: Add the reset lines of the RK3588 VEPU121 cores

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

Describe the AXI and AHB reset lines of the four VEPU121 JPEG encoder
cores, so that the driver can reset a core that is stuck after a fault
of its IOMMU.

The IOMMU of each core sits in the same register block and is clocked
by the same clocks, so these resets likely cover its bus interface as
well. This relies on the driver not leaving a VPU behind an IOMMU in
reset after remove ("media: verisilicon: Don't hold a VPU behind an
IOMMU in reset after remove").

Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
index aa69fd3793fd..227465d446a2 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
@@ -1325,6 +1325,8 @@ vepu121_0: video-codec@fdba0000 {
 		clock-names = "aclk", "hclk";
 		iommus = <&vepu121_0_mmu>;
 		power-domains = <&power RK3588_PD_VDPU>;
+		resets = <&cru SRST_A_JPEG_ENCODER0>, <&cru SRST_H_JPEG_ENCODER0>;
+		reset-names = "axi", "ahb";
 	};
 
 	vepu121_0_mmu: iommu@fdba0800 {
@@ -1345,6 +1347,8 @@ vepu121_1: video-codec@fdba4000 {
 		clock-names = "aclk", "hclk";
 		iommus = <&vepu121_1_mmu>;
 		power-domains = <&power RK3588_PD_VDPU>;
+		resets = <&cru SRST_A_JPEG_ENCODER1>, <&cru SRST_H_JPEG_ENCODER1>;
+		reset-names = "axi", "ahb";
 	};
 
 	vepu121_1_mmu: iommu@fdba4800 {
@@ -1365,6 +1369,8 @@ vepu121_2: video-codec@fdba8000 {
 		clock-names = "aclk", "hclk";
 		iommus = <&vepu121_2_mmu>;
 		power-domains = <&power RK3588_PD_VDPU>;
+		resets = <&cru SRST_A_JPEG_ENCODER2>, <&cru SRST_H_JPEG_ENCODER2>;
+		reset-names = "axi", "ahb";
 	};
 
 	vepu121_2_mmu: iommu@fdba8800 {
@@ -1385,6 +1391,8 @@ vepu121_3: video-codec@fdbac000 {
 		clock-names = "aclk", "hclk";
 		iommus = <&vepu121_3_mmu>;
 		power-domains = <&power RK3588_PD_VDPU>;
+		resets = <&cru SRST_A_JPEG_ENCODER3>, <&cru SRST_H_JPEG_ENCODER3>;
+		reset-names = "axi", "ahb";
 	};
 
 	vepu121_3_mmu: iommu@fdbac800 {

-- 
2.55.0

Re: [PATCH 2/5] media: verisilicon: Don't hold a VPU behind an IOMMU in reset after remove

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] media: verisilicon: null pointer dereference in hantro_probe()
  [1]
--

commit 4e4d9ad2d3b9dfeb29c03b860ceeca44937863cf
Author: Arun Iyengar <aiyengar1990@gmail.com>

media: verisilicon: Don't hold a VPU behind an IOMMU in reset after remove

This commit leaves the resets of a VPU behind an IOMMU deasserted during
probe error cleanup and driver removal so the IOMMU can access its
registers when it subsequently runtime suspends.

> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 26e5d5547466e..702d878c7b509 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -1069,6 +1069,22 @@ static struct v4l2_m2m_dev *hantro_get_v4l2_m2m_dev(struct hantro_dev *vpu)
>  	return v4l2_m2m_init(&vpu_m2m_ops);
>  }
>  
> +/*
> + * The resets may also cover the bus interface that the IOMMU of the VPU is
> + * accessed through, e.g. on the RK3588 AV1 decoder. The IOMMU is a separate
> + * device which is runtime resumed with the VPU while it is unbound, and
> + * accesses its registers again when it runtime suspends afterwards. Holding
> + * the VPU in reset at that point causes an asynchronous external abort, so
> + * leave the resets deasserted for a VPU behind an IOMMU.
> + */
> +static void hantro_reset_assert(struct hantro_dev *vpu)
> +{
> +	if (device_iommu_mapped(vpu->dev))
> +		return;
> +
> +	reset_control_assert(vpu->resets);
> +}
> +
>  static int hantro_probe(struct platform_device *pdev)
>  {
>  	const struct of_device_id *match;

[Severity: High]
This problem wasn't introduced by this patch, but can hantro_probe() hit a
NULL pointer dereference [1] when dereferencing match->data?

drivers/media/platform/verisilicon/hantro_drv.c:hantro_probe() {
	...
	match = of_match_node(of_hantro_match, pdev->dev.of_node);
	vpu->variant = match->data;
	...
}

If a probe is forced via sysfs bind on a device without a matching OF node,
of_match_node() returns NULL and match->data is dereferenced
unconditionally.

[1] https://sashiko.dev/#/bug/linux-4e7cd977-2daf-4048-ad53-7801b4bba80e

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-hantro-robust-v1-0-6b6269c05bfa@gmail.com?part=2

Re: [PATCH 3/5] media: verisilicon: Reset the Rockchip encoder cores on timeout

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] media: verisilicon: uncleared IRQ disable bit in
  hantro_h1_jpeg_enc_run() [1]
--

commit 433d2270fcbefc689a4dc88d1c36fa283eb8f9a5
Author: Arun Iyengar <aiyengar1990@gmail.com>

media: verisilicon: Reset the Rockchip encoder cores on timeout

This commit pulses the optional hardware reset lines in the Rockchip
VEPU1 and VEPU2 reset callbacks to recover encoder cores that become
stuck on the bus after an IOMMU fault and job timeout.

> diff --git a/drivers/media/platform/verisilicon/rockchip_vpu_hw.c b/drivers/media/platform/verisilicon/rockchip_vpu_hw.c
> index 02673be9878e1..0dced75158334 100644
> --- a/drivers/media/platform/verisilicon/rockchip_vpu_hw.c
> +++ b/drivers/media/platform/verisilicon/rockchip_vpu_hw.c

[ ... ]

> @@ -468,6 +469,33 @@ static void rk3066_vpu_dec_reset(struct hantro_ctx *ctx)
>  	vdpu_write(vpu, G1_REG_CONFIG_DEC_CLK_GATE_E, G1_REG_CONFIG);
>  }
>  
> +/*
> + * Stopping the encoder through its registers isn't enough when it is stuck
> + * on the bus, for instance after a translation fault in its IOMMU: it keeps
> + * AXI transactions outstanding and never becomes idle, which also prevents
> + * the IOMMU from stalling. Pulse the reset lines of the core, if the device
> + * tree describes them, to return it to its idle state.
> + */
> +static void rockchip_vpu_enc_hw_reset(struct hantro_dev *vpu)
> +{
> +	int ret;
> +
> +	if (!vpu->resets)
> +		return;
> +
> +	ret = reset_control_assert(vpu->resets);
> +	if (ret) {
> +		dev_warn(vpu->dev, "Failed to assert resets: %d\n", ret);
> +		return;
> +	}
> +
> +	udelay(5);
> +
> +	ret = reset_control_deassert(vpu->resets);
> +	if (ret)
> +		dev_err(vpu->dev, "Failed to deassert resets: %d\n", ret);
> +}
> +
>  static void rockchip_vpu1_enc_reset(struct hantro_ctx *ctx)
>  {
>  	struct hantro_dev *vpu = ctx->dev;
> @@ -475,6 +503,7 @@ static void rockchip_vpu1_enc_reset(struct hantro_ctx *ctx)
>  	vepu_write(vpu, H1_REG_INTERRUPT_DIS_BIT, H1_REG_INTERRUPT);
>  	vepu_write(vpu, 0, H1_REG_ENC_CTRL);
>  	vepu_write(vpu, 0, H1_REG_AXI_CTRL);
> +	rockchip_vpu_enc_hw_reset(vpu);
>  }

[Severity: Medium]
This problem wasn't introduced by this patch, but when vpu->resets is not
present in the device tree, does rockchip_vpu1_enc_reset() leave
H1_REG_INTERRUPT_DIS_BIT set in H1_REG_INTERRUPT [1] across later
runs?

hantro_h1_jpeg_enc_run() starts the encoder without writing to
H1_REG_INTERRUPT to clear H1_REG_INTERRUPT_DIS_BIT, so after a watchdog
reset all subsequent H1 JPEG encoding jobs have completion interrupts
disabled and time out as well.

[1] https://sashiko.dev/#/bug/linux-22957774-1c15-4f2d-87b4-d6b2e043ac2a

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-hantro-robust-v1-0-6b6269c05bfa@gmail.com?part=3

Re: [PATCH 4/5] media: verisilicon: Reinitialize the IOMMU after a timeout

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [High] media: verisilicon: failed attach in hantro_iommu_restore()
  detaches IOMMU
- [High] media: verisilicon: race in hantro_iommu_restore() with
  vsi_iommu_flush_tlb()
- [Medium] media: verisilicon: unconditional IOMMU domain swap in
  hantro_iommu_restore()
- [Medium] media: verisilicon: hantro_watchdog() restores IOMMU without
  stopping AV1 core
- [Medium] media: verisilicon: hantro_iommu_restore() fails with -EBUSY
  on ARM32
- [Medium] media: verisilicon: check iommu_is_dma_domain() in
  hantro_alloc_empty_domain()
--

commit 61759d53b32f194a0a0f3fec45e34dc338e8cad9
Author: Arun Iyengar <aiyengar1990@gmail.com>

media: verisilicon: Reinitialize the IOMMU after a timeout

This commit allocates an empty IOMMU paging domain at probe and attaches
and detaches it in hantro_watchdog() after a job timeout to reinitialize
the VPU IOMMU.

> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 702d878c7b509..c85c8f07db34d 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
[ ... ]
> @@ -112,6 +113,31 @@ void hantro_irq_done(struct hantro_dev *vpu,
>  	}
>  }
>  
> +/*
> + * A job that timed out can leave the IOMMU stalled, for instance on a page
> + * fault the core never recovered from, and a hardware reset of the core can
> + * leave the IOMMU state out of sync with its driver. Attach the empty domain
> + * and detach it again to return to the default DMA domain: the IOMMU driver
> + * then disables and fully reinitializes the IOMMU, which keeps all the
> + * mappings of the default domain.
> + */
> +static void hantro_iommu_restore(struct hantro_dev *vpu)
> +{
> +	int ret;
> +
> +	if (!vpu->empty_domain)
> +		return;
> +
> +	ret = iommu_attach_device(vpu->empty_domain, vpu->dev);
> +	if (ret) {
> +		dev_warn(vpu->dev, "Failed to attach the empty IOMMU domain: %d\n",
> +			 ret);
> +		return;
> +	}

[Severity: High]
Can a failed iommu_attach_device() here leave the Rockchip IOMMU
permanently detached from its default DMA domain?

Most Rockchip VPU device tree nodes lack reset lines and the decoder reset
callbacks only write registers, so a wedged core can keep AXI transactions
pending and cause rk_iommu_enable() to fail when attaching
vpu->empty_domain:

drivers/iommu/rockchip-iommu.c:rk_iommu_attach_device() {
	...
	ret = rk_iommu_identity_attach(&rk_identity_domain, dev, old);
	if (ret)
		return ret;

	iommu->domain = domain;
	...
	ret = rk_iommu_enable(iommu);
	if (ret) {
		WARN_ON(rk_iommu_identity_attach(&rk_identity_domain, dev,
						 iommu->domain));
	}
	...
}

Because rk_iommu_attach_device() falls back to &rk_identity_domain while
__iommu_group_set_domain_internal() leaves group->domain as default_domain
without re-attaching the single group device, rk_iommu_resume() skips
rk_iommu_enable() on subsequent runtime PM cycles and the IOMMU stays in
identity bypass while DMA buffers still use default_domain IOVAs.

> +
> +	iommu_detach_device(vpu->empty_domain, vpu->dev);
> +}

[Severity: High]
Can attaching and detaching vpu->empty_domain in hantro_iommu_restore()
race with concurrent DMA mappings on the RK3588 AV1 decoder and corrupt
list traversal in vsi_iommu_flush_tlb()?

hantro_watchdog() runs without vpu_mutex while V4L2 buffer ioctls (such as
VIDIOC_REQBUFS or VIDIOC_QBUF) map or unmap buffers on default_domain. On
rockchip,rk3588-av1-vpu (which sits behind verisilicon,iommu-1.2),
vsi_iommu_attach_device() moves iommu->node from default_domain->iommus to
empty_domain->iommus under empty_domain->lock without taking
default_domain->lock, while vsi_iommu_flush_tlb() drops iommu->lock before
advancing to the next entry:

CPU0 (hantro_watchdog)            CPU1 (V4L2 buffer map/unmap)
hantro_iommu_restore()            vsi_iommu_map()
                                    lock(&default_domain->lock)
                                    vsi_iommu_flush_tlb(default_domain)
                                      list_for_each_entry(iommu,
                                          &default_domain->iommus, node)
                                        lock(&iommu->lock)
                                        ...
                                        unlock(&iommu->lock)
  iommu_attach_device(empty_domain)
    vsi_iommu_attach_device()
      lock(&empty_domain->lock)
      lock(&iommu->lock)
      list_del_init(&iommu->node)
      list_add_tail(&iommu->node,
          &empty_domain->iommus)
      unlock(&iommu->lock)
      unlock(&empty_domain->lock)
                                        pm_runtime_put_autosuspend()
                                        /* follows node.next into
                                         * &empty_domain->iommus */
                                        <-- out-of-bounds dereference

vsi_iommu_flush_tlb() misses the &default_domain->iommus loop termination
check and dereferences container_of(&empty_domain->iommus, struct
vsi_iommu, node) out of bounds.

>  
>  void hantro_watchdog(struct work_struct *work)
>  {
>  	struct hantro_dev *vpu;
> @@ -124,6 +150,7 @@ void hantro_watchdog(struct work_struct *work)
>  		vpu_err("frame processing timed out!\n");
>  		if (ctx->codec_ops->reset)
>  			ctx->codec_ops->reset(ctx);
> +		hantro_iommu_restore(vpu);
>  		hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
>  	}
>  }

[Severity: Medium]
Does calling hantro_iommu_restore() when ctx->codec_ops->reset is NULL
switch the IOMMU to an empty page table while the RK3588 AV1 decoder core
is still running?

rk3588_vpu981_codec_ops[HANTRO_MODE_AV1_DEC] defines no .reset callback,
so on a timeout the AV1 core is neither stopped nor reset before
vsi_iommu_attach_device() programs an empty page table and flushes the TLB
while the core may still be issuing DMA transactions.

> @@ -1085,6 +1112,32 @@ static void hantro_reset_assert(struct hantro_dev *vpu)
>  	reset_control_assert(vpu->resets);
>  }
>  
> +static void hantro_free_empty_domain(void *data)
> +{
> +	iommu_domain_free(data);
> +}
> +
> +static int hantro_alloc_empty_domain(struct hantro_dev *vpu)
> +{
> +	struct iommu_domain *domain;
> +
> +	if (!iommu_get_domain_for_dev(vpu->dev))
> +		return 0;
> +
> +	domain = iommu_paging_domain_alloc(vpu->dev);

[Severity: Medium]
Does allocating vpu->empty_domain for every Hantro device with an IOMMU
domain leak DMA mappings and clock enable references on Allwinner H6?

allwinner,sun50i-h6-vpu-g2 uses sun50i-iommu, where
sun50i_iommu_domain_alloc_paging() initializes sun50i_domain->refcnt to 1
and sun50i_iommu_attach_device() increments it to 2 before calling
sun50i_iommu_identity_attach():

drivers/iommu/sun50i-iommu.c:sun50i_iommu_attach_device() {
	...
	refcount_inc(&sun50i_domain->refcnt);

	if (iommu->domain == domain)
		return 0;

	sun50i_iommu_identity_attach(&sun50i_iommu_identity_domain, dev, old);

	sun50i_iommu_attach_domain(iommu, sun50i_domain);

	return 0;
}

Because sun50i_iommu_identity_attach() only decrements refcnt from 2 to 1,
it never calls sun50i_iommu_detach_domain(), so each attach/detach cycle
in hantro_iommu_restore() calls sun50i_iommu_enable() twice without
disabling, leaking two dma_map_single() mappings and two
clk_prepare_enable() references per timeout (or failing with -EINVAL when
sibling devices share the singleton generic_single_device_group).

[Severity: Medium]
On 32-bit ARM platforms such as RK3288, does allocating vpu->empty_domain
cause iommu_attach_device() in hantro_iommu_restore() to fail with -EBUSY
on every timeout?

CONFIG_ROCKCHIP_IOMMU selects CONFIG_ARM_DMA_USE_IOMMU on 32-bit ARM,
where group->default_domain is forced to IOMMU_DOMAIN_IDENTITY while
arm_setup_iommu_dma_ops() attaches an unmanaged mapping->domain to
group->domain, causing __iommu_attach_group() to reject
iommu_attach_device(vpu->empty_domain, vpu->dev) with -EBUSY because
group->domain != group->default_domain.

[Severity: Medium]
What happens in hantro_iommu_restore() when the VPU is attached to an
IOMMU_DOMAIN_IDENTITY passthrough domain?

iommu_get_domain_for_dev() is still non-NULL in passthrough mode, so
hantro_iommu_restore() transiently enables empty paging on a device doing
direct DMA and then detaches back to group->default_domain; on RK3588 AV1,
that re-attaches the standalone static struct iommu_domain
vsi_identity_domain via vsi_iommu_identity_attach(), which passes it to
to_vsi_domain() and takes spin_lock_irqsave(&vsi_domain->lock, flags) on
out-of-bounds memory. Checking iommu_is_dma_domain() on the domain
returned by iommu_get_domain_for_dev(vpu->dev) in
hantro_alloc_empty_domain() would avoid allocating empty_domain for
identity and unmanaged ARM32 domains.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-hantro-robust-v1-0-6b6269c05bfa@gmail.com?part=4