Re: [PATCH] acpi: reboot: log reset parameters

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.07.2026 02:18, dmukhin@xxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx> 
> 
> Xen does not provide much details for system reset debugging in case
> system reset happens via ACPI subsystem.
> 
> Log reset I/O address and reset value.
> 
> While here, fix the missing default case, guard it with
> ASSERT_UNREACHABLE() and drop full stops in the loglines.

On what basis (i.e. thanks to which earlier checks) would this assertion be
legitimate to add? Besides being a wrong use of an assertion, it also breaks
fallback to alternative reboot methods in case one doesn't work.

> --- a/xen/drivers/acpi/reboot.c
> +++ b/xen/drivers/acpi/reboot.c
> @@ -6,6 +6,7 @@ void acpi_reboot(void)
>  {
>       struct acpi_generic_address *rr;
>       u8 reset_value;
> +     pci_sbdf_t sbdf;
>  
>       rr = &acpi_gbl_FADT.reset_register;
>  
> @@ -21,17 +22,24 @@ void acpi_reboot(void)
>        * on a device on bus 0. */
>       switch (rr->space_id) {
>       case ACPI_ADR_SPACE_PCI_CONFIG:
> -             printk("Resetting with ACPI PCI RESET_REG.\n");
> +             sbdf = PCI_SBDF(0, 0, rr->address >> 32, rr->address >> 16);
> +             printk("Resetting with ACPI PCI %pp RESET_REG at 0x%"PRIx64" 
> (0x%x)\n",
> +                     &sbdf, rr->address & 0xffu, reset_value);

As indicated on other occasions - %#x and alike please in favor of 0x%x.

I also see no reason for the 'u' suffix on the literal number. Plus if one
was wanted, it would want to be 'U', to match the Misra-demanded 'L'.

Also - nit: Indentation.

>               /* Write the value that resets us. */
> -             pci_conf_write8(PCI_SBDF(0, 0, rr->address >> 32,
> -                                      rr->address >> 16),
> -                             (rr->address & 255),
> -                             reset_value);
> +             pci_conf_write8(sbdf, rr->address & 0xffu, reset_value);
>               break;
>       case ACPI_ADR_SPACE_SYSTEM_MEMORY:
> -     case ACPI_ADR_SPACE_SYSTEM_IO:
> -             printk("Resetting with ACPI MEMORY or I/O RESET_REG.\n");
> +             printk("Resetting with ACPI MEMORY at 0x%"PRIx64" (0x%x)\n",
> +                     rr->address, reset_value);
>               acpi_hw_low_level_write(8, reset_value, rr);
>               break;
> +     case ACPI_ADR_SPACE_SYSTEM_IO:
> +             printk("Resetting with I/O RESET_REG at 0x%"PRIx64" (0x%x)\n",
> +                     rr->address, reset_value);
> +             acpi_hw_low_level_write(8, reset_value, rr);
> +             break;
> +     default:
> +             ASSERT_UNREACHABLE();
> +             break;
>       }
>  }

As you're already touching the entire switch(), would you mind also inserting
the missing blank lines between case blocks?

Jan