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

[dmukhin@xxxxxxxx]

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

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
>