Re: [PATCH] ata: libata-core: Add ATA_HORKAGE_NOLPM for Apacer AS340

[Damien Le Moal <dlemoal@kernel.org>]

On 5/31/24 6:55 AM, Niklas Cassel wrote:
> On Thu, May 30, 2024 at 11:27:04PM +0200, Niklas Cassel wrote:
>> Commit 7627a0edef54 ("ata: ahci: Drop low power policy board type")
>> dropped the board_ahci_low_power board type, and instead enables LPM if:
>> -The AHCI controller reports that it supports LPM (Partial/Slumber), and
>> -CONFIG_SATA_MOBILE_LPM_POLICY != 0, and
>> -The port is not defined as external in the per port PxCMD register, and
>> -The port is not defined as hotplug capable in the per port PxCMD
>>  register.
>>
>> Partial and Slumber LPM states can either be initiated by HIPM or DIPM.
>>
>> For HIPM (host initiated power management) to get enabled, both the AHCI
>> controller and the drive have to report that they support HIPM.
>>
>> For DIPM (device initiated power management) to get enabled, only the
>> drive has to report that it supports DIPM. However, the HBA will reject
>> device requests to enter LPM states which the HBA does not support.
>>
>> The problem is that Apacer AS340 drives do not handle low power modes
>> correctly. The problem was most likely not seen before because no one
>> had used this drive with a AHCI controller with LPM enabled.
>>
>> Add a quirk so that we do not enable LPM for this drive, since we see
>> command timeouts if we do (even though the drive claims to support DIPM).
>>
>> Fixes: 7627a0edef54 ("ata: ahci: Drop low power policy board type")
>> Cc: stable@vger.kernel.org
>> Reported-by: Tim Teichmann <teichmanntim@outlook.de>
>> Closes: https://lore.kernel.org/linux-ide/87bk4pbve8.ffs@tglx/
>> Signed-off-by: Niklas Cassel <cassel@kernel.org>
>> ---
>> On the system reporting this issue, the HBA supports SALP (HIPM) and
>> LPM states Partial and Slumber.
>>
>> This drive only supports DIPM but not HIPM, however, that should not
>> matter, as a DIPM request from the device still has to be acked by the
>> HBA, and according to AHCI 1.3.1, section 5.3.2.11 P:Idle, if the link
>> layer has negotiated to low power state based on device power management
>> request, the HBA will jump to state PM:LowPower.
>>
>> In PM:LowPower, the HBA will automatically request to wake the link
>> (exit from Partial/Slumber) when a new command is queued (by writing to
>> PxCI). Thus, there should be no need for host software to request an
>> explicit wakeup (by writing PxCMD.ICC to 1).
>>
>> Therefore, even with only DIPM supported/enabled, we shouldn't see command
>> timeouts with the current code. Also, only enabling only DIPM (by
>> modifying the AHCI driver) with another drive (which support both DIPM
>> and HIPM), shows no errors. Thus, it seems like the drive is the problem.
>>
>>  drivers/ata/libata-core.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 4f35aab81a0a..25b400f1c3de 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -4155,6 +4155,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
>>  						ATA_HORKAGE_ZERO_AFTER_TRIM |
>>  						ATA_HORKAGE_NOLPM },
>>  
>> +	/* Apacer models with LPM issues */
>> +	{ "Apacer AS340*",		NULL,	ATA_HORKAGE_NOLPM },
>> +
>>  	/* These specific Samsung models/firmware-revs do not handle LPM well */
>>  	{ "SAMSUNG MZMPC128HBFU-000MV", "CXM14M1Q", ATA_HORKAGE_NOLPM },
>>  	{ "SAMSUNG SSD PM830 mSATA *",  "CXM13D1Q", ATA_HORKAGE_NOLPM },
>> -- 
>> 2.45.1
>>
> 
> One interesting fact which:
> Apacer AS340, CT240BX500SSD1, and R3SL240G all have in common:
> their SSD controller is made by Silicon Motion:
> https://smarthdd.com/database/Apacer-AS340-120GB/U1014A0/
> https://smarthdd.com/database/CT240BX500SSD1/M6CR013/
> https://smarthdd.com/database/R3SL240G/P0422C/
> 
> Not sure if that is relevant or not...

Well, I guess that probably explains why they all have the same issue with LPM.

-- 
Damien Le Moal
Western Digital Research