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

[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

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