linux-kernel - Re: [PATCH v3 2/5] PCI/DPC: Allow dpc_probe() even if DPC is handled in firmware

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]

Date:   Fri, 31 May 2019 08:15:26 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     sathyanarayanan.kuppuswamy@...ux.intel.com
Cc:     linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        ashok.raj@...el.com, keith.busch@...el.com
Subject: Re: [PATCH v3 2/5] PCI/DPC: Allow dpc_probe() even if DPC is handled
 in firmware

On Tue, May 14, 2019 at 03:18:14PM -0700, sathyanarayanan.kuppuswamy@...ux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
> 
> Error Disconnect Recover (EDR) support allows OS to handle the error
> recovery even when DPC configuration is handled by firmware. So allow
> dpc_probe() to continue even if firmware first mode is enabed. This is
> a prepratory patch for adding EDR support.

I assume this code is based on some language in the spec, and it needs
to be tied back to it.  The only mention of EDR I'm aware of is in
ACPI v6.3, sec 5.6.6 and 6.3.5.2, so it needs at least a citation.

I don't see any specific reference to firmware-first there, so please
also connect the dots about how we conclude that we need DPC even when
firmware-first is enabled.

> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
> ---
>  drivers/pci/pcie/dpc.c | 49 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 36 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 7b77754a82de..0c9ce876e450 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -20,6 +20,8 @@ struct dpc_dev {
>  	u16			cap_pos;
>  	bool			rp_extensions;
>  	u8			rp_log_size;
> +	/* Set True if DPC is handled in firmware */
> +	bool			firmware_dpc;
>  };
>  
>  static const char * const rp_pio_error_string[] = {
> @@ -67,6 +69,9 @@ void pci_save_dpc_state(struct pci_dev *dev)
>  	if (!dpc)
>  		return;
>  
> +	if (dpc->firmware_dpc)
> +		return;
> +
>  	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
>  	if (!save_state)
>  		return;
> @@ -88,6 +93,9 @@ void pci_restore_dpc_state(struct pci_dev *dev)
>  	if (!dpc)
>  		return;
>  
> +	if (dpc->firmware_dpc)
> +		return;
> +
>  	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
>  	if (!save_state)
>  		return;
> @@ -292,9 +300,6 @@ static int dpc_probe(struct pcie_device *dev)
>  	int status;
>  	u16 ctl, cap;
>  
> -	if (pcie_aer_get_firmware_first(pdev))
> -		return -ENOTSUPP;
> -
>  	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
>  	if (!dpc)
>  		return -ENOMEM;
> @@ -303,13 +308,25 @@ static int dpc_probe(struct pcie_device *dev)
>  	dpc->dev = dev;
>  	set_service_data(dev, dpc);
>  
> -	status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
> -					   dpc_handler, IRQF_SHARED,
> -					   "pcie-dpc", dpc);
> -	if (status) {
> -		dev_warn(device, "request IRQ%d failed: %d\n", dev->irq,
> -			 status);
> -		return status;
> +	if (pcie_aer_get_firmware_first(pdev))
> +		dpc->firmware_dpc = 1;
> +
> +	/*
> +	 * If DPC is handled in firmware and ACPI support is not enabled in OS,
> +	 * then its not useful to proceed with probe. So, return error.
> +	 */
> +	if (dpc->firmware_dpc && !IS_ENABLED(CONFIG_ACPI))
> +		return -ENODEV;
> +
> +	if (!dpc->firmware_dpc) {
> +		status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
> +						   dpc_handler, IRQF_SHARED,
> +						   "pcie-dpc", dpc);
> +		if (status) {
> +			dev_warn(device, "request IRQ%d failed: %d\n", dev->irq,
> +				 status);
> +			return status;
> +		}
>  	}
>  
>  	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
> @@ -324,9 +341,12 @@ static int dpc_probe(struct pcie_device *dev)
>  			dpc->rp_log_size = 0;
>  		}
>  	}
> -
> -	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
> -	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);
> +	if (!dpc->firmware_dpc) {
> +		ctl = (ctl & 0xfff4) |
> +			(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN);
> +		pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL,
> +				      ctl);
> +	}
>  
>  	dev_info(device, "DPC error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
>  		cap & PCI_EXP_DPC_IRQ, FLAG(cap, PCI_EXP_DPC_CAP_RP_EXT),
> @@ -344,6 +364,9 @@ static void dpc_remove(struct pcie_device *dev)
>  	struct pci_dev *pdev = dev->port;
>  	u16 ctl;
>  
> +	if (dpc->firmware_dpc)
> +		return;
> +
>  	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
>  	ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN);
>  	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);
> -- 
> 2.20.1
>