Re: Re: Re: Re: Re: [PATCH 0/2] Power management fixes

[Niklas Cassel <cassel@kernel.org>]

Hello Dieter,

On Fri, Feb 02, 2024 at 03:53:36PM +0100, Dieter Mummenschanz wrote:
> 
> > Yes, this part of the problem can be avoided by adding an explicit entry
> > that uses "board_ahci_low_power", as I explained in:
> > https://lore.kernel.org/linux-ide/ZaATdGDOo5jiBqCR@x1-carbon/T/#u[https://lore.kernel.org/linux-> ide/ZaATdGDOo5jiBqCR@x1-carbon/T/#u]
> 
> > We seem to have a problem with Tiger Lake, but that problem seems to be
> > related to Intel VMD.
> 
> > From looking at your logs, you don't seem to have Intel VMD, however
> > I'm guessing that some other motherboards that uses Cannon Lake might
> > have Intel VMD, so I guess the safest thing is to wait until that issue
> > has been resolved before adding a "board_ahci_low_power" entry for
> > Cannon Lake.
> 
> This mighht be kind of naive but what about adding an kernel option to force LPM even if the chipset/board is not oficially supported?

That is a good suggestion.
However, I'm hoping that this series:
https://lore.kernel.org/linux-ide/3e50681d-7a68-4c4a-91f6-278a3cfe23f8@amd.com/T/

Will avoid the need for that, as it would kill the
"board_ahci_low_power" board type completely.


> 
> > I see now that Damien's revert (patch 2/2 in this series) is not a simple
> > $ git revert fd3a6837d8e18cb7be80dcca1283276290336a7a
> > it seems to have some other small changes in the same patch as well.
> 
> > Sorry for asking you to test something once more...
> > But could you please test with:
> > v6.6-rc2 + git revert fd3a6837d8e18cb7be80dcca1283276290336a7a
> 
> No problem. I've explicitly reverted with 6.8-rc2 and have the kernel running for 1 1/2 days now with 3 suspends and on all of them my system transitions to pc8 so I guess we're good but I'll keep on testing to be sure. Hope this helps.

After talking with Damien, we are still completely lost.

commit fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()")
only modifies the ata_pci_shutdown_one(), which is only called on shutdown.

It is not called in the system suspend / system resume path,
so it should not be possible for this commit to have an impact.


Reading all the emails and bugzilla again, what you are seeing is that:

On a fresh boot, and leaving the computer untouched for a few minutes,
and looking at e.g. turbostat, some of your CPUs have spent time in
the lower-power C-states, e.g. C8/C9/C10.

However, if you do a system suspend + system resume, and then leave
the computer untouched for a few minutes, and look at e.g. turbotstat,
_none_ of your CPUs have spend time in the lower-power C-states.

Is this correct?


If so, it seems that suspend/resume must have messed up some setting...

The commit that added the code to ata_pci_shutdown_one()
was added in:
5b6fba546da2 ("ata: libata-core: Detach a port devices on shutdown")
This was first added in v6.7-rc1.
(So this commit was never included in v6.6.)

The code added in 5b6fba546da2 was later removed in:
commit fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()")
which was first added in v6.7-rc1.

Are you certain that v6.6 works and v6.7 is bad?

There are quite few libata patches between v6.6 and v6.7:
$ git log --oneline v6.7 --not v6.6 -- drivers/ata

Damien has sent many libata patches related to suspend/resume in the
last couple of kernel releases, so it is possible that something has
regressed. We just have a really hard time seeing how fd3a6837d8e1
could be the bad commit.


Kind regards,
Niklas