RE: 2.6.12-rc1-mm1: Kernel BUG at pci:389

RE: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Li, Shaohua]

>
>> > Yes, but it is needed. There are many drivers, and they look at
>> > numerical value of PMSG_*. I'm proceeding in steps. I hopefully
killed
>> > all direct accesses to the constants, and will switch constants to
>> > something else... But that is going to be tommorow (need some
sleep).
>> The patches are going to acquire correct PCI device sleep state for
>> suspend/resume. We discussed the issue several months ago. My plan is
we
>> first introduce 'platform_pci_set_power_state', then merge the
>> 'platform_pci_choose_state' patch after Pavel's pm_message_t
conversion
>> finished. Maybe Len mislead my comments.
>>
>> Anyway for the callback, my intend is platform_pci_choose_state
accept
>> the pm_message_t parameter, and it return an 'int', since platform
>> method possibly failed and then pci_choose_state translate the return
>> value to pci_power_t.
>
>You can't just retype around like that. You may want it take
>pci_power_t * as an argument, and then return 0/-ENODEV or something
>like that. But you can't retype between int and pm_message_t...
No, taking pci_power_t as an argument is meaningless. For ACPI, we
should know the exact sleep state, pm_message_t will tell us. But I'm ok
to let it return a pci_power_t, and the failure case returns -ENODEV.

>
>Plus that function should have a documentation somewhere!
I will add it.

>
>> > Could you just revert those two patches? First one is very
>> > wrong. Second one might be fixed, but... See comments below.
>> I think the platform_pci_set_power_state should be ok, did you see it
>> causes oops?
>
>No its just ugly and uses __force in "creative" way. That one can be
>recovered.
Do you mean this?

> +	static int state_conv[] = {
> +		[0] = 0,
> +		[1] = 1,
> +		[2] = 2,
> +		[3] = 3,
> +		[4] = 3
> +	};
> +	int acpi_state = state_conv[(int __force) state];

The table should be
		[PCI_D0] = 0,

I'm not sure, but then could we use state_conv[state] directly? It seems
wrong to me (the array accepts a pci_power_t as index?)

Thanks,
Shaohua

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Pavel Machek]

Hi!

> >> > Yes, but it is needed. There are many drivers, and they look at
> >> > numerical value of PMSG_*. I'm proceeding in steps. I hopefully
> killed
> >> > all direct accesses to the constants, and will switch constants to
> >> > something else... But that is going to be tommorow (need some
> sleep).
> >> The patches are going to acquire correct PCI device sleep state for
> >> suspend/resume. We discussed the issue several months ago. My plan is
> we
> >> first introduce 'platform_pci_set_power_state', then merge the
> >> 'platform_pci_choose_state' patch after Pavel's pm_message_t
> conversion
> >> finished. Maybe Len mislead my comments.
> >>
> >> Anyway for the callback, my intend is platform_pci_choose_state
> accept
> >> the pm_message_t parameter, and it return an 'int', since platform
> >> method possibly failed and then pci_choose_state translate the return
> >> value to pci_power_t.
> >
> >You can't just retype around like that. You may want it take
> >pci_power_t * as an argument, and then return 0/-ENODEV or something
> >like that. But you can't retype between int and pm_message_t...
> No, taking pci_power_t as an argument is meaningless. For ACPI, we
> should know the exact sleep state, pm_message_t will tell us. But I'm ok
> to let it return a pci_power_t, and the failure case returns
> -ENODEV.

You can't put -ENODEV into pci_power_t ... but maybe we should create
PCI_ERROR and pass it in cases like this one?

> >> > Could you just revert those two patches? First one is very
> >> > wrong. Second one might be fixed, but... See comments below.
> >> I think the platform_pci_set_power_state should be ok, did you see it
> >> causes oops?
> >
> >No its just ugly and uses __force in "creative" way. That one can be
> >recovered.
> Do you mean this?
> 
> > +	static int state_conv[] = {
> > +		[0] = 0,
> > +		[1] = 1,
> > +		[2] = 2,
> > +		[3] = 3,
> > +		[4] = 3
> > +	};
> > +	int acpi_state = state_conv[(int __force) state];
> 
> The table should be
> 		[PCI_D0] = 0,
> 
> I'm not sure, but then could we use state_conv[state] directly? It seems

I think so. Of course it is wrong, but it is less wrong than forcing
it to integer than index, without using macros at all.

Or perhaps you should do

switch (state) {
case PCI_D0: ...
}

...and handle default case somehow.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Li Shaohua]

On Tue, 2005-03-22 at 20:20, Pavel Machek wrote:
> Hi!
> 
> > >> > Yes, but it is needed. There are many drivers, and they look at
> > >> > numerical value of PMSG_*. I'm proceeding in steps. I hopefully
> > killed
> > >> > all direct accesses to the constants, and will switch constants
> to
> > >> > something else... But that is going to be tommorow (need some
> > sleep).
> > >> The patches are going to acquire correct PCI device sleep state
> for
> > >> suspend/resume. We discussed the issue several months ago. My
> plan is
> > we
> > >> first introduce 'platform_pci_set_power_state', then merge the
> > >> 'platform_pci_choose_state' patch after Pavel's pm_message_t
> > conversion
> > >> finished. Maybe Len mislead my comments.
> > >>
> > >> Anyway for the callback, my intend is platform_pci_choose_state
> > accept
> > >> the pm_message_t parameter, and it return an 'int', since
> platform
> > >> method possibly failed and then pci_choose_state translate the
> return
> > >> value to pci_power_t.
> > >
> > >You can't just retype around like that. You may want it take
> > >pci_power_t * as an argument, and then return 0/-ENODEV or
> something
> > >like that. But you can't retype between int and pm_message_t...
> > No, taking pci_power_t as an argument is meaningless. For ACPI, we
> > should know the exact sleep state, pm_message_t will tell us. But
> I'm ok
> > to let it return a pci_power_t, and the failure case returns
> > -ENODEV.
> 
> You can't put -ENODEV into pci_power_t ... but maybe we should create
> PCI_ERROR and pass it in cases like this one?
That makes sense, please do it.

> 
> > >> > Could you just revert those two patches? First one is very
> > >> > wrong. Second one might be fixed, but... See comments below.
> > >> I think the platform_pci_set_power_state should be ok, did you
> see it
> > >> causes oops?
> > >
> > >No its just ugly and uses __force in "creative" way. That one can
> be
> > >recovered.
> > Do you mean this?
> > 
> > > +   static int state_conv[] = {
> > > +           [0] = 0,
> > > +           [1] = 1,
> > > +           [2] = 2,
> > > +           [3] = 3,
> > > +           [4] = 3
> > > +   };
> > > +   int acpi_state = state_conv[(int __force) state];
> > 
> > The table should be
> >               [PCI_D0] = 0,
> > 
> > I'm not sure, but then could we use state_conv[state] directly? It
> seems
> 
> I think so. Of course it is wrong, but it is less wrong than forcing
> it to integer than index, without using macros at all.
> 
> Or perhaps you should do
> 
> switch (state) {
> case PCI_D0: ...
> }
> 
> ...and handle default case somehow.
That's ok for me. I'll change it later.

Thanks,
Shaohua

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Pavel Machek]

Hi!

> > You can't put -ENODEV into pci_power_t ... but maybe we should create
> > PCI_ERROR and pass it in cases like this one?
> That makes sense, please do it.

Added:

#define PCI_POWER_ERROR ((pci_power_t __force) -1)

									Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Pavel Machek]

Hi!

> Will this do it for the moment?

Its certainly better.

What about

> > > +static int acpi_pci_set_power_state(struct pci_dev *dev,
> > pci_power_t state)
> > > +{
> > > +     acpi_handle handle = DEVICE_ACPI_HANDLE(&dev->dev);
> > > +     static int state_conv[] = {
> > > +             [0] = 0,
> > > +             [1] = 1,
> > > +             [2] = 2,
> > > +             [3] = 3,
> > > +             [4] = 3
> > > +     };
> > > +     int acpi_state = state_conv[(int __force) state];

...this force? Then platform_pci_choose_state should not be NULL by
default and acpi_pci_choose_state should really have some more
reasonable calling convention.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Pavel Machek]

Hi!

> > Yes, but it is needed. There are many drivers, and they look at
> > numerical value of PMSG_*. I'm proceeding in steps. I hopefully killed
> > all direct accesses to the constants, and will switch constants to
> > something else... But that is going to be tommorow (need some sleep).
> The patches are going to acquire correct PCI device sleep state for
> suspend/resume. We discussed the issue several months ago. My plan is we
> first introduce 'platform_pci_set_power_state', then merge the
> 'platform_pci_choose_state' patch after Pavel's pm_message_t conversion
> finished. Maybe Len mislead my comments. 
> 
> Anyway for the callback, my intend is platform_pci_choose_state accept
> the pm_message_t parameter, and it return an 'int', since platform
> method possibly failed and then pci_choose_state translate the return
> value to pci_power_t.

You can't just retype around like that. You may want it take
pci_power_t * as an argument, and then return 0/-ENODEV or something
like that. But you can't retype between int and pm_message_t...

Plus that function should have a documentation somewhere!

> > Could you just revert those two patches? First one is very
> > wrong. Second one might be fixed, but... See comments below.
> I think the platform_pci_set_power_state should be ok, did you see it
> causes oops?

No its just ugly and uses __force in "creative" way. That one can be
recovered.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Greg KH]

On Mon, Mar 21, 2005 at 06:27:33PM -0800, Andrew Morton wrote:
> OK, well unless someone has objections I'll just send all these
> 
> swsusp-add-missing-refrigerator-calls.patch
> suspend-to-ram-update-videotxt-with-more-systems.patch
> pm-remove-obsolete-pm_-from-vtc.patch
> swsusp-small-updates.patch
> swsusp-1-1-kill-swsusp_restore.patch
> fix-pm_message_t-in-generic-code.patch
> fix-u32-vs-pm_message_t-in-usb.patch
> more-pm_message_t-fixes.patch
> fix-u32-vs-pm_message_t-confusion-in-oss.patch
> fix-u32-vs-pm_message_t-confusion-in-pcmcia.patch
> fix-u32-vs-pm_message_t-confusion-in-framebuffers.patch
> fix-u32-vs-pm_message_t-confusion-in-mmc.patch
> fix-u32-vs-pm_message_t-confusion-in-serials.patch
> fix-u32-vs-pm_message_t-in-macintosh.patch
> fix-u32-vs-pm_message_t-confusion-in-agp.patch
> 
> to Linus when he reappears and then I'll duck for cover and let you guys
> sort it out ;)

No objection from me, that's probably the best way for this to get into
the tree.

thanks,

greg k-h

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Len Brown]

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

Will this do it for the moment?

If so, lets use it until Pavel's flag-day is over -- when we'll send an
updated patch.

thanks,
-Len



[-- Attachment #2: acpi_pci_choose_state_tbd.patch --]
[-- Type: text/plain, Size: 744 bytes --]

===== drivers/pci/pci-acpi.c 1.4 vs edited =====
--- 1.4/drivers/pci/pci-acpi.c	2005-03-03 04:28:23 -05:00
+++ edited/drivers/pci/pci-acpi.c	2005-03-21 22:59:39 -05:00
@@ -237,19 +237,8 @@
 
 static int acpi_pci_choose_state(struct pci_dev *pdev, pm_message_t state)
 {
-	char dstate_str[] = "_S0D";
-	acpi_status status;
-	unsigned long val;
-	struct device *dev = &pdev->dev;
+	/* TBD */
 
-	/* Fixme: the check is wrong after pm_message_t is a struct */
-	if ((state >= PM_SUSPEND_MAX) || !DEVICE_ACPI_HANDLE(dev))
-		return -EINVAL;
-	dstate_str[2] += state;	/* _S1D, _S2D, _S3D, _S4D */
-	status = acpi_evaluate_integer(DEVICE_ACPI_HANDLE(dev), dstate_str,
-		NULL, &val);
-	if (ACPI_SUCCESS(status))
-		return val;
 	return -ENODEV;
 }
 

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Li Shaohua]

On Tue, 2005-03-22 at 09:35, Pavel Machek wrote:
> Hi!
> 
> > > and that says:
> > > 
> > > #define PMSG_FREEZE     ((__force pm_message_t) 3)
> > > 
> > > ... I certainly have _FREEZE defined as 1 in my local tree, but I
> do
> > > not see that change in -mm yet.
> > 
> > Both 2.6.12-rc1-mm1 and 2.6.12-rc1 have:
> > 
> > #define PMSG_FREEZE     ((__force pm_message_t) 3)
> > #define PMSG_SUSPEND    ((__force pm_message_t) 3)
> > #define PMSG_ON         ((__force pm_message_t) 0)
> > 
> > which looks odd.
> 
> Yes, but it is needed. There are many drivers, and they look at
> numerical value of PMSG_*. I'm proceeding in steps. I hopefully killed
> all direct accesses to the constants, and will switch constants to
> something else... But that is going to be tommorow (need some sleep).
The patches are going to acquire correct PCI device sleep state for
suspend/resume. We discussed the issue several months ago. My plan is we
first introduce 'platform_pci_set_power_state', then merge the
'platform_pci_choose_state' patch after Pavel's pm_message_t conversion
finished. Maybe Len mislead my comments. 

Anyway for the callback, my intend is platform_pci_choose_state accept
the pm_message_t parameter, and it return an 'int', since platform
method possibly failed and then pci_choose_state translate the return
value to pci_power_t.

> > > I reproduced it here.. I do not know who introduced
> > > platform_pci_choose_state, but it is *very* wrong. It returns
> > > it. Should it return pci_power_t? It probably should to match
> > > pci_choose_state, but that int is retyped to pm_message_t. Oops.
> > 
> > That change came from Len.  I've appended the two relevant patches
> below.
> > 
> > So hm.  We have incompatible changes in flight.  That doesn't happen
> very
> > often.
> > 
> > Could I suggest that you prepare a fixup against 2.6.12-rc1-mm1 and
> send
> > that to Len and myself?  If that fixup is not suitable for a
> 2.6.12-rc1
> > based tree then I can look after it until things get flushed out.
> 
> Could you just revert those two patches? First one is very
> wrong. Second one might be fixed, but... See comments below.
I think the platform_pci_set_power_state should be ok, did you see it
causes oops?

> 
> And they are both "dangerous" -- they introduce new and untested
> functionality while I'm trying to transition from int to
> pm_message_t. They also affect all the drivers.
> 
> Len, please Cc me on patches that affect suspend.
> 
> > @@ -17,6 +17,7 @@
> >  #include <acpi/acpi_bus.h>
> >  
> >  #include <linux/pci-acpi.h>
> > +#include "pci.h"
> 
> 
> Should be <linux/pci.h>?
I suppose it's not exported out side of PCI, so I used 'pci.h' 

> 
> > +static int acpi_pci_choose_state(struct pci_dev *pdev, pm_message_t
> state)
> > +{
> 
> Should return pci_power_t, probably.
Should return int as I said above.

> 
> > +     char dstate_str[] = "_S0D";
> > +     acpi_status status;
> > +     unsigned long val;
> > +     struct device *dev = &pdev->dev;
> > +
> > +     /* Fixme: the check is wrong after pm_message_t is a struct */
> 
> Exactly.
> 
> > +     if ((state >= PM_SUSPEND_MAX) || !DEVICE_ACPI_HANDLE(dev))
> 
> PM_SUSPEND_MAX and friends is going to disappear.
Yep, this should be fixed. 

> 
> > +             return -EINVAL;
> > +     dstate_str[2] += state; /* _S1D, _S2D, _S3D, _S4D */
> 
> Ugh, assumes numerical values of states actually meaning anything. It
> definitely should not. Should be switch(state.event), but that code
> is not merged, yet.... => I'll send code that switches pm_message_t to
> struct, tommorow. But it may compile-time break some obscure
> drivers...
> 
> > diff -Nru a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > --- a/drivers/pci/pci-acpi.c  2005-03-21 17:02:38 -08:00
> > +++ b/drivers/pci/pci-acpi.c  2005-03-21 17:02:38 -08:00
> > @@ -253,6 +253,24 @@
> >       return -ENODEV;
> >  }
> >  
> > +static int acpi_pci_set_power_state(struct pci_dev *dev,
> pci_power_t state)
> > +{
> > +     acpi_handle handle = DEVICE_ACPI_HANDLE(&dev->dev);
> > +     static int state_conv[] = {
> > +             [0] = 0,
> > +             [1] = 1,
> > +             [2] = 2,
> > +             [3] = 3,
> > +             [4] = 3
> > +     };
> > +     int acpi_state = state_conv[(int __force) state];
> 
> The table should be
>                 [PCI_D0] = 0,
> ...
Ok, please revert the 'platform_pci_choose_pci' patch, I will add it
after Pavel's conversion is finished. Or after Pavel's is done, I can
send a quick fix.

Thanks,
Shaohua

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> wrote:
>
> On Po 21-03-05 17:52:32, Andrew Morton wrote:
> > Pavel Machek <pavel@ucw.cz> wrote:
> > >
> > > > Could I suggest that you prepare a fixup against 2.6.12-rc1-mm1 and send
> > >  > that to Len and myself?  If that fixup is not suitable for a 2.6.12-rc1
> > >  > based tree then I can look after it until things get flushed out.
> > > 
> > >  Could you just revert those two patches? First one is very
> > >  wrong. Second one might be fixed, but... See comments below.
> > 
> > I could revert them locally, but that wouldn't gain us much.
> 
> You mean that Len has to revert them or revert is "ineffective"?

The patches are in Len's tree.

> > Greg hasn't taken the pm_message_t patches yet.  Perhaps that's for the best.
> > 
> > Perhaps I should just jam everything-from-Pavel into Linus's tree as soon
> > as he returns and then we can fix up the downstream fallout in the various
> > bk trees?
> 
> Yes, that would help a lot. I was waiting with
> "turn-pm_message_t-into-struct" until all pm_message_t patches reached
> Linus so that there's not a mess "in flight". Len's patch pretty much
> depends on pm_message_t already being converted... (and I'd prefer it
> to wait a while, so we can see which problems were introduced by
> conversion and which are due to ACPI BIOS bugs).

OK, well unless someone has objections I'll just send all these

swsusp-add-missing-refrigerator-calls.patch
suspend-to-ram-update-videotxt-with-more-systems.patch
pm-remove-obsolete-pm_-from-vtc.patch
swsusp-small-updates.patch
swsusp-1-1-kill-swsusp_restore.patch
fix-pm_message_t-in-generic-code.patch
fix-u32-vs-pm_message_t-in-usb.patch
more-pm_message_t-fixes.patch
fix-u32-vs-pm_message_t-confusion-in-oss.patch
fix-u32-vs-pm_message_t-confusion-in-pcmcia.patch
fix-u32-vs-pm_message_t-confusion-in-framebuffers.patch
fix-u32-vs-pm_message_t-confusion-in-mmc.patch
fix-u32-vs-pm_message_t-confusion-in-serials.patch
fix-u32-vs-pm_message_t-in-macintosh.patch
fix-u32-vs-pm_message_t-confusion-in-agp.patch

to Linus when he reappears and then I'll duck for cover and let you guys
sort it out ;)

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Pavel Machek]

On Po 21-03-05 17:52:32, Andrew Morton wrote:
> Pavel Machek <pavel@ucw.cz> wrote:
> >
> > > Could I suggest that you prepare a fixup against 2.6.12-rc1-mm1 and send
> >  > that to Len and myself?  If that fixup is not suitable for a 2.6.12-rc1
> >  > based tree then I can look after it until things get flushed out.
> > 
> >  Could you just revert those two patches? First one is very
> >  wrong. Second one might be fixed, but... See comments below.
> 
> I could revert them locally, but that wouldn't gain us much.

You mean that Len has to revert them or revert is "ineffective"?

> Greg hasn't taken the pm_message_t patches yet.  Perhaps that's for the best.
> 
> Perhaps I should just jam everything-from-Pavel into Linus's tree as soon
> as he returns and then we can fix up the downstream fallout in the various
> bk trees?

Yes, that would help a lot. I was waiting with
"turn-pm_message_t-into-struct" until all pm_message_t patches reached
Linus so that there's not a mess "in flight". Len's patch pretty much
depends on pm_message_t already being converted... (and I'd prefer it
to wait a while, so we can see which problems were introduced by
conversion and which are due to ACPI BIOS bugs).

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Dave Jones]

On Mon, Mar 21, 2005 at 05:06:23PM -0800, Andrew Morton wrote:

 > # drivers/pci/pci-acpi.c
 > #   2005/03/19 00:15:24-05:00 len.brown@intel.com +46 -1
 > #   add platform_pci_choose_state()
 > # 
 > diff -Nru a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
 > --- a/drivers/pci/pci-acpi.c	2005-03-21 17:01:44 -08:00
 > +++ b/drivers/pci/pci-acpi.c	2005-03-21 17:01:44 -08:00
 > @@ -1,6 +1,6 @@
 >  /*
 >   * File:	pci-acpi.c
 > - * Purpose:	Provide PCI support in ACPI
 > + * Purpose:	Provde PCI support in ACPI

Oops.

		Dave

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> wrote:
>
> > Could I suggest that you prepare a fixup against 2.6.12-rc1-mm1 and send
>  > that to Len and myself?  If that fixup is not suitable for a 2.6.12-rc1
>  > based tree then I can look after it until things get flushed out.
> 
>  Could you just revert those two patches? First one is very
>  wrong. Second one might be fixed, but... See comments below.

I could revert them locally, but that wouldn't gain us much.

Greg hasn't taken the pm_message_t patches yet.  Perhaps that's for the best.

Perhaps I should just jam everything-from-Pavel into Linus's tree as soon
as he returns and then we can fix up the downstream fallout in the various
bk trees?

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Pavel Machek]

Hi!

> And they are both "dangerous" -- they introduce new and untested
> functionality while I'm trying to transition from int to
> pm_message_t. They also affect all the drivers.

Actually, there's one even more severe problem with
platform_pci_choose_state...

If we are doing freeze for swsusp snapshot (or freeze for kexec or
something similar, that ACPI does not know about), it is very wrong to
ask ACPI to tell us power levels for devices. ACPI does not even know
about those states, it can not tell us anything meaningfull.

So if this hook is to be reintroduced, it should go down in the
function, and only trigger for ACPI S3 and ACPI S1 cases. Maybe for
swsusp/plaform (== ACPI S4).

But I'd prefer the hook to go away for now, it clearly needs
infrastructure that is not yet there, and provides nothing.

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Pavel Machek]

Hi!

> > and that says:
> > 
> > #define PMSG_FREEZE     ((__force pm_message_t) 3)
> > 
> > ... I certainly have _FREEZE defined as 1 in my local tree, but I do
> > not see that change in -mm yet.
> 
> Both 2.6.12-rc1-mm1 and 2.6.12-rc1 have:
> 
> #define PMSG_FREEZE     ((__force pm_message_t) 3)
> #define PMSG_SUSPEND    ((__force pm_message_t) 3)
> #define PMSG_ON         ((__force pm_message_t) 0)
> 
> which looks odd.

Yes, but it is needed. There are many drivers, and they look at
numerical value of PMSG_*. I'm proceeding in steps. I hopefully killed
all direct accesses to the constants, and will switch constants to
something else... But that is going to be tommorow (need some sleep).

> > I reproduced it here.. I do not know who introduced
> > platform_pci_choose_state, but it is *very* wrong. It returns
> > it. Should it return pci_power_t? It probably should to match
> > pci_choose_state, but that int is retyped to pm_message_t. Oops.
> 
> That change came from Len.  I've appended the two relevant patches below.
> 
> So hm.  We have incompatible changes in flight.  That doesn't happen very
> often.
> 
> Could I suggest that you prepare a fixup against 2.6.12-rc1-mm1 and send
> that to Len and myself?  If that fixup is not suitable for a 2.6.12-rc1
> based tree then I can look after it until things get flushed out.

Could you just revert those two patches? First one is very
wrong. Second one might be fixed, but... See comments below.

And they are both "dangerous" -- they introduce new and untested
functionality while I'm trying to transition from int to
pm_message_t. They also affect all the drivers.

Len, please Cc me on patches that affect suspend.

> @@ -17,6 +17,7 @@
>  #include <acpi/acpi_bus.h>
>  
>  #include <linux/pci-acpi.h>
> +#include "pci.h"


Should be <linux/pci.h>?

> +static int acpi_pci_choose_state(struct pci_dev *pdev, pm_message_t state)
> +{

Should return pci_power_t, probably.

> +	char dstate_str[] = "_S0D";
> +	acpi_status status;
> +	unsigned long val;
> +	struct device *dev = &pdev->dev;
> +
> +	/* Fixme: the check is wrong after pm_message_t is a struct */

Exactly.

> +	if ((state >= PM_SUSPEND_MAX) || !DEVICE_ACPI_HANDLE(dev))

PM_SUSPEND_MAX and friends is going to disappear.

> +		return -EINVAL;
> +	dstate_str[2] += state;	/* _S1D, _S2D, _S3D, _S4D */

Ugh, assumes numerical values of states actually meaning anything. It
definitely should not. Should be switch(state.event), but that code
is not merged, yet.... => I'll send code that switches pm_message_t to
struct, tommorow. But it may compile-time break some obscure drivers...

> diff -Nru a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> --- a/drivers/pci/pci-acpi.c	2005-03-21 17:02:38 -08:00
> +++ b/drivers/pci/pci-acpi.c	2005-03-21 17:02:38 -08:00
> @@ -253,6 +253,24 @@
>  	return -ENODEV;
>  }
>  
> +static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> +{
> +	acpi_handle handle = DEVICE_ACPI_HANDLE(&dev->dev);
> +	static int state_conv[] = {
> +		[0] = 0,
> +		[1] = 1,
> +		[2] = 2,
> +		[3] = 3,
> +		[4] = 3
> +	};
> +	int acpi_state = state_conv[(int __force) state];

The table should be
		[PCI_D0] = 0,
...

and then it should not need __force.

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Andrew Morton]

Pavel Machek <pavel@suse.cz> wrote:
>
> Hi!
> 
> > > On Monday, 21 of March 2005 11:51, you wrote:
> > > > 
> > > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.12-rc1/2.6.12-rc1-mm1/
> > > 
> > > I get the following BUG every time I try to suspend my box to disk.
> > 
> > Pavel, that's the BUG() in pci_choose_state().  I did have some
> > reject-fixing to do on that wrt a change in Greg's tree, so maybe there was
> > some incompatible intent in there.
> > 
> > I dunno why pci_choose_state() is saying that it received PCI_D1, when
> > prepare_devices() is passing down PMSG_FREEZE?
> 
> Uf, I don't know what version that was.. I think I have
> 
> VERSION = 2
> PATCHLEVEL = 6
> SUBLEVEL = 12
> EXTRAVERSION =-rc1-mm1

yes, the report was against 2.6.12-rc1-mm1.

> and that says:
> 
> #define PMSG_FREEZE     ((__force pm_message_t) 3)
> 
> ... I certainly have _FREEZE defined as 1 in my local tree, but I do
> not see that change in -mm yet.

Both 2.6.12-rc1-mm1 and 2.6.12-rc1 have:

#define PMSG_FREEZE     ((__force pm_message_t) 3)
#define PMSG_SUSPEND    ((__force pm_message_t) 3)
#define PMSG_ON         ((__force pm_message_t) 0)

which looks odd.

> Possibly pm.h changes went in faster than pci.c or something like
> that?

grep says that 2.6.12-rc1-mm1 has these patches from you:

fix-suspend-resume-on-via-velocity.patch
x86-fix-esp-corruption-cpu-bug-take-2.patch
swsusp-add-missing-refrigerator-calls.patch
suspend-to-ram-update-videotxt-with-more-systems.patch
pm-remove-obsolete-pm_-from-vtc.patch
swsusp-small-updates.patch
swsusp-1-1-kill-swsusp_restore.patch
pcmcia-id_table-for-orinoco_cs.patch
fix-pm_message_t-in-generic-code.patch
fix-u32-vs-pm_message_t-in-usb.patch
more-pm_message_t-fixes.patch
fix-u32-vs-pm_message_t-confusion-in-oss.patch
fix-u32-vs-pm_message_t-confusion-in-pcmcia.patch
fix-u32-vs-pm_message_t-confusion-in-framebuffers.patch
fix-u32-vs-pm_message_t-confusion-in-mmc.patch
fix-u32-vs-pm_message_t-confusion-in-serials.patch
fix-u32-vs-pm_message_t-in-macintosh.patch
fix-u32-vs-pm_message_t-confusion-in-agp.patch

> I reproduced it here.. I do not know who introduced
> platform_pci_choose_state, but it is *very* wrong. It returns
> it. Should it return pci_power_t? It probably should to match
> pci_choose_state, but that int is retyped to pm_message_t. Oops.

That change came from Len.  I've appended the two relevant patches below.

So hm.  We have incompatible changes in flight.  That doesn't happen very
often.

Could I suggest that you prepare a fixup against 2.6.12-rc1-mm1 and send
that to Len and myself?  If that fixup is not suitable for a 2.6.12-rc1
based tree then I can look after it until things get flushed out.

(Len, platform_pci_set_power_state shouldn't be initialised to NULL).

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/03/19 00:15:48-05:00 len.brown@intel.com 
#   [ACPI] PCI can now get suspend state from firmware
#   
#   pci_choose_state() can now call
#   	platform_pci_choose_state()
#   		and ACPI can answer
#   
#   http://bugzilla.kernel.org/show_bug.cgi?id=4277
#   
#   Signed-off-by: David Shaohua Li <shaohua.li@intel.com>
#   Signed-off-by: Len Brown <len.brown@intel.com>
# 
# drivers/pci/pci.h
#   2005/03/19 00:15:24-05:00 len.brown@intel.com +3 -0
#   add platform_pci_choose_state()
# 
# drivers/pci/pci.c
#   2005/03/19 00:15:24-05:00 len.brown@intel.com +7 -0
#   add platform_pci_choose_state()
# 
# drivers/pci/pci-acpi.c
#   2005/03/19 00:15:24-05:00 len.brown@intel.com +46 -1
#   add platform_pci_choose_state()
# 
diff -Nru a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
--- a/drivers/pci/pci-acpi.c	2005-03-21 17:01:44 -08:00
+++ b/drivers/pci/pci-acpi.c	2005-03-21 17:01:44 -08:00
@@ -1,6 +1,6 @@
 /*
  * File:	pci-acpi.c
- * Purpose:	Provide PCI support in ACPI
+ * Purpose:	Provde PCI support in ACPI
  *
  * Copyright (C) 2005 David Shaohua Li <shaohua.li@intel.com>
  * Copyright (C) 2004 Tom Long Nguyen <tom.l.nguyen@intel.com>
@@ -17,6 +17,7 @@
 #include <acpi/acpi_bus.h>
 
 #include <linux/pci-acpi.h>
+#include "pci.h"
 
 static u32 ctrlset_buf[3] = {0, 0, 0};
 static u32 global_ctrlsets = 0;
@@ -209,6 +210,49 @@
 }
 EXPORT_SYMBOL(pci_osc_control_set);
 
+/*
+ * _SxD returns the D-state with the highest power
+ * (lowest D-state number) supported in the S-state "x".
+ *
+ * If the devices does not have a _PRW
+ * (Power Resources for Wake) supporting system wakeup from "x"
+ * then the OS is free to choose a lower power (higher number
+ * D-state) than the return value from _SxD.
+ *
+ * But if _PRW is enabled at S-state "x", the OS
+ * must not choose a power lower than _SxD --
+ * unless the device has an _SxW method specifying
+ * the lowest power (highest D-state number) the device
+ * may enter while still able to wake the system.
+ *
+ * ie. depending on global OS policy:
+ *
+ * if (_PRW at S-state x)
+ *	choose from highest power _SxD to lowest power _SxW
+ * else // no _PRW at S-state x
+ * 	choose highest power _SxD or any lower power
+ *
+ * currently we simply return _SxD, if present.
+ */
+
+static int acpi_pci_choose_state(struct pci_dev *pdev, pm_message_t state)
+{
+	char dstate_str[] = "_S0D";
+	acpi_status status;
+	unsigned long val;
+	struct device *dev = &pdev->dev;
+
+	/* Fixme: the check is wrong after pm_message_t is a struct */
+	if ((state >= PM_SUSPEND_MAX) || !DEVICE_ACPI_HANDLE(dev))
+		return -EINVAL;
+	dstate_str[2] += state;	/* _S1D, _S2D, _S3D, _S4D */
+	status = acpi_evaluate_integer(DEVICE_ACPI_HANDLE(dev), dstate_str,
+		NULL, &val);
+	if (ACPI_SUCCESS(status))
+		return val;
+	return -ENODEV;
+}
+
 /* ACPI bus type */
 static int pci_acpi_find_device(struct device *dev, acpi_handle *handle)
 {
@@ -255,6 +299,7 @@
 	ret = register_acpi_bus_type(&pci_acpi_bus);
 	if (ret)
 		return 0;
+	platform_pci_choose_state = acpi_pci_choose_state;
 	return 0;
 }
 arch_initcall(pci_acpi_init);
diff -Nru a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c	2005-03-21 17:01:44 -08:00
+++ b/drivers/pci/pci.c	2005-03-21 17:01:44 -08:00
@@ -317,12 +317,19 @@
  * Returns PCI power state suitable for given device and given system
  * message.
  */
+int (*platform_pci_choose_state)(struct pci_dev *dev, pm_message_t state) = NULL;
 
 pci_power_t pci_choose_state(struct pci_dev *dev, u32 state)
 {
+	int	ret;
 	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
 		return PCI_D0;
 
+	if (platform_pci_choose_state) {
+		ret = platform_pci_choose_state(dev, state);
+		if (ret >= 0)
+			state = ret;
+	}
 	switch (state) {
 	case 0:	return PCI_D0;
 	case 2: return PCI_D2;
diff -Nru a/drivers/pci/pci.h b/drivers/pci/pci.h
--- a/drivers/pci/pci.h	2005-03-21 17:01:44 -08:00
+++ b/drivers/pci/pci.h	2005-03-21 17:01:44 -08:00
@@ -11,6 +11,9 @@
 				  void (*alignf)(void *, struct resource *,
 					  	 unsigned long, unsigned long),
 				  void *alignf_data);
+/* Firmware callbacks */
+extern int (*platform_pci_choose_state)(struct pci_dev *dev, pm_message_t state);
+
 /* PCI /proc functions */
 #ifdef CONFIG_PROC_FS
 extern int pci_proc_attach_device(struct pci_dev *dev);



# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/03/19 00:16:18-05:00 len.brown@intel.com 
#   [ACPI] pci_set_power_state() now calls
#   	platform_pci_set_power_state()
#   		and ACPI can answer
#   
#   http://bugzilla.kernel.org/show_bug.cgi?id=4277
#   
#   Signed-off-by: David Shaohua Li <shaohua.li@intel.com>
#   Signed-off-by: Len Brown <len.brown@intel.com>
# 
# drivers/pci/pci.h
#   2005/03/03 04:20:56-05:00 len.brown@intel.com +1 -0
#   pci_set_power_state() now calls platform_pci_set_power_state()
# 
# drivers/pci/pci.c
#   2005/03/03 04:20:56-05:00 len.brown@intel.com +9 -2
#   pci_set_power_state() now calls platform_pci_set_power_state()
# 
# drivers/pci/pci-acpi.c
#   2005/03/03 04:28:23-05:00 len.brown@intel.com +19 -0
#   pci_set_power_state() now calls platform_pci_set_power_state()
# 
# drivers/acpi/bus.c
#   2005/03/03 04:20:56-05:00 len.brown@intel.com +7 -1
#   pci_set_power_state() now calls platform_pci_set_power_state()
# 
diff -Nru a/drivers/acpi/bus.c b/drivers/acpi/bus.c
--- a/drivers/acpi/bus.c	2005-03-21 17:02:38 -08:00
+++ b/drivers/acpi/bus.c	2005-03-21 17:02:38 -08:00
@@ -212,6 +212,12 @@
 		ACPI_DEBUG_PRINT((ACPI_DB_WARN, "Device is not power manageable\n"));
 		return_VALUE(-ENODEV);
 	}
+	/*
+	 * Get device's current power state if it's unknown
+	 * This means device power state isn't initialized or previous setting failed
+	 */
+	if (device->power.state == ACPI_STATE_UNKNOWN)
+		acpi_bus_get_power(device->handle, &device->power.state);
 	if (state == device->power.state) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device is already at D%d\n", state));
 		return_VALUE(0);
@@ -231,7 +237,7 @@
 	 * On transitions to a high-powered state we first apply power (via
 	 * power resources) then evalute _PSx.  Conversly for transitions to
 	 * a lower-powered state.
-	 */ 
+	 */
 	if (state < device->power.state) {
 		if (device->power.flags.power_resources) {
 			result = acpi_power_transition(device, state);
diff -Nru a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
--- a/drivers/pci/pci-acpi.c	2005-03-21 17:02:38 -08:00
+++ b/drivers/pci/pci-acpi.c	2005-03-21 17:02:38 -08:00
@@ -253,6 +253,24 @@
 	return -ENODEV;
 }
 
+static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
+{
+	acpi_handle handle = DEVICE_ACPI_HANDLE(&dev->dev);
+	static int state_conv[] = {
+		[0] = 0,
+		[1] = 1,
+		[2] = 2,
+		[3] = 3,
+		[4] = 3
+	};
+	int acpi_state = state_conv[(int __force) state];
+
+	if (!handle)
+		return -ENODEV;
+	return acpi_bus_set_power(handle, acpi_state);
+}
+
+
 /* ACPI bus type */
 static int pci_acpi_find_device(struct device *dev, acpi_handle *handle)
 {
@@ -300,6 +318,7 @@
 	if (ret)
 		return 0;
 	platform_pci_choose_state = acpi_pci_choose_state;
+	platform_pci_set_power_state = acpi_pci_set_power_state;
 	return 0;
 }
 arch_initcall(pci_acpi_init);
diff -Nru a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c	2005-03-21 17:02:38 -08:00
+++ b/drivers/pci/pci.c	2005-03-21 17:02:38 -08:00
@@ -240,7 +240,7 @@
  * -EIO if device does not support PCI PM.
  * 0 if we can successfully change the power state.
  */
-
+int (*platform_pci_set_power_state)(struct pci_dev *dev, pci_power_t t) = NULL;
 int
 pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 {
@@ -304,8 +304,15 @@
 		msleep(10);
 	else if (state == PCI_D2 || dev->current_state == PCI_D2)
 		udelay(200);
-	dev->current_state = state;
 
+	/*
+	 * Give firmware a chance to be called, such as ACPI _PRx, _PSx
+	 * Firmware method after natice method ?
+	 */
+	if (platform_pci_set_power_state)
+		platform_pci_set_power_state(dev, state);
+
+	dev->current_state = state;
 	return 0;
 }
 
diff -Nru a/drivers/pci/pci.h b/drivers/pci/pci.h
--- a/drivers/pci/pci.h	2005-03-21 17:02:38 -08:00
+++ b/drivers/pci/pci.h	2005-03-21 17:02:38 -08:00
@@ -13,6 +13,7 @@
 				  void *alignf_data);
 /* Firmware callbacks */
 extern int (*platform_pci_choose_state)(struct pci_dev *dev, pm_message_t state);
+extern int (*platform_pci_set_power_state)(struct pci_dev *dev, pci_power_t state);
 
 /* PCI /proc functions */
 #ifdef CONFIG_PROC_FS

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Pavel Machek]

Hi!

> > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.12-rc1/2.6.12-rc1-mm1/
> > 
> > I get the following BUG every time I try to suspend my box to disk.
> 
> Pavel, that's the BUG() in pci_choose_state().  I did have some
> reject-fixing to do on that wrt a change in Greg's tree, so maybe there was
> some incompatible intent in there.
> 
> I dunno why pci_choose_state() is saying that it received PCI_D1, when
> prepare_devices() is passing down PMSG_FREEZE?

This works it around:

--- clean-mm/drivers/pci/pci.c	2005-03-21 11:39:32.000000000 +0100
+++ linux-mm/drivers/pci/pci.c	2005-03-22 01:41:48.000000000 +0100
@@ -376,11 +376,13 @@
 	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
 		return PCI_D0;
 
+#if 0
 	if (platform_pci_choose_state) {
 		ret = platform_pci_choose_state(dev, state);
 		if (ret >= 0)
 			state = ret;
 	}
+#endif
 	switch (state) {
 	case 0: return PCI_D0;
 	case 3: return PCI_D3hot;

platform_pci_choose_state is very wrong, and it would be nice to just
revert the patch that introduced it. pm_message_t is going to became a
structure, and I don't want to have another place to fixup.

Hmm, it looks like I should do switch to the structure *now* so that
pm_message_t becomes incompatible with int and people can't get it
wrong...

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Pavel Machek]

Hi!

> > On Monday, 21 of March 2005 11:51, you wrote:
> > > 
> > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.12-rc1/2.6.12-rc1-mm1/
> > 
> > I get the following BUG every time I try to suspend my box to disk.
> 
> Pavel, that's the BUG() in pci_choose_state().  I did have some
> reject-fixing to do on that wrt a change in Greg's tree, so maybe there was
> some incompatible intent in there.
> 
> I dunno why pci_choose_state() is saying that it received PCI_D1, when
> prepare_devices() is passing down PMSG_FREEZE?

Uf, I don't know what version that was.. I think I have

VERSION = 2
PATCHLEVEL = 6
SUBLEVEL = 12
EXTRAVERSION =-rc1-mm1

and that says:

#define PMSG_FREEZE     ((__force pm_message_t) 3)

... I certainly have _FREEZE defined as 1 in my local tree, but I do
not see that change in -mm yet.

Possibly pm.h changes went in faster than pci.c or something like
that?

I reproduced it here.. I do not know who introduced
platform_pci_choose_state, but it is *very* wrong. It returns
it. Should it return pci_power_t? It probably should to match
pci_choose_state, but that int is retyped to pm_message_t. Oops.

								Pavel


> 
> 
> > Greets,
> > Rafael
> > 
> > 
> > Stopping tasks: ===================================================================|
> > Freeing memory... done (66711 pages freed)
> > They asked me for state 1
> > ----------- [cut here ] --------- [please bite here ] ---------
> > Kernel BUG at pci:389
> > invalid operand: 0000 [1]
> > CPU 0
> > Modules linked in: usbserial parport_pc lp parport thermal processor fan button battery ac soundcore snd_page_alloc ipt_TOS ipt_LOG ipt_limit v
> > Pid: 9141, comm: do_acpi_sleep Not tainted 2.6.12-rc1-mm1
> > RIP: 0010:[<ffffffff80283a70>] <ffffffff80283a70>{pci_choose_state+96}
> > RSP: 0000:ffff810020fbfd78  EFLAGS: 00010292
> > RAX: 000000000000001d RBX: 0000000000000001 RCX: 0000000000000000
> > RDX: 0000000000000000 RSI: 00000000000044e0 RDI: ffffffff8041d140
> > RBP: ffff81002fc151c0 R08: 0000000000000000 R09: ffff81002a535c48
> > R10: 00000000ffffffff R11: 0000000000000000 R12: ffff81002fc151c0
> > R13: 0000000000000003 R14: 0000000000000000 R15: 0000000000000080
> > FS:  00002aaaab28b800(0000) GS:ffffffff8055c840(0000) knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> > CR2: 00002aaaaaac2000 CR3: 000000001dd8a000 CR4: 00000000000006e0
> > Process do_acpi_sleep (pid: 9141, threadinfo ffff810020fbe000, task ffff810020d527e0)
> > Stack: ffff81002c349628 0000000000000000 ffff81002c349628 ffffffff8032218a
> >        ffff81002fc149a8 0000000000000000 ffff81002fc15230 0000000000000000
> >        ffffffff8048f680 0000000000000003
> > Call Trace:<ffffffff8032218a>{usb_hcd_pci_suspend+74} <ffffffff8028519e>{pci_device_suspend+30}
> >        <ffffffff802ee3d2>{suspend_device+50} <ffffffff802ee4f1>{device_suspend+129}
> >        <ffffffff80166ceb>{prepare_devices+11} <ffffffff80167095>{pm_suspend_disk+21}
> >        <ffffffff80164206>{enter_state+70} <ffffffff8016442d>{state_store+109}
> >        <ffffffff801f275f>{subsys_attr_store+31} <ffffffff801f2c1c>{sysfs_write_file+204}
> >        <ffffffff8019c6c9>{vfs_write+233} <ffffffff8019c863>{sys_write+83}
> >        <ffffffff8010f092>{system_call+126}
> > 
> > Code: 0f 0b 7a 3e 3e 80 ff ff ff ff 85 01 31 d2 66 90 48 8b 5c 24
> > RIP <ffffffff80283a70>{pci_choose_state+96} RSP <ffff810020fbfd78>
> > 
> > 
> > 
> > -- 
> > - Would you tell me, please, which way I ought to go from here?
> > - That depends a good deal on where you want to get to.
> > 		-- Lewis Carroll "Alice's Adventures in Wonderland"

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Andrew Morton]

"Rafael J. Wysocki" <rjw@sisk.pl> wrote:
>
> Hi,
> 
> On Monday, 21 of March 2005 11:51, you wrote:
> > 
> > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.12-rc1/2.6.12-rc1-mm1/
> 
> I get the following BUG every time I try to suspend my box to disk.

Pavel, that's the BUG() in pci_choose_state().  I did have some
reject-fixing to do on that wrt a change in Greg's tree, so maybe there was
some incompatible intent in there.

I dunno why pci_choose_state() is saying that it received PCI_D1, when
prepare_devices() is passing down PMSG_FREEZE?



> Greets,
> Rafael
> 
> 
> Stopping tasks: ===================================================================|
> Freeing memory... done (66711 pages freed)
> They asked me for state 1
> ----------- [cut here ] --------- [please bite here ] ---------
> Kernel BUG at pci:389
> invalid operand: 0000 [1]
> CPU 0
> Modules linked in: usbserial parport_pc lp parport thermal processor fan button battery ac soundcore snd_page_alloc ipt_TOS ipt_LOG ipt_limit v
> Pid: 9141, comm: do_acpi_sleep Not tainted 2.6.12-rc1-mm1
> RIP: 0010:[<ffffffff80283a70>] <ffffffff80283a70>{pci_choose_state+96}
> RSP: 0000:ffff810020fbfd78  EFLAGS: 00010292
> RAX: 000000000000001d RBX: 0000000000000001 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: 00000000000044e0 RDI: ffffffff8041d140
> RBP: ffff81002fc151c0 R08: 0000000000000000 R09: ffff81002a535c48
> R10: 00000000ffffffff R11: 0000000000000000 R12: ffff81002fc151c0
> R13: 0000000000000003 R14: 0000000000000000 R15: 0000000000000080
> FS:  00002aaaab28b800(0000) GS:ffffffff8055c840(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> CR2: 00002aaaaaac2000 CR3: 000000001dd8a000 CR4: 00000000000006e0
> Process do_acpi_sleep (pid: 9141, threadinfo ffff810020fbe000, task ffff810020d527e0)
> Stack: ffff81002c349628 0000000000000000 ffff81002c349628 ffffffff8032218a
>        ffff81002fc149a8 0000000000000000 ffff81002fc15230 0000000000000000
>        ffffffff8048f680 0000000000000003
> Call Trace:<ffffffff8032218a>{usb_hcd_pci_suspend+74} <ffffffff8028519e>{pci_device_suspend+30}
>        <ffffffff802ee3d2>{suspend_device+50} <ffffffff802ee4f1>{device_suspend+129}
>        <ffffffff80166ceb>{prepare_devices+11} <ffffffff80167095>{pm_suspend_disk+21}
>        <ffffffff80164206>{enter_state+70} <ffffffff8016442d>{state_store+109}
>        <ffffffff801f275f>{subsys_attr_store+31} <ffffffff801f2c1c>{sysfs_write_file+204}
>        <ffffffff8019c6c9>{vfs_write+233} <ffffffff8019c863>{sys_write+83}
>        <ffffffff8010f092>{system_call+126}
> 
> Code: 0f 0b 7a 3e 3e 80 ff ff ff ff 85 01 31 d2 66 90 48 8b 5c 24
> RIP <ffffffff80283a70>{pci_choose_state+96} RSP <ffff810020fbfd78>
> 
> 
> 
> -- 
> - Would you tell me, please, which way I ought to go from here?
> - That depends a good deal on where you want to get to.
> 		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Rafael J. Wysocki]

Hi,

On Monday, 21 of March 2005 11:51, you wrote:
> 
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.12-rc1/2.6.12-rc1-mm1/

I get the following BUG every time I try to suspend my box to disk.

Greets,
Rafael


Stopping tasks: ===================================================================|
Freeing memory... done (66711 pages freed)
They asked me for state 1
----------- [cut here ] --------- [please bite here ] ---------
Kernel BUG at pci:389
invalid operand: 0000 [1]
CPU 0
Modules linked in: usbserial parport_pc lp parport thermal processor fan button battery ac soundcore snd_page_alloc ipt_TOS ipt_LOG ipt_limit v
Pid: 9141, comm: do_acpi_sleep Not tainted 2.6.12-rc1-mm1
RIP: 0010:[<ffffffff80283a70>] <ffffffff80283a70>{pci_choose_state+96}
RSP: 0000:ffff810020fbfd78  EFLAGS: 00010292
RAX: 000000000000001d RBX: 0000000000000001 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 00000000000044e0 RDI: ffffffff8041d140
RBP: ffff81002fc151c0 R08: 0000000000000000 R09: ffff81002a535c48
R10: 00000000ffffffff R11: 0000000000000000 R12: ffff81002fc151c0
R13: 0000000000000003 R14: 0000000000000000 R15: 0000000000000080
FS:  00002aaaab28b800(0000) GS:ffffffff8055c840(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00002aaaaaac2000 CR3: 000000001dd8a000 CR4: 00000000000006e0
Process do_acpi_sleep (pid: 9141, threadinfo ffff810020fbe000, task ffff810020d527e0)
Stack: ffff81002c349628 0000000000000000 ffff81002c349628 ffffffff8032218a
       ffff81002fc149a8 0000000000000000 ffff81002fc15230 0000000000000000
       ffffffff8048f680 0000000000000003
Call Trace:<ffffffff8032218a>{usb_hcd_pci_suspend+74} <ffffffff8028519e>{pci_device_suspend+30}
       <ffffffff802ee3d2>{suspend_device+50} <ffffffff802ee4f1>{device_suspend+129}
       <ffffffff80166ceb>{prepare_devices+11} <ffffffff80167095>{pm_suspend_disk+21}
       <ffffffff80164206>{enter_state+70} <ffffffff8016442d>{state_store+109}
       <ffffffff801f275f>{subsys_attr_store+31} <ffffffff801f2c1c>{sysfs_write_file+204}
       <ffffffff8019c6c9>{vfs_write+233} <ffffffff8019c863>{sys_write+83}
       <ffffffff8010f092>{system_call+126}

Code: 0f 0b 7a 3e 3e 80 ff ff ff ff 85 01 31 d2 66 90 48 8b 5c 24
RIP <ffffffff80283a70>{pci_choose_state+96} RSP <ffff810020fbfd78>



-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"