This board definition was originally created for mobile devices to
designate default link power managmeent policy to influence runtime
power consumption.
As this is interesting for more than just mobile designs, rename the
board to `board_ahci_low_power` to make it clear it is about default
policy.
Signed-off-by: Mario Limonciello <[email protected]>
---
changes from v1->v2:
* Rename to board_ahci_low_power
drivers/ata/ahci.c | 96 +++++++++++++++++++++++-----------------------
1 file changed, 48 insertions(+), 48 deletions(-)
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index ab5811ef5a53..995ef962eb92 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -49,8 +49,8 @@ enum {
enum board_ids {
/* board IDs by feature in alphabetical order */
board_ahci,
+ board_ahci_low_power,
board_ahci_ign_iferr,
- board_ahci_mobile,
board_ahci_no_debounce_delay,
board_ahci_nomsi,
board_ahci_noncq,
@@ -135,7 +135,7 @@ static const struct ata_port_info ahci_port_info[] = {
.udma_mask = ATA_UDMA6,
.port_ops = &ahci_ops,
},
- [board_ahci_mobile] = {
+ [board_ahci_low_power] = {
AHCI_HFLAGS (AHCI_HFLAG_IS_MOBILE),
.flags = AHCI_FLAG_COMMON,
.pio_mask = ATA_PIO4,
@@ -275,13 +275,13 @@ static const struct pci_device_id ahci_pci_tbl[] = {
{ PCI_VDEVICE(INTEL, 0x2924), board_ahci }, /* ICH9 */
{ PCI_VDEVICE(INTEL, 0x2925), board_ahci }, /* ICH9 */
{ PCI_VDEVICE(INTEL, 0x2927), board_ahci }, /* ICH9 */
- { PCI_VDEVICE(INTEL, 0x2929), board_ahci_mobile }, /* ICH9M */
- { PCI_VDEVICE(INTEL, 0x292a), board_ahci_mobile }, /* ICH9M */
- { PCI_VDEVICE(INTEL, 0x292b), board_ahci_mobile }, /* ICH9M */
- { PCI_VDEVICE(INTEL, 0x292c), board_ahci_mobile }, /* ICH9M */
- { PCI_VDEVICE(INTEL, 0x292f), board_ahci_mobile }, /* ICH9M */
+ { PCI_VDEVICE(INTEL, 0x2929), board_ahci_low_power }, /* ICH9M */
+ { PCI_VDEVICE(INTEL, 0x292a), board_ahci_low_power }, /* ICH9M */
+ { PCI_VDEVICE(INTEL, 0x292b), board_ahci_low_power }, /* ICH9M */
+ { PCI_VDEVICE(INTEL, 0x292c), board_ahci_low_power }, /* ICH9M */
+ { PCI_VDEVICE(INTEL, 0x292f), board_ahci_low_power }, /* ICH9M */
{ PCI_VDEVICE(INTEL, 0x294d), board_ahci }, /* ICH9 */
- { PCI_VDEVICE(INTEL, 0x294e), board_ahci_mobile }, /* ICH9M */
+ { PCI_VDEVICE(INTEL, 0x294e), board_ahci_low_power }, /* ICH9M */
{ PCI_VDEVICE(INTEL, 0x502a), board_ahci }, /* Tolapai */
{ PCI_VDEVICE(INTEL, 0x502b), board_ahci }, /* Tolapai */
{ PCI_VDEVICE(INTEL, 0x3a05), board_ahci }, /* ICH10 */
@@ -291,9 +291,9 @@ static const struct pci_device_id ahci_pci_tbl[] = {
{ PCI_VDEVICE(INTEL, 0x3b23), board_ahci }, /* PCH AHCI */
{ PCI_VDEVICE(INTEL, 0x3b24), board_ahci }, /* PCH RAID */
{ PCI_VDEVICE(INTEL, 0x3b25), board_ahci }, /* PCH RAID */
- { PCI_VDEVICE(INTEL, 0x3b29), board_ahci_mobile }, /* PCH M AHCI */
+ { PCI_VDEVICE(INTEL, 0x3b29), board_ahci_low_power }, /* PCH M AHCI */
{ PCI_VDEVICE(INTEL, 0x3b2b), board_ahci }, /* PCH RAID */
- { PCI_VDEVICE(INTEL, 0x3b2c), board_ahci_mobile }, /* PCH M RAID */
+ { PCI_VDEVICE(INTEL, 0x3b2c), board_ahci_low_power }, /* PCH M RAID */
{ PCI_VDEVICE(INTEL, 0x3b2f), board_ahci }, /* PCH AHCI */
{ PCI_VDEVICE(INTEL, 0x19b0), board_ahci_pcs7 }, /* DNV AHCI */
{ PCI_VDEVICE(INTEL, 0x19b1), board_ahci_pcs7 }, /* DNV AHCI */
@@ -316,9 +316,9 @@ static const struct pci_device_id ahci_pci_tbl[] = {
{ PCI_VDEVICE(INTEL, 0x19cE), board_ahci_pcs7 }, /* DNV AHCI */
{ PCI_VDEVICE(INTEL, 0x19cF), board_ahci_pcs7 }, /* DNV AHCI */
{ PCI_VDEVICE(INTEL, 0x1c02), board_ahci }, /* CPT AHCI */
- { PCI_VDEVICE(INTEL, 0x1c03), board_ahci_mobile }, /* CPT M AHCI */
+ { PCI_VDEVICE(INTEL, 0x1c03), board_ahci_low_power }, /* CPT M AHCI */
{ PCI_VDEVICE(INTEL, 0x1c04), board_ahci }, /* CPT RAID */
- { PCI_VDEVICE(INTEL, 0x1c05), board_ahci_mobile }, /* CPT M RAID */
+ { PCI_VDEVICE(INTEL, 0x1c05), board_ahci_low_power }, /* CPT M RAID */
{ PCI_VDEVICE(INTEL, 0x1c06), board_ahci }, /* CPT RAID */
{ PCI_VDEVICE(INTEL, 0x1c07), board_ahci }, /* CPT RAID */
{ PCI_VDEVICE(INTEL, 0x1d02), board_ahci }, /* PBG AHCI */
@@ -327,29 +327,29 @@ static const struct pci_device_id ahci_pci_tbl[] = {
{ PCI_VDEVICE(INTEL, 0x2826), board_ahci }, /* PBG/Lewisburg RAID*/
{ PCI_VDEVICE(INTEL, 0x2323), board_ahci }, /* DH89xxCC AHCI */
{ PCI_VDEVICE(INTEL, 0x1e02), board_ahci }, /* Panther Point AHCI */
- { PCI_VDEVICE(INTEL, 0x1e03), board_ahci_mobile }, /* Panther M AHCI */
+ { PCI_VDEVICE(INTEL, 0x1e03), board_ahci_low_power }, /* Panther M AHCI */
{ PCI_VDEVICE(INTEL, 0x1e04), board_ahci }, /* Panther Point RAID */
{ PCI_VDEVICE(INTEL, 0x1e05), board_ahci }, /* Panther Point RAID */
{ PCI_VDEVICE(INTEL, 0x1e06), board_ahci }, /* Panther Point RAID */
- { PCI_VDEVICE(INTEL, 0x1e07), board_ahci_mobile }, /* Panther M RAID */
+ { PCI_VDEVICE(INTEL, 0x1e07), board_ahci_low_power }, /* Panther M RAID */
{ PCI_VDEVICE(INTEL, 0x1e0e), board_ahci }, /* Panther Point RAID */
{ PCI_VDEVICE(INTEL, 0x8c02), board_ahci }, /* Lynx Point AHCI */
- { PCI_VDEVICE(INTEL, 0x8c03), board_ahci_mobile }, /* Lynx M AHCI */
+ { PCI_VDEVICE(INTEL, 0x8c03), board_ahci_low_power }, /* Lynx M AHCI */
{ PCI_VDEVICE(INTEL, 0x8c04), board_ahci }, /* Lynx Point RAID */
- { PCI_VDEVICE(INTEL, 0x8c05), board_ahci_mobile }, /* Lynx M RAID */
+ { PCI_VDEVICE(INTEL, 0x8c05), board_ahci_low_power }, /* Lynx M RAID */
{ PCI_VDEVICE(INTEL, 0x8c06), board_ahci }, /* Lynx Point RAID */
- { PCI_VDEVICE(INTEL, 0x8c07), board_ahci_mobile }, /* Lynx M RAID */
+ { PCI_VDEVICE(INTEL, 0x8c07), board_ahci_low_power }, /* Lynx M RAID */
{ PCI_VDEVICE(INTEL, 0x8c0e), board_ahci }, /* Lynx Point RAID */
- { PCI_VDEVICE(INTEL, 0x8c0f), board_ahci_mobile }, /* Lynx M RAID */
- { PCI_VDEVICE(INTEL, 0x9c02), board_ahci_mobile }, /* Lynx LP AHCI */
- { PCI_VDEVICE(INTEL, 0x9c03), board_ahci_mobile }, /* Lynx LP AHCI */
- { PCI_VDEVICE(INTEL, 0x9c04), board_ahci_mobile }, /* Lynx LP RAID */
- { PCI_VDEVICE(INTEL, 0x9c05), board_ahci_mobile }, /* Lynx LP RAID */
- { PCI_VDEVICE(INTEL, 0x9c06), board_ahci_mobile }, /* Lynx LP RAID */
- { PCI_VDEVICE(INTEL, 0x9c07), board_ahci_mobile }, /* Lynx LP RAID */
- { PCI_VDEVICE(INTEL, 0x9c0e), board_ahci_mobile }, /* Lynx LP RAID */
- { PCI_VDEVICE(INTEL, 0x9c0f), board_ahci_mobile }, /* Lynx LP RAID */
- { PCI_VDEVICE(INTEL, 0x9dd3), board_ahci_mobile }, /* Cannon Lake PCH-LP AHCI */
+ { PCI_VDEVICE(INTEL, 0x8c0f), board_ahci_low_power }, /* Lynx M RAID */
+ { PCI_VDEVICE(INTEL, 0x9c02), board_ahci_low_power }, /* Lynx LP AHCI */
+ { PCI_VDEVICE(INTEL, 0x9c03), board_ahci_low_power }, /* Lynx LP AHCI */
+ { PCI_VDEVICE(INTEL, 0x9c04), board_ahci_low_power }, /* Lynx LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c05), board_ahci_low_power }, /* Lynx LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c06), board_ahci_low_power }, /* Lynx LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c07), board_ahci_low_power }, /* Lynx LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c0e), board_ahci_low_power }, /* Lynx LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c0f), board_ahci_low_power }, /* Lynx LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9dd3), board_ahci_low_power }, /* Cannon Lake PCH-LP AHCI */
{ PCI_VDEVICE(INTEL, 0x1f22), board_ahci }, /* Avoton AHCI */
{ PCI_VDEVICE(INTEL, 0x1f23), board_ahci }, /* Avoton AHCI */
{ PCI_VDEVICE(INTEL, 0x1f24), board_ahci }, /* Avoton RAID */
@@ -381,26 +381,26 @@ static const struct pci_device_id ahci_pci_tbl[] = {
{ PCI_VDEVICE(INTEL, 0x8d66), board_ahci }, /* Wellsburg RAID */
{ PCI_VDEVICE(INTEL, 0x8d6e), board_ahci }, /* Wellsburg RAID */
{ PCI_VDEVICE(INTEL, 0x23a3), board_ahci }, /* Coleto Creek AHCI */
- { PCI_VDEVICE(INTEL, 0x9c83), board_ahci_mobile }, /* Wildcat LP AHCI */
- { PCI_VDEVICE(INTEL, 0x9c85), board_ahci_mobile }, /* Wildcat LP RAID */
- { PCI_VDEVICE(INTEL, 0x9c87), board_ahci_mobile }, /* Wildcat LP RAID */
- { PCI_VDEVICE(INTEL, 0x9c8f), board_ahci_mobile }, /* Wildcat LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c83), board_ahci_low_power }, /* Wildcat LP AHCI */
+ { PCI_VDEVICE(INTEL, 0x9c85), board_ahci_low_power }, /* Wildcat LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c87), board_ahci_low_power }, /* Wildcat LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c8f), board_ahci_low_power }, /* Wildcat LP RAID */
{ PCI_VDEVICE(INTEL, 0x8c82), board_ahci }, /* 9 Series AHCI */
- { PCI_VDEVICE(INTEL, 0x8c83), board_ahci_mobile }, /* 9 Series M AHCI */
+ { PCI_VDEVICE(INTEL, 0x8c83), board_ahci_low_power }, /* 9 Series M AHCI */
{ PCI_VDEVICE(INTEL, 0x8c84), board_ahci }, /* 9 Series RAID */
- { PCI_VDEVICE(INTEL, 0x8c85), board_ahci_mobile }, /* 9 Series M RAID */
+ { PCI_VDEVICE(INTEL, 0x8c85), board_ahci_low_power }, /* 9 Series M RAID */
{ PCI_VDEVICE(INTEL, 0x8c86), board_ahci }, /* 9 Series RAID */
- { PCI_VDEVICE(INTEL, 0x8c87), board_ahci_mobile }, /* 9 Series M RAID */
+ { PCI_VDEVICE(INTEL, 0x8c87), board_ahci_low_power }, /* 9 Series M RAID */
{ PCI_VDEVICE(INTEL, 0x8c8e), board_ahci }, /* 9 Series RAID */
- { PCI_VDEVICE(INTEL, 0x8c8f), board_ahci_mobile }, /* 9 Series M RAID */
- { PCI_VDEVICE(INTEL, 0x9d03), board_ahci_mobile }, /* Sunrise LP AHCI */
- { PCI_VDEVICE(INTEL, 0x9d05), board_ahci_mobile }, /* Sunrise LP RAID */
- { PCI_VDEVICE(INTEL, 0x9d07), board_ahci_mobile }, /* Sunrise LP RAID */
+ { PCI_VDEVICE(INTEL, 0x8c8f), board_ahci_low_power }, /* 9 Series M RAID */
+ { PCI_VDEVICE(INTEL, 0x9d03), board_ahci_low_power }, /* Sunrise LP AHCI */
+ { PCI_VDEVICE(INTEL, 0x9d05), board_ahci_low_power }, /* Sunrise LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9d07), board_ahci_low_power }, /* Sunrise LP RAID */
{ PCI_VDEVICE(INTEL, 0xa102), board_ahci }, /* Sunrise Point-H AHCI */
- { PCI_VDEVICE(INTEL, 0xa103), board_ahci_mobile }, /* Sunrise M AHCI */
+ { PCI_VDEVICE(INTEL, 0xa103), board_ahci_low_power }, /* Sunrise M AHCI */
{ PCI_VDEVICE(INTEL, 0xa105), board_ahci }, /* Sunrise Point-H RAID */
{ PCI_VDEVICE(INTEL, 0xa106), board_ahci }, /* Sunrise Point-H RAID */
- { PCI_VDEVICE(INTEL, 0xa107), board_ahci_mobile }, /* Sunrise M RAID */
+ { PCI_VDEVICE(INTEL, 0xa107), board_ahci_low_power }, /* Sunrise M RAID */
{ PCI_VDEVICE(INTEL, 0xa10f), board_ahci }, /* Sunrise Point-H RAID */
{ PCI_VDEVICE(INTEL, 0xa182), board_ahci }, /* Lewisburg AHCI*/
{ PCI_VDEVICE(INTEL, 0xa186), board_ahci }, /* Lewisburg RAID*/
@@ -413,13 +413,13 @@ static const struct pci_device_id ahci_pci_tbl[] = {
{ PCI_VDEVICE(INTEL, 0xa356), board_ahci }, /* Cannon Lake PCH-H RAID */
{ PCI_VDEVICE(INTEL, 0x06d7), board_ahci }, /* Comet Lake-H RAID */
{ PCI_VDEVICE(INTEL, 0xa386), board_ahci }, /* Comet Lake PCH-V RAID */
- { PCI_VDEVICE(INTEL, 0x0f22), board_ahci_mobile }, /* Bay Trail AHCI */
- { PCI_VDEVICE(INTEL, 0x0f23), board_ahci_mobile }, /* Bay Trail AHCI */
- { PCI_VDEVICE(INTEL, 0x22a3), board_ahci_mobile }, /* Cherry Tr. AHCI */
- { PCI_VDEVICE(INTEL, 0x5ae3), board_ahci_mobile }, /* ApolloLake AHCI */
- { PCI_VDEVICE(INTEL, 0x34d3), board_ahci_mobile }, /* Ice Lake LP AHCI */
- { PCI_VDEVICE(INTEL, 0x02d3), board_ahci_mobile }, /* Comet Lake PCH-U AHCI */
- { PCI_VDEVICE(INTEL, 0x02d7), board_ahci_mobile }, /* Comet Lake PCH RAID */
+ { PCI_VDEVICE(INTEL, 0x0f22), board_ahci_low_power }, /* Bay Trail AHCI */
+ { PCI_VDEVICE(INTEL, 0x0f23), board_ahci_low_power }, /* Bay Trail AHCI */
+ { PCI_VDEVICE(INTEL, 0x22a3), board_ahci_low_power }, /* Cherry Tr. AHCI */
+ { PCI_VDEVICE(INTEL, 0x5ae3), board_ahci_low_power }, /* ApolloLake AHCI */
+ { PCI_VDEVICE(INTEL, 0x34d3), board_ahci_low_power }, /* Ice Lake LP AHCI */
+ { PCI_VDEVICE(INTEL, 0x02d3), board_ahci_low_power }, /* Comet Lake PCH-U AHCI */
+ { PCI_VDEVICE(INTEL, 0x02d7), board_ahci_low_power }, /* Comet Lake PCH RAID */
/* JMicron 360/1/3/5/6, match class to avoid IDE function */
{ PCI_VENDOR_ID_JMICRON, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
@@ -447,7 +447,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
{ PCI_VDEVICE(AMD, 0x7801), board_ahci_no_debounce_delay }, /* AMD Hudson-2 (AHCI mode) */
{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
- { PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
+ { PCI_VDEVICE(AMD, 0x7901), board_ahci_low_power }, /* AMD Green Sardine */
/* AMD is using RAID class only for ahci controllers */
{ PCI_VENDOR_ID_AMD, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
PCI_CLASS_STORAGE_RAID << 8, 0xffffff, board_ahci },
--
2.34.1
[Public]
> On Fri, Feb 25, 2022 at 12:11:11AM -0600, Mario Limonciello wrote:
> > This board definition was originally created for mobile devices to
> > designate default link power managmeent policy to influence runtime
> > power consumption.
> >
> > As this is interesting for more than just mobile designs, rename the
> > board to `board_ahci_low_power` to make it clear it is about default
> > policy.
>
> Is there any good reason to not just apply the policy to all devices
> by default?
That sure would make this all cleaner.
I think Hans knows more of the history here than anyone else. I had
presumed there was some data loss scenarios with some of the older
chipsets.
Hans?
On Fri, Feb 25, 2022 at 12:11:11AM -0600, Mario Limonciello wrote:
> This board definition was originally created for mobile devices to
> designate default link power managmeent policy to influence runtime
> power consumption.
>
> As this is interesting for more than just mobile designs, rename the
> board to `board_ahci_low_power` to make it clear it is about default
> policy.
Is there any good reason to not just apply the policy to all devices
by default?
Hi,
On 2/25/22 17:19, Limonciello, Mario wrote:
> [Public]
>
>> On 2/25/22 17:04, Limonciello, Mario wrote:
>>> [Public]
>>>
>>>> On Fri, Feb 25, 2022 at 12:11:11AM -0600, Mario Limonciello wrote:
>>>>> This board definition was originally created for mobile devices to
>>>>> designate default link power managmeent policy to influence runtime
>>>>> power consumption.
>>>>>
>>>>> As this is interesting for more than just mobile designs, rename the
>>>>> board to `board_ahci_low_power` to make it clear it is about default
>>>>> policy.
>>>>
>>>> Is there any good reason to not just apply the policy to all devices
>>>> by default?
>>>
>>> That sure would make this all cleaner.
>>>
>>> I think Hans knows more of the history here than anyone else. I had
>>> presumed there was some data loss scenarios with some of the older
>>> chipsets.
>>
>> When I first introduced this change there were reports of crashes and
>> data corruption caused by setting the policy to min_power, these were
>> tied to some motherboards and/or to some drives.
>>
>> This is the whole reason why I only enabled this on a subset of all the
>> AHCI chipsets.
>>
>> At least on devices with a chipset which is currently marked as
>> mobile, the motherboard specific issues could be fixed with a BIOS
>> update. But I doubt that similar BIOS fixes have also been rolled
>> out to all desktop boards (and have been applied by all users),
>> and I also don't know about older boards.
>>
>> So enabling this on all chipsets is definitely not without risks.
>>
>
> This was before min_power_with_partial and min_power_with_dipm
> were introduced though right?
The issues where some laptops needed BIOS updates was with fedora
using min_power_with_dipm as default for mobile chipsets.
> Maybe another way to look at this
> is to drop the policy min_power, which overall is dangerous.
Maybe, see above. I'm not going to block this if people want
to give this a try, but it is going to require someone keeping
a very close look at any issues popping up and we must be
prepared to roll-back the change if necessary.
Regards,
Hans
Hi,
On 2/25/22 22:13, Limonciello, Mario wrote:
> [AMD Official Use Only]
>
>>>>>> On Fri, Feb 25, 2022 at 12:11:11AM -0600, Mario Limonciello wrote:
>>>>>>> This board definition was originally created for mobile devices to
>>>>>>> designate default link power managmeent policy to influence runtime
>>>>>>> power consumption.
>>>>>>>
>>>>>>> As this is interesting for more than just mobile designs, rename the
>>>>>>> board to `board_ahci_low_power` to make it clear it is about default
>>>>>>> policy.
>>>>>>
>>>>>> Is there any good reason to not just apply the policy to all devices
>>>>>> by default?
>>>>>
>>>>> That sure would make this all cleaner.
>>>>>
>>>>> I think Hans knows more of the history here than anyone else. I had
>>>>> presumed there was some data loss scenarios with some of the older
>>>>> chipsets.
>>>>
>>>> When I first introduced this change there were reports of crashes and
>>>> data corruption caused by setting the policy to min_power, these were
>>>> tied to some motherboards and/or to some drives.
>>>>
>>>> This is the whole reason why I only enabled this on a subset of all the
>>>> AHCI chipsets.
>>>>
>>>> At least on devices with a chipset which is currently marked as
>>>> mobile, the motherboard specific issues could be fixed with a BIOS
>>>> update. But I doubt that similar BIOS fixes have also been rolled
>>>> out to all desktop boards (and have been applied by all users),
>>>> and I also don't know about older boards.
>>>>
>>>> So enabling this on all chipsets is definitely not without risks.
>>>>
>>>
>>> This was before min_power_with_partial and min_power_with_dipm
>>> were introduced though right?
>>
>> The issues where some laptops needed BIOS updates was with fedora
>> using min_power_with_dipm as default for mobile chipsets.
>>
>
> Do you know if the drives actually supported slumber and partial?
> I wonder if that was the real problem that they were being set when
> they shouldn't be.>
> I added something for this in 2/2 in the RFC series you can look at.
Fedora defaults to ATA_LPM_MED_POWER_WITH_DIPM so patch 2/2 is
a no-op on Fedora; and IIRC (it has been a long time) the
need for BIOS updates on some mobile devices was with
standard Fedora kernels / settings.
Regards,
Hans
>
>>> Maybe another way to look at this
>>> is to drop the policy min_power, which overall is dangerous.
>>
>> Maybe, see above. I'm not going to block this if people want
>> to give this a try, but it is going to require someone keeping
>> a very close look at any issues popping up and we must be
>> prepared to roll-back the change if necessary.
>>
>
> Per Paul's suggestion I sent out v3 of this series and then I sent
> out a separate RFC series (you're on CC). For this type of
> thing if y'all think it makes sense to do.
[AMD Official Use Only]
> >>>> On Fri, Feb 25, 2022 at 12:11:11AM -0600, Mario Limonciello wrote:
> >>>>> This board definition was originally created for mobile devices to
> >>>>> designate default link power managmeent policy to influence runtime
> >>>>> power consumption.
> >>>>>
> >>>>> As this is interesting for more than just mobile designs, rename the
> >>>>> board to `board_ahci_low_power` to make it clear it is about default
> >>>>> policy.
> >>>>
> >>>> Is there any good reason to not just apply the policy to all devices
> >>>> by default?
> >>>
> >>> That sure would make this all cleaner.
> >>>
> >>> I think Hans knows more of the history here than anyone else. I had
> >>> presumed there was some data loss scenarios with some of the older
> >>> chipsets.
> >>
> >> When I first introduced this change there were reports of crashes and
> >> data corruption caused by setting the policy to min_power, these were
> >> tied to some motherboards and/or to some drives.
> >>
> >> This is the whole reason why I only enabled this on a subset of all the
> >> AHCI chipsets.
> >>
> >> At least on devices with a chipset which is currently marked as
> >> mobile, the motherboard specific issues could be fixed with a BIOS
> >> update. But I doubt that similar BIOS fixes have also been rolled
> >> out to all desktop boards (and have been applied by all users),
> >> and I also don't know about older boards.
> >>
> >> So enabling this on all chipsets is definitely not without risks.
> >>
> >
> > This was before min_power_with_partial and min_power_with_dipm
> > were introduced though right?
>
> The issues where some laptops needed BIOS updates was with fedora
> using min_power_with_dipm as default for mobile chipsets.
>
Do you know if the drives actually supported slumber and partial?
I wonder if that was the real problem that they were being set when
they shouldn't be.
I added something for this in 2/2 in the RFC series you can look at.
> > Maybe another way to look at this
> > is to drop the policy min_power, which overall is dangerous.
>
> Maybe, see above. I'm not going to block this if people want
> to give this a try, but it is going to require someone keeping
> a very close look at any issues popping up and we must be
> prepared to roll-back the change if necessary.
>
Per Paul's suggestion I sent out v3 of this series and then I sent
out a separate RFC series (you're on CC). For this type of
thing if y'all think it makes sense to do.
Dear Hans, dear Christoph,
Am 25.02.22 um 17:16 schrieb Hans de Goede:
> Hi,
>
> On 2/25/22 17:04, Limonciello, Mario wrote:
>> [Public]
>>
>>> On Fri, Feb 25, 2022 at 12:11:11AM -0600, Mario Limonciello wrote:
>>>> This board definition was originally created for mobile devices to
>>>> designate default link power managmeent policy to influence runtime
>>>> power consumption.
>>>>
>>>> As this is interesting for more than just mobile designs, rename the
>>>> board to `board_ahci_low_power` to make it clear it is about default
>>>> policy.
>>>
>>> Is there any good reason to not just apply the policy to all devices
>>> by default?
>>
>> That sure would make this all cleaner.
>>
>> I think Hans knows more of the history here than anyone else. I had
>> presumed there was some data loss scenarios with some of the older
>> chipsets.
>
> When I first introduced this change there were reports of crashes and
> data corruption caused by setting the policy to min_power, these were
> tied to some motherboards and/or to some drives.
>
> This is the whole reason why I only enabled this on a subset of all the
> AHCI chipsets.
>
> At least on devices with a chipset which is currently marked as
> mobile, the motherboard specific issues could be fixed with a BIOS
> update. But I doubt that similar BIOS fixes have also been rolled
> out to all desktop boards (and have been applied by all users),
> and I also don't know about older boards.
>
> So enabling this on all chipsets is definitely not without risks.
Exactly, even requiring to update the firmware would go against Linux’
no regression rule.
When new chipset are added from now on, we should ask the submitter to
test with LPM first though.
Mario’s patches look fine to me, and other changes should be done in
follow-up patches.
All are:
Reviewed-by: Paul Menzel <[email protected]>
Kind regards,
Paul
Hi,
On 2/25/22 17:04, Limonciello, Mario wrote:
> [Public]
>
>> On Fri, Feb 25, 2022 at 12:11:11AM -0600, Mario Limonciello wrote:
>>> This board definition was originally created for mobile devices to
>>> designate default link power managmeent policy to influence runtime
>>> power consumption.
>>>
>>> As this is interesting for more than just mobile designs, rename the
>>> board to `board_ahci_low_power` to make it clear it is about default
>>> policy.
>>
>> Is there any good reason to not just apply the policy to all devices
>> by default?
>
> That sure would make this all cleaner.
>
> I think Hans knows more of the history here than anyone else. I had
> presumed there was some data loss scenarios with some of the older
> chipsets.
When I first introduced this change there were reports of crashes and
data corruption caused by setting the policy to min_power, these were
tied to some motherboards and/or to some drives.
This is the whole reason why I only enabled this on a subset of all the
AHCI chipsets.
At least on devices with a chipset which is currently marked as
mobile, the motherboard specific issues could be fixed with a BIOS
update. But I doubt that similar BIOS fixes have also been rolled
out to all desktop boards (and have been applied by all users),
and I also don't know about older boards.
So enabling this on all chipsets is definitely not without risks.
Regards,
Hans
>
> Hans?
>
[Public]
> On 2/25/22 17:04, Limonciello, Mario wrote:
> > [Public]
> >
> >> On Fri, Feb 25, 2022 at 12:11:11AM -0600, Mario Limonciello wrote:
> >>> This board definition was originally created for mobile devices to
> >>> designate default link power managmeent policy to influence runtime
> >>> power consumption.
> >>>
> >>> As this is interesting for more than just mobile designs, rename the
> >>> board to `board_ahci_low_power` to make it clear it is about default
> >>> policy.
> >>
> >> Is there any good reason to not just apply the policy to all devices
> >> by default?
> >
> > That sure would make this all cleaner.
> >
> > I think Hans knows more of the history here than anyone else. I had
> > presumed there was some data loss scenarios with some of the older
> > chipsets.
>
> When I first introduced this change there were reports of crashes and
> data corruption caused by setting the policy to min_power, these were
> tied to some motherboards and/or to some drives.
>
> This is the whole reason why I only enabled this on a subset of all the
> AHCI chipsets.
>
> At least on devices with a chipset which is currently marked as
> mobile, the motherboard specific issues could be fixed with a BIOS
> update. But I doubt that similar BIOS fixes have also been rolled
> out to all desktop boards (and have been applied by all users),
> and I also don't know about older boards.
>
> So enabling this on all chipsets is definitely not without risks.
>
This was before min_power_with_partial and min_power_with_dipm
were introduced though right? Maybe another way to look at this
is to drop the policy min_power, which overall is dangerous.