[PATCH] kexec: fix pci device initialization fail after kexec (2.6.23-rc2). (Related to e1000 doesn't resume properly from standby)

[PATCH] kexec: fix pci device initialization fail after kexec (2.6.23-rc2). (Related to e1000 doesn't resume properly from standby)

[Joonwoo Park]

Hi.
I think that the pci_set_power_state() has bug.
The specification says that some delays is required.

Simon, please can I know whether it works to you?
It is seems to the things are related.

Joonwoo Park.

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 37c00f6..9f78064 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -493,8 +493,14 @@ pci_set_power_state(struct pci_dev *dev, pci_power_t state)
   * restore at least the BARs so that the device will be
   * accessible to its driver.
   */
- if (need_restore)
+ if (need_restore) {
+   /* The specification also says that "Must ensure that all of
+    * its PCI signal drivers remain disabled for the duration of
+    * the D3hot to D0 Uninitialized state transition".
+    */
+   msleep(10);
    pci_restore_bars(dev);
+ }

  return 0;
 }


2007/8/6, Simon Arlott <simon@fire.lp0.eu>:
> On Mon, August 6, 2007 11:44, Pavel Machek wrote:
> > Hi!
> >
> >> >00:0a.0 Ethernet controller: Intel Corp. 82546EB Gigabit Ethernet
> >> >Controller (Copper) (rev 01)
> >> >        Subsystem: Intel Corp.: Unknown device 1012
> >> >        Flags: bus master, 66Mhz, medium devsel, latency 32, IRQ 5
> >> >        Memory at e3020000 (64-bit, non-prefetchable) [size=128K]
> >> >        I/O ports at b000 [size=64]
> >> >        Capabilities: [dc] Power Management version 2
> >> >        Capabilities: [e4] PCI-X non-bridge device.
> >> >        Capabilities: [f0] Message Signalled Interrupts: 64bit+ Queue=0/0
> >> >        Enable-
> >> >
> >> >00:0a.1 Ethernet controller: Intel Corp. 82546EB Gigabit Ethernet
> >> >Controller (Copper) (rev 01)
> >> >        Subsystem: Intel Corp.: Unknown device 1012
> >> >        Flags: bus master, 66Mhz, medium devsel, latency 32, IRQ 12
> >> >        Memory at e3000000 (64-bit, non-prefetchable) [size=128K]
> >> >        I/O ports at b400 [size=64]
> >> >        Capabilities: [dc] Power Management version 2
> >> >        Capabilities: [e4] PCI-X non-bridge device.
> >> >        Capabilities: [f0] Message Signalled Interrupts: 64bit+ Queue=0/0
> >> >        Enable-
> >> >
> >> >[  950.132046] Stopping tasks ... done.
> >> >[  950.459794] Suspending console(s)
> >> >[  951.776277] pnp: Device 00:0c disabled.
> >> >[  951.776673] pnp: Device 00:0a disabled.
> >> >[  951.776984] pnp: Device 00:09 disabled.
> >> >[  951.777306] pnp: Device 00:08 disabled.
> >> >[  951.777786] ACPI: PCI interrupt for device 0000:00:11.5 disabled
> >> >[  951.995359] ACPI: PCI interrupt for device 0000:00:11.3 disabled
> >> >[  952.006094] ACPI: PCI interrupt for device 0000:00:11.2 disabled
> >> >[  952.022243] ACPI handle has no context!
> >> >[  952.033068] ACPI: PCI interrupt for device 0000:00:0c.2 disabled
> >> >[  952.044086] ACPI: PCI interrupt for device 0000:00:0c.1 disabled
> >> >[  952.055083] ACPI: PCI interrupt for device 0000:00:0c.0 disabled
> >> >[  952.282211] ACPI: PCI interrupt for device 0000:00:0a.1 disabled
> >> >[  952.282221] ACPI handle has no context!
> >> >[  952.537474] ACPI: PCI interrupt for device 0000:00:0a.0 disabled
> >> >[  952.537495] ACPI handle has no context!
> >> >
> >> >[  956.857085] Back to C!
> >
> > Are you sure that is standby? Looks like suspend-to-RAM to me.
>
> It's S1 (power-on suspend/standby), my BIOS/motherboard doesn't support S2 or S3.
>
> --
> Simon Arlott
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH] kexec: fix pci device initialization fail after kexec (2.6.23-rc2). (Related to e1000 doesn't resume properly from standby)

[Rafael J. Wysocki]

On Monday, 6 August 2007 15:42, Joonwoo Park wrote:
> Hi.
> I think that the pci_set_power_state() has bug.
> The specification says that some delays is required.

And they are in place, AFAICS (from drivers/pci/pci.c):

	/* Mandatory power management transition delays */
	/* see PCI PM 1.1 5.6.1 table 18 */
	if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
		msleep(pci_pm_d3_delay);
	else if (state == PCI_D2 || dev->current_state == PCI_D2)
		udelay(200);

> Simon, please can I know whether it works to you?
> It is seems to the things are related.
> 
> Joonwoo Park.
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 37c00f6..9f78064 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -493,8 +493,14 @@ pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>    * restore at least the BARs so that the device will be
>    * accessible to its driver.
>    */
> - if (need_restore)
> + if (need_restore) {
> +   /* The specification also says that "Must ensure that all of
> +    * its PCI signal drivers remain disabled for the duration of
> +    * the D3hot to D0 Uninitialized state transition".
> +    */
> +   msleep(10);

It's too late for the delay.

>     pci_restore_bars(dev);
> + }
> 
>   return 0;
>  }

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [PATCH] kexec: fix pci device initialization fail after kexec (2.6.23-rc2). (Related to e1000 doesn't resume properly from standby)

[Joonwoo Park]

2007/8/6, Rafael J. Wysocki <rjw@sisk.pl>:
> On Monday, 6 August 2007 15:42, Joonwoo Park wrote:
> > Hi.
> > I think that the pci_set_power_state() has bug.
> > The specification says that some delays is required.
>
> And they are in place, AFAICS (from drivers/pci/pci.c):
>
>        /* Mandatory power management transition delays */
>        /* see PCI PM 1.1 5.6.1 table 18 */
>        if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
>                msleep(pci_pm_d3_delay);
>        else if (state == PCI_D2 || dev->current_state == PCI_D2)
>                udelay(200);
>

The problem is occurred when state is 'PCI_D0', so those codes can't cover it.
But pci pm specification 5.4.1 says that when programmed to D0 the
equivalent of a warm reset, delay for the duration of the D3hot to D0
Uninitialized state
transition (10ms) to pci signal drivers remain disabled is required.

> > Simon, please can I know whether it works to you?
> > It is seems to the things are related.
> >
> > Joonwoo Park.
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 37c00f6..9f78064 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -493,8 +493,14 @@ pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> >    * restore at least the BARs so that the device will be
> >    * accessible to its driver.
> >    */
> > - if (need_restore)
> > + if (need_restore) {
> > +   /* The specification also says that "Must ensure that all of
> > +    * its PCI signal drivers remain disabled for the duration of
> > +    * the D3hot to D0 Uninitialized state transition".
> > +    */
> > +   msleep(10);
>
> It's too late for the delay.
>

How about this?

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 37c00f6..974dd04 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -471,6 +471,8 @@ pci_set_power_state(struct pci_dev *dev, pci_power_t state)
                msleep(pci_pm_d3_delay);
        else if (state == PCI_D2 || dev->current_state == PCI_D2)
                udelay(200);
+      else if (state == PCI_D0)
+              msleep(10);

        /*
         * Give firmware a chance to be called, such as ACPI _PRx, _PSx


> >     pci_restore_bars(dev);
> > + }
> >
> >   return 0;
> >  }
>
> Greetings,
> Rafael
>
>
> --
> "Premature optimization is the root of all evil." - Donald Knuth
>

Thanks,
Joonwoo Park.

Re: [PATCH] kexec: fix pci device initialization fail after kexec (2.6.23-rc2). (Related to e1000 doesn't resume properly from standby)

[Simon Arlott]

On 06/08/07 16:50, Joonwoo Park wrote:
> 2007/8/6, Rafael J. Wysocki <rjw@sisk.pl>:
>> On Monday, 6 August 2007 15:42, Joonwoo Park wrote:
>> > Hi.
>> > I think that the pci_set_power_state() has bug.
>> > The specification says that some delays is required.
>>
>> And they are in place, AFAICS (from drivers/pci/pci.c):
>>
>>        /* Mandatory power management transition delays */
>>        /* see PCI PM 1.1 5.6.1 table 18 */
>>        if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
>>                msleep(pci_pm_d3_delay);
>>        else if (state == PCI_D2 || dev->current_state == PCI_D2)
>>                udelay(200);
>>
> 
> The problem is occurred when state is 'PCI_D0', so those codes can't cover it.
> But pci pm specification 5.4.1 says that when programmed to D0 the
> equivalent of a warm reset, delay for the duration of the D3hot to D0
> Uninitialized state
> transition (10ms) to pci signal drivers remain disabled is required.
> 
>> > Simon, please can I know whether it works to you?
>> > It is seems to the things are related.
>> >
>> > Joonwoo Park.
>> >
>> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> > index 37c00f6..9f78064 100644
>> > --- a/drivers/pci/pci.c
>> > +++ b/drivers/pci/pci.c
>> > @@ -493,8 +493,14 @@ pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>> >    * restore at least the BARs so that the device will be
>> >    * accessible to its driver.
>> >    */
>> > - if (need_restore)
>> > + if (need_restore) {
>> > +   /* The specification also says that "Must ensure that all of
>> > +    * its PCI signal drivers remain disabled for the duration of
>> > +    * the D3hot to D0 Uninitialized state transition".
>> > +    */
>> > +   msleep(10);
>>
>> It's too late for the delay.
>>
> 
> How about this?

It doesn't fix it.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 37c00f6..974dd04 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -471,6 +471,8 @@ pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>                 msleep(pci_pm_d3_delay);
>         else if (state == PCI_D2 || dev->current_state == PCI_D2)
>                 udelay(200);
> +      else if (state == PCI_D0)
> +              msleep(10);
> 
>         /*
>          * Give firmware a chance to be called, such as ACPI _PRx, _PSx
> 
> 
>> >     pci_restore_bars(dev);
>> > + }
>> >
>> >   return 0;
>> >  }

Your patches have spaces everywhere instead of tabs.

-- 
Simon Arlott

Re: [PATCH] kexec: fix pci device initialization fail after kexec (2.6.23-rc2). (Related to e1000 doesn't resume properly from standby)

[Rafael J. Wysocki]

On Monday, 6 August 2007 17:50, Joonwoo Park wrote:
> 2007/8/6, Rafael J. Wysocki <rjw@sisk.pl>:
> > On Monday, 6 August 2007 15:42, Joonwoo Park wrote:
> > > Hi.
> > > I think that the pci_set_power_state() has bug.
> > > The specification says that some delays is required.
> >
> > And they are in place, AFAICS (from drivers/pci/pci.c):
> >
> >        /* Mandatory power management transition delays */
> >        /* see PCI PM 1.1 5.6.1 table 18 */
> >        if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
> >                msleep(pci_pm_d3_delay);
> >        else if (state == PCI_D2 || dev->current_state == PCI_D2)
> >                udelay(200);
> >
> 
> The problem is occurred when state is 'PCI_D0', so those codes can't cover it.
> But pci pm specification 5.4.1 says that when programmed to D0 the
> equivalent of a warm reset, delay for the duration of the D3hot to D0
> Uninitialized state
> transition (10ms) to pci signal drivers remain disabled is required.

Section 5.4.1 of PCI PM 1.1. spec is about D3_hot.  Specifically, it says
that if a device in D3_hot is programmed to D0, it performs the equivalent of
a warm reset.  IOW, this is supposed to happen if the current state is D3_hot
and the targed state is D0, which is covered by the code snippet above.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [linux-pm] Re: [PATCH] kexec: fix pci device initialization fail after kexec (2.6.23-rc2). (Related to e1000 doesn't resume properly from standby)

[David Brownell]

On Monday 06 August 2007, Rafael J. Wysocki wrote:
> Section 5.4.1 of PCI PM 1.1. spec is about D3_hot.  Specifically, it says
> that if a device in D3_hot is programmed to D0, it performs the equivalent of
> a warm reset.  IOW, this is supposed to happen if the current state is D3_hot
> and the targed state is D0, which is covered by the code snippet above.

Actually, it's *allowed* to do a warm reset.  Not required.
And not all PCI devices will do such a reset.   Some of them
will return to D0_initialized ... not D0_uninitialized (where
uninitialized means "did the reset").

It's D3_cold where a (powerup) reset is *required*.

A common example would be a USB host controller.  If one of
those did a warm reset it'd trash all the connection state
which was carefully retained during sleep ... and there'd be
no point to supporting D3_hot rather than D3_cold.

As a rule of thumb, it's probably safe to assume that if
a device can generate wakeup events, it probably doesn't
trash all of its state when it resumes from D3_hot.

Of course, if it can generate wakeup events from D3_cold,
the difference between Vaux and Vcc power wells comes into
play.  The Vaux powered bits will retain state, while the
stuff powered by Vcc gets a powerup reset.

If it can't generate wakeup events, a PCI device has no
particular need to retain state after D3.  Implementors
can choose whichever is most convenient.

- Dave

Re: [PATCH] kexec: fix pci device initialization fail after kexec (2.6.23-rc2). (Related to e1000 doesn't resume properly from standby)

[Joonwoo Park]

2007/8/7, Rafael J. Wysocki <rjw@sisk.pl>:
> On Monday, 6 August 2007 17:50, Joonwoo Park wrote:
> > 2007/8/6, Rafael J. Wysocki <rjw@sisk.pl>:
> > > On Monday, 6 August 2007 15:42, Joonwoo Park wrote:
> > > > Hi.
> > > > I think that the pci_set_power_state() has bug.
> > > > The specification says that some delays is required.
> > >
> > > And they are in place, AFAICS (from drivers/pci/pci.c):
> > >
> > >        /* Mandatory power management transition delays */
> > >        /* see PCI PM 1.1 5.6.1 table 18 */
> > >        if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
> > >                msleep(pci_pm_d3_delay);
> > >        else if (state == PCI_D2 || dev->current_state == PCI_D2)
> > >                udelay(200);
> > >
> >
> > The problem is occurred when state is 'PCI_D0', so those codes can't cover it.
> > But pci pm specification 5.4.1 says that when programmed to D0 the
> > equivalent of a warm reset, delay for the duration of the D3hot to D0
> > Uninitialized state
> > transition (10ms) to pci signal drivers remain disabled is required.
>
> Section 5.4.1 of PCI PM 1.1. spec is about D3_hot.  Specifically, it says
> that if a device in D3_hot is programmed to D0, it performs the equivalent of
> a warm reset.  IOW, this is supposed to happen if the current state is D3_hot
> and the targed state is D0, which is covered by the code snippet above.

IMHO, it is seems to the spec says just *programmed  to D0* not
*programmed from D3hot to D0*.
Actually, I got current_state UNKNOWN and state PCI_D0 after kexec's
start new kernel with dual port 82546EB fiber ethernet card.

>
> Greetings,
> Rafael
>
>
> --
> "Premature optimization is the root of all evil." - Donald Knuth
>

Simon, this patch was checked with checkpatch.pl and fixed some fault.

Signed-off-by: Joonwoo Park <joonwpark81@gmail.com>

---
 drivers/pci/pci.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 37c00f6..0f086d9 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -467,7 +467,8 @@ pci_set_power_state(struct pci_dev *dev, pci_power_t state)

 	/* Mandatory power management transition delays */
 	/* see PCI PM 1.1 5.6.1 table 18 */
-	if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
+	if (state == PCI_D3hot || dev->current_state == PCI_D3hot ||
+	    state == PCI_D0)
 		msleep(pci_pm_d3_delay);
 	else if (state == PCI_D2 || dev->current_state == PCI_D2)
 		udelay(200);
-


Best regards,
Joonwoo Park.

Re: [PATCH] kexec: fix pci device initialization fail after kexec (2.6.23-rc2). (Related to e1000 doesn't resume properly from standby)

[Rafael J. Wysocki]

On Tuesday, 7 August 2007 06:30, Joonwoo Park wrote:
> 2007/8/7, Rafael J. Wysocki <rjw@sisk.pl>:
> > On Monday, 6 August 2007 17:50, Joonwoo Park wrote:
> > > 2007/8/6, Rafael J. Wysocki <rjw@sisk.pl>:
> > > > On Monday, 6 August 2007 15:42, Joonwoo Park wrote:
> > > > > Hi.
> > > > > I think that the pci_set_power_state() has bug.
> > > > > The specification says that some delays is required.
> > > >
> > > > And they are in place, AFAICS (from drivers/pci/pci.c):
> > > >
> > > >        /* Mandatory power management transition delays */
> > > >        /* see PCI PM 1.1 5.6.1 table 18 */
> > > >        if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
> > > >                msleep(pci_pm_d3_delay);
> > > >        else if (state == PCI_D2 || dev->current_state == PCI_D2)
> > > >                udelay(200);
> > > >
> > >
> > > The problem is occurred when state is 'PCI_D0', so those codes can't cover it.
> > > But pci pm specification 5.4.1 says that when programmed to D0 the
> > > equivalent of a warm reset, delay for the duration of the D3hot to D0
> > > Uninitialized state
> > > transition (10ms) to pci signal drivers remain disabled is required.
> >
> > Section 5.4.1 of PCI PM 1.1. spec is about D3_hot.  Specifically, it says
> > that if a device in D3_hot is programmed to D0, it performs the equivalent of
> > a warm reset.  IOW, this is supposed to happen if the current state is D3_hot
> > and the targed state is D0, which is covered by the code snippet above.
> 
> IMHO, it is seems to the spec says just *programmed  to D0* not
> *programmed from D3hot to D0*.

But the title of the section is "Software Accessible D3 (D3hot)", isn't it?

And the first paragraph of the section is

"Functions in D3hot must respond to configuration space accesses as long as
power and clock are supplied so that they can be returned to D0 by software."

If this doesn't imply D3hot to D0 transition being discussed in the next
paragraph, then I don't know what it's there for.

> Actually, I got current_state UNKNOWN and state PCI_D0 after kexec's
> start new kernel with dual port 82546EB fiber ethernet card.

IMO, pci_set_power_state() is correct and your problem is related to something
else.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth