| Message ID | 20210801142518.1224-3-ameynarkhede03@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Expose and manage PCI device reset | expand |
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 <lkp@intel.com>
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/kbuild-all@lists.01.org
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: x86_64-randconfig-r025-20210801 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 4f71f59bf3d9914188a11d0c41bedbb339d36ff5)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# 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
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/pci/pci.c:4636:6: warning: no previous prototype for function 'pcie_has_flr' [-Wmissing-prototypes]
bool pcie_has_flr(struct pci_dev *dev)
^
drivers/pci/pci.c:4636:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
bool pcie_has_flr(struct pci_dev *dev)
^
static
1 warning generated.
vim +/pcie_has_flr +4636 drivers/pci/pci.c
3775a209d38aa3 Casey Leedom 2013-08-06 4628
a60a2b73ba69ab Christoph Hellwig 2017-04-14 4629 /**
a60a2b73ba69ab Christoph Hellwig 2017-04-14 4630 * pcie_has_flr - check if a device supports function level resets
a60a2b73ba69ab Christoph Hellwig 2017-04-14 4631 * @dev: device to check
a60a2b73ba69ab Christoph Hellwig 2017-04-14 4632 *
a60a2b73ba69ab Christoph Hellwig 2017-04-14 4633 * Returns true if the device advertises support for PCIe function level
a60a2b73ba69ab Christoph Hellwig 2017-04-14 4634 * resets.
a60a2b73ba69ab Christoph Hellwig 2017-04-14 4635 */
2d2917f7747805 Alex Williamson 2018-08-09 @4636 bool pcie_has_flr(struct pci_dev *dev)
3775a209d38aa3 Casey Leedom 2013-08-06 4637 {
f65fd1aa4f9881 Sasha Neftin 2017-04-03 4638 if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
a60a2b73ba69ab Christoph Hellwig 2017-04-14 4639 return false;
3775a209d38aa3 Casey Leedom 2013-08-06 4640
e8213ae6f11e62 Amey Narkhede 2021-08-01 4641 return FIELD_GET(PCI_EXP_DEVCAP_FLR, dev->devcap) == 1;
a60a2b73ba69ab Christoph Hellwig 2017-04-14 4642 }
3775a209d38aa3 Casey Leedom 2013-08-06 4643
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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 <ameynarkhede03@gmail.com> > --- > 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.
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 <ameynarkhede03@gmail.com> > > --- > > 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.
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);
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@gmail.com> --- 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(-)