linux-kernel - Re: [PATCH v10 1/8] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

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: <20210727221229.GA750669@bjorn-Precision-5520>
Date:   Tue, 27 Jul 2021 17:12:29 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Amey Narkhede <ameynarkhede03@...il.com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>, alex.williamson@...hat.com,
        Raphael Norwitz <raphael.norwitz@...anix.com>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        kw@...ux.com, Shanker Donthineni <sdonthineni@...dia.com>,
        Sinan Kaya <okaya@...nel.org>, Len Brown <lenb@...nel.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH v10 1/8] PCI: Add pcie_reset_flr to follow calling
 convention of other reset methods

On Fri, Jul 09, 2021 at 06:08:06PM +0530, Amey Narkhede wrote:
> Add has_pcie_flr bitfield in struct pci_dev to indicate support for PCIe
> FLR to avoid reading PCI_EXP_DEVCAP multiple times.
> 
> Currently there is separate function pcie_has_flr() to probe if PCIe FLR
> is supported by the device which does not match the calling convention
> followed by reset methods which use second function argument to decide
> whether to probe or not. Add new function pcie_reset_flr() that follows
> the calling convention of reset methods.
> 
> Signed-off-by: Amey Narkhede <ameynarkhede03@...il.com>
> ---
>  drivers/crypto/cavium/nitrox/nitrox_main.c |  4 +-
>  drivers/pci/pci.c                          | 59 +++++++++++-----------
>  drivers/pci/pcie/aer.c                     | 12 ++---
>  drivers/pci/probe.c                        |  6 ++-
>  drivers/pci/quirks.c                       |  9 ++--
>  include/linux/pci.h                        |  3 +-
>  6 files changed, 45 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c
> index facc8e6bc..15d6c8452 100644
> --- a/drivers/crypto/cavium/nitrox/nitrox_main.c
> +++ b/drivers/crypto/cavium/nitrox/nitrox_main.c
> @@ -306,9 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev)
>  		return -ENOMEM;
>  	}
>  
> -	/* check flr support */
> -	if (pcie_has_flr(pdev))
> -		pcie_flr(pdev);
> +	pcie_reset_flr(pdev, 0);

I'm not really a fan of exposing the "probe" argument outside
drivers/pci/.  I think this would be the only occurrence.  Is there a
way to avoid that?

Can we just make pcie_flr() do the equivalent of pcie_has_flr()
internally?

>  static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
>  {
> -	if (!pcie_has_flr(dev))
> -		return -ENOTTY;
> +	int ret = pcie_reset_flr(dev, probe);
>  
>  	if (probe)
> -		return 0;
> -
> -	pcie_flr(dev);
> +		return ret;
>  
>  	msleep(250);

Can we structure this like the following?  I think it's easier to
understand.

  if (probe)
    return pcie_reset_flr(dev, 1);

  pcie_reset_flr(dev, 0);
  msleep(250);
  return 0;

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c20211e59..d432428fd 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -337,6 +337,7 @@ struct pci_dev {
>  	u8		msi_cap;	/* MSI capability offset */
>  	u8		msix_cap;	/* MSI-X capability offset */
>  	u8		pcie_mpss:3;	/* PCIe Max Payload Size Supported */
> +	u8		has_pcie_flr:1; /* PCIe FLR supported */

Let's add a devcap member instead.  Then we can use it for some
ASPM-related things as well.  We *could* use it to replace pcie_mpss,
since that comes from PCI_EXP_DEVCAP, too, but for now I think it's
easier to just keep it because it's encoded, and some drivers and
quirks use it so it would be a fair amount of work to change that.
Example patch below that could become the first in the series.


diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aacf575c15cf..5a99061ea53a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4635,8 +4635,7 @@ bool pcie_has_flr(struct pci_dev *dev)
 	if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
 		return false;
 
-	pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap);
-	return cap & PCI_EXP_DEVCAP_FLR;
+	return dev->devcap & PCI_EXP_DEVCAP_FLR;
 }
 EXPORT_SYMBOL_GPL(pcie_has_flr);
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 79177ac37880..52ae26bcc68c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1498,8 +1498,8 @@ void set_pcie_port_type(struct pci_dev *pdev)
 	pdev->pcie_cap = pos;
 	pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
 	pdev->pcie_flags_reg = reg16;
-	pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
-	pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
+	pci_read_config_dword(pdev, pos + PCI_EXP_DEVCAP, &pdev->devcap);
+	pdev->pcie_mpss = pdev->devcap & PCI_EXP_DEVCAP_PAYLOAD;
 
 	parent = pci_upstream_bridge(pdev);
 	if (!parent)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 540b377ca8f6..294d1c857a57 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -334,6 +334,7 @@ struct pci_dev {
 	struct pci_dev  *rcec;          /* Associated RCEC device */
 #endif
 	u8		pcie_cap;	/* PCIe capability offset */
+	u32		devcap;		/* PCIe Device Capabilities */
 	u8		msi_cap;	/* MSI capability offset */
 	u8		msix_cap;	/* MSI-X capability offset */
 	u8		pcie_mpss:3;	/* PCIe Max Payload Size Supported */