Re: [PATCH v2 1/3] ACPI: Allow PCI to be disabled for reboot

[Rafael J. Wysocki]

On Tue, Dec 11, 2018 at 5:54 PM Sinan Kaya <okaya@xxxxxxxxxx> wrote:
>
> On 12/11/2018 5:12 AM, Rafael J. Wysocki wrote:
> > On Sat, Dec 8, 2018 at 10:47 PM Sinan Kaya <okaya@xxxxxxxxxx> wrote:
> >>
> >> Make PCI reboot conditional on PCI support being present on the kernel
> >> configuration.
> >>
> >> Signed-off-by: Sinan Kaya <okaya@xxxxxxxxxx>
> >
> > Same comment as for patch [2/3]: make the subject say clearly that
> > this is about CONFIG_PCI.
>
> Sure
>
> >>          case ACPI_ADR_SPACE_PCI_CONFIG:
> >> +       {
> >> +#ifdef CONFIG_PCI
> >> +               unsigned int devfn;
> >> +               struct pci_bus *bus0;
> >> +
> >>                  /* The reset register can only live on bus 0. */
> >>                  bus0 = pci_find_bus(0, 0);
> >>                  if (!bus0)
> >> @@ -45,7 +48,10 @@ void acpi_reboot(void)
> >>                  pci_bus_write_config_byte(bus0, devfn,
> >>                                  (rr->address & 0xffff), reset_value);
> >>                  break;
> >> -
> >> +#else
> >> +               return;
> >
> > Why not "break"?
> >
>
> I struggled between break and return. Existing code seems to return on failure
> when bus0 is NULL. I assumed it would be more logical to return as someone could
> put some code after here that assumes everything is in order.

Well, there's no such code ATM, so there's no practical difference
between the two and you don't really need the #else branch at all, do
you?