Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1623104 > unrolled thread

export pcie_flr and remove copies of it in drivers

Back to article view | Back to linux.kernel

Contents

  export pcie_flr and remove copies of it in drivers Christoph Hellwig <hch@lst.de> - 2017-04-13 17:00 +0200
    [PATCH 7/7] liquidio: use pcie_flr instead of duplicating it Christoph Hellwig <hch@lst.de> - 2017-04-13 17:00 +0200
    [PATCH 1/7] PCI: export pcie_flr Christoph Hellwig <hch@lst.de> - 2017-04-13 17:00 +0200
      Re: [PATCH 1/7] PCI: export pcie_flr Bjorn Helgaas <helgaas@kernel.org> - 2017-04-14 16:40 +0200
        Re: [PATCH 1/7] PCI: export pcie_flr Christoph Hellwig <hch@lst.de> - 2017-04-14 18:00 +0200
    Re: export pcie_flr and remove copies of it in drivers Bjorn Helgaas <helgaas@kernel.org> - 2017-04-14 16:50 +0200
      Re: export pcie_flr and remove copies of it in drivers Bjorn Helgaas <bhelgaas@google.com> - 2017-04-14 17:00 +0200
        Re: export pcie_flr and remove copies of it in drivers Christoph Hellwig <hch@lst.de> - 2017-04-14 18:00 +0200
      Re: export pcie_flr and remove copies of it in drivers Christoph Hellwig <hch@lst.de> - 2017-04-14 18:00 +0200

#1623104 — export pcie_flr and remove copies of it in drivers

Hi all,

this exports the PCI layer pcie_flr helper, and removes various opencoded
copies of it.

[toc] | [next] | [standalone]

#1623105 — [PATCH 7/7] liquidio: use pcie_flr instead of duplicating it

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/net/ethernet/cavium/liquidio/lio_vf_main.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
index 7b83be4ce1fe..321fe1d5b7b9 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
@@ -899,20 +899,7 @@ static void octeon_pci_flr(struct octeon_device *oct)
 	pci_write_config_word(oct->pci_dev, PCI_COMMAND,
 			      PCI_COMMAND_INTX_DISABLE);
 
-	/* Wait for Transaction Pending bit clean */
-	msleep(100);
-	pcie_capability_read_word(oct->pci_dev, PCI_EXP_DEVSTA, &status);
-	if (status & PCI_EXP_DEVSTA_TRPND) {
-		dev_info(&oct->pci_dev->dev, "Function reset incomplete after 100ms, sleeping for 5 seconds\n");
-		ssleep(5);
-		pcie_capability_read_word(oct->pci_dev, PCI_EXP_DEVSTA,
-					  &status);
-		if (status & PCI_EXP_DEVSTA_TRPND)
-			dev_info(&oct->pci_dev->dev, "Function reset still incomplete after 5s, reset anyway\n");
-	}
-	pcie_capability_set_word(oct->pci_dev, PCI_EXP_DEVCTL,
-				 PCI_EXP_DEVCTL_BCR_FLR);
-	mdelay(100);
+	pcie_flr(oct->pci_dev);
 
 	pci_cfg_access_unlock(oct->pci_dev);
 
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1623108 — [PATCH 1/7] PCI: export pcie_flr

Currently we opencode the FLR sequence in lots of place, export a core
helper instead.  We split out the probing for FLR support as all the
non-core callers already know their hardware.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/pci.c   | 34 +++++++++++++++++++++++++---------
 include/linux/pci.h |  1 +
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7904d02ffdb9..3256a63c5d08 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3773,24 +3773,38 @@ static void pci_flr_wait(struct pci_dev *dev)
 			 (i - 1) * 100);
 }
 
-static int pcie_flr(struct pci_dev *dev, int probe)
+/**
+ * pcie_has_flr - check if a device supports function level resets
+ * @dev:	device to check
+ *
+ * Returns true if the device advertises support for PCIe function level
+ * resets.
+ */
+static bool pcie_has_flr(struct pci_dev *dev)
 {
 	u32 cap;
 
 	pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap);
-	if (!(cap & PCI_EXP_DEVCAP_FLR))
-		return -ENOTTY;
-
-	if (probe)
-		return 0;
+	return cap & PCI_EXP_DEVCAP_FLR;
+}
 
+/**
+ * pcie_flr - initiate a PCIe function level reset
+ * @dev:	device to reset
+ *
+ * Initiate a function level reset on @dev.  The caller should ensure the
+ * device supports FLR before calling this function, e.g. by using the
+ * pcie_has_flr helper.
+ */
+void pcie_flr(struct pci_dev *dev)
+{
 	if (!pci_wait_for_pending_transaction(dev))
 		dev_err(&dev->dev, "timed out waiting for pending transaction; performing function level reset anyway\n");
 
 	pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
 	pci_flr_wait(dev);
-	return 0;
 }
+EXPORT_SYMBOL_GPL(pcie_flr);
 
 static int pci_af_flr(struct pci_dev *dev, int probe)
 {
@@ -3971,9 +3985,11 @@ static int __pci_dev_reset(struct pci_dev *dev, int probe)
 	if (rc != -ENOTTY)
 		goto done;
 
-	rc = pcie_flr(dev, probe);
-	if (rc != -ENOTTY)
+	if (pcie_has_flr(dev)) {
+		pcie_flr(dev);
+		rc = 0;
 		goto done;
+	}
 
 	rc = pci_af_flr(dev, probe);
 	if (rc != -ENOTTY)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index eb3da1a04e6c..f35e51eddad0 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1052,6 +1052,7 @@ int pcie_get_mps(struct pci_dev *dev);
 int pcie_set_mps(struct pci_dev *dev, int mps);
 int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
 			  enum pcie_link_width *width);
+void pcie_flr(struct pci_dev *dev);
 int __pci_reset_function(struct pci_dev *dev);
 int __pci_reset_function_locked(struct pci_dev *dev);
 int pci_reset_function(struct pci_dev *dev);
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1623702 — Re: [PATCH 1/7] PCI: export pcie_flr

[+cc Alex]

On Thu, Apr 13, 2017 at 04:53:33PM +0200, Christoph Hellwig wrote:
> Currently we opencode the FLR sequence in lots of place, export a core
> helper instead.  We split out the probing for FLR support as all the
> non-core callers already know their hardware.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/pci/pci.c   | 34 +++++++++++++++++++++++++---------
>  include/linux/pci.h |  1 +
>  2 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7904d02ffdb9..3256a63c5d08 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3773,24 +3773,38 @@ static void pci_flr_wait(struct pci_dev *dev)
>  			 (i - 1) * 100);
>  }
>  
> -static int pcie_flr(struct pci_dev *dev, int probe)
> +/**
> + * pcie_has_flr - check if a device supports function level resets
> + * @dev:	device to check
> + *
> + * Returns true if the device advertises support for PCIe function level
> + * resets.
> + */
> +static bool pcie_has_flr(struct pci_dev *dev)
>  {
>  	u32 cap;
>  
>  	pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap);
> -	if (!(cap & PCI_EXP_DEVCAP_FLR))
> -		return -ENOTTY;
> -
> -	if (probe)
> -		return 0;
> +	return cap & PCI_EXP_DEVCAP_FLR;
> +}
>  
> +/**
> + * pcie_flr - initiate a PCIe function level reset
> + * @dev:	device to reset
> + *
> + * Initiate a function level reset on @dev.  The caller should ensure the
> + * device supports FLR before calling this function, e.g. by using the
> + * pcie_has_flr helper.

s/pcie_has_flr/pcie_has_flr()/

> + */
> +void pcie_flr(struct pci_dev *dev)
> +{
>  	if (!pci_wait_for_pending_transaction(dev))
>  		dev_err(&dev->dev, "timed out waiting for pending transaction; performing function level reset anyway\n");
>  
>  	pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
>  	pci_flr_wait(dev);
> -	return 0;
>  }
> +EXPORT_SYMBOL_GPL(pcie_flr);
>  
>  static int pci_af_flr(struct pci_dev *dev, int probe)
>  {
> @@ -3971,9 +3985,11 @@ static int __pci_dev_reset(struct pci_dev *dev, int probe)
>  	if (rc != -ENOTTY)
>  		goto done;
>  
> -	rc = pcie_flr(dev, probe);
> -	if (rc != -ENOTTY)
> +	if (pcie_has_flr(dev)) {
> +		pcie_flr(dev);
> +		rc = 0;
>  		goto done;
> +	}

This performs an FLR (if supported) always, regardless of "probe".
I think it should look something like this instead:

  if (pcie_has_flr(dev)) {
    if (!probe)
      pcie_flr(dev);
    rc = 0;
    goto done;
  }

>  	rc = pci_af_flr(dev, probe);
>  	if (rc != -ENOTTY)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index eb3da1a04e6c..f35e51eddad0 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1052,6 +1052,7 @@ int pcie_get_mps(struct pci_dev *dev);
>  int pcie_set_mps(struct pci_dev *dev, int mps);
>  int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
>  			  enum pcie_link_width *width);
> +void pcie_flr(struct pci_dev *dev);
>  int __pci_reset_function(struct pci_dev *dev);
>  int __pci_reset_function_locked(struct pci_dev *dev);
>  int pci_reset_function(struct pci_dev *dev);
> -- 
> 2.11.0
> 

[toc] | [prev] | [next] | [standalone]

#1623749 — Re: [PATCH 1/7] PCI: export pcie_flr

> s/pcie_has_flr/pcie_has_flr()/

Ok.

> This performs an FLR (if supported) always, regardless of "probe".
> I think it should look something like this instead:
> 
>   if (pcie_has_flr(dev)) {
>     if (!probe)
>       pcie_flr(dev);
>     rc = 0;
>     goto done;
>   }

Indeed!

[toc] | [prev] | [next] | [standalone]

#1623706

On Thu, Apr 13, 2017 at 04:53:32PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this exports the PCI layer pcie_flr helper, and removes various opencoded
> copies of it.

Looks good to me (except the comment on probe).  If you want to apply
the whole series via netdev or some non-PCI tree, here's my ack for
the drivers/pci parts, assuming the probe thing is resolved:

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Otherwise, I'd be glad to take the series given acks for the non-PCI
parts.  Just let me know.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1623709

On Fri, Apr 14, 2017 at 9:41 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Apr 13, 2017 at 04:53:32PM +0200, Christoph Hellwig wrote:
>> Hi all,
>>
>> this exports the PCI layer pcie_flr helper, and removes various opencoded
>> copies of it.
>
> Looks good to me (except the comment on probe).  If you want to apply
> the whole series via netdev or some non-PCI tree, here's my ack for
> the drivers/pci parts, assuming the probe thing is resolved:
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> Otherwise, I'd be glad to take the series given acks for the non-PCI
> parts.  Just let me know.

I do already have a patch (c5e4f0192ad2 ("PCI: Avoid FLR for Intel
82579 NICs")) on my pci/virtualization branch that touches pcie_flr()
and will conflict with this one.

[toc] | [prev] | [next] | [standalone]

#1623743

On Fri, Apr 14, 2017 at 09:51:48AM -0500, Bjorn Helgaas wrote:
> > Otherwise, I'd be glad to take the series given acks for the non-PCI
> > parts.  Just let me know.
> 
> I do already have a patch (c5e4f0192ad2 ("PCI: Avoid FLR for Intel
> 82579 NICs")) on my pci/virtualization branch that touches pcie_flr()
> and will conflict with this one.

I'll take a look and will resend on top of that branch.

[toc] | [prev] | [next] | [standalone]

#1623746

On Fri, Apr 14, 2017 at 09:41:08AM -0500, Bjorn Helgaas wrote:
> Looks good to me (except the comment on probe).  If you want to apply
> the whole series via netdev or some non-PCI tree, here's my ack for
> the drivers/pci parts, assuming the probe thing is resolved:
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> Otherwise, I'd be glad to take the series given acks for the non-PCI
> parts.  Just let me know.

I guess the best would be if you take the first three patches (once
resent) for the PCI tree for 4.12, then we can do the other patches
in the next merge window.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web