Andreas Mohr | 1 Jan 2010 18:07
Picon
Favicon

Re: [PATCH] PCI / PM: Use per-device D3 delays

Hi,

While the bug report mentions "So it's just quirky hardware.",
the implementation of your patch makes it seem like this delay attribute is
totally "norm"al behaviour - I'm missing some more aggressive wording.

Perhaps rename d3_delay to d3_delay__quirk or add something to the
comment, like "D3->D0 transition time in ms (out-of-spec PCI device quirk)"?
Or maybe something like
"custom D3->D0 transition time in ms (for quirky hardware violating the PCI spec's <= 100ms)".

Thanks for your ongoing great PCI efforts,

Andreas Mohr
Rafael J. Wysocki | 1 Jan 2010 19:55
Picon
Gravatar

Re: [PATCH] PCI / PM: Use per-device D3 delays

On Friday 01 January 2010, Andreas Mohr wrote:
> Hi,
> 
> While the bug report mentions "So it's just quirky hardware.",
> the implementation of your patch makes it seem like this delay attribute is
> totally "norm"al behaviour - I'm missing some more aggressive wording.

That's because it works both ways (please look at the changelog).

I know of a few devices that don't need the PCI-prescribed 10 ms wait when
going from D3 to D0 and their drivers may use the d3_delay field to actually
set a _shorter_ delay.

> Perhaps rename d3_delay to d3_delay__quirk or add something to the
> comment, like "D3->D0 transition time in ms (out-of-spec PCI device quirk)"?
> Or maybe something like
> "custom D3->D0 transition time in ms (for quirky hardware violating the PCI spec's <= 100ms)".
> 
> Thanks for your ongoing great PCI efforts,

You're welcome.

Rafael
Andreas Mohr | 1 Jan 2010 21:12
Picon
Favicon

Re: [PATCH] PCI / PM: Use per-device D3 delays

Hi,

On Fri, Jan 01, 2010 at 07:55:27PM +0100, Rafael J. Wysocki wrote:
> On Friday 01 January 2010, Andreas Mohr wrote:
> > Hi,
> > 
> > While the bug report mentions "So it's just quirky hardware.",
> > the implementation of your patch makes it seem like this delay attribute is
> > totally "norm"al behaviour - I'm missing some more aggressive wording.
> 
> That's because it works both ways (please look at the changelog).

Ah, ok.

> I know of a few devices that don't need the PCI-prescribed 10 ms wait when
> going from D3 to D0 and their drivers may use the d3_delay field to actually
> set a _shorter_ delay.

Then why is the value lower-bounded by pci_pm_d3_delay
(which, puzzlingly, was initialized to PCI_PM_D3_WAIT and thus 10
before, which the patch now removes!), in pci_dev_d3_sleep()?
(and pci_pm_d3_delay is being quirked in drivers/pci/quirks.c only,
to 120)

Confused,

Andreas Mohr
Rafael J. Wysocki | 1 Jan 2010 22:42
Picon
Gravatar

Re: [PATCH] PCI / PM: Use per-device D3 delays

On Friday 01 January 2010, Andreas Mohr wrote:
> Hi,
> 
> On Fri, Jan 01, 2010 at 07:55:27PM +0100, Rafael J. Wysocki wrote:
> > On Friday 01 January 2010, Andreas Mohr wrote:
> > > Hi,
> > > 
> > > While the bug report mentions "So it's just quirky hardware.",
> > > the implementation of your patch makes it seem like this delay attribute is
> > > totally "norm"al behaviour - I'm missing some more aggressive wording.
> > 
> > That's because it works both ways (please look at the changelog).
> 
> Ah, ok.
> 
> > I know of a few devices that don't need the PCI-prescribed 10 ms wait when
> > going from D3 to D0 and their drivers may use the d3_delay field to actually
> > set a _shorter_ delay.
> 
> Then why is the value lower-bounded by pci_pm_d3_delay
> (which, puzzlingly, was initialized to PCI_PM_D3_WAIT and thus 10
> before, which the patch now removes!),

That's because dev->d3_delay is initialized to PCI_PM_D3_WAIT for all devices.

> in pci_dev_d3_sleep()? (and pci_pm_d3_delay is being quirked in
> drivers/pci/quirks.c only, to 120)

Exactly because pci_pm_d3_delay is only necessary for some quirky chipsets
that require longer delays for _all_ devices (note that this cannot be handled
(Continue reading)

Andreas Mohr | 1 Jan 2010 22:56
Picon
Favicon

Re: [PATCH] PCI / PM: Use per-device D3 delays

On Fri, Jan 01, 2010 at 10:42:28PM +0100, Rafael J. Wysocki wrote:
> On Friday 01 January 2010, Andreas Mohr wrote:
> > Then why is the value lower-bounded by pci_pm_d3_delay
> > (which, puzzlingly, was initialized to PCI_PM_D3_WAIT and thus 10
> > before, which the patch now removes!),
> 
> That's because dev->d3_delay is initialized to PCI_PM_D3_WAIT for all devices.

Doh, I should have viewn the altered mechanism correctly, sorry.
Seems quite correct and useful after all.

Andreas Mohr
Andreas Mohr | 1 Jan 2010 22:56
Picon
Favicon

Re: [PATCH] PCI / PM: Use per-device D3 delays

On Fri, Jan 01, 2010 at 10:42:28PM +0100, Rafael J. Wysocki wrote:
> On Friday 01 January 2010, Andreas Mohr wrote:
> > Then why is the value lower-bounded by pci_pm_d3_delay
> > (which, puzzlingly, was initialized to PCI_PM_D3_WAIT and thus 10
> > before, which the patch now removes!),
> 
> That's because dev->d3_delay is initialized to PCI_PM_D3_WAIT for all devices.

Doh, I should have viewn the altered mechanism correctly, sorry.
Seems quite correct and useful after all.

Andreas Mohr
Rafael J. Wysocki | 1 Jan 2010 22:42
Picon
Gravatar

Re: [PATCH] PCI / PM: Use per-device D3 delays

On Friday 01 January 2010, Andreas Mohr wrote:
> Hi,
> 
> On Fri, Jan 01, 2010 at 07:55:27PM +0100, Rafael J. Wysocki wrote:
> > On Friday 01 January 2010, Andreas Mohr wrote:
> > > Hi,
> > > 
> > > While the bug report mentions "So it's just quirky hardware.",
> > > the implementation of your patch makes it seem like this delay attribute is
> > > totally "norm"al behaviour - I'm missing some more aggressive wording.
> > 
> > That's because it works both ways (please look at the changelog).
> 
> Ah, ok.
> 
> > I know of a few devices that don't need the PCI-prescribed 10 ms wait when
> > going from D3 to D0 and their drivers may use the d3_delay field to actually
> > set a _shorter_ delay.
> 
> Then why is the value lower-bounded by pci_pm_d3_delay
> (which, puzzlingly, was initialized to PCI_PM_D3_WAIT and thus 10
> before, which the patch now removes!),

That's because dev->d3_delay is initialized to PCI_PM_D3_WAIT for all devices.

> in pci_dev_d3_sleep()? (and pci_pm_d3_delay is being quirked in
> drivers/pci/quirks.c only, to 120)

Exactly because pci_pm_d3_delay is only necessary for some quirky chipsets
that require longer delays for _all_ devices (note that this cannot be handled
(Continue reading)

Andreas Mohr | 1 Jan 2010 21:12
Picon
Favicon

Re: [PATCH] PCI / PM: Use per-device D3 delays

Hi,

On Fri, Jan 01, 2010 at 07:55:27PM +0100, Rafael J. Wysocki wrote:
> On Friday 01 January 2010, Andreas Mohr wrote:
> > Hi,
> > 
> > While the bug report mentions "So it's just quirky hardware.",
> > the implementation of your patch makes it seem like this delay attribute is
> > totally "norm"al behaviour - I'm missing some more aggressive wording.
> 
> That's because it works both ways (please look at the changelog).

Ah, ok.

> I know of a few devices that don't need the PCI-prescribed 10 ms wait when
> going from D3 to D0 and their drivers may use the d3_delay field to actually
> set a _shorter_ delay.

Then why is the value lower-bounded by pci_pm_d3_delay
(which, puzzlingly, was initialized to PCI_PM_D3_WAIT and thus 10
before, which the patch now removes!), in pci_dev_d3_sleep()?
(and pci_pm_d3_delay is being quirked in drivers/pci/quirks.c only,
to 120)

Confused,

Andreas Mohr
Rafael J. Wysocki | 1 Jan 2010 19:55
Picon
Gravatar

Re: [PATCH] PCI / PM: Use per-device D3 delays

On Friday 01 January 2010, Andreas Mohr wrote:
> Hi,
> 
> While the bug report mentions "So it's just quirky hardware.",
> the implementation of your patch makes it seem like this delay attribute is
> totally "norm"al behaviour - I'm missing some more aggressive wording.

That's because it works both ways (please look at the changelog).

I know of a few devices that don't need the PCI-prescribed 10 ms wait when
going from D3 to D0 and their drivers may use the d3_delay field to actually
set a _shorter_ delay.

> Perhaps rename d3_delay to d3_delay__quirk or add something to the
> comment, like "D3->D0 transition time in ms (out-of-spec PCI device quirk)"?
> Or maybe something like
> "custom D3->D0 transition time in ms (for quirky hardware violating the PCI spec's <= 100ms)".
> 
> Thanks for your ongoing great PCI efforts,

You're welcome.

Rafael

Gmane