Re: [PATCH v2 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 2 potential issue(s) to consider:
- [High] drm/panthor: resource leak due to inverted error labels in delegate_domains
- [High] drm/panthor: incorrect error cleanup and teardown order violates LIFO
--

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

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

This commit delegates the neural power domain to the CSF before delegating
other domains to prevent a hard lock-up or firmware OOPS on v14+.
It also updates power state transitions and debugging output for the
neural domain.

> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.c b/drivers/gpu/drm/panthor/panthor_pwr.c
> index dd7b6ef8ea209..7a841cfa86fd5 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;
> +		goto err_retract_neural;
>  
>  	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: High]
Does this skip retracting the NEURAL domain on a TILER delegation failure?

If delegate_domain(ptdev, PWR_COMMAND_DOMAIN_TILER) fails, the code jumps to
err_retract_shader. Because the err_retract_shader label is placed below
err_retract_neural, it skips retracting the NEURAL domain.

Since the commit message notes that an inconsistent delegation state causes a
hard lock-up or firmware OOPS, shouldn't the labels be placed in LIFO order
so err_retract_shader can fall through to err_retract_neural?

>  
> @@ -530,6 +552,10 @@ 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;
>  

[Severity: High]
Does this violate the LIFO teardown order for the power domains?

In panthor_pwr_delegate_domains(), the domains are delegated in the order:
NEURAL, SHADER, TILER.

Here in panthor_pwr_l2_power_off(), the NEURAL domain is forced off before
TILER and SHADER. Should the teardown order be TILER, SHADER, then NEURAL
to safely unwind the delegation?

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