* [PATCH] pci: msi: return error code instead of only warn
@ 2018-09-24 14:00 xiangxia.m.yue
2018-09-25 15:36 ` Christoph Hellwig
2018-09-25 20:30 ` Bjorn Helgaas
0 siblings, 2 replies; 3+ messages in thread
From: xiangxia.m.yue @ 2018-09-24 14:00 UTC (permalink / raw)
To: helgaas, hch; +Cc: linux-pci, Tonghao Zhang
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
The drivers may enable the msi/msix more than once.
Actually, this is the driver bug. But the bug may panic
the kernel and should be fixed. We can return the error
code instead of WARN_ON, to tell the drivers that the
msi/msix is already enabled.
When enable them again, the call tree is shown on kernel
3.10.0, but linux upstream in the same way.
Call Trace:
dump_stack+0x19/0x1b
warn_slowpath_common+0x70/0xb0
warn_slowpath_null+0x1a/0x20
pci_enable_msix+0x3c9/0x3e0
igbuio_pci_open+0x72/0x300 [igb_uio]
uio_open+0xcc/0x120 [uio]
chrdev_open+0xa1/0x1e0
[...]
do_sys_open+0xf3/0x1f0
SyS_open+0x1e/0x20
system_call_fastpath+0x16/0x1b
------------[ cut here ]------------
WARNING: at fs/sysfs/dir.c:526 sysfs_add_one+0xa5/0xd0()
sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:03.0/0000:01:00.1/msi_irqs'
Call Trace:
dump_stack+0x19/0x1b
warn_slowpath_common+0x70/0xb0
warn_slowpath_fmt+0x5c/0x80
sysfs_add_one+0xa5/0xd0
create_dir+0x7c/0xe0
sysfs_create_subdir+0x1c/0x20
internal_create_group+0x6d/0x290
sysfs_create_groups+0x4a/0xa0
populate_msi_sysfs+0x1cd/0x210
pci_enable_msix+0x31c/0x3e0
igbuio_pci_open+0x72/0x300 [igb_uio]
uio_open+0xcc/0x120 [uio]
chrdev_open+0xa1/0x1e0
[...]
do_sys_open+0xf3/0x1f0
SyS_open+0x1e/0x20
system_call_fastpath+0x16/0x1b
---[ end trace 11042e2848880209 ]---
Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ffffffffa056b4fa
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
drivers/pci/msi.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 4d88afd..4326e27 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -958,7 +958,6 @@ static int __pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries,
}
}
}
- WARN_ON(!!dev->msix_enabled);
/* Check whether driver already requested for MSI irq */
if (dev->msi_enabled) {
@@ -1028,8 +1027,6 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
if (!pci_msi_supported(dev, minvec))
return -EINVAL;
- WARN_ON(!!dev->msi_enabled);
-
/* Check whether driver already requested MSI-X irqs */
if (dev->msix_enabled) {
pci_info(dev, "can't enable MSI (MSI-X already enabled)\n");
@@ -1039,6 +1036,9 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
if (maxvec < minvec)
return -ERANGE;
+ if (WARN_ON_ONCE(!!dev->msi_enabled))
+ return -EINVAL;
+
nvec = pci_msi_vec_count(dev);
if (nvec < 0)
return nvec;
@@ -1087,6 +1087,9 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
if (maxvec < minvec)
return -ERANGE;
+ if (WARN_ON_ONCE(!!dev->msix_enabled))
+ return -EINVAL;
+
for (;;) {
if (affd) {
nvec = irq_calc_affinity_vectors(minvec, nvec, affd);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] pci: msi: return error code instead of only warn
2018-09-24 14:00 [PATCH] pci: msi: return error code instead of only warn xiangxia.m.yue
@ 2018-09-25 15:36 ` Christoph Hellwig
2018-09-25 20:30 ` Bjorn Helgaas
1 sibling, 0 replies; 3+ messages in thread
From: Christoph Hellwig @ 2018-09-25 15:36 UTC (permalink / raw)
To: xiangxia.m.yue; +Cc: helgaas, hch, linux-pci
On Mon, Sep 24, 2018 at 07:00:41AM -0700, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> The drivers may enable the msi/msix more than once.
> Actually, this is the driver bug. But the bug may panic
> the kernel and should be fixed. We can return the error
> code instead of WARN_ON, to tell the drivers that the
> msi/msix is already enabled.
>
> When enable them again, the call tree is shown on kernel
> 3.10.0, but linux upstream in the same way.
You can use up to 73 characters per line for your changelog, and doing so
makes it a little more readable.
> + if (WARN_ON_ONCE(!!dev->msi_enabled))
> + return -EINVAL;
> +
> nvec = pci_msi_vec_count(dev);
> if (nvec < 0)
> return nvec;
> @@ -1087,6 +1087,9 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
> if (maxvec < minvec)
> return -ERANGE;
>
> + if (WARN_ON_ONCE(!!dev->msix_enabled))
> + return -EINVAL;
> +
You can drop the "!!", WARN_ON_ONCE will do the implicit bool conversion
for you.
Except for that the patch looks good to me.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] pci: msi: return error code instead of only warn
2018-09-24 14:00 [PATCH] pci: msi: return error code instead of only warn xiangxia.m.yue
2018-09-25 15:36 ` Christoph Hellwig
@ 2018-09-25 20:30 ` Bjorn Helgaas
1 sibling, 0 replies; 3+ messages in thread
From: Bjorn Helgaas @ 2018-09-25 20:30 UTC (permalink / raw)
To: xiangxia.m.yue; +Cc: hch, linux-pci
On Mon, Sep 24, 2018 at 07:00:41AM -0700, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> The drivers may enable the msi/msix more than once.
> Actually, this is the driver bug. But the bug may panic
> the kernel and should be fixed. We can return the error
> code instead of WARN_ON, to tell the drivers that the
> msi/msix is already enabled.
>
> When enable them again, the call tree is shown on kernel
> 3.10.0, but linux upstream in the same way.
>
> Call Trace:
> dump_stack+0x19/0x1b
> warn_slowpath_common+0x70/0xb0
> warn_slowpath_null+0x1a/0x20
> pci_enable_msix+0x3c9/0x3e0
> igbuio_pci_open+0x72/0x300 [igb_uio]
> uio_open+0xcc/0x120 [uio]
> chrdev_open+0xa1/0x1e0
> [...]
> do_sys_open+0xf3/0x1f0
> SyS_open+0x1e/0x20
> system_call_fastpath+0x16/0x1b
> ------------[ cut here ]------------
> WARNING: at fs/sysfs/dir.c:526 sysfs_add_one+0xa5/0xd0()
> sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:03.0/0000:01:00.1/msi_irqs'
>
> Call Trace:
> dump_stack+0x19/0x1b
> warn_slowpath_common+0x70/0xb0
> warn_slowpath_fmt+0x5c/0x80
> sysfs_add_one+0xa5/0xd0
> create_dir+0x7c/0xe0
> sysfs_create_subdir+0x1c/0x20
> internal_create_group+0x6d/0x290
> sysfs_create_groups+0x4a/0xa0
> populate_msi_sysfs+0x1cd/0x210
> pci_enable_msix+0x31c/0x3e0
> igbuio_pci_open+0x72/0x300 [igb_uio]
> uio_open+0xcc/0x120 [uio]
> chrdev_open+0xa1/0x1e0
> [...]
> do_sys_open+0xf3/0x1f0
> SyS_open+0x1e/0x20
> system_call_fastpath+0x16/0x1b
> ---[ end trace 11042e2848880209 ]---
> Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ffffffffa056b4fa
>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
I cleaned up whitespace (extra spaces after "return") and !! (as Christoph
mentioned) and applied the patch below to pci/msi for v4.20, thanks!
commit 4c1ef72e9b71a19fb405ebfcd37c0a5e16fa44ca
Author: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Date: Mon Sep 24 07:00:41 2018 -0700
PCI/MSI: Warn and return error if driver enables MSI/MSI-X twice
It is a serious driver defect to enable MSI or MSI-X more than once. Doing
so may panic the kernel as in the stack trace below:
Call Trace:
sysfs_add_one+0xa5/0xd0
create_dir+0x7c/0xe0
sysfs_create_subdir+0x1c/0x20
internal_create_group+0x6d/0x290
sysfs_create_groups+0x4a/0xa0
populate_msi_sysfs+0x1cd/0x210
pci_enable_msix+0x31c/0x3e0
igbuio_pci_open+0x72/0x300 [igb_uio]
uio_open+0xcc/0x120 [uio]
chrdev_open+0xa1/0x1e0
[...]
do_sys_open+0xf3/0x1f0
SyS_open+0x1e/0x20
system_call_fastpath+0x16/0x1b
---[ end trace 11042e2848880209 ]---
Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ffffffffa056b4fa
We want to keep the WARN_ON() and stack trace so the driver can be fixed,
but we can avoid the kernel panic by returning an error. We may still get
warnings like this:
Call Trace:
pci_enable_msix+0x3c9/0x3e0
igbuio_pci_open+0x72/0x300 [igb_uio]
uio_open+0xcc/0x120 [uio]
chrdev_open+0xa1/0x1e0
[...]
do_sys_open+0xf3/0x1f0
SyS_open+0x1e/0x20
system_call_fastpath+0x16/0x1b
------------[ cut here ]------------
WARNING: at fs/sysfs/dir.c:526 sysfs_add_one+0xa5/0xd0()
sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:03.0/0000:01:00.1/msi_irqs'
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
[bhelgaas: changelog, fix patch whitespace, remove !!]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index f2ef896464b3..af24ed50a245 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -958,7 +958,6 @@ static int __pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries,
}
}
}
- WARN_ON(!!dev->msix_enabled);
/* Check whether driver already requested for MSI irq */
if (dev->msi_enabled) {
@@ -1028,8 +1027,6 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
if (!pci_msi_supported(dev, minvec))
return -EINVAL;
- WARN_ON(!!dev->msi_enabled);
-
/* Check whether driver already requested MSI-X irqs */
if (dev->msix_enabled) {
pci_info(dev, "can't enable MSI (MSI-X already enabled)\n");
@@ -1039,6 +1036,9 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
if (maxvec < minvec)
return -ERANGE;
+ if (WARN_ON_ONCE(dev->msi_enabled))
+ return -EINVAL;
+
nvec = pci_msi_vec_count(dev);
if (nvec < 0)
return nvec;
@@ -1087,6 +1087,9 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
if (maxvec < minvec)
return -ERANGE;
+ if (WARN_ON_ONCE(dev->msix_enabled))
+ return -EINVAL;
+
for (;;) {
if (affd) {
nvec = irq_calc_affinity_vectors(minvec, nvec, affd);
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-09-25 20:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-24 14:00 [PATCH] pci: msi: return error code instead of only warn xiangxia.m.yue
2018-09-25 15:36 ` Christoph Hellwig
2018-09-25 20:30 ` Bjorn Helgaas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.