On 2/7/24 06:13, 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@xxxxxxx/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, see: > https://studylib.net/doc/10034428/esata---microsoft-center > > 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. > > Damien: considering that the Intel VMD + ahci_intel_pcs_quirk() bug turned > out to have nothing to do with LPM, it was simply the fact that the > ahci_intel_pcs_quirk() was only applied if there was an explicit entry in > ahci_pci_tbl. So since that bug is totally unrelated to LPM, I no longer > think that this series need to wait for a fix for that bug. > > > Link to v1: > https://lore.kernel.org/linux-ide/20240201161507.1147521-1-cassel@xxxxxxxxxx/ > > Changes since v1: > -Picked up tags from Damien. > -Moved the comment in front of ahci_mark_external_port() to inside the > function. > -Modified the comment in patch 4/5 to more clearly state hotplug removal > events. > -Rewrote the commit message for patch 4/5 to be more detailed. I tested the series on an AMD machine which has AMD AHCI (FCH SATA Controller [AHCI mode] (rev 51), PCI IDs: 1022:7901) and also a Marvell 88SE9128 PCIe SATA adapter (PCI ID 1b4b:9128). SATA Hotplug for the Marvell adapter works out of the box using 6.8-rc3 but *does not work* at all for the AMD AHCI (not even link down events are seen when a drive is pulled from a front loading slot). Applying the series on top of 6.8-rc3, hotplug with the Marvell adapter continues to work just fine and the AMD AHCI hotplug also finally works ! When pulling a drive I see link down events and re-plugging the drive I see: [ 156.984966] ata20: found unknown device (class 0) [ 157.148919] ata20: softreset failed (device not ready) [ 162.922094] ata20: link is slow to respond, please be patient (ready=0) [ 167.193191] ata20: found unknown device (class 0) [ 167.357190] ata20: softreset failed (device not ready) [ 168.701232] ata20: SATA link up 6.0 Gbps (SStatus 133 SControl 300) [ 168.712580] ata20.00: ATA-12: ... ... [ 168.829910] sd 19:0:0:0: [sdj] Write Protect is off [ 168.835403] sd 19:0:0:0: [sdj] Mode Sense: 00 3a 00 00 [ 168.835584] sd 19:0:0:0: [sdj] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA [ 168.845872] sd 19:0:0:0: [sdj] Preferred minimum I/O size 4096 bytes [ 170.601826] sd 19:0:0:0: [sdj] Attached SCSI removable disk So this seems all good to me. Feel free to add: Tested-by: Damien Le Moal <dlemoal@xxxxxxxxxx> -- Damien Le Moal Western Digital Research
