LinuxLists.cc - [PATCH v13 2/9] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

2021-08-01 14:28:26

Subject: [PATCH v13 2/9] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

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 <[email protected]>
---
drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +--
drivers/pci/pci.c | 40 +++++++++++++++-------
drivers/pci/pcie/aer.c | 12 +++----
drivers/pci/quirks.c | 9 ++---
include/linux/pci.h | 2 +-
5 files changed, 38 insertions(+), 29 deletions(-)

diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c
index facc8e6bc580..15d6c8452807 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);

pci_restore_state(pdev);

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1fafd05caa41..f219a3dc6750 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4626,15 +4626,13 @@ bool pcie_has_flr(struct pci_dev *dev)

return FIELD_GET(PCI_EXP_DEVCAP_FLR, dev->devcap) == 1;
}
-EXPORT_SYMBOL_GPL(pcie_has_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.
+ * Initiate a function level reset unconditionally on @dev without
+ * checking any flags and DEVCAP
*/
int pcie_flr(struct pci_dev *dev)
{
@@ -4655,7 +4653,26 @@ int pcie_flr(struct pci_dev *dev)

return pci_dev_wait(dev, "FLR", PCIE_RESET_READY_POLL_MS);
}
-EXPORT_SYMBOL_GPL(pcie_flr);
+EXPORT_SYMBOL(pcie_flr);
+
+/**
+ * pcie_reset_flr - initiate a PCIe function level reset
+ * @dev: device to reset
+ * @probe: If set, only check if the device can be reset this way.
+ *
+ * Initiate a function level reset on @dev.
+ */
+int pcie_reset_flr(struct pci_dev *dev, int probe)
+{
+ if (!pcie_has_flr(dev))
+ return -ENOTTY;
+
+ if (probe)
+ return 0;
+
+ return pcie_flr(dev);
+}
+EXPORT_SYMBOL_GPL(pcie_reset_flr);

static int pci_af_flr(struct pci_dev *dev, int probe)
{
@@ -5137,11 +5154,9 @@ int __pci_reset_function_locked(struct pci_dev *dev)
rc = pci_dev_specific_reset(dev, 0);
if (rc != -ENOTTY)
return rc;
- if (pcie_has_flr(dev)) {
- rc = pcie_flr(dev);
- if (rc != -ENOTTY)
- return rc;
- }
+ rc = pcie_reset_flr(dev, 0);
+ if (rc != -ENOTTY)
+ return rc;
rc = pci_af_flr(dev, 0);
if (rc != -ENOTTY)
return rc;
@@ -5172,8 +5187,9 @@ int pci_probe_reset_function(struct pci_dev *dev)
rc = pci_dev_specific_reset(dev, 1);
if (rc != -ENOTTY)
return rc;
- if (pcie_has_flr(dev))
- return 0;
+ rc = pcie_reset_flr(dev, 1);
+ if (rc != -ENOTTY)
+ return rc;
rc = pci_af_flr(dev, 1);
if (rc != -ENOTTY)
return rc;
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index ec943cee5ecc..98077595a73e 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1405,13 +1405,11 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
}

if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
- if (pcie_has_flr(dev)) {
- rc = pcie_flr(dev);
- pci_info(dev, "has been reset (%d)\n", rc);
- } else {
- pci_info(dev, "not reset (no FLR support)\n");
- rc = -ENOTTY;
- }
+ rc = pcie_reset_flr(dev, 0);
+ if (!rc)
+ pci_info(dev, "has been reset\n");
+ else
+ pci_info(dev, "not reset (no FLR support: %d)\n", rc);
} else {
rc = pci_bus_error_reset(dev);
pci_info(dev, "%s Port link has been reset (%d)\n",
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index d85914afe65a..b48e7ef8b641 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3819,7 +3819,7 @@ static int nvme_disable_and_flr(struct pci_dev *dev, int probe)
u32 cfg;

if (dev->class != PCI_CLASS_STORAGE_EXPRESS ||
- !pcie_has_flr(dev) || !pci_resource_start(dev, 0))
+ pcie_reset_flr(dev, 1) || !pci_resource_start(dev, 0))
return -ENOTTY;

if (probe)
@@ -3888,13 +3888,10 @@ static int nvme_disable_and_flr(struct pci_dev *dev, int probe)
*/
static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
{
- if (!pcie_has_flr(dev))
- return -ENOTTY;
-
if (probe)
- return 0;
+ return pcie_reset_flr(dev, 1);

- pcie_flr(dev);
+ pcie_reset_flr(dev, 0);

msleep(250);

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 697b1f085c7b..aa85e7d3147e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1226,7 +1226,7 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev,
enum pci_bus_speed *speed,
enum pcie_link_width *width);
void pcie_print_link_status(struct pci_dev *dev);
-bool pcie_has_flr(struct pci_dev *dev);
+int pcie_reset_flr(struct pci_dev *dev, int probe);
int pcie_flr(struct pci_dev *dev);
int __pci_reset_function_locked(struct pci_dev *dev);
int pci_reset_function(struct pci_dev *dev);
--
2.32.0

2021-08-01 17:07:37

by kernel test robot

[permalink] [raw]

Subject: [RFC PATCH] PCI: pcie_has_flr() can be static

drivers/pci/pci.c:4636:6: warning: symbol 'pcie_has_flr' was not declared. Should it be static?

Reported-by: kernel test robot <[email protected]>
Signed-off-by: kernel test robot <[email protected]>
---
pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 338c98bb60968..dba075bd11114 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4633,7 +4633,7 @@ EXPORT_SYMBOL(pci_wait_for_pending_transaction);
* Returns true if the device advertises support for PCIe function level
* resets.
*/
-bool pcie_has_flr(struct pci_dev *dev)
+static bool pcie_has_flr(struct pci_dev *dev)
{
if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
return false;

2021-08-01 17:08:29

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v13 2/9] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

Hi Amey,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pci/next]
[also build test WARNING on pm/linux-next cryptodev/master crypto/master linus/master v5.14-rc3 next-20210730]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Amey-Narkhede/PCI-Expose-and-manage-PCI-device-reset/20210801-222806
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-randconfig-s002-20210801 (attached as .config)
compiler: gcc-10 (Ubuntu 10.3.0-1ubuntu1~20.04) 10.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-341-g8af24329-dirty
# https://github.com/0day-ci/linux/commit/ce304ecb5f2709fa58ef6f16cf8e89ddbd7e42cd
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Amey-Narkhede/PCI-Expose-and-manage-PCI-device-reset/20210801-222806
git checkout ce304ecb5f2709fa58ef6f16cf8e89ddbd7e42cd
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/pci/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

sparse warnings: (new ones prefixed by >>)
drivers/pci/pci.c:1052:13: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1052:21: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1052:31: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1052:39: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1061:35: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1061:54: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1062:19: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1062:37: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1092:23: sparse: sparse: invalid assignment: |=
drivers/pci/pci.c:1092:23: sparse: left side has type unsigned short
drivers/pci/pci.c:1092:23: sparse: right side has type restricted pci_power_t
drivers/pci/pci.c:1097:57: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1119:28: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted pci_power_t [usertype] current_state @@ got int @@
drivers/pci/pci.c:1119:28: sparse: expected restricted pci_power_t [usertype] current_state
drivers/pci/pci.c:1119:28: sparse: got int
drivers/pci/pci.c:1168:36: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted pci_power_t [usertype] current_state @@ got int @@
drivers/pci/pci.c:1168:36: sparse: expected restricted pci_power_t [usertype] current_state
drivers/pci/pci.c:1168:36: sparse: got int
drivers/pci/pci.c:1340:13: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1340:21: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1342:18: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1342:26: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1365:13: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1365:22: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1372:46: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1372:54: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:1730:38: sparse: sparse: array of flexible structures
drivers/pci/pci.c:1917:36: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted pci_power_t [usertype] current_state @@ got int @@
drivers/pci/pci.c:1917:36: sparse: expected restricted pci_power_t [usertype] current_state
drivers/pci/pci.c:1917:36: sparse: got int
drivers/pci/pci.c:2313:44: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:2614:61: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:2615:45: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:2804:20: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:2804:38: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:2827:49: sparse: sparse: restricted pci_power_t degrades to integer
drivers/pci/pci.c:2827:67: sparse: sparse: restricted pci_power_t degrades to integer
>> drivers/pci/pci.c:4636:6: sparse: sparse: symbol 'pcie_has_flr' was not declared. Should it be static?
drivers/pci/pci.c:4768:13: sparse: sparse: invalid assignment: |=
drivers/pci/pci.c:4768:13: sparse: left side has type unsigned short
drivers/pci/pci.c:4768:13: sparse: right side has type restricted pci_power_t
drivers/pci/pci.c:4773:13: sparse: sparse: invalid assignment: |=
drivers/pci/pci.c:4773:13: sparse: left side has type unsigned short
drivers/pci/pci.c:4773:13: sparse: right side has type restricted pci_power_t

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (5.57 kB)
.config.gz (37.62 kB)
Download all attachments

2021-08-01 18:29:43

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v13 2/9] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

Attachments:

(No filename) (3.25 kB)
.config.gz (34.72 kB)
Download all attachments

2021-08-02 22:46:23

by Bjorn Helgaas

[permalink] [raw]

Subject: Re: [PATCH v13 2/9] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

On Sun, Aug 01, 2021 at 07:55:11PM +0530, Amey Narkhede wrote:
> 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 <[email protected]>
> ---
> drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +--
> drivers/pci/pci.c | 40 +++++++++++++++-------
> drivers/pci/pcie/aer.c | 12 +++----
> drivers/pci/quirks.c | 9 ++---
> include/linux/pci.h | 2 +-
> 5 files changed, 38 insertions(+), 29 deletions(-)

> int pcie_flr(struct pci_dev *dev)
> {
> @@ -4655,7 +4653,26 @@ int pcie_flr(struct pci_dev *dev)
>
> return pci_dev_wait(dev, "FLR", PCIE_RESET_READY_POLL_MS);
> }
> -EXPORT_SYMBOL_GPL(pcie_flr);
> +EXPORT_SYMBOL(pcie_flr);

Why this change? If it's unintentional and there's no other reason to
repost, I can fix it up locally.

2021-08-03 04:51:26

by Amey Narkhede

[permalink] [raw]

Subject: Re: [PATCH v13 2/9] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

On 21/08/02 05:44PM, Bjorn Helgaas wrote:
> On Sun, Aug 01, 2021 at 07:55:11PM +0530, Amey Narkhede wrote:
> > 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 <[email protected]>
> > ---
> > drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +--
> > drivers/pci/pci.c | 40 +++++++++++++++-------
> > drivers/pci/pcie/aer.c | 12 +++----
> > drivers/pci/quirks.c | 9 ++---
> > include/linux/pci.h | 2 +-
> > 5 files changed, 38 insertions(+), 29 deletions(-)
>
> > int pcie_flr(struct pci_dev *dev)
> > {
> > @@ -4655,7 +4653,26 @@ int pcie_flr(struct pci_dev *dev)
> >
> > return pci_dev_wait(dev, "FLR", PCIE_RESET_READY_POLL_MS);
> > }
> > -EXPORT_SYMBOL_GPL(pcie_flr);
> > +EXPORT_SYMBOL(pcie_flr);
>
> Why this change? If it's unintentional and there's no other reason to
> repost, I can fix it up locally.
Yeah my bad it was unintentional.