Re: [PATCH v2 09/11] accel/rocket: add devfreq support

[Igor Paunovic <royalnet026@gmail.com>]

On the five findings from the Sashiko review of this patch, in its
order. All of this is from reading the code; none of it was reproduced.

1. Cores left powered after unbind: not with the driver's own settings.
rocket_core_fini(), which this series does not change, calls
pm_runtime_dont_use_autosuspend() before pm_runtime_disable(); that
runs rpm_idle() synchronously and suspends the core on the spot. It
can happen if root sets power/autosuspend_delay_ms to 0 and unbinds a
core with the clock raised: the put then queues an asynchronous suspend
that pm_runtime_disable() can cancel. v3 will drop those references
synchronously on the teardown path.

2. Ignored return value: true, but here an error does not leave the
clock raised. Scaling down, the OPP core sets the clock before the
supply, so a regulator error comes after clk_set_rate() has already
asked for the 200 MHz boot rate, which TF-A serves from GPLL, and a rate
the firmware refuses never reaches the caller. v3 will say so in a
comment at both call sites.

3. Rounding the boot rate up: not with the in-tree devicetree, where
assigned-clock-rates gives exactly the lowest OPP. Without it, a boot
rate above 200 MHz would map to a PVTPLL OPP and the cores would be
released there; that is the code-read-only case under "Not done". v3
will create the devfreq device only when the boot rate is at or below
the lowest OPP (without assigned-clock-rates this firmware reports
198 MHz, GPLL/6).

4. Restore racing with rocket_devfreq_fini(): real, and this patch
introduces it. device_shutdown() pins only the core it shuts down, and
__device_release_driver() drops its runtime PM reference before
->remove(), so the last core to go idle can reach
rocket_npu_restore_boot_rate() from its autosuspend timer while
rocket_devfreq_fini() clears ->owner and removes the OPP table and
config. The window is a few microseconds and none of my runs set it up.
v3 will take pm_runtime_get_noresume() and pm_runtime_barrier() on
every core before ->owner is cleared, and drop them afterwards.

5. Concurrent unbinds: real. Each sysfs unbind takes only its own
device lock, so two unbinds can both remove the same devfreq device.
The unlocked probe and remove predate this series (Sashiko's finding on
the standalone slot-search patch, which I agreed with [1] and the cover
letter lists as open), but this patch makes the result worse. The OPP
table is not freed twice. v3 will add a driver-wide mutex around
rocket_probe(), rocket_remove() and rocket_shutdown().

I will wait for review of v2 before sending v3.

[1] https://lore.kernel.org/r/20260904135938.8757-1-royalnet026@gmail.com

Igor