Re: [PATCH 2/2] drm/panthor: Delegate neural power domain to CSF when present

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] panthor: resource leak due to incorrect LIFO label ordering in error path
- [Medium] panthor: incorrect hardware teardown order and duplicate TILER power-off
- [Low] panthor: build failure due to returning a label identifier
--

commit e4aa5571cda90f71765f06232acbfee948feed3e
Author: Daniel Stone <daniels@collabora.com>

drm/panthor: Delegate neural power domain to CSF when present

As the neural power domain is a child of the L2 power domain, control
over the L2 domain cannot be delegated to CSF unless the neural domain
has first been delegated. Given that we delegate all power control on
v14+, this eventually causes a hard lock-up and/or firmware OOPS when we
try to power on anything due to the inconsistent delegation state.

Fixes: ee4f9af07933 ("drm/panthor: Implement L2 power on/off via PWR_CONTROL")

> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.c b/drivers/gpu/drm/panthor/panthor_pwr.c
> index dd7b6ef8ea209..23954c863cc26 100644
> --- a/drivers/gpu/drm/panthor/panthor_pwr.c
> +++ b/drivers/gpu/drm/panthor/panthor_pwr.c

[ ... ]

> @@ -405,9 +418,15 @@ static int panthor_pwr_delegate_domains(struct panthor_device *ptdev)
>  	if (!ptdev->pwr)
>  		return 0;
>  
> +	if (panthor_hw_has_neural(ptdev)) {
> +		ret = delegate_domain(ptdev, PWR_COMMAND_DOMAIN_NEURAL);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = delegate_domain(ptdev, PWR_COMMAND_DOMAIN_SHADER);
>  	if (ret)
> -		return ret;
> +		return err_retract_neural;

[Severity: Low]
Will this cause a build failure?

Because err_retract_neural is a label and not a declared variable, returning it
is invalid C syntax and results in a compiler error. Should this be a
goto err_retract_neural; instead?

>  	ret = delegate_domain(ptdev, PWR_COMMAND_DOMAIN_TILER);
>  	if (ret)
> @@ -415,6 +434,8 @@ static int panthor_pwr_delegate_domains(struct panthor_device *ptdev)
>  
>  	return 0;
>  
> +err_retract_neural:
> +	retract_domain(ptdev, PWR_COMMAND_DOMAIN_NEURAL);
>  err_retract_shader:
>  	retract_domain(ptdev, PWR_COMMAND_DOMAIN_SHADER);
>  

[Severity: Medium]
Does this error path leak the NEURAL domain delegation?

The domains are delegated in the order: NEURAL -> SHADER -> TILER. On failure,
they typically must be retracted in reverse order. However, the error labels
are ordered with err_retract_neural: placed before err_retract_shader:.

If TILER delegation fails, the code executes goto err_retract_shader;, retracts
the SHADER domain, and immediately returns, bypassing the err_retract_neural:
block above it.

Should the labels be swapped to correctly implement the teardown?

[ ... ]

> @@ -530,6 +552,13 @@ void panthor_pwr_l2_power_off(struct panthor_device *ptdev)
>  	 * host control to be powered down in the right order before powering
>  	 * down the L2.
>  	 */
> +	if (has_neural &&
> +	    panthor_pwr_domain_force_off(ptdev, PWR_COMMAND_DOMAIN_NEURAL))
> +		return;
> +
> +	if (panthor_pwr_domain_force_off(ptdev, PWR_COMMAND_DOMAIN_TILER))
> +		return;
> +
>  	if (panthor_pwr_domain_force_off(ptdev, PWR_COMMAND_DOMAIN_TILER))
>  		return;
>  

[Severity: Medium]
Does this execute an incorrect hardware teardown order and duplicate the TILER
power-off?

The NEURAL power-off block is placed above the existing TILER and SHADER blocks,
while also duplicating the TILER block. This causes
panthor_pwr_domain_force_off(ptdev, PWR_COMMAND_DOMAIN_TILER) to execute twice.

The execution sequence becomes NEURAL -> TILER -> TILER -> SHADER. This violates
the strict teardown order required by the hardware (delegation is NEURAL ->
SHADER -> TILER, so teardown must be TILER -> SHADER -> NEURAL).

Could this lead to invalid hardware states or timeouts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015131301.711153-1-daniels@collabora.com?part=2