* kernel panic becase pci register more than once
@ 2018-09-09 8:26 Tonghao Zhang
2018-09-17 19:15 ` Bjorn Helgaas
0 siblings, 1 reply; 6+ messages in thread
From: Tonghao Zhang @ 2018-09-09 8:26 UTC (permalink / raw)
To: linux-pci
Hi all,
When I enable pci msi or msix more than once, the kernel will panic.
Now we just use the WARN_ON, should we add check for that has been
enabled.
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index f2ef896..324840f 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -958,7 +958,9 @@ static int __pci_enable_msix(struct pci_dev *dev,
struct msix_entry *entries,
}
}
}
- WARN_ON(!!dev->msix_enabled);
+
+ if (dev->msix_enabled)
+ return -EINVAL;
/* Check whether driver already requested for MSI irq */
if (dev->msi_enabled) {
@@ -1028,7 +1030,8 @@ 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);
+ if (dev->msi_enabled)
+ return -EINVAL;
/* Check whether driver already requested MSI-X irqs */
if (dev->msix_enabled) {
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: kernel panic becase pci register more than once
2018-09-09 8:26 kernel panic becase pci register more than once Tonghao Zhang
@ 2018-09-17 19:15 ` Bjorn Helgaas
2018-09-18 2:00 ` Tonghao Zhang
2018-09-20 6:40 ` Christoph Hellwig
0 siblings, 2 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2018-09-17 19:15 UTC (permalink / raw)
To: Tonghao Zhang; +Cc: linux-pci, Christoph Hellwig
[+cc Christoph]
On Sun, Sep 09, 2018 at 04:26:37PM +0800, Tonghao Zhang wrote:
> Hi all,
> When I enable pci msi or msix more than once, the kernel will panic.
> Now we just use the WARN_ON, should we add check for that has been
> enabled.
>
This needs a signed-off-by before I can apply it. See
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n416
A WARN_ON() shouldn't cause the kernel to panic. Do you mean that it
causes a backtrace? I think that's the intended behavior, because
calling this twice would be a driver bug, and we want to find and fix
those bugs.
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index f2ef896..324840f 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -958,7 +958,9 @@ static int __pci_enable_msix(struct pci_dev *dev,
> struct msix_entry *entries,
> }
> }
> }
> - WARN_ON(!!dev->msix_enabled);
> +
> + if (dev->msix_enabled)
> + return -EINVAL;
>
> /* Check whether driver already requested for MSI irq */
> if (dev->msi_enabled) {
> @@ -1028,7 +1030,8 @@ 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);
> + if (dev->msi_enabled)
> + return -EINVAL;
>
> /* Check whether driver already requested MSI-X irqs */
> if (dev->msix_enabled) {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: kernel panic becase pci register more than once
2018-09-17 19:15 ` Bjorn Helgaas
@ 2018-09-18 2:00 ` Tonghao Zhang
2018-09-18 2:00 ` Tonghao Zhang
2018-09-18 13:46 ` Bjorn Helgaas
2018-09-20 6:40 ` Christoph Hellwig
1 sibling, 2 replies; 6+ messages in thread
From: Tonghao Zhang @ 2018-09-18 2:00 UTC (permalink / raw)
To: helgaas; +Cc: linux-pci, hch
On Tue, Sep 18, 2018 at 3:15 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Christoph]
>
> On Sun, Sep 09, 2018 at 04:26:37PM +0800, Tonghao Zhang wrote:
> > Hi all,
> > When I enable pci msi or msix more than once, the kernel will panic.
> > Now we just use the WARN_ON, should we add check for that has been
> > enabled.
> >
>
> This needs a signed-off-by before I can apply it. See
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n416
OK
> A WARN_ON() shouldn't cause the kernel to panic. Do you mean that it
> causes a backtrace? I think that's the intended behavior, because
> calling this twice would be a driver bug, and we want to find and fix
> those bugs.
Yes, you are right, but we can return error code when drivers call this twice.
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index f2ef896..324840f 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -958,7 +958,9 @@ static int __pci_enable_msix(struct pci_dev *dev,
> > struct msix_entry *entries,
> > }
> > }
> > }
> > - WARN_ON(!!dev->msix_enabled);
> > +
> > + if (dev->msix_enabled)
> > + return -EINVAL;
> >
> > /* Check whether driver already requested for MSI irq */
> > if (dev->msi_enabled) {
> > @@ -1028,7 +1030,8 @@ 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);
> > + if (dev->msi_enabled)
> > + return -EINVAL;
> >
> > /* Check whether driver already requested MSI-X irqs */
> > if (dev->msix_enabled) {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: kernel panic becase pci register more than once
2018-09-18 2:00 ` Tonghao Zhang
@ 2018-09-18 2:00 ` Tonghao Zhang
2018-09-18 13:46 ` Bjorn Helgaas
1 sibling, 0 replies; 6+ messages in thread
From: Tonghao Zhang @ 2018-09-18 2:00 UTC (permalink / raw)
To: helgaas; +Cc: linux-pci, hch
On Tue, Sep 18, 2018 at 3:15 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Christoph]
>
> On Sun, Sep 09, 2018 at 04:26:37PM +0800, Tonghao Zhang wrote:
> > Hi all,
> > When I enable pci msi or msix more than once, the kernel will panic.
> > Now we just use the WARN_ON, should we add check for that has been
> > enabled.
> >
>
> This needs a signed-off-by before I can apply it. See
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n416
OK
> A WARN_ON() shouldn't cause the kernel to panic. Do you mean that it
> causes a backtrace? I think that's the intended behavior, because
> calling this twice would be a driver bug, and we want to find and fix
> those bugs.
Yes, you are right, but we can return error code when drivers call this twice.
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index f2ef896..324840f 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -958,7 +958,9 @@ static int __pci_enable_msix(struct pci_dev *dev,
> > struct msix_entry *entries,
> > }
> > }
> > }
> > - WARN_ON(!!dev->msix_enabled);
> > +
> > + if (dev->msix_enabled)
> > + return -EINVAL;
> >
> > /* Check whether driver already requested for MSI irq */
> > if (dev->msi_enabled) {
> > @@ -1028,7 +1030,8 @@ 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);
> > + if (dev->msi_enabled)
> > + return -EINVAL;
> >
> > /* Check whether driver already requested MSI-X irqs */
> > if (dev->msix_enabled) {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: kernel panic becase pci register more than once
2018-09-18 2:00 ` Tonghao Zhang
2018-09-18 2:00 ` Tonghao Zhang
@ 2018-09-18 13:46 ` Bjorn Helgaas
1 sibling, 0 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2018-09-18 13:46 UTC (permalink / raw)
To: Tonghao Zhang; +Cc: linux-pci, hch
On Tue, Sep 18, 2018 at 10:00:11AM +0800, Tonghao Zhang wrote:
> On Tue, Sep 18, 2018 at 3:15 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Sun, Sep 09, 2018 at 04:26:37PM +0800, Tonghao Zhang wrote:
> > > When I enable pci msi or msix more than once, the kernel will
> > > panic. Now we just use the WARN_ON, should we add check for
> > > that has been enabled.
> > A WARN_ON() shouldn't cause the kernel to panic. Do you mean that
> > it causes a backtrace? I think that's the intended behavior,
> > because calling this twice would be a driver bug, and we want to
> > find and fix those bugs.
> Yes, you are right, but we can return error code when drivers call
> this twice.
That's possible. We need a clear explanation for why we're making the
change, and that starts with a clear description of the problem, i.e.,
is the problem that the kernel crashes and a reboot is required, or is
the problem some messages in the dmesg log that look alarming but are
otherwise harmless? I think the latter.
If we decide that an error return is better than the WARN_ON(), we
need to look at where the check is done. For example, you currently
check in __pci_enable_msix(), which is only called from
__pci_enable_msix_range(). We should check for simple errors like
this as early as possible, which probably means
__pci_enable_msix_range().
> > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > > index f2ef896..324840f 100644
> > > --- a/drivers/pci/msi.c
> > > +++ b/drivers/pci/msi.c
> > > @@ -958,7 +958,9 @@ static int __pci_enable_msix(struct pci_dev *dev,
> > > struct msix_entry *entries,
> > > }
> > > }
> > > }
> > > - WARN_ON(!!dev->msix_enabled);
> > > +
> > > + if (dev->msix_enabled)
> > > + return -EINVAL;
> > >
> > > /* Check whether driver already requested for MSI irq */
> > > if (dev->msi_enabled) {
> > > @@ -1028,7 +1030,8 @@ 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);
> > > + if (dev->msi_enabled)
> > > + return -EINVAL;
> > >
> > > /* Check whether driver already requested MSI-X irqs */
> > > if (dev->msix_enabled) {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: kernel panic becase pci register more than once
2018-09-17 19:15 ` Bjorn Helgaas
2018-09-18 2:00 ` Tonghao Zhang
@ 2018-09-20 6:40 ` Christoph Hellwig
1 sibling, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2018-09-20 6:40 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Tonghao Zhang, linux-pci, Christoph Hellwig
On Mon, Sep 17, 2018 at 02:15:03PM -0500, Bjorn Helgaas wrote:
> A WARN_ON() shouldn't cause the kernel to panic. Do you mean that it
> causes a backtrace? I think that's the intended behavior, because
> calling this twice would be a driver bug, and we want to find and fix
> those bugs.
>
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index f2ef896..324840f 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -958,7 +958,9 @@ static int __pci_enable_msix(struct pci_dev *dev,
> > struct msix_entry *entries,
> > }
> > }
> > }
> > - WARN_ON(!!dev->msix_enabled);
> > +
> > + if (dev->msix_enabled)
> > + return -EINVAL;
This is a grave driver bug, so we should keep a trace. Then again
handling even grave driver bugs more gracefull is a good idea, so I'd
vote for:
if (WARN_ON_ONCE(dev->msix_enabled))
return -EINVAL;
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-09-20 6:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-09 8:26 kernel panic becase pci register more than once Tonghao Zhang
2018-09-17 19:15 ` Bjorn Helgaas
2018-09-18 2:00 ` Tonghao Zhang
2018-09-18 2:00 ` Tonghao Zhang
2018-09-18 13:46 ` Bjorn Helgaas
2018-09-20 6:40 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).