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

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

[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 <linas@austin.ibm.com>
Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
Signed-off-by: Michael Chan <mchan@broadcom.com>

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 87e0161..f57762d 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -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
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6fda33d..23c6c17 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 768b933..5575227 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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

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

[Linas Vepstas]

On Fri, Oct 19, 2007 at 02:36:56PM -0700, Matt Carlson wrote:
> 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 <linas@austin.ibm.com>
> Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> Signed-off-by: Michael Chan <mchan@broadcom.com>

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

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

[Michael Chan]

On Fri, 2007-10-19 at 18:29 -0500, linas@austin.ibm.com wrote:
> On Fri, Oct 19, 2007 at 02:36:56PM -0700, Matt Carlson wrote:
> > 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 <linas@austin.ibm.com>
> > Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> > Signed-off-by: Michael Chan <mchan@broadcom.com>
> 
> 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, the MSI state is saved automatically when the driver calls
pci_enable_msi(), so it doesn't need to be saved by pseries code.

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

[Linas Vepstas]

On Fri, Oct 19, 2007 at 05:36:17PM -0700, Michael Chan wrote:
> On Fri, 2007-10-19 at 18:29 -0500, linas@austin.ibm.com wrote:
> > On Fri, Oct 19, 2007 at 02:36:56PM -0700, Matt Carlson wrote:
> > > 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 <linas@austin.ibm.com>
> > > Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> > > Signed-off-by: Michael Chan <mchan@broadcom.com>
> > 
> > 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, 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

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

[David Miller]

From: linas@austin.ibm.com (Linas Vepstas)
Date: Fri, 19 Oct 2007 19:04:21 -0500

> 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.

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

[Linas Vepstas]

Hi,

On Fri, Oct 19, 2007 at 05:27:06PM -0700, David Miller wrote:
> From: linas@austin.ibm.com (Linas Vepstas)
> Date: Fri, 19 Oct 2007 19:04:21 -0500
> 
> > 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.
> 
> 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

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

[David Miller]

From: linas@austin.ibm.com (Linas Vepstas)
Date: Fri, 19 Oct 2007 19:46:10 -0500

> 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.

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

[Michael Ellerman]

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


On Fri, 2007-10-19 at 17:53 -0700, David Miller wrote:
> From: linas@austin.ibm.com (Linas Vepstas)
> Date: Fri, 19 Oct 2007 19:46:10 -0500
> 
> > 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.

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

-- 
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: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function

[Michael Chan]

On Sat, 2007-10-20 at 16:43 +1000, Michael Ellerman wrote:
> On Fri, 2007-10-19 at 17:53 -0700, David Miller wrote:
> > 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.
> 
> 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.
> 

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.

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

[Benjamin Herrenschmidt]

> 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.

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

[Linas Vepstas]

On Fri, Oct 19, 2007 at 05:53:08PM -0700, David Miller wrote:
> From: linas@austin.ibm.com (Linas Vepstas)
> Date: Fri, 19 Oct 2007 19:46:10 -0500
> 
> > 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.

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

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

[David Miller]

From: linas@austin.ibm.com (Linas Vepstas)
Date: Mon, 22 Oct 2007 14:54:52 -0500

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

Thank you.

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

[Benjamin Herrenschmidt]

On Mon, 2007-10-22 at 17:23 -0700, David Miller wrote:
> From: linas@austin.ibm.com (Linas Vepstas)
> Date: Mon, 22 Oct 2007 14:54:52 -0500
> 
> > 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.

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

[Benjamin Herrenschmidt]

> 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

> 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.

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

[Michael Chan]

On Fri, 2007-10-19 at 19:04 -0500, linas@austin.ibm.com 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.

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

[Linas Vepstas]

On Fri, Oct 19, 2007 at 06:12:03PM -0700, Michael Chan wrote:
> On Fri, 2007-10-19 at 19:04 -0500, linas@austin.ibm.com 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.

:-) 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

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

[David Miller]

From: "Matt Carlson" <mcarlson@broadcom.com>
Date: Fri, 19 Oct 2007 14:36:56 -0700

> 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 <linas@austin.ibm.com>
> Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> Signed-off-by: Michael Chan <mchan@broadcom.com>

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.

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

[Michael Ellerman]

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

On Sun, 2007-10-21 at 16:21 -0700, David Miller wrote:
> From: "Matt Carlson" <mcarlson@broadcom.com>
> Date: Fri, 19 Oct 2007 14:36:56 -0700
> 
> > 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 <linas@austin.ibm.com>
> > Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> > Signed-off-by: Michael Chan <mchan@broadcom.com>
> 
> 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.

> 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

-- 
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 5/7] pci: Export the pci_restore_msi_state() function

[Linas Vepstas]

On Mon, Oct 22, 2007 at 11:49:24AM +1000, Michael Ellerman wrote:
> 
> 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.

> 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

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

[Benjamin Herrenschmidt]

On Mon, 2007-10-22 at 13:13 -0500, Linas Vepstas wrote:
> On Mon, Oct 22, 2007 at 11:49:24AM +1000, Michael Ellerman wrote:
> > 
> > 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 ?

> > 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.

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

Ben.

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

[Linas Vepstas]

On Tue, Oct 23, 2007 at 07:24:27AM +1000, Benjamin Herrenschmidt wrote:
> 
> On Mon, 2007-10-22 at 13:13 -0500, Linas Vepstas wrote:
> > On Mon, Oct 22, 2007 at 11:49:24AM +1000, Michael Ellerman wrote:
> > > 
> > > 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.

> 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

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

[Benjamin Herrenschmidt]

> > 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.

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

[Michael Ellerman]

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


On Mon, 2007-10-22 at 13:13 -0500, Linas Vepstas wrote:
> On Mon, Oct 22, 2007 at 11:49:24AM +1000, Michael Ellerman wrote:
> > 
> > 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.

> > 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().

> 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

-- 
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 5/7] pci: Export the pci_restore_msi_state() function

[Michael Chan]

David Miller wrote:

> 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.

> 
> 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?

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

[David Miller]

From: "Michael Chan" <mchan@broadcom.com>
Date: Sun, 21 Oct 2007 21:01:17 -0700

> David Miller wrote:
> 
> > 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.

> > 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?

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

[Linas Vepstas]

On Sun, Oct 21, 2007 at 09:45:20PM -0700, David Miller wrote:
> 
> 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.

> 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".

>    pci_restor_msi_state() can get exported to modules in this
>    change

OK.

> 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

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

[Linas Vepstas]

On Sun, Oct 21, 2007 at 04:21:31PM -0700, David Miller wrote:
> From: "Matt Carlson" <mcarlson@broadcom.com>
> Date: Fri, 19 Oct 2007 14:36:56 -0700
> 
> > 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 <linas@austin.ibm.com>
> > Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> > Signed-off-by: Michael Chan <mchan@broadcom.com>
> 
> 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.

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