Re: [PATCH] ACPI: PCI: Avoid misleading _OSC messages for non-PCIe host bridges without _OSC

On Mon, Aug 3, 2026 at 12:05=E2=80=AFPM KONDO KAZUMA(=E8=BF=91=E8=97=A4=E3=
=80=80=E5=92=8C=E7=9C=9F)
<[email protected]> wrote:
>
> Hello,
>
> I received review comments from Sashiko:
> https://sashiko.dev/#/patchset/[email protected]=
om?part=3D1
>
> On 2026/07/31 18:21, [email protected] wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential is=
sue(s) to consider:
> > - [High] Early return bypasses ASPM disablement for non-PCIe and CXL ho=
st bridges without _OSC, causing an undocumented functional change and pote=
ntial instability
> > --
> >
> > --- Patch [1]: [PATCH] ACPI: PCI: Avoid misleading _OSC messages for no=
n-PCIe host bridges without _OSC ---
> > commit e68d29f653a1c34f8b7b08baaa548e2ff2d3663f
> > Author: KONDO KAZUMA( $B6aF#!!OB?? (B) <[email protected]>
> >
> > ACPI: PCI: Avoid misleading _OSC messages for non-PCIe host bridges wit=
hout _OSC
> >
> > This patch skips _OSC negotiation for non-PCIe host bridges that do not=
 define
> > an _OSC method before attempting to evaluate it. It aims to prevent mis=
leading
> > messages in the logs on some Intel servers where the OS appears to nego=
tiate
> > with the platform despite the lack of an _OSC method.
> >
> > Link: https://lore.kernel.org/linux-acpi/d0be949d-0e21-472e-a44b-cedb1d=
[email protected]/
> >
> >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> >> index 4c06c3ffd0cbb..3a24d018436b4 100644
> >> --- a/drivers/acpi/pci_root.c
> >> +++ b/drivers/acpi/pci_root.c
> >> @@ -574,6 +574,12 @@ static void negotiate_os_control(struct acpi_pci_=
root *root, int *no_aspm)
> >>              return;
> >>      }
> >>
> >> +    /* _OSC is optional for PCI host bridges */
> >> +    if (!is_pcie(root) && !acpi_has_method(handle, "_OSC")) {
> >> +            dev_dbg(&device->dev, "_OSC: negotiation skipped (method =
not present for non-PCIe host bridge)\n");
> >> +            return;
> >> +    }
> >> +
> >
> > [Severity: High]
> > Does this early return unintentionally skip disabling ASPM for these ho=
st
> > bridges?
> >
> > The commit message states this change produces no functional issues and=
 only
> > removes misleading messages. However, before this patch, if the _OSC me=
thod
> > was missing, the function would proceed, fail the negotiation, and fall
> > through to the failure path where *no_aspm =3D 1 is unconditionally set=
.
> >
> > By returning early here, it looks like *no_aspm =3D 1 is completely byp=
assed
> > for legacy host bridges.
> >
> > Additionally, is_pcie() only checks for ACPI_BRIDGE_TYPE_PCIE. Could th=
is
> > cause CXL host bridges (ACPI_BRIDGE_TYPE_CXL) to evaluate to false here=
?
> > If a CXL bridge lacks an _OSC method, it would take this early return a=
nd skip
> > ASPM disablement entirely, which could leave ASPM enabled inappropriate=
ly
> > without firmware negotiation.
>
> I think Sashiko's review is reasonable. According to [1], unless the
> platform explicitly grants the OS control over ASPM via _OSC, the OS
> should not change the ASPM state configured by the platform.

Though ASPM only is a thing for PCIe, isn't it?

> Also, as described in [2], if CXL 1.1 hosts do not publish a CXL _OSC,
> the OS should fall back to the PCIe _OSC (just as acpi_pci_query_osc()
> does).
>
> So, negotiate_os_control() should skip _OSC negotiation only when a
> legacy PCI host bridge does not have _OSC. For example:
>
>         /* _OSC is optional for PCI host bridges */

The comment above is somewhat redundant IMO.

>         if (!is_pcie(root) && !is_cxl(root) &&
>             !acpi_has_method(handle, "_OSC")) {
>                 dev_dbg(&device->dev, "_OSC: negotiation skipped (method =
not present for legacy PCI host bridge)\n");

Please make the comment say "Skipping a non-PCIe root without _OSC" or simi=
lar.

>
>                 /*
>                  * Disable ASPM unless the OS has been granted control ov=
er it
>                  * via _OSC.
>                  */
>                 *no_aspm =3D 1;

So I agree that setting *no_aspm to 1 in this case is a good idea, but
the comment above is slightly misleading.

I would just make it say "No ASPM for non-PCI host bridges" or equivalent.

>                 return;
>         }
>
> If I have misunderstood anything, please let me know.