Re: [PATCH v3 05/23] ata: libata-scsi: Disable scsi device manage_system_start_stop

[Hannes Reinecke <hare@suse.de>]

On 9/15/23 10:14, 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 that ATA device fixes the ordering of system 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 resume is scheduled in EH,
> thus avoiding to see new requests prematurely issued to the ATA device.
> Since libata sets manage_system_start_stop to 1, the scsi disk resume
> operation also results in issuing a START STOP UNIT command to the
> device being resumed so that the device exits standby power mode.
> 
> However, restoring the ATA device to the active power mode must be
> synchronized with libata EH processing of the port resume operation to
> avoid either 1) seeing the start stop unit command being received too
> early when the port is not yet resumed and ready to accept commands, or
> after the port resume process issues commands such as IDENTIFY to
> revalidate the device. In this last case, the risk is that the device
> revalidation fails with timeout errors as the drive is still spun down.
> 
> Commit 0a8589055936 ("ata,scsi: do not issue START STOP UNIT on resume")
> disabled issuing the START STOP UNIT command to avoid issues with it.
> But 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 IDENTIFY, READ LOG and SET FEATURES commands executed in
> libata EH context triggered by the ata port resume operation may thus
> fail.
> 
> Fix these synchronization issues is by handling a device power mode
> transitions for system suspend and resume directly in libata EH context,
> without relying on the scsi disk driver management triggered with the
> manage_system_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.
> 
> For resume, call ata_dev_power_set_active() in
> ata_eh_revalidate_and_attach() after the port has been enabled and
> before any other command is issued to the device.
> 
> With these changes, the manage_system_start_stop and no_start_on_resume
> scsi device flags do not need to be set in ata_scsi_dev_config(). The
> flag manage_runtime_start_stop is still set to allow the sd driver to
> spinup/spindown a disk through the sd runtime operations.
> 
> Fixes: 0a8589055936 ("ata,scsi: do not issue START STOP UNIT on resume")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-core.c | 90 +++++++++++++++++++++++++++++++++++++++
>   drivers/ata/libata-eh.c   | 46 +++++++++++++++++++-
>   drivers/ata/libata-scsi.c | 16 +++----
>   drivers/ata/libata.h      |  2 +
>   include/linux/libata.h    |  6 ++-
>   5 files changed, 148 insertions(+), 12 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg
Managing Directors: I. Totev, A. Myers, A. McDonald, M. B. Moerman
(HRB 36809, AG Nürnberg)