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