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

[KONDO KAZUMA(近藤 和真)]

Hello,

I received review comments from Sashiko:
https://sashiko.dev/#/patchset/20260731090820.493064-1-kazuma-kondo@xxxxxxx?part=1

On 2026/07/31 18:21, sashiko-bot@xxxxxxxxxx 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) <kazuma-kondo@xxxxxxx>
> 
> 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/d0be949d-0e21-472e-a44b-cedb1dd8695d@xxxxxxx/
> 
>> 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.

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 */
	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");

		/*
		 * Disable ASPM unless the OS has been granted control over it
		 * via _OSC.
		 */
		*no_aspm = 1;
		return;
	}

If I have misunderstood anything, please let me know.

[1] https://lore.kernel.org/all/1320961113-5050-1-git-send-email-mjg@xxxxxxxxxx/
[2] https://lore.kernel.org/all/20220413073618.291335-3-vishal.l.verma@xxxxxxxxx/

Thanks,
Kazuma

> 
>>  	support = calculate_support();
>>  
>>  	decode_osc_support(root, "OS supports", support);
> 
> [ ... ]
> 
>> @@ -615,10 +621,6 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
>>  		 */
>>  		*no_aspm = 1;
>>  
>> -		/* _OSC is optional for PCI host bridges */
>> -		if (status == AE_NOT_FOUND && !is_pcie(root))
>> -			return;
>> -
>>  		if (control) {
>>  			decode_osc_control(root, "OS requested", requested);
>>  			decode_osc_control(root, "platform willing to grant", control);
>