| Message ID | 20201217171431.502030-2-kbusch@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [1/3] PCI/ERR: Clear status of the reporting device | expand |
Hi Keith,
I love your patch! Perhaps something to improve:
[auto build test WARNING on pci/next]
[also build test WARNING on next-20201217]
[cannot apply to v5.10]
[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/Keith-Busch/PCI-ERR-Clear-status-of-the-reporting-device/20201218-011952
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: parisc-randconfig-r014-20201217 (attached as .config)
compiler: hppa64-linux-gcc (GCC) 9.3.0
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/8f1a820daa9ec2e25dfbdd5b4752df01e849b3aa
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Keith-Busch/PCI-ERR-Clear-status-of-the-reporting-device/20201218-011952
git checkout 8f1a820daa9ec2e25dfbdd5b4752df01e849b3aa
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc
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/pcie/aer.c: In function 'aer_root_reset':
>> drivers/pci/pcie/aer.c:15:21: warning: format '%s' expects argument of type 'char *', but argument 3 has type 'int' [-Wformat=]
15 | #define pr_fmt(fmt) "AER: " fmt
| ^~~~~~~
drivers/pci/pcie/aer.c:16:17: note: in expansion of macro 'pr_fmt'
16 | #define dev_fmt pr_fmt
| ^~~~~~
include/linux/dev_printk.h:118:17: note: in expansion of macro 'dev_fmt'
118 | _dev_info(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
include/linux/pci.h:2417:37: note: in expansion of macro 'dev_info'
2417 | #define pci_info(pdev, fmt, arg...) dev_info(&(pdev)->dev, fmt, ##arg)
| ^~~~~~~~
drivers/pci/pcie/aer.c:1417:3: note: in expansion of macro 'pci_info'
1417 | pci_info(dev, "%s Port link has been reset (%d)\n", rc,
| ^~~~~~~~
drivers/pci/pcie/aer.c:1417:19: note: format string is defined here
1417 | pci_info(dev, "%s Port link has been reset (%d)\n", rc,
| ~^
| |
| char *
| %d
>> drivers/pci/pcie/aer.c:15:21: warning: format '%d' expects argument of type 'int', but argument 4 has type 'char *' [-Wformat=]
15 | #define pr_fmt(fmt) "AER: " fmt
| ^~~~~~~
drivers/pci/pcie/aer.c:16:17: note: in expansion of macro 'pr_fmt'
16 | #define dev_fmt pr_fmt
| ^~~~~~
include/linux/dev_printk.h:118:17: note: in expansion of macro 'dev_fmt'
118 | _dev_info(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
include/linux/pci.h:2417:37: note: in expansion of macro 'dev_info'
2417 | #define pci_info(pdev, fmt, arg...) dev_info(&(pdev)->dev, fmt, ##arg)
| ^~~~~~~~
drivers/pci/pcie/aer.c:1417:3: note: in expansion of macro 'pci_info'
1417 | pci_info(dev, "%s Port link has been reset (%d)\n", rc,
| ^~~~~~~~
drivers/pci/pcie/aer.c:1417:48: note: format string is defined here
1417 | pci_info(dev, "%s Port link has been reset (%d)\n", rc,
| ~^
| |
| int
| %s
vim +15 drivers/pci/pcie/aer.c
9cc6f75b27e76d3 Frederick Lawler 2019-05-07 @15 #define pr_fmt(fmt) "AER: " fmt
9cc6f75b27e76d3 Frederick Lawler 2019-05-07 16 #define dev_fmt pr_fmt
9cc6f75b27e76d3 Frederick Lawler 2019-05-07 17
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Keith,
I love your patch! Perhaps something to improve:
[auto build test WARNING on pci/next]
[also build test WARNING on next-20201217]
[cannot apply to v5.10]
[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/Keith-Busch/PCI-ERR-Clear-status-of-the-reporting-device/20201218-011952
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: powerpc64-randconfig-r026-20201217 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project cee1e7d14f4628d6174b33640d502bff3b54ae45)
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
# install powerpc64 cross compiling tool for clang build
# apt-get install binutils-powerpc64-linux-gnu
# https://github.com/0day-ci/linux/commit/8f1a820daa9ec2e25dfbdd5b4752df01e849b3aa
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Keith-Busch/PCI-ERR-Clear-status-of-the-reporting-device/20201218-011952
git checkout 8f1a820daa9ec2e25dfbdd5b4752df01e849b3aa
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64
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/pcie/aer.c:1417:55: warning: format specifies type 'char *' but the argument has type 'int' [-Wformat]
pci_info(dev, "%s Port link has been reset (%d)\n", rc,
~~ ^~
%d
include/linux/pci.h:2417:67: note: expanded from macro 'pci_info'
#define pci_info(pdev, fmt, arg...) dev_info(&(pdev)->dev, fmt, ##arg)
~~~ ^~~
include/linux/dev_printk.h:118:33: note: expanded from macro 'dev_info'
_dev_info(dev, dev_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
>> drivers/pci/pcie/aer.c:1418:4: warning: format specifies type 'int' but the argument has type 'char *' [-Wformat]
pci_is_root_bus(dev->bus) ? "Root" : "Downstream");
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/pci.h:2417:67: note: expanded from macro 'pci_info'
#define pci_info(pdev, fmt, arg...) dev_info(&(pdev)->dev, fmt, ##arg)
~~~ ^~~
include/linux/dev_printk.h:118:33: note: expanded from macro 'dev_info'
_dev_info(dev, dev_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
2 warnings generated.
vim +1417 drivers/pci/pcie/aer.c
1367
1368 /**
1369 * aer_root_reset - reset Root Port hierarchy, RCEC, or RCiEP
1370 * @dev: pointer to Root Port, RCEC, or RCiEP
1371 *
1372 * Invoked by Port Bus driver when performing reset.
1373 */
1374 static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
1375 {
1376 int type = pci_pcie_type(dev);
1377 struct pci_dev *root;
1378 int aer;
1379 struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
1380 u32 reg32;
1381 int rc;
1382
1383 /*
1384 * Only Root Ports and RCECs have AER Root Command and Root Status
1385 * registers. If "dev" is an RCiEP, the relevant registers are in
1386 * the RCEC.
1387 */
1388 if (type == PCI_EXP_TYPE_RC_END)
1389 root = dev->rcec;
1390 else
1391 root = pcie_find_root_port(dev);
1392
1393 /*
1394 * If the platform retained control of AER, an RCiEP may not have
1395 * an RCEC visible to us, so dev->rcec ("root") may be NULL. In
1396 * that case, firmware is responsible for these registers.
1397 */
1398 aer = root ? root->aer_cap : 0;
1399
1400 if ((host->native_aer || pcie_ports_native) && aer) {
1401 /* Disable Root's interrupt in response to error messages */
1402 pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, ®32);
1403 reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
1404 pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
1405 }
1406
1407 if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
1408 if (pcie_has_flr(dev)) {
1409 rc = pcie_flr(dev);
1410 pci_info(dev, "has been reset (%d)\n", rc);
1411 } else {
1412 pci_info(dev, "not reset (no FLR support)\n");
1413 rc = -ENOTTY;
1414 }
1415 } else {
1416 rc = pci_bus_error_reset(dev);
> 1417 pci_info(dev, "%s Port link has been reset (%d)\n", rc,
> 1418 pci_is_root_bus(dev->bus) ? "Root" : "Downstream");
1419 }
1420
1421 if ((host->native_aer || pcie_ports_native) && aer) {
1422 /* Clear Root Error Status */
1423 pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, ®32);
1424 pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);
1425
1426 /* Enable Root Port's interrupt in response to error messages */
1427 pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, ®32);
1428 reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
1429 pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
1430 }
1431
1432 return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
1433 }
1434
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Keith, > On Dec 17, 2020, at 9:14 AM, Keith Busch <kbusch@kernel.org> wrote: > > The pci_dev parameter given to aer_root_reset() may be a downstream port > rather than the root port. Get the root port from the provided device in > order to clear the root's aer status, and update the message to indicate > which type of port was actually reset. > > Signed-off-by: Keith Busch <kbusch@kernel.org> > --- > drivers/pci/pcie/aer.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index 77b0f2c45bc0..b2b0e9eb5cfb 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -1388,7 +1388,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev) > if (type == PCI_EXP_TYPE_RC_END) > root = dev->rcec; > else > - root = dev; > + root = pcie_find_root_port(dev); This is a good sanity check on the dev being passed. This also reminds me to take a look at pcie_do_recovery() in terms of clarity with the two devices of interest being handled. Acked-by: Sean V Kelley <sean.v.kelley@intel.com> > > /* > * If the platform retained control of AER, an RCiEP may not have > @@ -1414,7 +1414,8 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev) > } > } else { > rc = pci_bus_error_reset(dev); > - pci_info(dev, "Root Port link has been reset (%d)\n", rc); > + pci_info(dev, "%s Port link has been reset (%d)\n", rc, Perhaps … "%s%s Port > + pci_is_root_bus(dev->bus) ? "Root" : "Downstream"); > } > > if ((host->native_aer || pcie_ports_native) && aer) { > -- > 2.24.1 >
On Mon, Jan 04, 2021 at 06:42:58PM +0000, Kelley, Sean V wrote: > > On Dec 17, 2020, at 9:14 AM, Keith Busch <kbusch@kernel.org> wrote: > > rc = pci_bus_error_reset(dev); > > - pci_info(dev, "Root Port link has been reset (%d)\n", rc); > > + pci_info(dev, "%s Port link has been reset (%d)\n", rc, > > Perhaps … "%s%s Port I am not sure I understand. What should the second string in this format say? I have to resend this patch anyway because I messed up the parmeter order and build bot caught me. I'll split it out from this patch since it is really a separate issue. > > + pci_is_root_bus(dev->bus) ? "Root" : "Downstream");
> On Jan 4, 2021, at 2:05 PM, Keith Busch <kbusch@kernel.org> wrote: > > On Mon, Jan 04, 2021 at 06:42:58PM +0000, Kelley, Sean V wrote: >>> On Dec 17, 2020, at 9:14 AM, Keith Busch <kbusch@kernel.org> wrote: >>> rc = pci_bus_error_reset(dev); >>> - pci_info(dev, "Root Port link has been reset (%d)\n", rc); >>> + pci_info(dev, "%s Port link has been reset (%d)\n", rc, >> >> Perhaps … "%s%s Port > > I am not sure I understand. What should the second string in this format > say? > > I have to resend this patch anyway because I messed up the parmeter > order and build bot caught me. I'll split it out from this patch since > it is really a separate issue. Never mind you only had one string, it was the order issue that the builedbot caught. Sean > > >>> + pci_is_root_bus(dev->bus) ? "Root" : "Downstream");
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index 77b0f2c45bc0..b2b0e9eb5cfb 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -1388,7 +1388,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev) if (type == PCI_EXP_TYPE_RC_END) root = dev->rcec; else - root = dev; + root = pcie_find_root_port(dev); /* * If the platform retained control of AER, an RCiEP may not have @@ -1414,7 +1414,8 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev) } } else { rc = pci_bus_error_reset(dev); - pci_info(dev, "Root Port link has been reset (%d)\n", rc); + pci_info(dev, "%s Port link has been reset (%d)\n", rc, + pci_is_root_bus(dev->bus) ? "Root" : "Downstream"); } if ((host->native_aer || pcie_ports_native) && aer) {
The pci_dev parameter given to aer_root_reset() may be a downstream port rather than the root port. Get the root port from the provided device in order to clear the root's aer status, and update the message to indicate which type of port was actually reset. Signed-off-by: Keith Busch <kbusch@kernel.org> --- drivers/pci/pcie/aer.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)