Hi Stefano,
On 23/06/2022 23:07, Stefano Stabellini wrote:
On Thu, 23 Jun 2022, dmitry.semen...@gmail.com wrote:
From: Dmytro Semenets <dmytro_semen...@epam.com>
So wouldn't it be better to remove the panic from the implementation of
call_psci_cpu_off?
I have asked to keep the panic() in call_psci_cpu_off(). If you remove
the panic() then we will hide the fact that the CPU was not properly
turned off and will consume more energy than expected.
The WFI loop is fine when shutting down or rebooting because we know
this will only happen for a short period of time.
The reason I am saying this is that stop_cpu is called in a number of
places beyond halt_this_cpu and as far as I can tell any of them could
trigger the panic. (I admit they are unlikely places but still.)
This is one of the example where the CPU will not be stopped for a short
period of time. We should deal with them differently (i.e. migrating the
trusted OS or else) so we give all the chance for the CPU to be fully
powered.
IMHO, this is a different issue and hence why I didn't ask Dmitry to
solve it.
Also the PSCI spec explicitely mention CPU_OFF as a way to place CPUs in
a "known state" and doesn't offer any other examples. So although what
you wrote in the commit message is correct, using CPU_OFF seems to be
the less error prone way (in the sense of triggering firmware errors) to
place CPUs in a known state.
The section you are referring to is starting with "One way". This imply
there are others methods.
FWIW, the spin loop above seems to be how Linux is dealing with the
shutdown/reboot.
So I would simply remove the panic from call_psci_cpu_off, so that we
try CPU_OFF first, and if it doesn't work, we use the WFI loop as
backup. Also we get to fix all the other callers of stop_cpu this way.
This reads strange. In the previous paragraph you suggested the CPU off
is a less error prone way to place CPUs in a known state. But here, you
are softening the stance and suggesting to fallback to the WFI loop.
So to me this indicates that WFI loop is fine. Otherwise, wouldn't the
user risk to see firmware errors (which BTW, I don't understand what
sort of firmware errors you are worried)? If yes, why would it be
acceptable?
For instance, Dmitry situation, the CPU0 would always WFI loop...
Cheers,
--
Julien Grall
