linux-kernel - Re: [PATCH v3 6/6] media: iris: Introduce vpu ops for vpu4 with necessary hooks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5a5908db-aa96-4cfa-a908-d393dd2d79c0@kernel.org>
Date: Sat, 6 Dec 2025 23:04:28 +0000
From: Bryan O'Donoghue <bod@...nel.org>
To: Vikash Garodia <vikash.garodia@....qualcomm.com>,
 Dikshita Agarwal <dikshita.agarwal@....qualcomm.com>,
 Abhinav Kumar <abhinav.kumar@...ux.dev>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>,
 Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
 Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Cc: linux-arm-msm@...r.kernel.org, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 Vishnu Reddy <busanna.reddy@....qualcomm.com>
Subject: Re: [PATCH v3 6/6] media: iris: Introduce vpu ops for vpu4 with
 necessary hooks

On 07/11/2025 09:49, Vikash Garodia wrote:
> Add power sequence for vpu4 by reusing from previous generation wherever
> possible. Hook up vpu4 op with vpu4 specific implemtation or resue from
> earlier generation wherever feasible, like clock calculation in this
> case.
> 
> Co-developed-by: Vishnu Reddy <busanna.reddy@....qualcomm.com>
> Signed-off-by: Vishnu Reddy <busanna.reddy@....qualcomm.com>
> Signed-off-by: Vikash Garodia <vikash.garodia@....qualcomm.com>
> ---

> +static void iris_vpu4x_power_off_hardware(struct iris_core *core)
> +{
> +	u32 efuse_value = readl(core->reg_base + WRAPPER_EFUSE_MONITOR);
> +	bool handshake_done, handshake_busy;
> +	u32 value, count = 0;
> +
> +	iris_vpu4x_genpd_set_hwmode(core, false, efuse_value);
> +
> +	if (!(efuse_value & DISABLE_VIDEO_APV_BIT))
> +		iris_vpu4x_power_off_apv(core);
> +
> +	value = readl(core->reg_base + WRAPPER_CORE_POWER_STATUS);
> +
> +	if (!(value & CORE_PWR_ON))
> +		goto disable_clocks_and_power;
> +
> +	value = readl(core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
> +
> +	if (value & CORE_CLK_HALT)
> +		writel(0x0, core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
> +
> +	readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN, value,
> +			   value & VPU_IDLE_BITS, 2000, 20000);
> +
> +	do {
> +		writel(REQ_POWER_DOWN_PREP, core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
> +		usleep_range(10, 20);
> +		value = readl(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS);
> +
> +		handshake_done = value & NOC_LPI_STATUS_DONE;
> +		handshake_busy = value & (NOC_LPI_STATUS_DENY | NOC_LPI_STATUS_ACTIVE);
> +
> +		if (handshake_done || !handshake_busy)
> +			break;
> +
> +		writel(0x0, core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
> +		usleep_range(10, 20);
> +
> +	} while (++count < 1000);
> +
> +	if (!handshake_done && handshake_busy)
> +		dev_err(core->dev, "LPI handshake timeout\n");
> +
> +	writel(MVP_NOC_RESET_REQ_MASK, core->reg_base + AON_WRAPPER_MVP_NOC_RESET_REQ);
> +	readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_RESET_ACK,
> +			   value, value == MVP_NOC_RESET_REQ_MASK, 200, 2000);

I realise it replicates what we already have upstream but...

if (!handshake_done && etc) is true then how does it make sense to 
continue on with the routine at all ?

We would expect the poll_timeout to timeout .. ?

If the documentation states loop 1000 times trying this then does it 
also say continue to try to power things off if that 1000 retries fails ?

I realise its a nit-pick but the logic seems a bit fluffy here. Could 
you tidy it up ?

Also now that I look at it - vpu2 and vpu3 both trap the timeout and 
jump to some kind of cleanup routine.

goto disable_power;

Why is that logic not being followed here ?

> +	writel(0x0, core->reg_base + AON_WRAPPER_MVP_NOC_RESET_SYNCRST);
> +	writel(0x0, core->reg_base + AON_WRAPPER_MVP_NOC_RESET_REQ);
> +	readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_RESET_ACK,
> +			   value, value == 0x0, 200, 2000);

Feels like there is little point in having a poll timeout if we throw 
away a timeout result code..

Not sure why you're changing up the logic from previous versions ?

---
bod