mbox series

[0/5] drop low power policy board type

Message ID 20240201161507.1147521-1-cassel@kernel.org
Headers show
Series drop low power policy board type | expand

Message

Niklas Cassel Feb. 1, 2024, 4:14 p.m. UTC 
The series is based on top of:
https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-next


Hello all,

This revives a patch sent out almost two years ago from Mario Limonciello:
https://lore.kernel.org/linux-ide/20220524170508.563-1-mario.limonciello@amd.com/T/#u

The reason why we did not merge it back then, is because LPM and hotplug
events are mutually exclusive.

The difference with this series compared to what was sent out back then:
I've added a patch that checks if the port is external, i.e. either
hotplug capable or eSATA. For external ports, we never enable LPM, as
that will break hotplug.

For ports that do not advertise themselves as external (typically laptops),
we set the LPM policy as requested.

This matches how Microsoft Windows does things.
Thanks to Werner Fischer for suggesting something like this at last year's
ALPSS conference.

There might of course be some platform firmware that e.g. incorrectly marks
its port as internal, even though it is external, but if we find any such
platforms we will need to deal with them using quirks.


Also note that we even if the user requested a certain policy, there is
no guarantee that he will get all the features for that policy, see:
https://github.com/torvalds/linux/blob/master/drivers/ata/libata-sata.c#L403-L414

However, I'd rather we not try to map all the combinations of
partial/slumber/devsleep in to a single policy represented by a single
integer, thus I do not try to "change" the requested policy.
The user will get all the features that are included in the requested
policy AND supported by the HBA.

Another difference (compared to an earlier version of Mario's series)
is that we do not try to change the default CONFIG_SATA_MOBILE_LPM_POLICY
value from 0 to 3, it will continue to be 0.
If you really don't want LPM even if your HBA supports it, and your port
is internal, one option is to leave the Kconfig set to the default value.

Note: in current mainline, there is an issue related to Intel VMD
which causes the link to not come up when VMD is enabled.
(The link does comes up when VMD is disabled in BIOS.)
In order to not get a bunch of bug reports related to LPM, we probably
want to wait with this series until we have fixed the VMD bug.


Kind regards,
Niklas


Mario Limonciello (1):
  ata: ahci: Drop low power policy board type

Niklas Cassel (4):
  ata: ahci: move marking of external port earlier
  ata: ahci: a hotplug capable port is an external port
  ata: ahci: drop hpriv param from ahci_update_initial_lpm_policy()
  ata: ahci: do not enable LPM on external ports

 drivers/ata/Kconfig   |   5 +-
 drivers/ata/ahci.c    | 134 +++++++++++++++++++++++-------------------
 drivers/ata/ahci.h    |   9 +--
 drivers/ata/libahci.c |   7 ---
 4 files changed, 77 insertions(+), 78 deletions(-)

Comments

Damien Le Moal Feb. 2, 2024, 2:13 a.m. UTC | #1 
On 2/2/24 01:14, Niklas Cassel wrote:
> The series is based on top of:
> https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-next
> 
> 
> Hello all,
> 
> This revives a patch sent out almost two years ago from Mario Limonciello:
> https://lore.kernel.org/linux-ide/20220524170508.563-1-mario.limonciello@amd.com/T/#u

You forgot to CC Mario.
Werner Fischer Feb. 2, 2024, 1:13 p.m. UTC | #2 
(adding Mario Limonciello in CC)

On Thu, 2024-02-01 at 17:14 +0100, Niklas Cassel wrote:

> This revives a patch sent out almost two years ago from Mario
> Limonciello:
> https://lore.kernel.org/linux-ide/20220524170508.563-1-mario.limonciello@amd.com/T/#u
> 
> The reason why we did not merge it back then, is because LPM and
> hotplug events are mutually exclusive.
>
> I've added a patch that checks if the port is external, i.e. either
> hotplug capable or eSATA. For external ports, we never enable LPM, as
> that will break hotplug.
> 
> For ports that do not advertise themselves as external (typically
> laptops), we set the LPM policy as requested.
>
> This matches how Microsoft Windows does things.
> Thanks to Werner Fischer for suggesting something like this at last
> year's ALPSS conference.
Thank you for the discussions last year at the ALPSS and thanks a lot
for implementing this now.

> There might of course be some platform firmware that e.g. incorrectly
> marks its port as internal, even though it is external, but if we
> find any such platforms we will need to deal with them using quirks.
I plan some testing within the upcoming two weeks.
I'll do testing with the Elkhart-Lake based system (0x4b63 SATA
controller) to verify whether LPM is activated.
And I'll reach out to colleagues to test with hot-pluggable servers.
Especially I'll try for them to get a system with an AMD 0x7901 SATA
controller like Supermicro H12SSL-NT, as the default
"board_ahci_mobile" for this SATA controller - see commit 1527f69204fe
("ata: ahci: Add Green Sardine vendor ID as board_ahci_mobile") - lead
to Hot-Plug not working with default Ubuntu or Proxmox Kernels [1].

As especially for AMD systems identical SATA controllers (like 0x7901
in this case) can be used in both mobile systems and servers, Niklas'
new patch series could could bring lasting improvement here.

[1] https://www.thomas-krenn.com/en/wiki/AMD_EPYC_Server_with_Ubuntu_-_Enable_SATA_Hot-Swap

--
Best regards,
Werner Fischer
Mario Limonciello Feb. 2, 2024, 3:12 p.m. UTC | #3 
On 2/2/2024 07:13, Werner Fischer wrote:
> (adding Mario Limonciello in CC)
> 
> On Thu, 2024-02-01 at 17:14 +0100, Niklas Cassel wrote:
> 
>> This revives a patch sent out almost two years ago from Mario
>> Limonciello:
>> https://lore.kernel.org/linux-ide/20220524170508.563-1-mario.limonciello@amd.com/T/#u
>>
>> The reason why we did not merge it back then, is because LPM and
>> hotplug events are mutually exclusive.
>>
>> I've added a patch that checks if the port is external, i.e. either
>> hotplug capable or eSATA. For external ports, we never enable LPM, as
>> that will break hotplug.
>>
>> For ports that do not advertise themselves as external (typically
>> laptops), we set the LPM policy as requested.
>>
>> This matches how Microsoft Windows does things.
>> Thanks to Werner Fischer for suggesting something like this at last
>> year's ALPSS conference.
> Thank you for the discussions last year at the ALPSS and thanks a lot
> for implementing this now.
> 
>> There might of course be some platform firmware that e.g. incorrectly
>> marks its port as internal, even though it is external, but if we
>> find any such platforms we will need to deal with them using quirks.
> I plan some testing within the upcoming two weeks.
> I'll do testing with the Elkhart-Lake based system (0x4b63 SATA
> controller) to verify whether LPM is activated.
> And I'll reach out to colleagues to test with hot-pluggable servers.
> Especially I'll try for them to get a system with an AMD 0x7901 SATA
> controller like Supermicro H12SSL-NT, as the default
> "board_ahci_mobile" for this SATA controller - see commit 1527f69204fe
> ("ata: ahci: Add Green Sardine vendor ID as board_ahci_mobile") - lead
> to Hot-Plug not working with default Ubuntu or Proxmox Kernels [1].
> 
> As especially for AMD systems identical SATA controllers (like 0x7901
> in this case) can be used in both mobile systems and servers, Niklas'
> new patch series could could bring lasting improvement here.
> 
> [1] https://www.thomas-krenn.com/en/wiki/AMD_EPYC_Server_with_Ubuntu_-_Enable_SATA_Hot-Swap
> 
> --
> Best regards,
> Werner Fischer

Thanks for looping me in and also reviving the patch.  The series looks 
good to me.  I do think that after this baked a bit and everyone is 
happy we should also revisit changing the upstream default to match the 
distros again too.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>