linux-kernel - Re: [PATCH v18 10/11] PCI/DPC: Add Error Disconnect Recover (EDR) support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240411180757.GA2190937@bhelgaas>
Date: Thu, 11 Apr 2024 13:07:57 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: sathyanarayanan.kuppuswamy@...ux.intel.com
Cc: bhelgaas@...gle.com, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org, ashok.raj@...el.com,
	Len Brown <lenb@...nel.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH v18 10/11] PCI/DPC: Add Error Disconnect Recover (EDR)
 support

On Mon, Mar 23, 2020 at 05:26:07PM -0700, sathyanarayanan.kuppuswamy@...ux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
> 
> Error Disconnect Recover (EDR) is a feature that allows ACPI firmware to
> notify OSPM that a device has been disconnected due to an error condition
> (ACPI v6.3, sec 5.6.6).  OSPM advertises its support for EDR on PCI devices
> via _OSC (see [1], sec 4.5.1, table 4-4).  The OSPM EDR notify handler
> should invalidate software state associated with disconnected devices and
> may attempt to recover them.  OSPM communicates the status of recovery to
> the firmware via _OST (sec 6.3.5.2).
> 
> For PCIe, firmware may use Downstream Port Containment (DPC) to support
> EDR.  Per [1], sec 4.5.1, table 4-6, even if firmware has retained control
> of DPC, OSPM may read/write DPC control and status registers during the EDR
> notification processing window, i.e., from the time it receives an EDR
> notification until it clears the DPC Trigger Status.
> 
> Note that per [1], sec 4.5.1 and 4.5.2.4,
> 
>   1. If the OS supports EDR, it should advertise that to firmware by
>      setting OSC_PCI_EDR_SUPPORT in _OSC Support.
> 
>   2. If the OS sets OSC_PCI_EXPRESS_DPC_CONTROL in _OSC Control to request
>      control of the DPC capability, it must also set OSC_PCI_EDR_SUPPORT in
>      _OSC Support.
> 
> Add an EDR notify handler to attempt recovery.
> 
> [1] Downstream Port Containment Related Enhancements ECN, Jan 28, 2019,
>     affecting PCI Firmware Specification, Rev. 3.2
>     https://members.pcisig.com/wg/PCI-SIG/document/12888

> +static int acpi_enable_dpc(struct pci_dev *pdev)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> +	union acpi_object *obj, argv4, req;
> +	int status;
> +
> +	/*
> +	 * Some firmware implementations will return default values for
> +	 * unsupported _DSM calls. So checking acpi_evaluate_dsm() return
> +	 * value for NULL condition is not a complete method for finding
> +	 * whether given _DSM function is supported or not. So use
> +	 * explicit func 0 call to find whether given _DSM function is
> +	 * supported or not.
> +	 */
> +        status = acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
> +				1ULL << EDR_PORT_DPC_ENABLE_DSM);
> +        if (!status)
> +                return 0;
> +
> +	status = 0;
> +	req.type = ACPI_TYPE_INTEGER;
> +	req.integer.value = 1;
> +
> +	argv4.type = ACPI_TYPE_PACKAGE;
> +	argv4.package.count = 1;
> +	argv4.package.elements = &req;
> +
> +	/*
> +	 * Per Downstream Port Containment Related Enhancements ECN to PCI
> +	 * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is
> +	 * optional.  Return success if it's not implemented.
> +	 */
> +	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
> +				EDR_PORT_DPC_ENABLE_DSM, &argv4);

This has been upstream for a while, just a follow-up question: this
_DSM function was defined by the ECN with Rev 5.  The ECN was
incorporated into the PCI Firmware spec r3.3 with slightly different
behavior as Rev 6.

The main differences are:

  ECN
    - Rev 5
    - Arg3 is an Integer
    - Return is 0 (DPC disabled) or 1 (DPC enabled)

  r3.3 spec
    - Rev 6
    - Arg3 is a Package of one Integer
    - Return is 0 (DPC disabled, Hot-Plug Surprise may be set), 1 (DPC
      enabled, Hot-Plug Surprise may be cleared), or 2 (failure)

So the question is whether this actually implements Rev 5 or Rev 6?
It looks like this builds a *package* for Arg3 (which would correspond
to Rev 6), but we're evaluating Rev 5, which specified an Integer.

The meaning of the Arg3 values is basically the same, so I don't see
an issue there, but it looks like if a platform implemented Rev 5
according to the ECN to take a bare Integer, this might not work
correctly.

> +	if (!obj)
> +		return 0;
> +
> +	if (obj->type != ACPI_TYPE_INTEGER) {
> +		pci_err(pdev, FW_BUG "Enable DPC _DSM returned non integer\n");
> +		status = -EIO;
> +	}
> +
> +	if (obj->integer.value != 1) {
> +		pci_err(pdev, "Enable DPC _DSM failed to enable DPC\n");
> +		status = -EIO;
> +	}
> +
> +	ACPI_FREE(obj);
> +
> +	return status;
> +}