Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
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 signature.asc Description: This is a digitally signed message part
Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
On Mon, 2007-10-22 at 17:23 -0700, David Miller wrote: > From: [EMAIL PROTECTED] (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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
> > 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > As discussed in the other thread, I'll try to set up a patch > for an arch callback for restoring msi state. Thank you. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
On Fri, Oct 19, 2007 at 05:53:08PM -0700, David Miller wrote:
> From: [EMAIL PROTECTED] (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
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
On Sun, Oct 21, 2007 at 04:21:31PM -0700, David Miller wrote: > From: "Matt Carlson" <[EMAIL PROTECTED]> > 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 <[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. 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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
From: "Michael Chan" <[EMAIL PROTECTED]> 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? - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
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? - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
On Sun, 2007-10-21 at 16:21 -0700, David Miller wrote: > From: "Matt Carlson" <[EMAIL PROTECTED]> > 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 <[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. > 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 signature.asc Description: This is a digitally signed message part
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
From: "Matt Carlson" <[EMAIL PROTECTED]> 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 <[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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
> 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
On Fri, 2007-10-19 at 17:53 -0700, David Miller wrote:
> From: [EMAIL PROTECTED] (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
signature.asc
Description: This is a digitally signed message part
Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
> 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
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
> 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.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
Hi, On Fri, Oct 19, 2007 at 05:27:06PM -0700, David Miller wrote: > From: [EMAIL PROTECTED] (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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
From: [EMAIL PROTECTED] (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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
On Fri, Oct 19, 2007 at 06:12:03PM -0700, Michael Chan wrote: > 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. :-) 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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
On Fri, Oct 19, 2007 at 05:36:17PM -0700, Michael Chan wrote: > On Fri, 2007-10-19 at 18:29 -0500, [EMAIL PROTECTED] 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 <[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, 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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
On Fri, 2007-10-19 at 18:29 -0500, [EMAIL PROTECTED] 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 <[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, the MSI state is saved automatically when the driver calls pci_enable_msi(), so it doesn't need to be saved by pseries code. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
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 <[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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[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]>
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
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
