Re: [PATCH 04/19] ata: libata-scsi: Disable scsi device manage_start_stop

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

On 9/11/23 15:46, Hannes Reinecke wrote:
> On 9/11/23 06:02, Damien Le Moal wrote:
>> The introduction of a device link to create a consumer/supplier
>> relationship between the scsi device of an ATA device and the ATA port
>> of the ATA device fixed the ordering of the suspend and resume
>> operations. For suspend, the scsi device is suspended first and the ata
>> port after it. This is fine as this allows the synchronize cache and
>> START STOP UNIT commands issued by the scsi disk driver to be executed
>> before the ata port is disabled.
>>
>> For resume operations, the ata port is resumed first, followed
>> by the scsi device. This allows having the request queue of the scsi
>> device to be unfrozen after the ata port restart is scheduled in EH,
>> thus avoiding to see new requests issued to the ATA device prematurely.
>> However, since libata sets manage_start_stop to 1, the scsi disk resume
>> operation also results in issuing a START STOP UNIT command to wakeup
>> the device. This is too late and that must be done before libata EH
>> resume handling starts revalidating the drive with IDENTIFY etc
>> commands. Commit 0a8589055936 ("ata,scsi: do not issue START STOP UNIT
>> on resume") disabled issuing the START STOP UNIT command to avoid
>> issues with it. However, this is incorrect as transitioning a device to
>> the active power mode from the standby power mode set on suspend
>> requires a media access command. The device link reset and subsequent
>> SET FEATURES, IDENTIFY and READ LOG commands executed in libata EH
>> context triggered by the ata port resume operation may thus fail.
>>
>> Fix this by handling a device power mode transitions for suspend and
>> resume in libata EH context without relying on the scsi disk management
>> triggered with the manage_start_stop flag.
>>
>> To do this, the following libata helper functions are introduced:
>>
>> 1) ata_dev_power_set_standby():
>>
>> This function issues a STANDBY IMMEDIATE command to transitiom a device
>> to the standby power mode. For HDDs, this spins down the disks. This
>> function applies only to ATA and ZAC devices and does nothing otherwise.
>> This function also does nothing for devices that have the
>> ATA_FLAG_NO_POWEROFF_SPINDOWN or ATA_FLAG_NO_HIBERNATE_SPINDOWN flag
>> set.
>>
>> For suspend, call ata_dev_power_set_standby() in
>> ata_eh_handle_port_suspend() before the port is disabled and frozen.
>> ata_eh_unload() is also modified to transition all enabled devices to
>> the standby power mode when the system is shutdown or devices removed.
>>
>> 2) ata_dev_power_set_active() and
>>
>> This function applies to ATA or ZAC devices and issues a VERIFY command
>> for 1 sector at LBA 0 to transition the device to the active power mode.
>> For HDDs, since this function will complete only once the disk spin up.
>> Its execution uses the same timeouts as for reset, to give the drive
>> enough time to complete spinup without triggering a command timeout.
>>
> Neat. But why VERIFY?

Ask that to T13 :) Need a media access command to get out of sleep state...
Could use a read, but then need a buffer, which is silly for just waking up a
drive. VERIFY is a mandatory command.

> Isn't there a dedicated command (ie the opposite of STANDBY IMMEDIATE)?
> And can we be sure that VERIFY is implemented everywhere?
> It's not that this command had been in active use until now ...

START STOP UNIT with start == 1 has been translated to a VERIFY command since
forever. This is according to SAT specs. There is no command to explicitly get
out of standby-mode. Even a reset should not change the drive power state
(though I do see a lot of drive waking up on COMRESET). A media access command
does that. See ACS specs "Power Management states and transitions". The only
exception is that you can use SET FEATURE command to wake up a drive, but only
from PUIS state (Power-Up in Standby), which is a different feature that is not
necessarilly supported by a device.

> 
> Cheers,
> 
> Hannes

-- 
Damien Le Moal
Western Digital Research