Skip to contentSkip to conversation

pci: Export the pci_restore_msi_state() function

PATCH Applied Matt Carlson 6 participants 27 messages Oct 19, 2007 – Oct 23, 2007

Subsystems (3)
Networking Network drivers PCI
lore
Share & tools
lore

Matt Carlson

author
#0 lore raw

PATCH 5/7 pci: Export the pci_restore_msi_state() function

This patch exports the pci_restore_msi_state() function. This function is needed to restore the MSI state during PCI error recovery.

Signed-off-by Linas Vepstas <[email protected]>
Signed-off-by Matt Carlson <[email protected]>
Signed-off-by Michael Chan <[email protected]>
drivers/pci/msi.c +1 -2
index 87e0161..f57762d 100644
@@ -224,7 +224,6 @@ static struct msi_desc* alloc_msi_entry(void)
 	return entry;
 }
 
-#ifdef CONFIG_PM
 static void __pci_restore_msi_state(struct pci_dev *dev)
 {
 	int pos;

@@ -282,7 +281,7 @@ void pci_restore_msi_state(struct pci_dev *dev)
 	__pci_restore_msi_state(dev);
 	__pci_restore_msix_state(dev);
 }
-#endif	/* CONFIG_PM */
+EXPORT_SYMBOL_GPL(pci_restore_msi_state);
 
 /**
  * msi_capability_init - configure device's MSI capability structure
drivers/pci/pci.h +0 -6
index 6fda33d..23c6c17 100644
@@ -45,12 +45,6 @@ static inline void pci_no_msi(void) { }
 static inline void pci_msi_init_pci_dev(struct pci_dev *dev) { }
 #endif
 
-#if defined(CONFIG_PCI_MSI) && defined(CONFIG_PM)
-void pci_restore_msi_state(struct pci_dev *dev);
-#else
-static inline void pci_restore_msi_state(struct pci_dev *dev) {}
-#endif
-
 #ifdef CONFIG_PCIEAER
 void pci_no_aer(void);
 #else
include/linux/pci.h +2 -0
index 768b933..5575227 100644
@@ -662,6 +662,7 @@ static inline int pci_enable_msix(struct pci_dev* dev,
 	struct msix_entry *entries, int nvec) {return -1;}
 static inline void pci_disable_msix(struct pci_dev *dev) {}
 static inline void msi_remove_pci_irq_vectors(struct pci_dev *dev) {}
+static inline void pci_restore_msi_state(struct pci_dev *dev) {}
 #else
 extern int pci_enable_msi(struct pci_dev *dev);
 extern void pci_disable_msi(struct pci_dev *dev);

@@ -669,6 +670,7 @@ extern int pci_enable_msix(struct pci_dev* dev,
 	struct msix_entry *entries, int nvec);
 extern void pci_disable_msix(struct pci_dev *dev);
 extern void msi_remove_pci_irq_vectors(struct pci_dev *dev);
+void pci_restore_msi_state(struct pci_dev *dev);
 #endif
 
 #ifdef CONFIG_HT_IRQ
#1 lore raw
Quoted 6 lines · Matt Carlson

This patch exports the pci_restore_msi_state() function. This function is needed to restore the MSI state during PCI error recovery.

Signed-off-by Linas Vepstas <[email protected]>
Signed-off-by Matt Carlson <[email protected]>
Signed-off-by Michael Chan <[email protected]>

Davem,

This patch is generically needed for recovery from PCI errors, and not just the tg3 that Matt is working on.

Matt, there are also several msi-related bugs in the pseries architecture implementation, those patches will go out to Paul Mackerras seperately. I was hoping today ... but things came up. One little iddy-biddy problem is that the pseries is not actually *saving* the msi state, and so, ahem, the restore isn't quite working out either. I'm still trying to navigate around that.

--linas

#2 lore raw
Quoted 13 lines · On Fri, 2007-10-19 at 18:29 -0500, [email protected] wrote:
Quoted 6 lines · Matt Carlson

This patch exports the pci_restore_msi_state() function. This function is needed to restore the MSI state during PCI error recovery.

Signed-off-by Linas Vepstas <[email protected]>
Signed-off-by Matt Carlson <[email protected]>
Signed-off-by Michael Chan <[email protected]>

Davem,

This patch is generically needed for recovery from PCI errors, and not just the tg3 that Matt is working on.

Quoted 8 lines

Matt, there are also several msi-related bugs in the pseries architecture implementation, those patches will go out to Paul Mackerras seperately. I was hoping today ... but things came up. One little iddy-biddy problem is that the pseries is not actually *saving* the msi state, and so, ahem, the restore isn't quite working out either. I'm still trying to navigate around that.

Linas, the MSI state is saved automatically when the driver calls pci_enable_msi(), so it doesn't need to be saved by pseries code.

#3 lore raw
Quoted 22 lines · Michael Chan
Quoted 13 lines · On Fri, 2007-10-19 at 18:29 -0500, [email protected] wrote:
Quoted 6 lines · Matt Carlson

This patch exports the pci_restore_msi_state() function. This function is needed to restore the MSI state during PCI error recovery.

Signed-off-by Linas Vepstas <[email protected]>
Signed-off-by Matt Carlson <[email protected]>
Signed-off-by Michael Chan <[email protected]>

Davem,

This patch is generically needed for recovery from PCI errors, and not just the tg3 that Matt is working on.

Quoted 8 lines

Matt, there are also several msi-related bugs in the pseries architecture implementation, those patches will go out to Paul Mackerras seperately. I was hoping today ... but things came up. One little iddy-biddy problem is that the pseries is not actually *saving* the msi state, and so, ahem, the restore isn't quite working out either. I'm still trying to navigate around that.

Quoted 2 lines

Linas, the MSI state is saved automatically when the driver calls pci_enable_msi(), so it doesn't need to be saved by pseries code.

I'm working in linux-2.6.23-rc8-mm1 at the moment, and I don't see that happening. viz. read_msi_msg() is not called anywhere, and I need to have valid msg->address_lo and msg->address_hi and msg->data in order to be able to restore.

In particular, this has to happen after the call to arch_setup_msi_irqs as otherwise, the arch hasn't yet filled these fields with correct values.

Perhaps this is fixed in the kernel you're working with?

--linas

#4 lore raw
Quoted 11 lines · On Fri, 2007-10-19 at 19:04 -0500, [email protected] wrote:

I'm working in linux-2.6.23-rc8-mm1 at the moment, and I don't see that happening. viz. read_msi_msg() is not called anywhere, and I need to have valid msg->address_lo and msg->address_hi and msg->data in order to be able to restore.

In particular, this has to happen after the call to arch_setup_msi_irqs
as otherwise, the arch hasn't yet filled these fields with correct values.

Perhaps this is fixed in the kernel you're working with?

It's possible that this doesn't work on pseries. I've only tested pci_restore_msi_state() on x86 in the context of suspend and resume. During resume, the MSI state gets restored correctly on x86.

#5 lore raw
Quoted 11 lines · Michael Chan
Quoted 10 lines · On Fri, 2007-10-19 at 19:04 -0500, [email protected] wrote:

I'm working in linux-2.6.23-rc8-mm1 at the moment, and I don't see that happening. viz. read_msi_msg() is not called anywhere, and I need to have valid msg->address_lo and msg->address_hi and msg->data in order to be able to restore.

In particular, this has to happen after the call to arch_setup_msi_irqs
as otherwise, the arch hasn't yet filled these fields with correct values.

Quoted 5 lines
Quoted 1 line

Perhaps this is fixed in the kernel you're working with?

It's possible that this doesn't work on pseries. I've only tested pci_restore_msi_state() on x86 in the context of suspend and resume. During resume, the MSI state gets restored correctly on x86.

:-) Yes, I think that is being done in arch/i386/kernel/io_apic.c and arch/ia64/kernel/msi_ia64.c and etc. but its not being done on most of the powerpc's. Its possible that none of the old macintosh laptops use msi, and so no one noticed before; I know that no one ever suspends/resumes the big servers I work on, sooo :-)

Actually, looking at arch/i386/kernel/io_apic.c, it looks like the msi state is being saved only when CONFIG_SMP is set, so it seems to me that the restore will fail on uni systems ... are there any of those left?

--linas

maintainer
#6 lore raw
In reply to Linas Vepstas

From: [email protected] (Linas Vepstas)
Date: Fri, 19 Oct 2007 19:04:21 -0500

Quoted 4 lines

I'm working in linux-2.6.23-rc8-mm1 at the moment, and I don't see that happening. viz. read_msi_msg() is not called anywhere, and I need to have valid msg->address_lo and msg->address_hi and msg->data in order to be able to restore.

The generic PCI layer will save away the PCI config space elements during pci_enable_msi(), including the MSI address and data values.

You can fetch the values you need from there during restore if you need them.

See the pci_restore_msi_state() call done from pci_restore_state() in drivers/pci/pci.c, that pci_restore_msi_state() code in drivers/pci/msi.c very much relies upon the entry->msg values being uptodate and valid.

The MSI arch layer code is supposed to fill the entry->msg values in via arch_setup_msi_irq(). Perhaps the pseries code is forgetting to do that.

So I can't really see what the problem is you're talking about.

#7 lore raw

[BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function

Hi,

Quoted 8 lines · David Miller

From: [email protected] (Linas Vepstas)
Date: Fri, 19 Oct 2007 19:04:21 -0500

Quoted 4 lines

I'm working in linux-2.6.23-rc8-mm1 at the moment, and I don't see that happening. viz. read_msi_msg() is not called anywhere, and I need to have valid msg->address_lo and msg->address_hi and msg->data in order to be able to restore.

Quoted 8 lines

See the pci_restore_msi_state() call done from pci_restore_state() in drivers/pci/pci.c, that pci_restore_msi_state() code in drivers/pci/msi.c very much relies upon the entry->msg values being uptodate and valid.

The MSI arch layer code is supposed to fill the entry->msg values in via arch_setup_msi_irq(). Perhaps the pseries code is forgetting to do that.

Yep. Thank you for confirming the correct location for the fix.

FWIW, it looks like not all that many arches do this; the output for grep -r address_hi * is pretty thin. Then, looking at i386/kernel/io_apic.c as an example, one can see that the msi state save happens "by accident" if CONFIG_SMP is enabled; and so its surely broekn on uniprocesor machines.

I'm cc'ing the powerpc mailing list to point this out:
it looks like only cell/axon_msi.c and mpic_u3msi.c bother do do anything. I guess that there aren't any old macintosh laptops that have msi on them? Because without this, suspend and resume breaks.

Paul,
On the off chance your reading this, I'll send a pseries patch on Monday, with luck (and some other patches too). I'm not touching any of the other plaforms, you and benh would know those better.

--linas

maintainer
#8 lore raw

Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function

From: [email protected] (Linas Vepstas)
Date: Fri, 19 Oct 2007 19:46:10 -0500

Quoted 5 lines

FWIW, it looks like not all that many arches do this; the output for grep -r address_hi * is pretty thin. Then, looking at i386/kernel/io_apic.c as an example, one can see that the msi state save happens "by accident" if CONFIG_SMP is enabled; and so its surely broekn on uniprocesor machines.

I don't see this, in all cases write_msi_msg() will transfer the given "*msg" to entry->msg by this assignment in drivers/pci/msi.c:

void write_msi_msg(unsigned int irq, struct msi_msg *msg) {
...

	entry->msg = *msg;
}

So as long as write_msi_msg() is invoked, it will be saved properly.

Platforms need not do this explicitly.

#9 lore raw

Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function

Quoted 16 lines · David Miller

From: [email protected] (Linas Vepstas)
Date: Fri, 19 Oct 2007 19:46:10 -0500

Quoted 5 lines

FWIW, it looks like not all that many arches do this; the output for grep -r address_hi * is pretty thin. Then, looking at i386/kernel/io_apic.c as an example, one can see that the msi state save happens "by accident" if CONFIG_SMP is enabled; and so its surely broekn on uniprocesor machines.

I don't see this, in all cases write_msi_msg() will transfer the given "*msg" to entry->msg by this assignment in drivers/pci/msi.c:

void write_msi_msg(unsigned int irq, struct msi_msg *msg) {
...

Quoted 7 lines
	entry->msg = *msg;
}

So as long as write_msi_msg() is invoked, it will be saved properly.

Platforms need not do this explicitly.

I'm short on context here, and it's Saturday, so excuse me if I'm missing the point somewhere.

On pseries machines we don't call write_msi_msg(), because we don't control the contents of the message, firmware does. So entry->msg will be bogus.

That's a pity, but AFAIK it shouldn't be a problem because we don't enable CONFIG_PM on those machines anyway. If we ever want to we'll need to sort out with firmware how that will work WRT restoring MSI state.

cheers

Signature · 8 lines
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
#10 lore raw

Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function

Quoted 19 lines · Michael Ellerman
Quoted 7 lines · David Miller

I don't see this, in all cases write_msi_msg() will transfer the given "*msg" to entry->msg by this assignment in drivers/pci/msi.c:

void write_msi_msg(unsigned int irq, struct msi_msg *msg) {
...

Quoted 7 lines
	entry->msg = *msg;
}

So as long as write_msi_msg() is invoked, it will be saved properly.

Platforms need not do this explicitly.

I'm short on context here, and it's Saturday, so excuse me if I'm missing the point somewhere.

Quoted 8 lines

On pseries machines we don't call write_msi_msg(), because we don't control the contents of the message, firmware does. So entry->msg will be bogus.

That's a pity, but AFAIK it shouldn't be a problem because we don't enable CONFIG_PM on those machines anyway. If we ever want to we'll need to sort out with firmware how that will work WRT restoring MSI state.

The PCI error recovery that Linas is working on requires the MSI state to be restored after we do PCI reset to recover from PCI errors.

#11 lore raw

Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function

In reply to Michael Ellerman
Quoted 4 lines

That's a pity, but AFAIK it shouldn't be a problem because we don't enable CONFIG_PM on those machines anyway. If we ever want to we'll need to sort out with firmware how that will work WRT restoring MSI state.

I think the current generic code for pci_restore_msi_state() or whatever it's called wilol directly call into write_msi_msg() etc... might be a problem.

Ben.

#12 lore raw

Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function

In reply to David Miller
Quoted 13 lines · David Miller

From: [email protected] (Linas Vepstas)
Date: Fri, 19 Oct 2007 19:46:10 -0500

Quoted 5 lines

FWIW, it looks like not all that many arches do this; the output for grep -r address_hi * is pretty thin. Then, looking at i386/kernel/io_apic.c as an example, one can see that the msi state save happens "by accident" if CONFIG_SMP is enabled; and so its surely broekn on uniprocesor machines.

I don't see this, in all cases write_msi_msg() will transfer the given "*msg" to entry->msg by this assignment in drivers/pci/msi.c:

Quoted 8 lines

void write_msi_msg(unsigned int irq, struct msi_msg *msg) {
...

	entry->msg = *msg;
}

So as long as write_msi_msg() is invoked, it will be saved properly.

As Michael Ellerman points out, the pseries msi setup is done by firmware, and so this bit never happens.

As discussed in the other thread, I'll try to set up a patch for an arch callback for restoring msi state.

-linas

maintainer
#13 lore raw

Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function

From: [email protected] (Linas Vepstas)
Date: Mon, 22 Oct 2007 14:54:52 -0500

Quoted 2 lines

As discussed in the other thread, I'll try to set up a patch for an arch callback for restoring msi state.

Thank you.

#14 lore raw

Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function

Quoted 7 lines · David Miller

From: [email protected] (Linas Vepstas)
Date: Mon, 22 Oct 2007 14:54:52 -0500

Quoted 2 lines

As discussed in the other thread, I'll try to set up a patch for an arch callback for restoring msi state.

From what it looks like at this stage, pSeries might need to

differenciate restoring MSI state after a device reset (PCI error recovery) from restoring MSI state after suspend/resume (if we ever implement that one).

The former apparently require manual saving & restoring of the config space bits. (Linas, do you have a pointer to the bit of PAPR spec that specifies that we need to save & restore the MSI message ourselves ?)

For the later (suspend/resume), that will definitely not work, or at least, will not be enough, especially with something like suspend to disk, where we'll need to have the firmware reconfigure the MSIs for us (to make sure, among others, that the interrupt controllers are properly configured for MSIs etc...).

Ben.

#15 lore raw

Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function

In reply to Linas Vepstas
Quoted 6 lines

I'm cc'ing the powerpc mailing list to point this out:
it looks like only cell/axon_msi.c and mpic_u3msi.c bother do do anything. I guess that there aren't any old macintosh laptops that have msi on them? Because without this, suspend and resume breaks.

The only macs that can do any form of MSIs are the G5s using mpic_u3msi.c

Quoted 5 lines

Paul,
On the off chance your reading this, I'll send a pseries patch on Monday, with luck (and some other patches too). I'm not touching any of the other plaforms, you and benh would know those better.

Or rather Michael as he wrote the ppc MSI support. I'll check with him

Ben.

maintainer
#16 lore raw
In reply to Matt Carlson

From: "Matt Carlson" <[email protected]>
Date: Fri, 19 Oct 2007 14:36:56 -0700

Quoted 6 lines

This patch exports the pci_restore_msi_state() function. This function is needed to restore the MSI state during PCI error recovery.

Signed-off-by Linas Vepstas <[email protected]>
Signed-off-by Matt Carlson <[email protected]>
Signed-off-by Michael Chan <[email protected]>

I'm not so sure about this.

Perhaps, instead, you should do a pci_msi_disable() and pci_msi_enable() in the error detection and recovery sequence.

Or, alternatively, save/restore those MSI registers by hand.

I'm trying to figure out how the E1000 driver handles this correctly, but I can't see it just by reading it over quickly.

#17 lore raw
Quoted 11 lines · David Miller

From: "Matt Carlson" <[email protected]>
Date: Fri, 19 Oct 2007 14:36:56 -0700

Quoted 6 lines

This patch exports the pci_restore_msi_state() function. This function is needed to restore the MSI state during PCI error recovery.

Signed-off-by Linas Vepstas <[email protected]>
Signed-off-by Matt Carlson <[email protected]>
Signed-off-by Michael Chan <[email protected]>

I'm not so sure about this.

On pseries there's a chance it will work for PCI error recovery, but if so it's just lucky that firmware has left everything configured the same way. For actual suspend/resume it will never work, we need to ask firmware to configure things.

Quoted 2 lines

Perhaps, instead, you should do a pci_msi_disable() and pci_msi_enable() in the error detection and recovery sequence.

Yes I think so. That way we can properly reconfigure via the firmware interface. The other option would be to design some new arch hook to do resume, but just doing a disable/enable seems simpler to me.

cheers

Signature · 8 lines
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
#18 lore raw
Quoted 4 lines · Michael Ellerman

On pseries there's a chance it will work for PCI error recovery, but if so it's just lucky that firmware has left everything configured the same way.

? The papr is quite clear that i is up to the OS to restore the msi state after an eeh error.

Quoted 3 lines

Yes I think so. That way we can properly reconfigure via the firmware interface. The other option would be to design some new arch hook to do resume, but just doing a disable/enable seems simpler to me.

Err, If you read the code for suspend/resume, it never actually calls disable/enable (and thus doesn't go to the firmware); it calls restore_msi_state() function!

If suspend/resume needs to call firmware to restore the state, then, at the moment, suspend/resume is broken. As I mentioned earlier, I presumed that no powerpc laptops currently use msi-enabled devices, as otherwise, this would have been flushed out.

--linas

#19 lore raw
Quoted 8 lines · Linas Vepstas
Quoted 4 lines · Michael Ellerman

On pseries there's a chance it will work for PCI error recovery, but if so it's just lucky that firmware has left everything configured the same way.

? The papr is quite clear that i is up to the OS to restore the msi state after an eeh error.

Via direct config space access or via firmware change-msi calls ?

Quoted 4 lines
Quoted 3 lines

Yes I think so. That way we can properly reconfigure via the firmware interface. The other option would be to design some new arch hook to do resume, but just doing a disable/enable seems simpler to me.

Quoted 8 lines

Err, If you read the code for suspend/resume, it never actually calls disable/enable (and thus doesn't go to the firmware); it calls restore_msi_state() function!

If suspend/resume needs to call firmware to restore the state, then, at the moment, suspend/resume is broken. As I mentioned earlier, I presumed that no powerpc laptops currently use msi-enabled devices, as otherwise, this would have been flushed out.

I don't know why you keep talking about powerpc laptops here ...

Ben.

#20 lore raw
Quoted 4 lines · Benjamin Herrenschmidt
Quoted 2 lines · Linas Vepstas
Quoted 1 line · Michael Ellerman
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

On pseries there's a chance it will work for PCI error recovery, but if so it's just lucky that firmware has left everything configured the same way.

? The papr is quite clear that i is up to the OS to restore the msi state after an eeh error.

Via direct config space access or via firmware change-msi calls ?

Direct config space access. It says that the OS is supposed to read the MSI config (after its been set up), save it, and restore it, (via direct config space writes) if the device is ever reset.

Quoted 1 line

I don't know why you keep talking about powerpc laptops here ...

Well, there are Apple laptops, right? Aren't those the "powermac" platform? Now, I don't know if they support MSI, but if they do, I get the impression that they might not restore msi state correctly, after being put into hardware suspend. But perhaps I'm mistaken; I was simply grepping for various msi-related functions in various arch subdirectories, comparing x86 to other arches, and noticed that code that would restore msi state seems to be missing for most arches and most powerpc platforms.

--linas

#21 lore raw
Quoted 11 lines
Quoted 2 lines

I don't know why you keep talking about powerpc laptops here ...

Well, there are Apple laptops, right? Aren't those the "powermac" platform? Now, I don't know if they support MSI, but if they do, I get the impression that they might not restore msi state correctly, after being put into hardware suspend. But perhaps I'm mistaken; I was simply grepping for various msi-related functions in various arch subdirectories, comparing x86 to other arches, and noticed that code that would restore msi state seems to be missing for most arches and most powerpc platforms.

Ah ok, i see. Well, platforms that use write_msi_msg() shouldn't need anything special right ? So only pSeries is an issue here....

PowerBooks don't indeed have MSI support, though G5's do and some people have been toying around with suspend/resume on them (hibernation only at that stage) but it doesn't matter at this stage. We are specifically talking about pSeries which is the "special" case here.

Ben.

#22 lore raw
In reply to Linas Vepstas
Quoted 8 lines · Linas Vepstas
Quoted 4 lines · Michael Ellerman

On pseries there's a chance it will work for PCI error recovery, but if so it's just lucky that firmware has left everything configured the same way.

? The papr is quite clear that i is up to the OS to restore the msi state after an eeh error.

Perhaps. I see R1-7.3.10.5.1-10, which says we should restore the config space after a reset operation, but after an isolate/unisolate we must recall RTAS. I thought EEH was doing the isolate/unisolate, but if it's just a reset then just blatting the config space back should work.

Quoted 7 lines
Quoted 3 lines

Yes I think so. That way we can properly reconfigure via the firmware interface. The other option would be to design some new arch hook to do resume, but just doing a disable/enable seems simpler to me.

Err, If you read the code for suspend/resume, it never actually calls disable/enable (and thus doesn't go to the firmware); it calls restore_msi_state() function!

I know. That was a proposed solution, to explicitly call disable/enable instead of restore_msi_state().

Quoted 4 lines

If suspend/resume needs to call firmware to restore the state, then, at the moment, suspend/resume is broken. As I mentioned earlier, I presumed that no powerpc laptops currently use msi-enabled devices, as otherwise, this would have been flushed out.

On _pseries_ suspend/resume with MSI is broken. The other powerpc platforms that implement MSI should be fine, although I don't think anyone's tested them, because they're not laptops and so suspend/resume is not that interesting.

cheers

Signature · 8 lines
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
#23 lore raw
In reply to David Miller
Quoted 4 lines · David Miller

I'm not so sure about this.

Perhaps, instead, you should do a pci_msi_disable() and pci_msi_enable() in the error detection and recovery sequence.

If we just detected PCI errors on this slot, I don't think it's a good idea to continue writing to the config space to disable MSI. Perhaps we can disable/enable after the slot reset.

Quoted 2 lines

Or, alternatively, save/restore those MSI registers by hand.

We can do that, but we'll have to do it ahead of time when we enable MSI, not after errors have been detected. But the address/data can change as the CPU affinity changes during run time, right?

maintainer
#24 lore raw

From: "Michael Chan" <[email protected]>
Date: Sun, 21 Oct 2007 21:01:17 -0700

Quoted 10 lines
Quoted 4 lines · David Miller

I'm not so sure about this.

Perhaps, instead, you should do a pci_msi_disable() and pci_msi_enable() in the error detection and recovery sequence.

If we just detected PCI errors on this slot, I don't think it's a good idea to continue writing to the config space to disable MSI. Perhaps we can disable/enable after the slot reset.

Right, it would have to be after the slot reset.

Quoted 5 lines
Quoted 1 line

Or, alternatively, save/restore those MSI registers by hand.

We can do that, but we'll have to do it ahead of time when we enable MSI, not after errors have been detected. But the address/data can change as the CPU affinity changes during run time, right?

Yes, it can.

The core issue is that the ARCH level MSI code invokes write_msi_msg(), not the generic code, exactly because there are platform level issues wherein the firmware is the only legal way to write the MSI settings in PCI config space.

However, the MSI state restore code was not architected similarly. It does the write_msi_msg() directly, instead of letting platform level code is in ARCH hooks.

Therefore I think we need to attack this in two stages:

1) First changeset moves the write_msi_msg() call currently in __pci_restore_msi_state() into an ARCH overridable handler.

This would allow powerpc to deal with this properly.

pci_restor_msi_state() can get exported to modules in this change

2) The Tigon3 error recovery changes, as they were.

But I have to ask, can anyone see how e1000 handles MSI properly in it's PCI error support?

#25 lore raw
Quoted 9 lines · David Miller

The core issue is that the ARCH level MSI code invokes write_msi_msg(), not the generic code, exactly because there are platform level issues wherein the firmware is the only legal way to write the MSI settings in PCI config space.

However, the MSI state restore code was not architected similarly. It does the write_msi_msg() directly, instead of letting platform level code is in ARCH hooks.

Yes, exactly.

Quoted 6 lines

Therefore I think we need to attack this in two stages:

1) First changeset moves the write_msi_msg() call currently in __pci_restore_msi_state() into an ARCH overridable handler.

This would allow powerpc to deal with this properly.

Yes!
I'll try to put together a patch later today, if I can get a fabled "round tuit".

Quoted 2 lines

pci_restor_msi_state() can get exported to modules in this change

OK.

Quoted 4 lines

2) The Tigon3 error recovery changes, as they were.

But I have to ask, can anyone see how e1000 handles MSI properly in it's PCI error support?

It doesn't. None of them do. :-( I didn't get access to msi-capable hardware until a few weeks ago; that's why this is coming up just now.

--linas

#26 lore raw
In reply to David Miller
Quoted 12 lines · David Miller

From: "Matt Carlson" <[email protected]>
Date: Fri, 19 Oct 2007 14:36:56 -0700

Quoted 6 lines

This patch exports the pci_restore_msi_state() function. This function is needed to restore the MSI state during PCI error recovery.

Signed-off-by Linas Vepstas <[email protected]>
Signed-off-by Matt Carlson <[email protected]>
Signed-off-by Michael Chan <[email protected]>

I'm not so sure about this.

Quoted 7 lines

Perhaps, instead, you should do a pci_msi_disable() and pci_msi_enable() in the error detection and recovery sequence.

Or, alternatively, save/restore those MSI registers by hand.

I'm trying to figure out how the E1000 driver handles this correctly, but I can't see it just by reading it over quickly.

The e1000 and the ixgb are broken as well ... right now, any driver that uses msi together with the pci error recovery will fail to get recovered correctly. There are several distinct bugs; one is that, msi state is not being restored; and the call to pci_restore_msi_state() was supposed to aid with that.

I'd rather not use pci_msi_disable(), because that has the side-effect of enabling legacy interupts; I'm concerned that this will have the potential for causing havoc of all sorts.

--linas