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

On Thu, Jul 30, 2026 at 08:26:38AM +0200, Jan Beulich wrote:
> 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.

Thanks for taking a look!

This file uses tabs - I can convert to spaces, but in separate patch.
What do you think?

> 
> >  		/* 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?

Yes, will do.

> 
> Jan
>