Re: git: e4ab361e5394 - main - fix poweroff regression from 9cdf326b4f by delaying shutdown_halt

On 2/6/24 2:13 AM, Andriy Gapon wrote:
> On 06/02/2024 11:41, Andriy Gapon wrote:
>> The branch main has been updated by avg:
>>
>> URL: https://cgit.FreeBSD.org/src/commit/?id=e4ab361e53945a6c3e9d68c5e5ffc11de40a35f2
>>
>> commit e4ab361e53945a6c3e9d68c5e5ffc11de40a35f2
>> Author:     Andriy Gapon <avg@FreeBSD.org>
>> AuthorDate: 2024-02-06 08:55:13 +0000
>> Commit:     Andriy Gapon <avg@FreeBSD.org>
>> CommitDate: 2024-02-06 08:55:13 +0000
>>
>>       fix poweroff regression from 9cdf326b4f by delaying shutdown_halt
>>       
>>       The regression affected ACPI-based systems without EFI poweroff support
>>       (including VMs).
>>       
>>       The key reason for the regression is that I overlooked that poweroff is
>>       requested by RB_POWEROFF | RB_HALT combination of flags.  In my opinion,
>>       that command is a bit bipolar, but since we've been doing that forever,
>>       then so be it.  Because of that flag combination, the order of
>>       shutdown_final handlers that check for either flag does matter.
>>       
>>       Some additional complexity comes from platform-specific shutdown_final
>>       handlers that aim to handle multiple reboot options at once.  E.g.,
>>       acpi_shutdown_final handles both poweroff and reboot / reset.  As
>>       explained in 9cdf326b4f, such a handler must run after shutdown_panic to
>>       give it a chance.  But as the change revealed, the handler must also run
>>       before shutdown_halt, so that the system can actually power off before
>>       entering the halt limbo.
>>       
>>       Previously, shutdown_panic and shutdown_halt had the same priority which
>>       appears to be incompatible with handlers that can do both poweroff and
>>       reset.
> 
> I want to add that having many handlers with priorities expressed like
> SHUTDOWN_PRI_LAST ± N while some of those handlers have implicit
> inter-dependencies (interactions, interference) also does not help to see a
> clear picture.
> 
> Perhaps it would be better to handle all (reasonable) RB flag combinations
> centrally in kern_reboot and then dispatch events like shutdown_reset,
> shutdown_poweroff, etc.  Handlers for those events would have a single and
> simple job of performing that one action (perhaps failing and letting another
> handler try).

I think having separate eventhandlers for shutdown, reset, and poweroff seems
sensible.  It also permits a given driver to use different priorities (maybe it
wants to be first for poweroff but last for reset, etc.)

> Also, I would split reboot howto into command and flag portions, so that only
> one command can be specified at a time.  E.g., I would consider RB_AUTOBOOT
> ("RB_REBOOT"), RB_POWEROFF, RB_HALT to be distinct commands.  Then, flags like
> RB_NOSYNC or RB_DUMP could be optional flags.
> 
> As an aside, some flags documented for reboot(2) do not seem to have much to do
> with reboot.  E.g., RB_DFLTROOT affects how a system boots up, but not how the
> system goes for a reboot.  Not surprisingly, that option is not handled by
> anything kicked off with reboot(2).
> Maybe, it would make more sense if we had fast reboot support and the running
> kernel could instruct the next kernel directly.  But, it's still a bit weird
> that flags like RB_POWEROFF and RB_DFLTROOT belong in the same domain and can be
> set together.

I would suggest deprecating flags that are no-ops.  In modern systems if you
want to control the next boot you do it via other means (nextboot, efibootmgr,
etc.) and reboot(2) is not a good API for that.

It might be hard to fully cleanup some of the hackiness here, but if you can
at least isolate the flag weirdness handling in kern_reboot by having the more
specific eventhandlers then that might fix most of the ugliness.

-- 
John Baldwin