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

On 30.07.2026 02:18, [email protected] wrote:
> From: Denis Mukhin <[email protected]> 
> 
> 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