On Tue, Aug 29, 2023 at 11:35:54AM +0200, Werner Fischer wrote:
> On Tue, 2023-08-29 at 15:09 +0900, Damien Le Moal wrote:
> > > Intel lists a total of 3 SATA controller IDs (4B60, 4B62, 4B63) in
> > > [1] for those mobile PCHs.
> > > This commit just adds 0x4b63 as I do not have test systems with
> > > 0x4b60 and 0x4b62 SATA controllers.
> >
> > Adding a mention about the other 2 IDs in a comment would be nice.
>
> Thank you for your time reviewing the patch.
> I will send a v2 with an added comment about the other 2 IDs.
Considering the big mess that was introduced the last time we tried to
enable LPM in an Intel AHCI controller, see:
104ff59af73a ("ata: ahci: Add Tiger Lake UP{3,4} AHCI controller")
and
6210038aeaf4 ("ata: ahci: Revert "ata: ahci: Add Tiger Lake UP{3,4} AHCI controller"")
This patch makes me feel quite uncomfortable.
Yes, an AHCI controller should be able to have LPM enabled, but in reality
there are so many SATA devices with broken LPM support, that we would need
to add hundreds of quirks for all the broken devices.
The simple fact that LPM is commonly broken in the wild, makes me think that
systems should need to opt-in explicitly to enable it.
Yes, our default LPM policy is 0:
0 => Keep firmware settings
But in reality most distros use:
3 => Medium power with Device Initiated PM enabled
And in the end, we end up breaking users systems:
https://bugzilla.kernel.org/show_bug.cgi?id=217114
And if their root partition is on the SATA drive... they won't be able to boot.
Yes, one problem is that you need to use the board_ahci_low_power in order for
the LPM polcies to apply. But it feels to me that accepting such a change is
too intrusive (because of the distros default of LPM policy 3).
If we somehow had a way where regular users are less affected...
E.g. if there was a way to try to link up with LPM enabled in the AHCI
controller by default, but if it fails X amount of times, then we disable
LPM in the controller (or maybe try to set policy to ATA_LPM_MAX_POWER).
Kind of like how we do for link speed, where we try a lower link speed on
the final attempt to establish a link:
https://github.com/torvalds/linux/blob/v6.5/drivers/ata/libata-eh.c#L3599
Another option might be to do the opposite.. Establish a link with LPM disabled
(or maybe with policy set to ATA_LPM_MAX_POWER), if we can establish a link,
make a record of it, and then enable LPM and reset the drive.
If we can't establish a link with LPM enabled, and we have a record that
the link was established without LPM, disable LPM again and reset the drive.
Not sure which is most feasible to implement.
Kind regards,
Niklas
