[PATCH] pci: let pci_device_shutdown to call pci_disable_msi

[PATCH] pci: let pci_device_shutdown to call pci_disable_msi

[Yinghai Lu]

this change

| commit 23a274c8a5adafc74a66f16988776fc7dd6f6e51
| Author: Prakash, Sathya <sathya.prakash@lsi.com>
| Date:   Fri Mar 7 15:53:21 2008 +0530
|
|     [SCSI] mpt fusion: Enable MSI by default for SAS controllers
|
|     This patch modifies the driver to enable MSI by default for all SAS chips.
|
|     Signed-off-by: Sathya Prakash <sathya.prakash@lsi.com>
|     Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
|
cause kexec RHEL 5.1 kernel fail.

root casue: the rhel 5.1 kernel still use INTx emulation.
and mptscsih_shutdown doesn't call pci_disable_msi to reenable INTx on kexec path

so try to call pci_disable_msi in shutdown patch

Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>

Index: linux-2.6/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci-driver.c
+++ linux-2.6/drivers/pci/pci-driver.c
@@ -360,6 +360,8 @@ static void pci_device_shutdown(struct d
 
 	if (drv && drv->shutdown)
 		drv->shutdown(pci_dev);
+	pci_disable_msi_simple(pci_dev);
+	pci_disable_msix_simple(pci_dev);
 }
 
 /**
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -722,9 +722,11 @@ static inline void pci_restore_msi_state
 { }
 #else
 extern int pci_enable_msi(struct pci_dev *dev);
+extern void pci_disable_msi_simple(struct pci_dev *dev);
 extern void pci_disable_msi(struct pci_dev *dev);
 extern int pci_enable_msix(struct pci_dev *dev,
 	struct msix_entry *entries, int nvec);
+extern void pci_disable_msix_simple(struct pci_dev *dev);
 extern void pci_disable_msix(struct pci_dev *dev);
 extern void msi_remove_pci_irq_vectors(struct pci_dev *dev);
 extern void pci_restore_msi_state(struct pci_dev *dev);
Index: linux-2.6/drivers/pci/msi.c
===================================================================
--- linux-2.6.orig/drivers/pci/msi.c
+++ linux-2.6/drivers/pci/msi.c
@@ -569,7 +569,7 @@ int pci_enable_msi(struct pci_dev* dev)
 }
 EXPORT_SYMBOL(pci_enable_msi);
 
-void pci_disable_msi(struct pci_dev* dev)
+static void __pci_disable_msi(struct pci_dev* dev, int msi_free)
 {
 	struct msi_desc *entry;
 	int default_irq;
@@ -591,11 +591,20 @@ void pci_disable_msi(struct pci_dev* dev
 	}
 
 	default_irq = entry->msi_attrib.default_irq;
-	msi_free_irqs(dev);
+	if (msi_free)
+		msi_free_irqs(dev);
 
 	/* Restore dev->irq to its default pin-assertion irq */
 	dev->irq = default_irq;
 }
+void pci_disable_msi_simple(struct pci_dev* dev)
+{
+	__pci_disable_msi(dev, 0);
+}
+void pci_disable_msi(struct pci_dev* dev)
+{
+	__pci_disable_msi(dev, 1);
+}
 EXPORT_SYMBOL(pci_disable_msi);
 
 static int msi_free_irqs(struct pci_dev* dev)
@@ -687,7 +696,7 @@ static void msix_free_all_irqs(struct pc
 	msi_free_irqs(dev);
 }
 
-void pci_disable_msix(struct pci_dev* dev)
+static void __pci_disable_msix(struct pci_dev* dev, int msix_free)
 {
 	if (!pci_msi_enable || !dev || !dev->msix_enabled)
 		return;
@@ -696,7 +705,16 @@ void pci_disable_msix(struct pci_dev* de
 	pci_intx_for_msi(dev, 1);
 	dev->msix_enabled = 0;
 
-	msix_free_all_irqs(dev);
+	if (msix_free)
+		msix_free_all_irqs(dev);
+}
+void pci_disable_msix_simple(struct pci_dev* dev)
+{
+	__pci_disable_msix(dev, 0);
+}
+void pci_disable_msix(struct pci_dev* dev)
+{
+	__pci_disable_msix(dev, 1);
 }
 EXPORT_SYMBOL(pci_disable_msix);
 

Re: [PATCH] pci: let pci_device_shutdown to call pci_disable_msi

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

On Tue, 2008-04-22 at 21:48 -0700, Yinghai Lu wrote:
> this change
> 
> | commit 23a274c8a5adafc74a66f16988776fc7dd6f6e51
> | Author: Prakash, Sathya <sathya.prakash@lsi.com>
> | Date:   Fri Mar 7 15:53:21 2008 +0530
> |
> |     [SCSI] mpt fusion: Enable MSI by default for SAS controllers
> |
> |     This patch modifies the driver to enable MSI by default for all SAS chips.
> |
> |     Signed-off-by: Sathya Prakash <sathya.prakash@lsi.com>
> |     Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> |
> cause kexec RHEL 5.1 kernel fail.
> 
> root casue: the rhel 5.1 kernel still use INTx emulation.
> and mptscsih_shutdown doesn't call pci_disable_msi to reenable INTx on kexec path
> 
> so try to call pci_disable_msi in shutdown patch

How is kdump going to work? Your shutdown routine won't be called and
you'll have the same problem in the 2nd kernel, won't you?

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] pci: let pci_device_shutdown to call pci_disable_msi

[Yinghai Lu]

On Tue, Apr 22, 2008 at 10:24 PM, Michael Ellerman
<michael@ellerman.id.au> wrote:
> On Tue, 2008-04-22 at 21:48 -0700, Yinghai Lu wrote:
>  > this change
>  >
>  > | commit 23a274c8a5adafc74a66f16988776fc7dd6f6e51
>  > | Author: Prakash, Sathya <sathya.prakash@lsi.com>
>  > | Date:   Fri Mar 7 15:53:21 2008 +0530
>  > |
>  > |     [SCSI] mpt fusion: Enable MSI by default for SAS controllers
>  > |
>  > |     This patch modifies the driver to enable MSI by default for all SAS chips.
>  > |
>  > |     Signed-off-by: Sathya Prakash <sathya.prakash@lsi.com>
>  > |     Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>  > |
>  > cause kexec RHEL 5.1 kernel fail.
>  >
>  > root casue: the rhel 5.1 kernel still use INTx emulation.
>  > and mptscsih_shutdown doesn't call pci_disable_msi to reenable INTx on kexec path
>  >
>  > so try to call pci_disable_msi in shutdown patch
>
>  How is kdump going to work? Your shutdown routine won't be called and
>  you'll have the same problem in the 2nd kernel, won't you?

for kdump, the first kernel and second kernel should be same version.

this one is for using kexec to load other kernel that may not have msi
support etc.

YH

Re: [PATCH] pci: let pci_device_shutdown to call pci_disable_msi

[Eric W. Biederman]

Yinghai Lu <yhlu.kernel.send@gmail.com> writes:

> this change
>
> | commit 23a274c8a5adafc74a66f16988776fc7dd6f6e51
> | Author: Prakash, Sathya <sathya.prakash@lsi.com>
> | Date:   Fri Mar 7 15:53:21 2008 +0530
> |
> |     [SCSI] mpt fusion: Enable MSI by default for SAS controllers
> |
> | This patch modifies the driver to enable MSI by default for all SAS chips.
> |
> |     Signed-off-by: Sathya Prakash <sathya.prakash@lsi.com>
> |     Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> |
> cause kexec RHEL 5.1 kernel fail.
>
> root casue: the rhel 5.1 kernel still use INTx emulation.
> and mptscsih_shutdown doesn't call pci_disable_msi to reenable INTx on kexec
> path
>
> so try to call pci_disable_msi in shutdown patch

Ok this looks like a reasonable approach.

Could you please change how this is factored.
And implement a pci_shutdown_msi and a pci_shutdown_msix that
just performs the hardware state change.

Then have pci_disable_msi and pci_disable_msix call them?

That should be much easier to maintain then a adding a function
that takes a magic flag. 

That is the design of the shutdown interface and it seems to
work well.

Eric

Re: [PATCH] pci: let pci_device_shutdown to call pci_disable_msi

[Eric W. Biederman]

Michael Ellerman <michael@ellerman.id.au> writes:

> On Tue, 2008-04-22 at 21:48 -0700, Yinghai Lu wrote:
>> this change
>> 
>> | commit 23a274c8a5adafc74a66f16988776fc7dd6f6e51
>> | Author: Prakash, Sathya <sathya.prakash@lsi.com>
>> | Date:   Fri Mar 7 15:53:21 2008 +0530
>> |
>> |     [SCSI] mpt fusion: Enable MSI by default for SAS controllers
>> |
>> | This patch modifies the driver to enable MSI by default for all SAS chips.
>> |
>> |     Signed-off-by: Sathya Prakash <sathya.prakash@lsi.com>
>> |     Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>> |
>> cause kexec RHEL 5.1 kernel fail.
>> 
>> root casue: the rhel 5.1 kernel still use INTx emulation.
>> and mptscsih_shutdown doesn't call pci_disable_msi to reenable INTx on kexec
> path
>> 
>> so try to call pci_disable_msi in shutdown patch
>
> How is kdump going to work? Your shutdown routine won't be called and
> you'll have the same problem in the 2nd kernel, won't you?

Taking a quick look our current msi initialization appears robust in
not assuming the state of the msi config bits.

So the only remaining problem is running older software that 
assumes the msi config bits are in the state they should be in
out of reset.

YH on that score it appears I goofed a little when I gave you
my suggestion on how to fix this in pci_disable_msi.

If we have crazy hardware that supports multi irqs in with
a plain msi capability.  During initialization we mask
all of the irqs.

from msi_capability_init:
        if (entry->msi_attrib.maskbit) {
                unsigned int maskbits, temp;
                /* All MSIs are unmasked by default, Mask them all */
                pci_read_config_dword(dev,
                        msi_mask_bits_reg(pos, is_64bit_address(control)),
                        &maskbits);
                temp = (1 << multi_msi_capable(control));
                temp = ((temp - 1) & ~temp);
                maskbits |= temp;
                pci_write_config_dword(dev,
                        msi_mask_bits_reg(pos, is_64bit_address(control)),
                        maskbits);
        }

So it appears to truly return to the reset state we should unmask
them all, instead of just that one.  Not that it matters in practice,
but handling that corner case would be polite.

Eric

Re: [PATCH] pci: let pci_device_shutdown to call pci_disable_msi

[Yinghai Lu]

On Wed, Apr 23, 2008 at 6:08 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Michael Ellerman <michael@ellerman.id.au> writes:
>
>  > On Tue, 2008-04-22 at 21:48 -0700, Yinghai Lu wrote:
>  >> this change
>  >>
>  >> | commit 23a274c8a5adafc74a66f16988776fc7dd6f6e51
>  >> | Author: Prakash, Sathya <sathya.prakash@lsi.com>
>  >> | Date:   Fri Mar 7 15:53:21 2008 +0530
>  >> |
>  >> |     [SCSI] mpt fusion: Enable MSI by default for SAS controllers
>  >> |
>  >> | This patch modifies the driver to enable MSI by default for all SAS chips.
>  >> |
>  >> |     Signed-off-by: Sathya Prakash <sathya.prakash@lsi.com>
>  >> |     Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>  >> |
>  >> cause kexec RHEL 5.1 kernel fail.
>  >>
>  >> root casue: the rhel 5.1 kernel still use INTx emulation.
>  >> and mptscsih_shutdown doesn't call pci_disable_msi to reenable INTx on kexec
>  > path
>  >>
>  >> so try to call pci_disable_msi in shutdown patch
>  >
>  > How is kdump going to work? Your shutdown routine won't be called and
>  > you'll have the same problem in the 2nd kernel, won't you?
>
>  Taking a quick look our current msi initialization appears robust in
>  not assuming the state of the msi config bits.
>
>  So the only remaining problem is running older software that
>  assumes the msi config bits are in the state they should be in
>  out of reset.
>
>  YH on that score it appears I goofed a little when I gave you
>  my suggestion on how to fix this in pci_disable_msi.
>
>  If we have crazy hardware that supports multi irqs in with
>  a plain msi capability.  During initialization we mask
>  all of the irqs.
>
>  from msi_capability_init:
>         if (entry->msi_attrib.maskbit) {
>                 unsigned int maskbits, temp;
>                 /* All MSIs are unmasked by default, Mask them all */
>                 pci_read_config_dword(dev,
>                         msi_mask_bits_reg(pos, is_64bit_address(control)),
>                         &maskbits);
>                 temp = (1 << multi_msi_capable(control));
>                 temp = ((temp - 1) & ~temp);
>                 maskbits |= temp;
>                 pci_write_config_dword(dev,
>                         msi_mask_bits_reg(pos, is_64bit_address(control)),
>                         maskbits);
>         }
>
>  So it appears to truly return to the reset state we should unmask
>  them all, instead of just that one.  Not that it matters in practice,
>  but handling that corner case would be polite.

will extend that a little bit.

YH

Re: [PATCH] pci: let pci_device_shutdown to call pci_disable_msi

[Yinghai Lu]

On Wed, Apr 23, 2008 at 5:57 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Yinghai Lu <yhlu.kernel.send@gmail.com> writes:
>
>  > this change
>  >
>  > | commit 23a274c8a5adafc74a66f16988776fc7dd6f6e51
>  > | Author: Prakash, Sathya <sathya.prakash@lsi.com>
>  > | Date:   Fri Mar 7 15:53:21 2008 +0530
>  > |
>  > |     [SCSI] mpt fusion: Enable MSI by default for SAS controllers
>  > |
>  > | This patch modifies the driver to enable MSI by default for all SAS chips.
>  > |
>  > |     Signed-off-by: Sathya Prakash <sathya.prakash@lsi.com>
>  > |     Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>  > |
>  > cause kexec RHEL 5.1 kernel fail.
>  >
>  > root casue: the rhel 5.1 kernel still use INTx emulation.
>  > and mptscsih_shutdown doesn't call pci_disable_msi to reenable INTx on kexec
>  > path
>  >
>  > so try to call pci_disable_msi in shutdown patch
>
>  Ok this looks like a reasonable approach.
>
>  Could you please change how this is factored.
>  And implement a pci_shutdown_msi and a pci_shutdown_msix that
>  just performs the hardware state change.
>
>  Then have pci_disable_msi and pci_disable_msix call them?
>
>  That should be much easier to maintain then a adding a function
>  that takes a magic flag.
>
>  That is the design of the shutdown interface and it seems to
>  work well.

will check that.

YH

[PATCH 1/2] pci/irq: restore mask_bits in msi shutdown -v3

[Yinghai Lu]

Yinghai found after using 2.6.25-rc3 later to kexec RHEL 5.1,
NIC can not be used.

bisected to

| commit 89d694b9dbe769ca1004e01db0ca43964806a611
| Author: Thomas Gleixner <tglx@linutronix.de>
| Date:   Mon Feb 18 18:25:17 2008 +0100
|
|   genirq: do not leave interupts enabled on free_irq
|
|   The default_disable() function was changed in commit:
|
|    76d2160147f43f982dfe881404cfde9fd0a9da21
|    genirq: do not mask interrupts by default
|
|   It removed the mask function in favour of the default delayed
|   interrupt disabling. Unfortunately this also broke the shutdown in
|   free_irq() when the last handler is removed from the interrupt for
|   those architectures which rely on the default implementations. Now we
|   can end up with a enabled interrupt line after the last handler was
|   removed, which can result in spurious interrupts.
|
|   Fix this by adding a default_shutdown function, which is only
|   installed, when the irqchip implementation does provide neither a
|   shutdown nor a disable function.
|
|   [@stable: affected versions: .21 - .24 ]

for MSI, default_shutdown will call mask_bit for msi device.  so all mask bits
will left disabled after free_irq.  then if kexec next kernel that only can
use msi_enable bit.  all device's MSI can not be used.

want to try to restore MSI mask bits that is saved before using msi in first
kernel.

Eric said:
This is over complicated and for hardware that erroneously triggers
a msi irq after free_irq may have potential problems.

So lets do the much simpler, much safer, and more general method of
restoring the mask bit to it's pci reset defined value (enabled) when
we disable the kernels use of msi.

it will work, because pci_diable_msi is called after free_irq is called.

v3: extend msi_set_mask_bit to msi_set_mask_bits to take mask, so we can fully
restore that to 0x00 instead of 0xfe.

Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>

Index: linux-2.6/drivers/pci/msi.c
===================================================================
--- linux-2.6.orig/drivers/pci/msi.c
+++ linux-2.6/drivers/pci/msi.c
@@ -123,7 +123,7 @@ static void msix_flush_writes(unsigned i
 	}
 }
 
-static void msi_set_mask_bit(unsigned int irq, int flag)
+static void msi_set_mask_bits(unsigned int irq, u32 mask, u32 flag)
 {
 	struct msi_desc *entry;
 
@@ -137,8 +137,8 @@ static void msi_set_mask_bit(unsigned in
 
 			pos = (long)entry->mask_base;
 			pci_read_config_dword(entry->dev, pos, &mask_bits);
-			mask_bits &= ~(1);
-			mask_bits |= flag;
+			mask_bits &= ~(mask);
+			mask_bits |= flag & mask;
 			pci_write_config_dword(entry->dev, pos, mask_bits);
 		} else {
 			msi_set_enable(entry->dev, !flag);
@@ -241,13 +241,13 @@ void write_msi_msg(unsigned int irq, str
 
 void mask_msi_irq(unsigned int irq)
 {
-	msi_set_mask_bit(irq, 1);
+	msi_set_mask_bits(irq, 1, 1);
 	msix_flush_writes(irq);
 }
 
 void unmask_msi_irq(unsigned int irq)
 {
-	msi_set_mask_bit(irq, 0);
+	msi_set_mask_bits(irq, 1, 0);
 	msix_flush_writes(irq);
 }
 
@@ -291,7 +291,8 @@ static void __pci_restore_msi_state(stru
 	msi_set_enable(dev, 0);
 	write_msi_msg(dev->irq, &entry->msg);
 	if (entry->msi_attrib.maskbit)
-		msi_set_mask_bit(dev->irq, entry->msi_attrib.masked);
+		msi_set_mask_bits(dev->irq, entry->msi_attrib.maskbits_mask,
+				  entry->msi_attrib.masked);
 
 	pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &control);
 	control &= ~(PCI_MSI_FLAGS_QSIZE | PCI_MSI_FLAGS_ENABLE);
@@ -315,7 +316,7 @@ static void __pci_restore_msix_state(str
 
 	list_for_each_entry(entry, &dev->msi_list, list) {
 		write_msi_msg(entry->irq, &entry->msg);
-		msi_set_mask_bit(entry->irq, entry->msi_attrib.masked);
+		msi_set_mask_bits(entry->irq, 1, entry->msi_attrib.masked);
 	}
 
 	BUG_ON(list_empty(&dev->msi_list));
@@ -382,6 +383,7 @@ static int msi_capability_init(struct pc
 		pci_write_config_dword(dev,
 			msi_mask_bits_reg(pos, is_64bit_address(control)),
 			maskbits);
+		entry->msi_attrib.maskbits_mask = temp;
 	}
 	list_add_tail(&entry->list, &dev->msi_list);
 
@@ -583,6 +585,11 @@ void pci_disable_msi(struct pci_dev* dev
 
 	BUG_ON(list_empty(&dev->msi_list));
 	entry = list_entry(dev->msi_list.next, struct msi_desc, list);
+	/* Return the the pci reset with msi irqs unmasked */
+	if (entry->msi_attrib.maskbit) {
+		u32 mask = entry->msi_attrib.maskbits_mask;
+		msi_set_mask_bits(dev->irq, mask, ~mask);
+	}
 	if (!entry->dev || entry->msi_attrib.type != PCI_CAP_ID_MSI) {
 		return;
 	}
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 94bb46d..8f29392 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -22,6 +22,7 @@ struct msi_desc {
 		__u8	masked	: 1;
 		__u8	is_64	: 1;	/* Address size: 0=32bit 1=64bit  */
 		__u8	pos;	 	/* Location of the msi capability */
+		__u32	maskbits_mask;  /* mask bits mask */
 		__u16	entry_nr;    	/* specific enabled entry 	  */
 		unsigned default_irq;	/* default pre-assigned irq	  */
 	}msi_attrib;

[PATCH 2/2] pci/irq: let pci_device_shutdown to call pci_msi_shutdown v2

[Yinghai Lu]

[PATCH 2/2] pci/irq: let pci_device_shutdown to call pci_msi_shutdown v2

this change

| commit 23a274c8a5adafc74a66f16988776fc7dd6f6e51
| Author: Prakash, Sathya <sathya.prakash@lsi.com>
| Date:   Fri Mar 7 15:53:21 2008 +0530
|
|     [SCSI] mpt fusion: Enable MSI by default for SAS controllers
|
|     This patch modifies the driver to enable MSI by default for all SAS chips.
|
|     Signed-off-by: Sathya Prakash <sathya.prakash@lsi.com>
|     Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
|
cause kexec RHEL 5.1 kernel fail.

root casue: the rhel 5.1 kernel still use INTx emulation.
and mptscsih_shutdown doesn't call pci_disable_msi to reenable INTx on kexec path

so try to call pci_msi_shutdown in shutdown patch
do the same thing to msix

Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>

Index: linux-2.6/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci-driver.c
+++ linux-2.6/drivers/pci/pci-driver.c
@@ -360,6 +360,8 @@ static void pci_device_shutdown(struct d
 
 	if (drv && drv->shutdown)
 		drv->shutdown(pci_dev);
+	pci_msi_shutdown(pci_dev);
+	pci_msix_shutdown(pci_dev);
 }
 
 /**
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -703,6 +703,8 @@ static inline int pci_enable_msi(struct 
 	return -1;
 }
 
+static inline void pci_msi_shutdown(struct pci_dev *dev)
+{ }
 static inline void pci_disable_msi(struct pci_dev *dev)
 { }
 
@@ -712,6 +714,8 @@ static inline int pci_enable_msix(struct
 	return -1;
 }
 
+static inline void pci_msix_shutdown(struct pci_dev *dev)
+{ }
 static inline void pci_disable_msix(struct pci_dev *dev)
 { }
 
@@ -722,9 +726,11 @@ static inline void pci_restore_msi_state
 { }
 #else
 extern int pci_enable_msi(struct pci_dev *dev);
+extern void pci_msi_shutdown(struct pci_dev *dev);
 extern void pci_disable_msi(struct pci_dev *dev);
 extern int pci_enable_msix(struct pci_dev *dev,
 	struct msix_entry *entries, int nvec);
+extern void pci_msix_shutdown(struct pci_dev *dev);
 extern void pci_disable_msix(struct pci_dev *dev);
 extern void msi_remove_pci_irq_vectors(struct pci_dev *dev);
 extern void pci_restore_msi_state(struct pci_dev *dev);
Index: linux-2.6/drivers/pci/msi.c
===================================================================
--- linux-2.6.orig/drivers/pci/msi.c
+++ linux-2.6/drivers/pci/msi.c
@@ -571,10 +571,9 @@ int pci_enable_msi(struct pci_dev* dev)
 }
 EXPORT_SYMBOL(pci_enable_msi);
 
-void pci_disable_msi(struct pci_dev* dev)
+void pci_msi_shutdown(struct pci_dev* dev)
 {
 	struct msi_desc *entry;
-	int default_irq;
 
 	if (!pci_msi_enable || !dev || !dev->msi_enabled)
 		return;
@@ -590,15 +589,26 @@ void pci_disable_msi(struct pci_dev* dev
 		u32 mask = entry->msi_attrib.maskbits_mask;
 		msi_set_mask_bits(dev->irq, mask, ~mask);
 	}
-	if (!entry->dev || entry->msi_attrib.type != PCI_CAP_ID_MSI) {
+	if (!entry->dev || entry->msi_attrib.type != PCI_CAP_ID_MSI)
 		return;
-	}
-
-	default_irq = entry->msi_attrib.default_irq;
-	msi_free_irqs(dev);
 
 	/* Restore dev->irq to its default pin-assertion irq */
-	dev->irq = default_irq;
+	dev->irq = entry->msi_attrib.default_irq;
+}
+void pci_disable_msi(struct pci_dev* dev)
+{
+	struct msi_desc *entry;
+
+	if (!pci_msi_enable || !dev || !dev->msi_enabled)
+		return;
+
+	pci_msi_shutdown(dev);
+
+	entry = list_entry(dev->msi_list.next, struct msi_desc, list);
+	if (!entry->dev || entry->msi_attrib.type != PCI_CAP_ID_MSI)
+		return;
+
+	msi_free_irqs(dev);
 }
 EXPORT_SYMBOL(pci_disable_msi);
 
@@ -691,7 +701,7 @@ static void msix_free_all_irqs(struct pc
 	msi_free_irqs(dev);
 }
 
-void pci_disable_msix(struct pci_dev* dev)
+void pci_msix_shutdown(struct pci_dev* dev)
 {
 	if (!pci_msi_enable || !dev || !dev->msix_enabled)
 		return;
@@ -699,6 +709,13 @@ void pci_disable_msix(struct pci_dev* de
 	msix_set_enable(dev, 0);
 	pci_intx_for_msi(dev, 1);
 	dev->msix_enabled = 0;
+}
+void pci_disable_msix(struct pci_dev* dev)
+{
+	if (!pci_msi_enable || !dev || !dev->msix_enabled)
+		return;
+
+	pci_msix_shutdown(dev);
 
 	msix_free_all_irqs(dev);
 }

Re: [PATCH] pci: let pci_device_shutdown to call pci_disable_msi

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1696 bytes --]

On Wed, 2008-04-23 at 06:08 -0700, Eric W. Biederman wrote:
> Michael Ellerman <michael@ellerman.id.au> writes:
> 
> > On Tue, 2008-04-22 at 21:48 -0700, Yinghai Lu wrote:
> >> this change
> >> 
> >> | commit 23a274c8a5adafc74a66f16988776fc7dd6f6e51
> >> | Author: Prakash, Sathya <sathya.prakash@lsi.com>
> >> | Date:   Fri Mar 7 15:53:21 2008 +0530
> >> |
> >> |     [SCSI] mpt fusion: Enable MSI by default for SAS controllers
> >> |
> >> | This patch modifies the driver to enable MSI by default for all SAS chips.
> >> |
> >> |     Signed-off-by: Sathya Prakash <sathya.prakash@lsi.com>
> >> |     Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> >> |
> >> cause kexec RHEL 5.1 kernel fail.
> >> 
> >> root casue: the rhel 5.1 kernel still use INTx emulation.
> >> and mptscsih_shutdown doesn't call pci_disable_msi to reenable INTx on kexec
> > path
> >> 
> >> so try to call pci_disable_msi in shutdown patch
> >
> > How is kdump going to work? Your shutdown routine won't be called and
> > you'll have the same problem in the 2nd kernel, won't you?
> 
> Taking a quick look our current msi initialization appears robust in
> not assuming the state of the msi config bits.

But does that help us? What if the device driver in the 2nd kernel
assumes it's using INTX, when in fact MSI is enabled on the device. In
that case none of the MSI code will even be called AFAIK.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] pci: let pci_device_shutdown to call pci_disable_msi

[Yinghai Lu]

On Wed, Apr 23, 2008 at 5:17 PM, Michael Ellerman
<michael@ellerman.id.au> wrote:
> On Wed, 2008-04-23 at 06:08 -0700, Eric W. Biederman wrote:
>  > Michael Ellerman <michael@ellerman.id.au> writes:
>  >
>  > > On Tue, 2008-04-22 at 21:48 -0700, Yinghai Lu wrote:
>  > >> this change
>  > >>
>  > >> | commit 23a274c8a5adafc74a66f16988776fc7dd6f6e51
>  > >> | Author: Prakash, Sathya <sathya.prakash@lsi.com>
>  > >> | Date:   Fri Mar 7 15:53:21 2008 +0530
>  > >> |
>  > >> |     [SCSI] mpt fusion: Enable MSI by default for SAS controllers
>  > >> |
>  > >> | This patch modifies the driver to enable MSI by default for all SAS chips.
>  > >> |
>  > >> |     Signed-off-by: Sathya Prakash <sathya.prakash@lsi.com>
>  > >> |     Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>  > >> |
>  > >> cause kexec RHEL 5.1 kernel fail.
>  > >>
>  > >> root casue: the rhel 5.1 kernel still use INTx emulation.
>  > >> and mptscsih_shutdown doesn't call pci_disable_msi to reenable INTx on kexec
>  > > path
>  > >>
>  > >> so try to call pci_disable_msi in shutdown patch
>  > >
>  > > How is kdump going to work? Your shutdown routine won't be called and
>  > > you'll have the same problem in the 2nd kernel, won't you?
>  >
>  > Taking a quick look our current msi initialization appears robust in
>  > not assuming the state of the msi config bits.
>
>  But does that help us? What if the device driver in the 2nd kernel
>  assumes it's using INTX, when in fact MSI is enabled on the device. In
>  that case none of the MSI code will even be called AFAIK.

ok. for kdump case, if driver can not use msi (?), with pci=nomsi
we need to call pci_intx_for_msi(dev, 1) at beginning of second kernel.

will produce another patch about that case.

YH

Re: [PATCH] pci: let pci_device_shutdown to call pci_disable_msi

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On Wed, Apr 23, 2008 at 6:22 PM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>
> On Wed, Apr 23, 2008 at 5:17 PM, Michael Ellerman
>  <michael@ellerman.id.au> wrote:
>  >  But does that help us? What if the device driver in the 2nd kernel
>  >  assumes it's using INTX, when in fact MSI is enabled on the device. In
>  >  that case none of the MSI code will even be called AFAIK.
>
>  ok. for kdump case, if driver can not use msi (?), with pci=nomsi
>  we need to call pci_intx_for_msi(dev, 1) at beginning of second kernel.
>
>  will produce another patch about that case.

attached kernel should fix second kernel by kdump doesn't have msi
support problem.

anyway kexec path doesn't need attached one, because shutdown in first
kernel already did pci_msi_shutdown and it called pci_initx_for_msi

also long term should teach every driver call pci_intx_for_msi(dev, 1)?

YH

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: enable_intx_for_kdump.patch --]
[-- Type: text/x-patch; name=enable_intx_for_kdump.patch, Size: 1937 bytes --]

[PATCH] pci/irq: enable INTx in pci_init

in case second kernel that is used by kdump don't have MSI support.

Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 26938da..ca05726 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -269,12 +269,6 @@ static struct msi_desc* alloc_msi_entry(void)
 	return entry;
 }
 
-static void pci_intx_for_msi(struct pci_dev *dev, int enable)
-{
-	if (!(dev->dev_flags & PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG))
-		pci_intx(dev, enable);
-}
-
 static void __pci_restore_msi_state(struct pci_dev *dev)
 {
 	int pos;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e4548ab..f5b0e50 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1381,6 +1381,13 @@ pci_intx(struct pci_dev *pdev, int enable)
 	}
 }
 
+/* in case second kernel doesn't have MSI support built-in */
+void pci_intx_for_msi(struct pci_dev *dev, int enable)
+{
+	if (!(dev->dev_flags & PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG))
+		pci_intx(dev, enable);
+}
+
 /**
  * pci_msi_off - disables any msi or msix capabilities
  * @dev: the PCI device to operate on
@@ -1637,6 +1644,7 @@ static int __devinit pci_init(void)
 	struct pci_dev *dev = NULL;
 
 	while ((dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev)) != NULL) {
+		pci_intx_for_msi(dev, 1);
 		pci_fixup_device(pci_fixup_final, dev);
 	}
 	return 0;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8f53f4b..d1080d0 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -592,6 +592,7 @@ int __must_check pci_set_mwi(struct pci_dev *dev);
 int pci_try_set_mwi(struct pci_dev *dev);
 void pci_clear_mwi(struct pci_dev *dev);
 void pci_intx(struct pci_dev *dev, int enable);
+void pci_intx_for_msi(struct pci_dev *dev, int enable);
 void pci_msi_off(struct pci_dev *dev);
 int pci_set_dma_mask(struct pci_dev *dev, u64 mask);
 int pci_set_consistent_dma_mask(struct pci_dev *dev, u64 mask);

Re: [PATCH 1/2] pci/irq: restore mask_bits in msi shutdown -v3

[Yinghai Lu]

On Wed, Apr 23, 2008 at 2:56 PM, Yinghai Lu <yhlu.kernel.send@gmail.com> wrote:
>
>  Yinghai found after using 2.6.25-rc3 later to kexec RHEL 5.1,
>  NIC can not be used.
>
>  bisected to
>
>  | commit 89d694b9dbe769ca1004e01db0ca43964806a611
>  | Author: Thomas Gleixner <tglx@linutronix.de>
>  | Date:   Mon Feb 18 18:25:17 2008 +0100
>  |
>  |   genirq: do not leave interupts enabled on free_irq
>  |
>  |   The default_disable() function was changed in commit:
>  |
>  |    76d2160147f43f982dfe881404cfde9fd0a9da21
>  |    genirq: do not mask interrupts by default
>  |
>  |   It removed the mask function in favour of the default delayed
>  |   interrupt disabling. Unfortunately this also broke the shutdown in
>  |   free_irq() when the last handler is removed from the interrupt for
>  |   those architectures which rely on the default implementations. Now we
>  |   can end up with a enabled interrupt line after the last handler was
>  |   removed, which can result in spurious interrupts.
>  |
>  |   Fix this by adding a default_shutdown function, which is only
>  |   installed, when the irqchip implementation does provide neither a
>  |   shutdown nor a disable function.
>  |
>  |   [@stable: affected versions: .21 - .24 ]
>
>  for MSI, default_shutdown will call mask_bit for msi device.  so all mask bits
>  will left disabled after free_irq.  then if kexec next kernel that only can
>  use msi_enable bit.  all device's MSI can not be used.
>
>  want to try to restore MSI mask bits that is saved before using msi in first
>  kernel.
>
>  Eric said:
>  This is over complicated and for hardware that erroneously triggers
>  a msi irq after free_irq may have potential problems.
>
>  So lets do the much simpler, much safer, and more general method of
>  restoring the mask bit to it's pci reset defined value (enabled) when
>  we disable the kernels use of msi.
>
>  it will work, because pci_diable_msi is called after free_irq is called.
>
>  v3: extend msi_set_mask_bit to msi_set_mask_bits to take mask, so we can fully
>  restore that to 0x00 instead of 0xfe.
>
>  Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>

andrew,

this one should replace

[PATCH] x86_64: restore mask_bits in msi shutdown

in -mm

YH

Re: [PATCH 1/2] pci/irq: restore mask_bits in msi shutdown -v3

[Jesse Barnes]

On Wednesday, April 23, 2008 2:56 pm Yinghai Lu wrote:
> Yinghai found after using 2.6.25-rc3 later to kexec RHEL 5.1,
> NIC can not be used.
>
> bisected to

Hi Yinghai, I've been thinking about these patches a bit...  They seem like an 
important bug fix (making sure kexec'd kernels work), but I'm a bit worried 
that the kexec'd kernel can't handle potentially broken MSI/INTx setups.  
Shouldn't the kexec'd kernel be a bit more robust?  I guess in this case 
you're kexec'ing an old kernel, so there's not much we can do, but it still 
makes me a little uneasy.

I guess for this particular set it doesn't matter much, since we should be 
restoring things in pci_msi*_shutdown and pci_shutdown_device either way.  
Can you clean up the changelog a bit and maybe make it more concise?  E.g. we 
probably don't need the whole commit message for the bisect, and we want to 
be clearer about what the failure mode is w/o the changes...

Thanks,
Jesse

Re: [PATCH 1/2] pci/irq: restore mask_bits in msi shutdown -v3

[Yinghai Lu]

On Fri, Apr 25, 2008 at 2:48 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Wednesday, April 23, 2008 2:56 pm Yinghai Lu wrote:
>  > Yinghai found after using 2.6.25-rc3 later to kexec RHEL 5.1,
>  > NIC can not be used.
>  >
>  > bisected to
>
>  Hi Yinghai, I've been thinking about these patches a bit...  They seem like an
>  important bug fix (making sure kexec'd kernels work), but I'm a bit worried
>  that the kexec'd kernel can't handle potentially broken MSI/INTx setups.
>  Shouldn't the kexec'd kernel be a bit more robust?  I guess in this case
>  you're kexec'ing an old kernel, so there's not much we can do, but it still
>  makes me a little uneasy.

Yes, it is important, and should be in 2.6.25 stable too.

the maskbits always 0x00 (enabled) from BIOS post..., so we should restore that.

>
>  I guess for this particular set it doesn't matter much, since we should be
>  restoring things in pci_msi*_shutdown and pci_shutdown_device either way.
>  Can you clean up the changelog a bit and maybe make it more concise?  E.g. we
>  probably don't need the whole commit message for the bisect, and we want to
>  be clearer about what the failure mode is w/o the changes...

without it, the second kernel can not use the devices, if second
kernel could use MSI but doesn't touch mask_bit.

---
[PATCH 1/2] pci/irq: restore mask_bits in msi shutdown -v3

Yinghai found after using 2.6.25-rc3 later to kexec RHEL 5.1,
NIC can not be used.

bisected to

| commit 89d694b9dbe769ca1004e01db0ca43964806a611
| Author: Thomas Gleixner <tglx@linutronix.de>
| Date:   Mon Feb 18 18:25:17 2008 +0100
|
|   genirq: do not leave interupts enabled on free_irq
|
|   The default_disable() function was changed in commit:
|
|    76d2160147f43f982dfe881404cfde9fd0a9da21
|    genirq: do not mask interrupts by default
|

for MSI, default_shutdown will call mask_bit for msi device.  so all mask bits
will left disabled after free_irq.  then if kexec next kernel that only can
use msi_enable bit.  all device's MSI can not be used.

So lets to restore the mask bit to it's pci reset defined value (enabled) when
we disable the kernels use of msi.

extend msi_set_mask_bit to msi_set_mask_bits to take mask, so we can fully
restore that to 0x00 instead of 0xfe.

Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>

---

YH

Re: [PATCH 1/2] pci/irq: restore mask_bits in msi shutdown -v3

[Jesse Barnes]

On Friday, April 25, 2008 3:08 pm Yinghai Lu wrote:
> On Fri, Apr 25, 2008 at 2:48 PM, Jesse Barnes <jbarnes@virtuousgeek.org> 
wrote:
> > On Wednesday, April 23, 2008 2:56 pm Yinghai Lu wrote:
> >  > Yinghai found after using 2.6.25-rc3 later to kexec RHEL 5.1,
> >  > NIC can not be used.
> >  >
> >  > bisected to
> >
> >  Hi Yinghai, I've been thinking about these patches a bit...  They seem
> > like an important bug fix (making sure kexec'd kernels work), but I'm a
> > bit worried that the kexec'd kernel can't handle potentially broken
> > MSI/INTx setups. Shouldn't the kexec'd kernel be a bit more robust?  I
> > guess in this case you're kexec'ing an old kernel, so there's not much we
> > can do, but it still makes me a little uneasy.
>
> Yes, it is important, and should be in 2.6.25 stable too.
>
> the maskbits always 0x00 (enabled) from BIOS post..., so we should restore
> that.
>
> >  I guess for this particular set it doesn't matter much, since we should
> > be restoring things in pci_msi*_shutdown and pci_shutdown_device either
> > way. Can you clean up the changelog a bit and maybe make it more concise?
> >  E.g. we probably don't need the whole commit message for the bisect, and
> > we want to be clearer about what the failure mode is w/o the changes...
>
> without it, the second kernel can not use the devices, if second
> kernel could use MSI but doesn't touch mask_bit.
>
> ---
> [PATCH 1/2] pci/irq: restore mask_bits in msi shutdown -v3
>
> Yinghai found after using 2.6.25-rc3 later to kexec RHEL 5.1,
> NIC can not be used.
>
> bisected to
>
> | commit 89d694b9dbe769ca1004e01db0ca43964806a611
> | Author: Thomas Gleixner <tglx@linutronix.de>
> | Date:   Mon Feb 18 18:25:17 2008 +0100
> |
> |   genirq: do not leave interupts enabled on free_irq
> |
> |   The default_disable() function was changed in commit:
> |
> |    76d2160147f43f982dfe881404cfde9fd0a9da21
> |    genirq: do not mask interrupts by default
>
> for MSI, default_shutdown will call mask_bit for msi device.  so all mask
> bits will left disabled after free_irq.  then if kexec next kernel that
> only can use msi_enable bit.  all device's MSI can not be used.
>
> So lets to restore the mask bit to it's pci reset defined value (enabled)
> when we disable the kernels use of msi.
>
> extend msi_set_mask_bit to msi_set_mask_bits to take mask, so we can fully
> restore that to 0x00 instead of 0xfe.
>
> Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>

Applied both (fixed up the changelog a little though), thanks.

Jesse