[PATCH v2 0/4] pci: fix unhandled interrupt on shutdown

[PATCH v2 0/4] pci: fix unhandled interrupt on shutdown

[Michael S. Tsirkin]

Fam Zheng noticed that pci shutdown disables msi and msix of a device while
device is still active. This was intended to fix kexec with fusion devices but
had the unintended effect of breaking even regular shutdown when using virtio.

The same problem would affect any driver which doesn't register
a level interrupt handler when using msix.

I think the fix is to avoid touching device on shutdown:
we clear bus master anyway, so we won't get any more
msi interrupts, and bus reset will clear the msi/msix
state eventually anyway.

The patches seems to all work well for me.  Given they affect all pci devices,
and the bug has been there since 2.6 times, I think there's no rush: we can
merge them for 4.1.

At the same time, once merged, they will likely make a good
stable candidate.

Michael S. Tsirkin (4):
  pci: disable msi/msix at probe time
  pci: don't disable msi/msix at shutdown
  pci: make msi/msix shutdown functions static
  virtio_pci: drop msi_off on probe

 include/linux/pci.h                | 4 ----
 drivers/pci/msi.c                  | 4 ++--
 drivers/pci/pci-driver.c           | 8 ++++++--
 drivers/virtio/virtio_pci_common.c | 3 ---
 4 files changed, 8 insertions(+), 11 deletions(-)

-- 
MST

[PATCH v2 1/4] pci: disable msi/msix at probe time

[Michael S. Tsirkin]

commit d52877c7b1afb8c37ebe17e2005040b79cb618b0
    pci/irq: let pci_device_shutdown to call pci_msi_shutdown v2

attempted to address the problem of kexec getting
started after linux enabled msi/msix for a device,
and drivers being confused by msi being enabled,
by disabling msi at shutdown.

But arguably, it's better to disable msi/msix when kexec
starts - for example, kexec might run after a crash (kdump)
and shutdown callbacks are not always invoked in that case.

Cc: Yinghai Lu <yhlu.kernel.send@gmail.com>
Cc: Ulrich Obergfell <uobergfe@redhat.com>
Cc: Fam Zheng <famz@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/pci/pci-driver.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 3cb2210..2ebd2a8 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -305,6 +305,12 @@ static long local_pci_probe(void *_ddi)
 	 */
 	pm_runtime_get_sync(dev);
 	pci_dev->driver = pci_drv;
+	/*
+	 * When using kexec, msi might be left enabled by the previous kernel,
+	 * this breaks things as some drivers assume msi/msi-x is off at boot.
+	 * Fix this by forcing msi off at startup.
+	 */
+	pci_msi_off(pci_dev);
 	rc = pci_drv->probe(pci_dev, ddi->id);
 	if (!rc)
 		return rc;
-- 
MST

[PATCH v2 2/4] pci: don't disable msi/msix at shutdown

[Michael S. Tsirkin]

This partially reverts commit d52877c7b1afb8c37ebe17e2005040b79cb618b0:
	"pci/irq: let pci_device_shutdown to call pci_msi_shutdown v2"

It's un-necessary now that we disable msi at start, and it actually
turns out to cause problems: some device drivers don't register a level
interrupt handler when they detect msi/msix capability, switching off
msi while device is going causes device to assert a level interrupt
which is never de-asserted, causing a kernel hang.

In particular, this was observed with virtio.

Cc: Yinghai Lu <yhlu.kernel.send@gmail.com>
Cc: Ulrich Obergfell <uobergfe@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Reported-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/pci/pci-driver.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 2ebd2a8..c7566f1 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -456,8 +456,6 @@ static void pci_device_shutdown(struct device *dev)
 
 	if (drv && drv->shutdown)
 		drv->shutdown(pci_dev);
-	pci_msi_shutdown(pci_dev);
-	pci_msix_shutdown(pci_dev);
 
 #ifdef CONFIG_KEXEC
 	/*
-- 
MST

[PATCH v2 3/4] pci: make msi/msix shutdown functions static

[Michael S. Tsirkin]

pci_msi_shutdown and pci_msix_shutdown are now internal to msi.c, drop
them from header and make them static.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/pci.h | 4 ----
 drivers/pci/msi.c   | 4 ++--
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 211e9da..a34df45 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1209,11 +1209,9 @@ struct msix_entry {
 
 #ifdef CONFIG_PCI_MSI
 int pci_msi_vec_count(struct pci_dev *dev);
-void pci_msi_shutdown(struct pci_dev *dev);
 void pci_disable_msi(struct pci_dev *dev);
 int pci_msix_vec_count(struct pci_dev *dev);
 int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec);
-void pci_msix_shutdown(struct pci_dev *dev);
 void pci_disable_msix(struct pci_dev *dev);
 void pci_restore_msi_state(struct pci_dev *dev);
 int pci_msi_enabled(void);
@@ -1237,13 +1235,11 @@ static inline int pci_enable_msix_exact(struct pci_dev *dev,
 }
 #else
 static inline int pci_msi_vec_count(struct pci_dev *dev) { return -ENOSYS; }
-static inline void pci_msi_shutdown(struct pci_dev *dev) { }
 static inline void pci_disable_msi(struct pci_dev *dev) { }
 static inline int pci_msix_vec_count(struct pci_dev *dev) { return -ENOSYS; }
 static inline int pci_enable_msix(struct pci_dev *dev,
 				  struct msix_entry *entries, int nvec)
 { return -ENOSYS; }
-static inline void pci_msix_shutdown(struct pci_dev *dev) { }
 static inline void pci_disable_msix(struct pci_dev *dev) { }
 static inline void pci_restore_msi_state(struct pci_dev *dev) { }
 static inline int pci_msi_enabled(void) { return 0; }
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index c3e7dfc..0e037af 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -908,7 +908,7 @@ int pci_msi_vec_count(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pci_msi_vec_count);
 
-void pci_msi_shutdown(struct pci_dev *dev)
+static void pci_msi_shutdown(struct pci_dev *dev)
 {
 	struct msi_desc *desc;
 	u32 mask;
@@ -1014,7 +1014,7 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
 }
 EXPORT_SYMBOL(pci_enable_msix);
 
-void pci_msix_shutdown(struct pci_dev *dev)
+static void pci_msix_shutdown(struct pci_dev *dev)
 {
 	struct msi_desc *entry;
 
-- 
MST

[PATCH v2 4/4] virtio_pci: drop msi_off on probe

[Michael S. Tsirkin]

pci core now disables msi on probe automatically,
drop this from device-specific code.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio_pci_common.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index e894eb2..806bb2c 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -501,9 +501,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 	INIT_LIST_HEAD(&vp_dev->virtqueues);
 	spin_lock_init(&vp_dev->lock);
 
-	/* Disable MSI/MSIX to bring device to a known good state. */
-	pci_msi_off(pci_dev);
-
 	/* enable the device */
 	rc = pci_enable_device(pci_dev);
 	if (rc)
-- 
MST

Re: [PATCH v2 0/4] pci: fix unhandled interrupt on shutdown

[Fam Zheng]

On Thu, 03/19 19:57, Michael S. Tsirkin wrote:
> Fam Zheng noticed that pci shutdown disables msi and msix of a device while
> device is still active. This was intended to fix kexec with fusion devices but
> had the unintended effect of breaking even regular shutdown when using virtio.

Series:
Reviewed-by: Fam Zheng <famz@redhat.com>

> 
> The same problem would affect any driver which doesn't register
> a level interrupt handler when using msix.
> 
> I think the fix is to avoid touching device on shutdown:
> we clear bus master anyway, so we won't get any more
> msi interrupts, and bus reset will clear the msi/msix
> state eventually anyway.
> 
> The patches seems to all work well for me.  Given they affect all pci devices,
> and the bug has been there since 2.6 times, I think there's no rush: we can
> merge them for 4.1.
> 
> At the same time, once merged, they will likely make a good
> stable candidate.
> 
> Michael S. Tsirkin (4):
>   pci: disable msi/msix at probe time
>   pci: don't disable msi/msix at shutdown
>   pci: make msi/msix shutdown functions static
>   virtio_pci: drop msi_off on probe
> 
>  include/linux/pci.h                | 4 ----
>  drivers/pci/msi.c                  | 4 ++--
>  drivers/pci/pci-driver.c           | 8 ++++++--
>  drivers/virtio/virtio_pci_common.c | 3 ---
>  4 files changed, 8 insertions(+), 11 deletions(-)
> 
> -- 
> MST
> 

Re: [PATCH v2 1/4] pci: disable msi/msix at probe time

[Bjorn Helgaas]

Hi Michael,

On Thu, Mar 19, 2015 at 07:57:52PM +0100, Michael S. Tsirkin wrote:
> commit d52877c7b1afb8c37ebe17e2005040b79cb618b0
>     pci/irq: let pci_device_shutdown to call pci_msi_shutdown v2
> 
> attempted to address the problem of kexec getting
> started after linux enabled msi/msix for a device,
> and drivers being confused by msi being enabled,
> by disabling msi at shutdown.
> 
> But arguably, it's better to disable msi/msix when kexec
> starts - for example, kexec might run after a crash (kdump)
> and shutdown callbacks are not always invoked in that case.
> 
> Cc: Yinghai Lu <yhlu.kernel.send@gmail.com>
> Cc: Ulrich Obergfell <uobergfe@redhat.com>
> Cc: Fam Zheng <famz@redhat.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/pci/pci-driver.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 3cb2210..2ebd2a8 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -305,6 +305,12 @@ static long local_pci_probe(void *_ddi)
>  	 */
>  	pm_runtime_get_sync(dev);
>  	pci_dev->driver = pci_drv;
> +	/*
> +	 * When using kexec, msi might be left enabled by the previous kernel,
> +	 * this breaks things as some drivers assume msi/msi-x is off at boot.
> +	 * Fix this by forcing msi off at startup.
> +	 */
> +	pci_msi_off(pci_dev);

I think this makes sense, but I have a few questions.  This is a device
initialization thing, so it seems like a better fit for the enumeration
path, e.g,. pci_msi_init_pci_dev(), than for the driver binding path.

But when CONFIG_PCI_MSI=y, pci_msi_init_pci_dev() already does basically
the same thing, so we shouldn't need this change unless CONFIG_PCI_MSI is
not set in the kdump kernel.

If this is a problem just with kexeced kernels that don't have
CONFIG_PCI_MSI=y, I think I would prefer to fix this by moving
pci_msi_init_pci_dev() outside the #ifdef so it works regardless of
CONFIG_PCI_MSI.  That would also be nice because we could clean up the
duplication between pci_msi_off() and pci_msi_init_pci_dev().  It would
also make the starting machine state less dependent on the new kernel,
which seems like a good thing.

Are there any bugzillas we could reference here?

>  	rc = pci_drv->probe(pci_dev, ddi->id);
>  	if (!rc)
>  		return rc;
> -- 
> MST
> 

Re: [PATCH v2 1/4] pci: disable msi/msix at probe time

[Michael S. Tsirkin]

On Mon, Mar 23, 2015 at 01:50:06PM -0500, Bjorn Helgaas wrote:
> Hi Michael,
> 
> On Thu, Mar 19, 2015 at 07:57:52PM +0100, Michael S. Tsirkin wrote:
> > commit d52877c7b1afb8c37ebe17e2005040b79cb618b0
> >     pci/irq: let pci_device_shutdown to call pci_msi_shutdown v2
> > 
> > attempted to address the problem of kexec getting
> > started after linux enabled msi/msix for a device,
> > and drivers being confused by msi being enabled,
> > by disabling msi at shutdown.
> > 
> > But arguably, it's better to disable msi/msix when kexec
> > starts - for example, kexec might run after a crash (kdump)
> > and shutdown callbacks are not always invoked in that case.
> > 
> > Cc: Yinghai Lu <yhlu.kernel.send@gmail.com>
> > Cc: Ulrich Obergfell <uobergfe@redhat.com>
> > Cc: Fam Zheng <famz@redhat.com>
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  drivers/pci/pci-driver.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 3cb2210..2ebd2a8 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -305,6 +305,12 @@ static long local_pci_probe(void *_ddi)
> >  	 */
> >  	pm_runtime_get_sync(dev);
> >  	pci_dev->driver = pci_drv;
> > +	/*
> > +	 * When using kexec, msi might be left enabled by the previous kernel,
> > +	 * this breaks things as some drivers assume msi/msi-x is off at boot.
> > +	 * Fix this by forcing msi off at startup.
> > +	 */
> > +	pci_msi_off(pci_dev);
> 
> I think this makes sense, but I have a few questions.  This is a device
> initialization thing, so it seems like a better fit for the enumeration
> path, e.g,. pci_msi_init_pci_dev(), than for the driver binding path.
> 
> But when CONFIG_PCI_MSI=y, pci_msi_init_pci_dev() already does basically
> the same thing, so we shouldn't need this change unless CONFIG_PCI_MSI is
> not set in the kdump kernel.
> 
> If this is a problem just with kexeced kernels that don't have
> CONFIG_PCI_MSI=y, I think I would prefer to fix this by moving
> pci_msi_init_pci_dev() outside the #ifdef so it works regardless of
> CONFIG_PCI_MSI.  That would also be nice because we could clean up the
> duplication between pci_msi_off() and pci_msi_init_pci_dev().  It would
> also make the starting machine state less dependent on the new kernel,
> which seems like a good thing.

What you say above makes sense.
OK so the simplest fix is something like below then.
Fixes the duplication and kexec for CONFIG_PCI_MSI=n.
Acceptable? Pls let me know, if yes I'll test and
resubmit properly.

> Are there any bugzillas we could reference here?

I'll check this point. Maybe not - the real bugfix is
patch 2/4, this was just found by reading code,
but it's a dependency to make sure 2/4 does not
introduce regressions.

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 0e037af..2ab59d4 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1062,18 +1062,8 @@ EXPORT_SYMBOL(pci_msi_enabled);
 void pci_msi_init_pci_dev(struct pci_dev *dev)
 {
 	INIT_LIST_HEAD(&dev->msi_list);
-
-	/* Disable the msi hardware to avoid screaming interrupts
-	 * during boot.  This is the power on reset default so
-	 * usually this should be a noop.
-	 */
 	dev->msi_cap = pci_find_capability(dev, PCI_CAP_ID_MSI);
-	if (dev->msi_cap)
-		msi_set_enable(dev, 0);
-
 	dev->msix_cap = pci_find_capability(dev, PCI_CAP_ID_MSIX);
-	if (dev->msix_cap)
-		msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
 }
 
 /**
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8d2f400..c455501 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1485,6 +1485,12 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
 
 static void pci_init_capabilities(struct pci_dev *dev)
 {
+	/* Disable the msi hardware to avoid screaming interrupts
+	 * during boot.  This is the power on reset default so
+	 * usually this should be a noop.
+	 */
+	pci_msi_off(dev);
+
 	/* MSI/MSI-X list */
 	pci_msi_init_pci_dev(dev);
 

Re: [PATCH v2 1/4] pci: disable msi/msix at probe time

[Bjorn Helgaas]

On Mon, Mar 23, 2015 at 08:22:39PM +0100, Michael S. Tsirkin wrote:
> On Mon, Mar 23, 2015 at 01:50:06PM -0500, Bjorn Helgaas wrote:
> > Hi Michael,
> > 
> > On Thu, Mar 19, 2015 at 07:57:52PM +0100, Michael S. Tsirkin wrote:
> > > commit d52877c7b1afb8c37ebe17e2005040b79cb618b0
> > >     pci/irq: let pci_device_shutdown to call pci_msi_shutdown v2
> > > 
> > > attempted to address the problem of kexec getting
> > > started after linux enabled msi/msix for a device,
> > > and drivers being confused by msi being enabled,
> > > by disabling msi at shutdown.
> > > 
> > > But arguably, it's better to disable msi/msix when kexec
> > > starts - for example, kexec might run after a crash (kdump)
> > > and shutdown callbacks are not always invoked in that case.
> > > 
> > > Cc: Yinghai Lu <yhlu.kernel.send@gmail.com>
> > > Cc: Ulrich Obergfell <uobergfe@redhat.com>
> > > Cc: Fam Zheng <famz@redhat.com>
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  drivers/pci/pci-driver.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > index 3cb2210..2ebd2a8 100644
> > > --- a/drivers/pci/pci-driver.c
> > > +++ b/drivers/pci/pci-driver.c
> > > @@ -305,6 +305,12 @@ static long local_pci_probe(void *_ddi)
> > >  	 */
> > >  	pm_runtime_get_sync(dev);
> > >  	pci_dev->driver = pci_drv;
> > > +	/*
> > > +	 * When using kexec, msi might be left enabled by the previous kernel,
> > > +	 * this breaks things as some drivers assume msi/msi-x is off at boot.
> > > +	 * Fix this by forcing msi off at startup.
> > > +	 */
> > > +	pci_msi_off(pci_dev);
> > 
> > I think this makes sense, but I have a few questions.  This is a device
> > initialization thing, so it seems like a better fit for the enumeration
> > path, e.g,. pci_msi_init_pci_dev(), than for the driver binding path.
> > 
> > But when CONFIG_PCI_MSI=y, pci_msi_init_pci_dev() already does basically
> > the same thing, so we shouldn't need this change unless CONFIG_PCI_MSI is
> > not set in the kdump kernel.
> > 
> > If this is a problem just with kexeced kernels that don't have
> > CONFIG_PCI_MSI=y, I think I would prefer to fix this by moving
> > pci_msi_init_pci_dev() outside the #ifdef so it works regardless of
> > CONFIG_PCI_MSI.  That would also be nice because we could clean up the
> > duplication between pci_msi_off() and pci_msi_init_pci_dev().  It would
> > also make the starting machine state less dependent on the new kernel,
> > which seems like a good thing.
> 
> What you say above makes sense.
> OK so the simplest fix is something like below then.
> Fixes the duplication and kexec for CONFIG_PCI_MSI=n.
> Acceptable? Pls let me know, if yes I'll test and
> resubmit properly.
> 
> > Are there any bugzillas we could reference here?
> 
> I'll check this point. Maybe not - the real bugfix is
> patch 2/4, this was just found by reading code,
> but it's a dependency to make sure 2/4 does not
> introduce regressions.

OK, can you add the bugzilla link to that patch, if there is one?

> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 0e037af..2ab59d4 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1062,18 +1062,8 @@ EXPORT_SYMBOL(pci_msi_enabled);
>  void pci_msi_init_pci_dev(struct pci_dev *dev)
>  {
>  	INIT_LIST_HEAD(&dev->msi_list);
> -
> -	/* Disable the msi hardware to avoid screaming interrupts
> -	 * during boot.  This is the power on reset default so
> -	 * usually this should be a noop.
> -	 */
>  	dev->msi_cap = pci_find_capability(dev, PCI_CAP_ID_MSI);
> -	if (dev->msi_cap)
> -		msi_set_enable(dev, 0);
> -
>  	dev->msix_cap = pci_find_capability(dev, PCI_CAP_ID_MSIX);
> -	if (dev->msix_cap)
> -		msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
>  }
>  
>  /**
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8d2f400..c455501 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1485,6 +1485,12 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
>  
>  static void pci_init_capabilities(struct pci_dev *dev)
>  {
> +	/* Disable the msi hardware to avoid screaming interrupts
> +	 * during boot.  This is the power on reset default so
> +	 * usually this should be a noop.
> +	 */
> +	pci_msi_off(dev);
> +
>  	/* MSI/MSI-X list */
>  	pci_msi_init_pci_dev(dev);

Could we move pci_msi_init_pci_dev() from msi.c to pci.c and make it look
something like the following?

  void pci_msi_off(struct pci_dev *dev)
  {
    if (dev->msi_cap) {
      ...
    }
    if (dev->msix_cap) {
      ...
    }
  }

  void pci_msi_init_pci_dev(struct pci_dev *dev)
  {
  #ifdef CONFIG_PCI_MSI
    INIT_LIST_HEAD(&dev->msi_list);
  #endif

    dev->msi_cap = pci_find_capability(dev, PCI_CAP_ID_MSI);
    dev->msix_cap = pci_find_capability(dev, PCI_CAP_ID_MSIX);
    pci_msi_off(dev);
  }

Then I think we could remove pci_msi_off() calls from a couple quirks as
well.  And we'd only have one MSI-related callout from
pci_init_capabilities().

Bjorn

Re: [PATCH v2 1/4] pci: disable msi/msix at probe time

[Michael S. Tsirkin]

On Mon, Mar 23, 2015 at 03:45:34PM -0500, Bjorn Helgaas wrote:
> On Mon, Mar 23, 2015 at 08:22:39PM +0100, Michael S. Tsirkin wrote:
> > On Mon, Mar 23, 2015 at 01:50:06PM -0500, Bjorn Helgaas wrote:
> > > Hi Michael,
> > > 
> > > On Thu, Mar 19, 2015 at 07:57:52PM +0100, Michael S. Tsirkin wrote:
> > > > commit d52877c7b1afb8c37ebe17e2005040b79cb618b0
> > > >     pci/irq: let pci_device_shutdown to call pci_msi_shutdown v2
> > > > 
> > > > attempted to address the problem of kexec getting
> > > > started after linux enabled msi/msix for a device,
> > > > and drivers being confused by msi being enabled,
> > > > by disabling msi at shutdown.
> > > > 
> > > > But arguably, it's better to disable msi/msix when kexec
> > > > starts - for example, kexec might run after a crash (kdump)
> > > > and shutdown callbacks are not always invoked in that case.
> > > > 
> > > > Cc: Yinghai Lu <yhlu.kernel.send@gmail.com>
> > > > Cc: Ulrich Obergfell <uobergfe@redhat.com>
> > > > Cc: Fam Zheng <famz@redhat.com>
> > > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  drivers/pci/pci-driver.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > > index 3cb2210..2ebd2a8 100644
> > > > --- a/drivers/pci/pci-driver.c
> > > > +++ b/drivers/pci/pci-driver.c
> > > > @@ -305,6 +305,12 @@ static long local_pci_probe(void *_ddi)
> > > >  	 */
> > > >  	pm_runtime_get_sync(dev);
> > > >  	pci_dev->driver = pci_drv;
> > > > +	/*
> > > > +	 * When using kexec, msi might be left enabled by the previous kernel,
> > > > +	 * this breaks things as some drivers assume msi/msi-x is off at boot.
> > > > +	 * Fix this by forcing msi off at startup.
> > > > +	 */
> > > > +	pci_msi_off(pci_dev);
> > > 
> > > I think this makes sense, but I have a few questions.  This is a device
> > > initialization thing, so it seems like a better fit for the enumeration
> > > path, e.g,. pci_msi_init_pci_dev(), than for the driver binding path.
> > > 
> > > But when CONFIG_PCI_MSI=y, pci_msi_init_pci_dev() already does basically
> > > the same thing, so we shouldn't need this change unless CONFIG_PCI_MSI is
> > > not set in the kdump kernel.
> > > 
> > > If this is a problem just with kexeced kernels that don't have
> > > CONFIG_PCI_MSI=y, I think I would prefer to fix this by moving
> > > pci_msi_init_pci_dev() outside the #ifdef so it works regardless of
> > > CONFIG_PCI_MSI.  That would also be nice because we could clean up the
> > > duplication between pci_msi_off() and pci_msi_init_pci_dev().  It would
> > > also make the starting machine state less dependent on the new kernel,
> > > which seems like a good thing.
> > 
> > What you say above makes sense.
> > OK so the simplest fix is something like below then.
> > Fixes the duplication and kexec for CONFIG_PCI_MSI=n.
> > Acceptable? Pls let me know, if yes I'll test and
> > resubmit properly.
> > 
> > > Are there any bugzillas we could reference here?
> > 
> > I'll check this point. Maybe not - the real bugfix is
> > patch 2/4, this was just found by reading code,
> > but it's a dependency to make sure 2/4 does not
> > introduce regressions.
> 
> OK, can you add the bugzilla link to that patch, if there is one?
> 
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index 0e037af..2ab59d4 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -1062,18 +1062,8 @@ EXPORT_SYMBOL(pci_msi_enabled);
> >  void pci_msi_init_pci_dev(struct pci_dev *dev)
> >  {
> >  	INIT_LIST_HEAD(&dev->msi_list);
> > -
> > -	/* Disable the msi hardware to avoid screaming interrupts
> > -	 * during boot.  This is the power on reset default so
> > -	 * usually this should be a noop.
> > -	 */
> >  	dev->msi_cap = pci_find_capability(dev, PCI_CAP_ID_MSI);
> > -	if (dev->msi_cap)
> > -		msi_set_enable(dev, 0);
> > -
> >  	dev->msix_cap = pci_find_capability(dev, PCI_CAP_ID_MSIX);
> > -	if (dev->msix_cap)
> > -		msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
> >  }
> >  
> >  /**
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 8d2f400..c455501 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1485,6 +1485,12 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
> >  
> >  static void pci_init_capabilities(struct pci_dev *dev)
> >  {
> > +	/* Disable the msi hardware to avoid screaming interrupts
> > +	 * during boot.  This is the power on reset default so
> > +	 * usually this should be a noop.
> > +	 */
> > +	pci_msi_off(dev);
> > +
> >  	/* MSI/MSI-X list */
> >  	pci_msi_init_pci_dev(dev);
> 
> Could we move pci_msi_init_pci_dev() from msi.c to pci.c and make it look
> something like the following?
> 
>   void pci_msi_off(struct pci_dev *dev)
>   {
>     if (dev->msi_cap) {
>       ...
>     }
>     if (dev->msix_cap) {
>       ...
>     }
>   }
> 
>   void pci_msi_init_pci_dev(struct pci_dev *dev)
>   {
>   #ifdef CONFIG_PCI_MSI
>     INIT_LIST_HEAD(&dev->msi_list);
>   #endif
> 
>     dev->msi_cap = pci_find_capability(dev, PCI_CAP_ID_MSI);
>     dev->msix_cap = pci_find_capability(dev, PCI_CAP_ID_MSIX);
>     pci_msi_off(dev);
>   }
> 
> Then I think we could remove pci_msi_off() calls from a couple quirks as
> well.  And we'd only have one MSI-related callout from
> pci_init_capabilities().
> 
> Bjorn

OK I was under the impression msi_cap/msix_cap aren't there
when CONFIG_PCI_MSI is not set, but I checked and they
actually are, so yes, will do.