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

Hello,

On 2026/08/03 20:47, Rafael J. Wysocki (Intel) wrote:
> On Mon, Aug 3, 2026 at 12:05 PM KONDO KAZUMA(近藤　和真)
> <[email protected]> wrote:
>>
>> Hello,
>>
>> I received review comments from Sashiko:
>> https://sashiko.dev/#/patchset/[email protected]?part=1
>>
>> On 2026/07/31 18:21, [email protected] wrote:
>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>> - [High] Early return bypasses ASPM disablement for non-PCIe and CXL host bridges without _OSC, causing an undocumented functional change and potential instability
>>> --
>>>
>>> --- Patch [1]: [PATCH] ACPI: PCI: Avoid misleading _OSC messages for non-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 without _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 misleading
>>> messages in the logs on some Intel servers where the OS appears to negotiate
>>> with the platform despite the lack of an _OSC method.
>>>
>>> Link: https://lore.kernel.org/linux-acpi/[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 host
>>> bridges?
>>>
>>> The commit message states this change produces no functional issues and only
>>> removes misleading messages. However, before this patch, if the _OSC method
>>> was missing, the function would proceed, fail the negotiation, and fall
>>> through to the failure path where *no_aspm = 1 is unconditionally set.
>>>
>>> By returning early here, it looks like *no_aspm = 1 is completely bypassed
>>> for legacy host bridges.
>>>
>>> Additionally, is_pcie() only checks for ACPI_BRIDGE_TYPE_PCIE. Could this
>>> 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 and skip
>>> ASPM disablement entirely, which could leave ASPM enabled inappropriately
>>> 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?

You're right. I was trying to understand the rationale for disabling
ASPM when _OSC negotiation is skipped, but it appears that I have been
looking at this the wrong way.

> 
>> 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 similar.
> 
>>
>>                 /*
>>                  * Disable ASPM unless the OS has been granted control over it
>>                  * via _OSC.
>>                  */
>>                 *no_aspm = 1;
> 
> So I agree that setting *no_aspm to 1 in this case is a good idea, but
> the comment above is slightly misleading.

Thank you for your advice! I'll fix the debug message and the comment.

Thanks,
Kazuma

> 
> 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.