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 <[email protected]> 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/[email protected]?part=2
