[PATCH v11 5/6] arm64: dts: amlogic: Add video decoder driver support for S4 SOCs

[sashiko-bot at kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: amlogic: vdec: Missing locking in the timeout/error path of aml_h264_dec_run()
- [High] media: amlogic: vdec: Infinite loop in threaded IRQ handler due to 0 timeout in read_poll_timeout()
--

commit caf90beec035f24ac592fd4c243200813bb1fcc4
Author: Zhentao Guo <zhentao.guo at amlogic.com>

arm64: dts: amlogic: Add video decoder driver support for S4 SOCs

This commit adds the vdec node to enable the Amlogic V4L2 stateless video 
decoder for S4 SOCs. It also enables the optee node to support loading the
video firmware via TEE helpers.

> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
> index 2a6fbd5308362..7a1dcb3951c82 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi

[ ... ]

> @@ -907,5 +912,33 @@ emmc: mmc at fe08c000 {

[ ... ]

> +		video-codec at fe320000 {
> +			compatible = "amlogic,s4-vdec";
> +			reg = <0x0 0xfe320000 0x0 0x10000>,
> +			      <0x0 0xfe036000 0x0 0x20>;
> +			reg-names = "dos",
> +				    "dmc";
> +			interrupts = <GIC_SPI 91 IRQ_TYPE_EDGE_RISING>,
> +				     <GIC_SPI 92 IRQ_TYPE_EDGE_RISING>;
> +			clocks = <&clkc_periphs CLKID_DOS>,
> +				 <&clkc_periphs CLKID_VDEC_SEL>,
> +				 <&clkc_periphs CLKID_HEVCF_SEL>;
> +			clock-names = "dos",
> +				      "vdec",
> +				      "hvdec";
> +			power-domains = <&pwrc PWRC_S4_DOS_VDEC_ID>,
> +					<&pwrc PWRC_S4_DOS_HEVC_ID>;
> +			power-domain-names = "vdec",
> +					     "hvdec";
> +			resets = <&reset RESET_DOS>;
> +			amlogic,canvas = <&canvas>;
> +			amlogic,secure-monitor = <&sm>;
> +		};

[Severity: High]
Enabling this device exposes a locking regression in the driver. In the
timeout/error path of aml_h264_dec_run(), aml_stop_vdec_hw() is called without
holding dec_hw->ctx_mutex:

drivers/media/platform/amlogic/vdec/h264.c:aml_h264_dec_run() {
...
		/*
		 * Halt the hardware and wait out any in-flight threaded IRQ
		 * handler before cleaning up, otherwise both contexts may run
		 * h264_release_decode_spec() concurrently.
		 */
		aml_stop_vdec_hw(dec_hw);
		synchronize_irq(dec_hw->dec_irq);
		h264_release_decode_spec(dec_hw, ctx);
	} else if (-ERESTARTSYS == ret) {
		ret = -1;
		aml_stop_vdec_hw(dec_hw);
		synchronize_irq(dec_hw->dec_irq);
		h264_release_decode_spec(dec_hw, ctx);
...
}

If wait_event_interruptible_timeout() times out or is interrupted, the
threaded IRQ handler vdec_threaded_isr_handler() may execute concurrently on
another CPU while holding ctx_mutex.

Because aml_stop_vdec_hw() is unprotected, can it reset the hardware
concurrently while the ISR is still interacting with it? 

If the ISR writes to DPB_STATUS_REG after the reset, it could inadvertently
restart the hardware decoding process. This means a restarted hardware block
could fire a new interrupt after synchronize_irq() returns, allowing a new
ISR to run concurrently with h264_release_decode_spec().

Will this lead to concurrent double-freeing of canvas indices?

[Severity: High]
Enabling this device also exposes an infinite loop risk in the threaded IRQ
handler. In h264_config_decode_buf(), read_poll_timeout() is called with a
timeout of 0:

drivers/media/platform/amlogic/vdec/h264.c:h264_config_decode_buf() {
...
	read_poll_timeout(read_dos_reg, reg_val,
			  !(reg_val & 0x800),
			  10, 0, true, hw, H264_CO_MB_RW_CTL);
...
}

The read_poll_timeout() macro treats a timeout of 0 as an infinite wait.

Since this executes inside the threaded interrupt handler on every H264 frame
decode, if the hardware hangs or the register condition is never met,
would the thread spin indefinitely and hang the system or consume 100% CPU on
the IRQ thread?

>  	};
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-b4-s4-vdec-upstream-v11-0-25939f85ada5@amlogic.com?part=5