Fix libata suspend/resume handling and code cleanup
Damien Le Moal
maintainerv5 cover Fix libata suspend/resume handling and code cleanup
The first 9 patches of this series fix several issues with suspend/resume power management operations in scsi and libata. The most significant changes introduced are in patch 4 and 5, where the manage_start_stop flag of scsi devices is split into the manage_system_start_stop and manage_runtime_start_stop flags to allow keeping scsi runtime power operations for spining up/down ATA devices but have libata do its own system suspend/resume device power state management using EH.
The remaining patches are code cleanup that do not introduce any significant functional change.
This series was tested on qemu and on various PCs and servers. I am CC-ing people who recently reported issues with suspend/resume. Additional testing would be much appreciated.
Changes from v4:
* Remove ata_scsi_dev_alloc() function in patch 3, coding it directly in ata_scsi_slave_alloc()
* Correct typo in patch 19 commit message
* Added Tested and review tags
Changes from v3:
* Corrected patch 1 (typo in commit message and WARN_ON() removal)
* Changed path 3 as suggested by Niklas (moved definition of ->slave_alloc)
* Rebased on rc2
* Added review tags
Changes from v2:
* Added patch 4 as simply disabling manage_start_stop from libata was breaking individual disk runtime suspend/autosuspend. Patch 5 was reworked accordingly to the changes in patch 4.
* Fixed patch 3: applying the link creation was missing and the link creation itself was also incorrect, preventing sd probe to execute correctly. Thanks to Geert for testing and reporting this issue.
* Split the "Fix delayed scsi_rescan_device() execution" patch into patch 6 (scsi part) and patch 7 (ata part).
* Modified patch 9 to not call sd_shutdown() from sd_remove() for devices that are not running.
* Added Chia-Lin Tested tag to unchanged patches
Changes from v1:
* Added patch 8 and 9 to fix compilation warnings with W=1
* Addressed John comment in patch 19
* Fixed patch 20 commit message (Sergei)
* Added Hannes Review tag
Damien Le Moal (23):
ata: libata-core: Fix ata_port_request_pm() locking
ata: libata-core: Fix port and device removal
ata: libata-scsi: link ata port and scsi device
scsi: sd: Differentiate system and runtime start/stop management
ata: libata-scsi: Disable scsi device manage_system_start_stop
scsi: Do not attempt to rescan suspended devices
ata: libata-scsi: Fix delayed scsi_rescan_device() execution
ata: libata-core: Do not register PM operations for SAS ports
scsi: sd: Do not issue commands to suspended disks on shutdown
ata: libata-core: Fix compilation warning in ata_dev_config_ncq()
ata: libata-eh: Fix compilation warning in ata_eh_link_report()
scsi: Remove scsi device no_start_on_resume flag
ata: libata-scsi: Cleanup ata_scsi_start_stop_xlat()
ata: libata-core: Synchronize ata_port_detach() with hotplug
ata: libata-core: Detach a port devices on shutdown
ata: libata-core: Remove ata_port_suspend_async()
ata: libata-core: Remove ata_port_resume_async()
ata: libata-core: Do not poweroff runtime suspended ports
ata: libata-core: Do not resume runtime suspended ports
ata: libata-sata: Improve ata_sas_slave_configure()
ata: libata-eh: Improve reset error messages
ata: libata-eh: Reduce "disable device" message verbosity
ata: libata: Cleanup inline DMA helper functions
drivers/ata/libata-core.c | 242 +++++++++++++++++++++++++-------- drivers/ata/libata-eh.c | 76 +++++++++-- drivers/ata/libata-sata.c | 5 +- drivers/ata/libata-scsi.c | 142 ++++++++++--------- drivers/ata/libata-transport.c | 9 +- drivers/ata/libata.h | 6 + drivers/firewire/sbp2.c | 9 +- drivers/scsi/scsi_scan.c | 18 ++- drivers/scsi/sd.c | 88 ++++++++---- include/linux/libata.h | 26 ++-- include/scsi/scsi_device.h | 4 +- include/scsi/scsi_host.h | 2 +- 12 files changed, 439 insertions(+), 188 deletions(-)
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 1/23 ata: libata-core: Fix ata_port_request_pm() locking
The function ata_port_request_pm() checks the port flag ATA_PFLAG_PM_PENDING and calls ata_port_wait_eh() if this flag is set to ensure that power management operations for a port are not scheduled simultaneously. However, this flag check is done without holding the port lock.
Fix this by taking the port lock on entry to the function and checking the flag under this lock. The lock is released and re-taken if ata_port_wait_eh() needs to be called. The two WARN_ON() macros checking that the ATA_PFLAG_PM_PENDING flag was cleared are removed as the first call is racy and the second one done without holding the port lock.
--- drivers/ata/libata-core.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
drivers/ata/libata-core.c +9 -9
@@ -5037,17 +5037,19 @@ static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,struct ata_link *link;unsigned long flags; -/* Previous resume operation might still be in -* progress. Wait for PM_PENDING to clear. +spin_lock_irqsave (ap->lock, flags); + +/* +* A previous PM operation might still be in progress. Wait for +* ATA_PFLAG_PM_PENDING to clear. */ if (ap->pflags &ATA_PFLAG_PM_PENDING ) { +spin_unlock_irqrestore (ap->lock, flags);ata_port_wait_eh (ap); -WARN_ON (ap->pflags &ATA_PFLAG_PM_PENDING ); +spin_lock_irqsave (ap->lock, flags); } -/* request PM ops to EH */ -spin_lock_irqsave (ap->lock, flags); - +/* Request PM operation to EH */ ap->pm_mesg = mesg; ap->pflags |=ATA_PFLAG_PM_PENDING ;ata_for_each_link (link, ap,HOST_FIRST ) {
@@ -5059,10 +5061,8 @@ static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,spin_unlock_irqrestore (ap->lock, flags); -if (!async) { +if (!async)ata_port_wait_eh (ap); -WARN_ON (ap->pflags &ATA_PFLAG_PM_PENDING ); - } }/*
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 2/23 ata: libata-core: Fix port and device removal
In reply to Damien Le MoalWhenever an ATA adapter driver is removed (e.g. rmmod), ata_port_detach() is called repeatedly for all the adapter ports to remove (unload) the devices attached to the port and delete the port device itself. Removing of devices is done using libata EH with the ATA_PFLAG_UNLOADING port flag set. This causes libata EH to execute ata_eh_unload() which disables all devices attached to the port.
ata_port_detach() finishes by calling scsi_remove_host() to remove the scsi host associated with the port. This function will trigger the removal of all scsi devices attached to the host and in the case of disks, calls to sd_shutdown() which will flush the device write cache and stop the device. However, given that the devices were already disabled by ata_eh_unload(), the synchronize write cache command and start stop unit commands fail. E.g. running "rmmod ahci" with first removing sd_mod results in error messages like:
ata13.00: disable device
sd 0:0:0:0: [sda] Synchronizing SCSI cache
sd 0:0:0:0: [sda] Synchronize Cache(10) failed: Result: hostbyte=DID_BAD_TARGET driverbyte=DRIVER_OK sd 0:0:0:0: [sda] Stopping disk
sd 0:0:0:0: [sda] Start/Stop Unit failed: Result: hostbyte=DID_BAD_TARGET driverbyte=DRIVER_OK
Fix this by removing all scsi devices of the ata devices connected to the port before scheduling libata EH to disable the ATA devices. Also delete the WAR_ON() call checking that the ATA_PFLAG_UNLOADING flag was cleared as that is done without holding the port lock.
--- drivers/ata/libata-core.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
drivers/ata/libata-core.c +20 -1
@@ -5948,11 +5948,30 @@ static void ata_port_detach(struct ata_port *ap)struct ata_link *link;struct ata_device *dev; -/* tell EH we're leaving & flush EH */ +/* Wait for any ongoing EH */ +ata_port_wait_eh (ap); + +mutex_lock (&ap->scsi_scan_mutex);spin_lock_irqsave (ap->lock, flags); + +/* Remove scsi devices */ +ata_for_each_link (link, ap,HOST_FIRST ) { +ata_for_each_dev (dev, link,ALL ) { +if (dev->sdev) { +spin_unlock_irqrestore (ap->lock, flags); +scsi_remove_device (dev->sdev); +spin_lock_irqsave (ap->lock, flags); + dev->sdev =NULL ; + } + } + } + +/* Tell EH to disable all devices */ ap->pflags |=ATA_PFLAG_UNLOADING ;ata_port_schedule_eh (ap); +spin_unlock_irqrestore (ap->lock, flags); +mutex_unlock (&ap->scsi_scan_mutex);/* wait till EH commits suicide */ ata_port_wait_eh (ap);
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 3/23 ata: libata-scsi: link ata port and scsi device
In reply to Damien Le MoalThere is no direct device ancestry defined between an ata_device and its scsi device which prevents the power management code from correctly ordering suspend and resume operations. Create such ancestry with the ata device as the parent to ensure that the scsi device (child) is suspended before the ata device and that resume handles the ata device before the scsi device.
The parent-child (supplier-consumer) relationship is established between the ata_port (parent) and the scsi device (child) with the function device_add_link(). The parent used is not the ata_device as the PM operations are defined per port and the status of all devices connected through that port is controlled from the port operations.
The device link is established with the new function ata_scsi_slave_alloc(), and this function is used to define the ->slave_alloc callback of the scsi host template of all ata drivers.
--- drivers/ata/libata-scsi.c | 45 ++++++++++++++++++++++++++++++++++----- include/linux/libata.h | 2 ++ 2 files changed, 42 insertions(+), 5 deletions(-)
drivers/ata/libata-scsi.c +40 -5
@@ -1089,6 +1089,42 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)return 0 ; } +/** +* ata_scsi_slave_alloc - Early setup of SCSI device +* @sdev: SCSI device to examine +* +* This is called from scsi_alloc_sdev() when the scsi device +* associated with an ATA device is scanned on a port. +* +* LOCKING: +* Defined by SCSI layer. We don't really care. +*/ + +int ata_scsi_slave_alloc (struct scsi_device *sdev) +{ +struct ata_port *ap =ata_shost_to_port (sdev->host); +struct device_link *link; + +ata_scsi_sdev_config (sdev); + +/* +* Create a link from the ata_port device to the scsi device to ensure +* that PM does suspend/resume in the correct order: the scsi device is +* consumer (child) and the ata port the supplier (parent). +*/ + link =device_link_add (&sdev->sdev_gendev, &ap->tdev, +DL_FLAG_STATELESS | +DL_FLAG_PM_RUNTIME |DL_FLAG_RPM_ACTIVE ); +if (!link) { +ata_port_err (ap,"Failed to create link to scsi device %s\n" , +dev_name (&sdev->sdev_gendev)); +return -ENODEV ; + } + +return 0 ; +} +EXPORT_SYMBOL_GPL (ata_scsi_slave_alloc); +/** * ata_scsi_slave_config - Set SCSI device attributes * @sdev: SCSI device to examine
@@ -1105,14 +1141,11 @@ int ata_scsi_slave_config(struct scsi_device *sdev) {struct ata_port *ap =ata_shost_to_port (sdev->host);struct ata_device *dev =__ata_scsi_find_dev (ap, sdev); -int rc =0 ; - -ata_scsi_sdev_config (sdev);if (dev) - rc =ata_scsi_dev_config (sdev, dev); +return ata_scsi_dev_config (sdev, dev); -return rc; +return 0 ; }EXPORT_SYMBOL_GPL (ata_scsi_slave_config);
@@ -1136,6 +1169,8 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev)unsigned long flags;struct ata_device *dev; +device_link_remove (&sdev->sdev_gendev, &ap->tdev); +spin_lock_irqsave (ap->lock, flags); dev =__ata_scsi_find_dev (ap, sdev);if (dev && dev->sdev) {
include/linux/libata.h +2 -0
@@ -1148,6 +1148,7 @@ extern int ata_std_bios_param(struct scsi_device *sdev,struct block_device *bdev, sector_t capacity, int geom[]);extern void ata_scsi_unlock_native_capacity (struct scsi_device *sdev); +extern int ata_scsi_slave_alloc (struct scsi_device *sdev);extern int ata_scsi_slave_config (struct scsi_device *sdev);extern void ata_scsi_slave_destroy (struct scsi_device *sdev); extern int ata_scsi_change_queue_depth(struct scsi_device *sdev,
@@ -1396,6 +1397,7 @@ extern const struct attribute_group *ata_common_sdev_groups[]; .this_id = ATA_SHT_THIS_ID, \ .emulated = ATA_SHT_EMULATED, \ .proc_name = drv_name, \ + .slave_alloc = ata_scsi_slave_alloc, \ .slave_destroy = ata_scsi_slave_destroy, \ .bios_param = ata_std_bios_param, \ .unlock_native_capacity = ata_scsi_unlock_native_capacity,\
Signature · 1 line
2.41.0
Re: [PATCH v5 03/23] ata: libata-scsi: link ata port and scsi device
Quoted 17 lines · Damien Le Moal
There is no direct device ancestry defined between an
ata_deviceand its scsi device which prevents the power management code from correctly ordering suspend and resume operations. Create such ancestry with the ata device as the parent to ensure that the scsi device (child) is suspended before the ata device and that resume handles the ata device before the scsi device.The parent-child (supplier-consumer) relationship is established between the
ata_port(parent) and the scsi device (child) with the functiondevice_add_link(). The parent used is not theata_deviceas the PM operations are defined per port and the status of all devices connected through that port is controlled from the port operations.The device link is established with the new function
ata_scsi_slave_alloc(), and this function is used to define the ->slave_alloc callback of the scsi host template of all ata drivers.
Quoted 6 lines
Fixes a19a93e4c6a9 ("scsi: core: pm: Rely on the device driver core for async power management")Signed-off-by Damien Le Moal<[email protected]>Reviewed-by Hannes Reinecke<[email protected]>Reviewed-by Niklas Cassel<[email protected]>Tested-by Geert Uytterhoeven<[email protected]>
Feel free to add:
Damien Le Moal
maintainerPATCH v5 4/23 scsi: sd: Differentiate system and runtime start/stop management
In reply to Damien Le MoalThe underlying device and driver of a scsi disk may have different system and runtime power mode control requirements. This is because runtime power management affects only the scsi disk, while sustem level power management affects all devices, including the controller for the scsi disk.
For instance, issuing a START STOP UNIT command when a scsi disk is runtime suspended and resumed is fine: the command is translated to a STANDBY IMMEDIATE command to spin down the ATA disk and to a VERIFY command to wake it up. The scsi disk runtime operations have no effect on the ata port device used to connect the ATA disk. However, for system suspend/resume operations, the ATA port used to connect the device will also be suspended and resumed, with the resum operation requiring re-validating the device link and the device itseld. In this case, issuing a VERIFY command to spinup the disk must be done before starting to revalidate the device, when the ata port is being resumed. In such case, we must not allow the scsi disk driver to issue START STOP UNIT commands.
Allow a low level driver to refine the scsi disk start/stop management by differentiating system and runtime cases with two new scsi device
flags: manage_system_start_stop and manage_runtime_start_stop. These new flags replace the current manage_start_stop flag. Drivers setting the manage_start_stop are modifed to set both new flags, thus preserving the existing start/stop management behavior.
--- drivers/ata/libata-scsi.c | 3 +- drivers/firewire/sbp2.c | 9 +++-- drivers/scsi/sd.c | 78 ++++++++++++++++++++++++++++---------- include/scsi/scsi_device.h | 3 +- 4 files changed, 69 insertions(+), 24 deletions(-)
drivers/ata/libata-scsi.c +2 -1
@@ -1056,7 +1056,8 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)* will be woken up by ata_port_pm_resume() with a port reset * and device revalidation. */ - sdev->manage_start_stop =1 ; + sdev->manage_system_start_stop =1 ; + sdev->manage_runtime_start_stop =1 ; sdev->no_start_on_resume =1 ; }
drivers/firewire/sbp2.c +6 -3
@@ -81,7 +81,8 @@ MODULE_PARM_DESC(exclusive_login, "Exclusive login to sbp2 device "* * - power condition * Set the power condition field in the START STOP UNIT commands sent by -* sd_mod on suspend, resume, and shutdown (if manage_start_stop is on). +* sd_mod on suspend, resume, and shutdown (if manage_system_start_stop or +* manage_runtime_start_stop is on). * Some disks need this to spin down or to resume properly. * * - override internal blacklist
@@ -1517,8 +1518,10 @@ static int sbp2_scsi_slave_configure(struct scsi_device *sdev) sdev->use_10_for_rw =1 ; -if (sbp2_param_exclusive_login) - sdev->manage_start_stop =1 ; +if (sbp2_param_exclusive_login) { + sdev->manage_system_start_stop =1 ; + sdev->manage_runtime_start_stop =1 ; + }if (sdev->type ==TYPE_ROM ) sdev->use_10_for_ms =1 ;
drivers/scsi/sd.c +59 -19
157 lines not on this page · raw message
include/scsi/scsi_device.h +2 -1
@@ -193,7 +193,8 @@ struct scsi_device { unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */ unsigned no_start_on_add:1; /* do not issue start on add */ unsigned allow_restart:1; /* issue START_UNIT in error handler */ - unsigned manage_start_stop:1; /* Let HLD (sd) manage start/stop */ + unsigned manage_system_start_stop:1; /* Let HLD (sd) manage system start/stop */ + unsigned manage_runtime_start_stop:1; /* Let HLD (sd) manage runtime start/stop */ unsigned no_start_on_resume:1; /* Do not issue START_STOP_UNIT on resume */ unsigned start_stop_pwr_cond:1; /* Set power cond. in START_STOP_UNIT */ unsigned no_uld_attach:1; /* disable connecting to upper level drivers */
Signature · 1 line
2.41.0
Re: [PATCH v5 04/23] scsi: sd: Differentiate system and runtime start/stop management
Damien,
Just a few minor nits...
Quoted 1 line
The underlying device and driver of a scsi disk may have different
Maybe it's just me, but I always trip over lowercase "scsi". And you write ATA in upper case...
Quoted 1 line
device will also be suspended and resumed, with the resum operation
resume
Quoted 1 line
requiring re-validating the device link and the device itseld. In this
itself
drivers/scsi/sd.c cache_type_store(struct device *dev, struct device_attribute *attr, · line 205
Quoted 10 lines
drivers/scsi/sd.c +0
-2@@ -201,18 +201,50 @@ cache_type_store(struct device *dev, struct device_attribute *attr, } static ssize_t -manage_start_stop_show (struct device *dev,struct device_attribute *attr, -char *buf)
Shouldn't we leave this for backwards compatibility?
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Damien Le Moal
maintainerRe: [PATCH v5 04/23] scsi: sd: Differentiate system and runtime start/stop management
Quoted 25 lines · Martin K. Petersen
Damien,
Just a few minor nits...
Quoted 1 line
The underlying device and driver of a scsi disk may have different
Maybe it's just me, but I always trip over lowercase "scsi". And you write ATA in upper case...
Quoted 1 line
device will also be suspended and resumed, with the resum operation
resume
Quoted 1 line
requiring re-validating the device link and the device itseld. In this
itself
Quoted 7 lines
drivers/scsi/sd.c +0
-0@@ -201,18 +201,50 @@ cache_type_store(struct device *dev, struct device_attribute *attr, }
Quoted 5 lines
drivers/scsi/sd.c cache_type_store(struct device *dev, struct device_attribute *attr, · line 205
Quoted 3 lines
drivers/scsi/sd.c +0
-2static ssize_t -manage_start_stop_show (struct device *dev,struct device_attribute *attr, -char *buf)Shouldn't we leave this for backwards compatibility?
Good point. But what should it show/control ? Both system & runtime flags (show a AND of the flag and set/reset both flags ?). Or make it read-only and show a AND of both flags ?
Signature · 2 lines
Damien Le Moal Western Digital Research
Damien Le Moal
maintainerPATCH v5 5/23 ata: libata-scsi: Disable scsi device manage_system_start_stop
In reply to Damien Le MoalThe 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.
--- 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(-)
drivers/ata/libata-core.c +90 -0
@@ -1972,6 +1972,96 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,return rc; } +/** +* ata_dev_power_set_standby - Set a device power mode to standby +* @dev: target device +* +* Issue a STANDBY IMMEDIATE command to set a device power mode to standby. +* For an HDD device, this spins down the disks. +* +* LOCKING: +* Kernel thread context (may sleep). +*/ +void ata_dev_power_set_standby (struct ata_device *dev) +{ +unsigned long ap_flags = dev->link->ap->flags; +struct ata_taskfile tf; +unsigned int err_mask; + +/* Issue STANDBY IMMEDIATE command only if supported by the device */ +if (dev->class !=ATA_DEV_ATA && dev->class !=ATA_DEV_ZAC ) +return ; + +/* +* Some odd clown BIOSes issue spindown on power off (ACPI S4 or S5) +* causing some drives to spin up and down again. For these, do nothing +* if we are being called on shutdown. +*/ +if ((ap_flags &ATA_FLAG_NO_POWEROFF_SPINDOWN ) && + system_state ==SYSTEM_POWER_OFF ) +return ; + +if ((ap_flags &ATA_FLAG_NO_HIBERNATE_SPINDOWN ) && +system_entering_hibernation ()) +return ; + +ata_tf_init (dev, &tf); + tf.flags |=ATA_TFLAG_DEVICE |ATA_TFLAG_ISADDR ; + tf.protocol =ATA_PROT_NODATA ; + tf.command =ATA_CMD_STANDBYNOW1 ; + +ata_dev_notice (dev,"Entering standby power mode\n" ); + + err_mask =ata_exec_internal (dev, &tf,NULL ,DMA_NONE ,NULL ,0 ,0 ); +if (err_mask) +ata_dev_err (dev,"STANDBY IMMEDIATE failed (err_mask=0x%x)\n" , + err_mask); +} + +/** +* ata_dev_power_set_active - Set a device power mode to active +* @dev: target device +* +* Issue a VERIFY command to enter to ensure that the device is in the +* active power mode. For a spun-down HDD (standby or idle power mode), +* the VERIFY command will complete after the disk spins up. +* +* LOCKING: +* Kernel thread context (may sleep). +*/ +void ata_dev_power_set_active (struct ata_device *dev) +{ +struct ata_taskfile tf; +unsigned int err_mask; + +/* +* Issue READ VERIFY SECTORS command for 1 sector at lba=0 only +* if supported by the device. +*/ +if (dev->class !=ATA_DEV_ATA && dev->class !=ATA_DEV_ZAC ) +return ; + +ata_tf_init (dev, &tf); + tf.flags |=ATA_TFLAG_DEVICE |ATA_TFLAG_ISADDR ; + tf.protocol =ATA_PROT_NODATA ; + tf.command =ATA_CMD_VERIFY ; + tf.nsect =1 ; +if (dev->flags &ATA_DFLAG_LBA ) { + tf.flags |=ATA_TFLAG_LBA ; + tf.device |=ATA_LBA ; + }else { +/* CHS */ + tf.lbal =0x1 ;/* sect */ + } + +ata_dev_notice (dev,"Entering active power mode\n" ); + + err_mask =ata_exec_internal (dev, &tf,NULL ,DMA_NONE ,NULL ,0 ,0 ); +if (err_mask) +ata_dev_err (dev,"VERIFY failed (err_mask=0x%x)\n" , + err_mask); +} +/** * ata_read_log_page - read a specific log page * @dev: target device
drivers/ata/libata-eh.c +45 -1
100 lines not on this page · raw message
drivers/ata/libata-scsi.c +7 -9
@@ -1050,15 +1050,13 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev) } }else { sdev->sector_size =ata_id_logical_sector_size (dev->id); +/* -* Stop the drive on suspend but do not issue START STOP UNIT -* on resume as this is not necessary and may fail: the device -* will be woken up by ata_port_pm_resume() with a port reset -* and device revalidation. +* Ask the sd driver to issue START STOP UNIT on runtime suspend +* and resume only. For system level suspend/resume, devices +* power state is handled directly by libata EH. */ - sdev->manage_system_start_stop =1 ; sdev->manage_runtime_start_stop =1 ; - sdev->no_start_on_resume =1 ; }/*
@@ -1231,7 +1229,7 @@ static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc) }if (cdb[4 ] &0x1 ) { - tf->nsect =1 ;/* 1 sector, lba=0 */ + tf->nsect =1 ;/* 1 sector, lba=0 */ if (qc->dev->flags &ATA_DFLAG_LBA ) { tf->flags |=ATA_TFLAG_LBA ;
@@ -1247,7 +1245,7 @@ static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc) tf->lbah =0x0 ;/* cyl high */ } - tf->command =ATA_CMD_VERIFY ;/* READ VERIFY */ + tf->command =ATA_CMD_VERIFY ;/* READ VERIFY */ }else {/* Some odd clown BIOSen issue spindown on power off (ACPI S4 * or S5) causing some drives to spin up and down again.
@@ -1257,7 +1255,7 @@ static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc) goto skip;if ((qc->ap->flags &ATA_FLAG_NO_HIBERNATE_SPINDOWN ) && -system_entering_hibernation ()) +system_entering_hibernation ()) goto skip;/* Issue ATA STANDBY IMMEDIATE command */
drivers/ata/libata.h +2 -0
@@ -60,6 +60,8 @@ extern int ata_dev_reread_id(struct ata_device *dev, unsigned int readid_flags);extern int ata_dev_revalidate (struct ata_device *dev,unsigned int new_class,unsigned int readid_flags);extern int ata_dev_configure (struct ata_device *dev); +extern void ata_dev_power_set_standby (struct ata_device *dev); +extern void ata_dev_power_set_active (struct ata_device *dev);extern int sata_down_spd_limit (struct ata_link *link,u32 spd_limit);extern int ata_down_xfermask_limit (struct ata_device *dev,unsigned int sel); extern unsigned int ata_dev_set_feature(struct ata_device *dev,
include/linux/libata.h +4 -2
@@ -192,6 +192,7 @@ enum {ATA_PFLAG_UNLOADING = (1 <<9 ),/* driver is being unloaded */ ATA_PFLAG_UNLOADED = (1 <<10 ),/* driver is unloaded */ +ATA_PFLAG_RESUMING = (1 <<16 ),/* port is being resumed */ ATA_PFLAG_SUSPENDED = (1 <<17 ),/* port is suspended (power) */ ATA_PFLAG_PM_PENDING = (1 <<18 ),/* PM operation pending */ ATA_PFLAG_INIT_GTM_VALID = (1 << 19), /* initial gtm data valid */
@@ -318,9 +319,10 @@ enum {ATA_EH_ENABLE_LINK = (1 <<3 ),ATA_EH_PARK = (1 <<5 ),/* unload heads and stop I/O */ ATA_EH_GET_SUCCESS_SENSE = (1 <<6 ),/* Get sense data for successful cmd */ +ATA_EH_SET_ACTIVE = (1 <<7 ),/* Set a device to active power mode */ ATA_EH_PERDEV_MASK =ATA_EH_REVALIDATE |ATA_EH_PARK | -ATA_EH_GET_SUCCESS_SENSE , +ATA_EH_GET_SUCCESS_SENSE |ATA_EH_SET_ACTIVE ,ATA_EH_ALL_ACTIONS =ATA_EH_REVALIDATE |ATA_EH_RESET | ATA_EH_ENABLE_LINK,
@@ -357,7 +359,7 @@ enum {/* This should match the actual table size of * ata_eh_cmd_timeout_table in libata-eh.c. */ - ATA_EH_CMD_TIMEOUT_TABLE_SIZE = 7, + ATA_EH_CMD_TIMEOUT_TABLE_SIZE = 8,/* Horkage types. May be set by libata or controller on drives (some horkage may be drive/controller pair dependent */
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 6/23 scsi: Do not attempt to rescan suspended devices
In reply to Damien Le Moalscsi_rescan_device() takes a scsi device lock before executing a device handler and device driver rescan methods. Waiting for the completion of any command issued to the device by these methods will thus be done with the device lock held. As a result, there is a risk of deadlocking within the power management code if scsi_rescan_device() is called to handle a device resume with the associated scsi device not yet resumed.
Avoid such situation by checking that the target scsi device is in the running state, that is, fully capable of executing commands, before proceeding with the rescan and bailout returning -EWOULDBLOCK otherwise. With this error return, the caller can retry rescaning the device after a delay.
The state check is done with the device lock held and is thus safe against incoming suspend power management operations.
--- drivers/scsi/scsi_scan.c | 18 +++++++++++++++++- include/scsi/scsi_host.h | 2 +- 2 files changed, 18 insertions(+), 2 deletions(-)
drivers/scsi/scsi_scan.c +17 -1
@@ -1619,12 +1619,24 @@ int scsi_add_device(struct Scsi_Host *host, uint channel, }EXPORT_SYMBOL (scsi_add_device); -void scsi_rescan_device (struct scsi_device *sdev) +int scsi_rescan_device (struct scsi_device *sdev) {struct device *dev = &sdev->sdev_gendev; +int ret =0 ;device_lock (dev); +/* +* Bail out if the device is not running. Otherwise, the rescan may +* block waiting for commands to be executed, with us holding the +* device lock. This can result in a potential deadlock in the power +* management core code when system resume is on-going. +*/ +if (sdev->sdev_state !=SDEV_RUNNING ) { + ret = -EWOULDBLOCK ; + goto unlock; + } +scsi_attach_vpd (sdev);scsi_cdl_check (sdev);
@@ -1638,7 +1650,11 @@ void scsi_rescan_device(struct scsi_device *sdev) drv->rescan (dev);module_put (dev->driver->owner); } + +unlock:device_unlock (dev); + +return ret; }EXPORT_SYMBOL (scsi_rescan_device);
include/scsi/scsi_host.h +1 -1
@@ -764,7 +764,7 @@ scsi_template_proc_dir(const struct scsi_host_template *sht);#define scsi_template_proc_dir (sht) NULL#endif extern void scsi_scan_host (struct Scsi_Host *); -extern void scsi_rescan_device (struct scsi_device *); +extern int scsi_rescan_device (struct scsi_device *sdev);extern void scsi_remove_host (struct Scsi_Host *);extern struct Scsi_Host *scsi_host_get (struct Scsi_Host *);extern int scsi_host_busy (struct Scsi_Host *shost);
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 7/23 ata: libata-scsi: Fix delayed scsi_rescan_device() execution
In reply to Damien Le MoalCommit 6aa0365a3c85 ("ata: libata-scsi: Avoid deadlock on rescan after device resume") modified ata_scsi_dev_rescan() to check the scsi device "is_suspended" power field to ensure that the scsi device associated with an ATA device is fully resumed when scsi_rescan_device() is executed. However, this fix is problematic as:
1) It relies on a PM internal field that should not be used without PM device locking protection.
2) The check for is_suspended and the call to scsi_rescan_device() are not atomic and a suspend PM event may be triggered between them, casuing scsi_rescan_device() to be called on a suspended device and in that function blocking while holding the scsi device lock. This would deadlock a following resume operation.
These problems can trigger PM deadlocks on resume, especially with resume operations triggered quickly after or during suspend operations. E.g., a simple bash script like:
for (( i=0; i<10; i++ )); do
echo "+2 > /sys/class/rtc/rtc0/wakealarm
echo mem > /sys/power/state
done
that triggers a resume 2 seconds after starting suspending a system can quickly lead to a PM deadlock preventing the system from correctly resuming.
Fix this by replacing the check on is_suspended with a check on the return value given by scsi_rescan_device() as that function will fail if called against a suspended device. Also make sure rescan tasks already scheduled are first cancelled before suspending an ata port.
--- drivers/ata/libata-core.c | 16 ++++++++++++++++ drivers/ata/libata-scsi.c | 33 +++++++++++++++------------------ 2 files changed, 31 insertions(+), 18 deletions(-)
drivers/ata/libata-core.c +16 -0
@@ -5168,11 +5168,27 @@ static const unsigned int ata_port_suspend_ehi = ATA_EHI_QUIETstatic void ata_port_suspend (struct ata_port *ap,pm_message_t mesg) { +/* +* We are about to suspend the port, so we do not care about +* scsi_rescan_device() calls scheduled by previous resume operations. +* The next resume will schedule the rescan again. So cancel any rescan +* that is not done yet. +*/ +cancel_delayed_work_sync (&ap->scsi_rescan_task); +ata_port_request_pm (ap, mesg,0 , ata_port_suspend_ehi, false); }static void ata_port_suspend_async (struct ata_port *ap,pm_message_t mesg) { +/* +* We are about to suspend the port, so we do not care about +* scsi_rescan_device() calls scheduled by previous resume operations. +* The next resume will schedule the rescan again. So cancel any rescan +* that is not done yet. +*/ +cancel_delayed_work_sync (&ap->scsi_rescan_task); +ata_port_request_pm (ap, mesg,0 , ata_port_suspend_ehi, true); }
drivers/ata/libata-scsi.c +15 -18
@@ -4756,7 +4756,7 @@ void ata_scsi_dev_rescan(struct work_struct *work)struct ata_link *link;struct ata_device *dev;unsigned long flags; -bool delay_rescan = false; +int ret =0 ;mutex_lock (&ap->scsi_scan_mutex);spin_lock_irqsave (ap->lock, flags);
@@ -4765,37 +4765,34 @@ void ata_scsi_dev_rescan(struct work_struct *work)ata_for_each_dev (dev, link,ENABLED ) {struct scsi_device *sdev = dev->sdev; +/* +* If the port was suspended before this was scheduled, +* bail out. +*/ +if (ap->pflags &ATA_PFLAG_SUSPENDED ) + goto unlock; +if (!sdev)continue ;if (scsi_device_get (sdev))continue ; -/* -* If the rescan work was scheduled because of a resume -* event, the port is already fully resumed, but the -* SCSI device may not yet be fully resumed. In such -* case, executing scsi_rescan_device() may cause a -* deadlock with the PM code on device_lock(). Prevent -* this by giving up and retrying rescan after a short -* delay. -*/ - delay_rescan = sdev->sdev_gendev.power.is_suspended; -if (delay_rescan) { -scsi_device_put (sdev); -break ; - } -spin_unlock_irqrestore (ap->lock, flags); -scsi_rescan_device (sdev); + ret =scsi_rescan_device (sdev);scsi_device_put (sdev);spin_lock_irqsave (ap->lock, flags); + +if (ret) + goto unlock; } } +unlock:spin_unlock_irqrestore (ap->lock, flags);mutex_unlock (&ap->scsi_scan_mutex); -if (delay_rescan) +/* Reschedule with a delay if scsi_rescan_device() returned an error */ +if (ret)schedule_delayed_work (&ap->scsi_rescan_task,msecs_to_jiffies (5 )); }
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 8/23 ata: libata-core: Do not register PM operations for SAS ports
In reply to Damien Le Moallibsas does its own domain based power management of ports. For such ports, libata should not use a device type defining power management operations as executing these operations for suspend/resume in addition to libsas calls to ata_sas_port_suspend() and ata_sas_port_resume() is not necessary (and likely dangerous to do, even though problems are not seen currently).
Introduce the new ata_port_sas_type device_type for ports managed by libsas. This new device type is used in ata_tport_add() and is defined without power management operations.
--- drivers/ata/libata-core.c | 2 +- drivers/ata/libata-transport.c | 9 ++++++++- drivers/ata/libata.h | 2 ++ 3 files changed, 11 insertions(+), 2 deletions(-)
drivers/ata/libata-core.c +1 -1
@@ -5335,7 +5335,7 @@ EXPORT_SYMBOL_GPL(ata_host_resume);#endif const struct device_type ata_port_type = { - .name ="ata_port" , + .name =ATA_PORT_TYPE_NAME , #ifdef CONFIG_PM .pm = &ata_port_pm_ops, #endif
drivers/ata/libata-transport.c +8 -1
@@ -266,6 +266,10 @@ void ata_tport_delete(struct ata_port *ap)put_device (dev); } +static const struct device_type ata_port_sas_type = { + .name =ATA_PORT_TYPE_NAME , +}; +/** ata_tport_add - initialize a transport ATA port structure * * @parent: parent device
@@ -283,7 +287,10 @@ int ata_tport_add(struct device *parent,struct device *dev = &ap->tdev;device_initialize (dev); - dev->type = &ata_port_type; +if (ap->flags &ATA_FLAG_SAS_HOST ) + dev->type = &ata_port_sas_type; +else + dev->type = &ata_port_type; dev->parent = parent;ata_host_get (ap->host);
drivers/ata/libata.h +2 -0
@@ -30,6 +30,8 @@ enum { ATA_DNXFER_QUIET = (1 << 31), }; +#define ATA_PORT_TYPE_NAME "ata_port" +extern atomic_t ata_print_id;extern int atapi_passthru16;extern int libata_fua;
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 9/23 scsi: sd: Do not issue commands to suspended disks on shutdown
In reply to Damien Le MoalIf an error occurs when resuming a host adapter before the devices attached to the adapter are resumed, the adapter low level driver may remove the scsi host, resulting in a call to sd_remove() for the disks of the host. This in turn results in a call to sd_shutdown() which will issue a synchronize cache command and a start stop unit command to spindown the disk. sd_shutdown() issues the commands only if the device is not already suspended but does not check the power state for system-wide suspend/resume. That is, the commands may be issued with the device in a suspended state, which causes PM resume to hang, forcing a reset of the machine to recover.
Fix this by not calling sd_shutdown() in sd_remove() if the device is not running.
--- drivers/scsi/sd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/scsi/sd.c +2 -1
@@ -3727,7 +3727,8 @@ static int sd_remove(struct device *dev)device_del (&sdkp->disk_dev);del_gendisk (sdkp->disk); -sd_shutdown (dev); +if (sdkp->device->sdev_state ==SDEV_RUNNING ) +sd_shutdown (dev);put_disk (sdkp->disk);return 0 ;
Signature · 1 line
2.41.0
Re: [PATCH v5 09/23] scsi: sd: Do not issue commands to suspended disks on shutdown
Quoted 5 lines · Damien Le Moal
drivers/scsi/sd.c +0
-0
drivers/scsi/sd.c static int sd_remove(struct device *dev) · line 3734
Quoted 9 lines
drivers/scsi/sd.c +2
-1@@ -3727,7 +3727,8 @@ static int sd_remove(struct device *dev)device_del (&sdkp->disk_dev);del_gendisk (sdkp->disk); -sd_shutdown (dev); +if (sdkp->device->sdev_state ==SDEV_RUNNING ) +sd_shutdown (dev);put_disk (sdkp->disk);return 0 ;
Doesn't this patch involve a behavior change? I'm concerned the above patch will always cause the sd_shutdown() call to be skipped if scsi_remove_host() is called, whether or not the scsi_remove_host() call is triggered by a resume failure.
Thanks,
Bart.
Damien Le Moal
maintainerRe: [PATCH v5 09/23] scsi: sd: Do not issue commands to suspended disks on shutdown
Quoted 13 lines · Bart Van Assche
Quoted 12 lines · Damien Le Moal
drivers/scsi/sd.c +2
-1@@ -3727,7 +3727,8 @@ static int sd_remove(struct device *dev)device_del (&sdkp->disk_dev);del_gendisk (sdkp->disk); -sd_shutdown (dev); +if (sdkp->device->sdev_state ==SDEV_RUNNING ) +sd_shutdown (dev);
Quoted 7 lines
drivers/scsi/sd.c static int sd_remove(struct device *dev) · line 3734
Quoted 2 lines
drivers/scsi/sd.c +0
-0put_disk (sdkp->disk);return 0 ;Doesn't this patch involve a behavior change? I'm concerned the above patch will always cause the
sd_shutdown()call to be skipped ifscsi_remove_host()is called, whether or not thescsi_remove_host()call is triggered by a resume failure.
I guess the point is: what are the possible states of the scsi device when sd_remove() is called ? From what I have seen so far, it is RUNNING. But I am not 100% sure this true all the time. But given that sd_shutdown() issues commands, I would guess that it makes sense that it is always running.
Looking at the code, scsi_remove_host() calls scsi_forget_host() which calls __scsi_remove_device() for any device that is not in the SDEV_DEL state. __scsi_remove_device() then sets the state to SDEV_CANCEL. So it appears that the state should always be CANCEL and not running. However, my tests showed it to be running. I am not fully understanding how sd_remove() end up being called...
I will run tests again without this patch for libata suspend/resume. This patch was added because of the hang I saw with the pm80xx driver (using libsas/libata) failing to resume. That resume failure is fixed and libata does not need this patch I think. But I will double check and drop it for now.
I think we should investigate this further though, to make sure that we can always safely call sd_shutdown(). __scsi_remove_device() has this comment:
/*
* If blocked, we go straight to DEL and restart the queue so
* any commands issued during driver shutdown (like sync
* cache) are errored immediately.
*/
Which kind of give a hint that we should probably not blindy always try to call sd_shutdown().
Signature · 2 lines
Damien Le Moal Western Digital Research
Re: [PATCH v5 09/23] scsi: sd: Do not issue commands to suspended disks on shutdown
Quoted 5 lines · Damien Le Moal
Looking at the code,
scsi_remove_host()callsscsi_forget_host()which calls__scsi_remove_device()for any device that is not in theSDEV_DELstate.__scsi_remove_device()then sets the state toSDEV_CANCEL. So it appears that the state should always be CANCEL and not running. However, my tests showed it to be running. I am not fully understanding howsd_remove()end up being called...
I think this is how sd_sync_cache() gets called from inside scsi_remove_host():
scsi_remove_host()
-> scsi_forget_host()
-> __scsi_remove_device()
-> device_del(&sdev->sdev_gendev)
-> bus_remove_device()
-> device_release_driver()
-> __device_release_driver()
-> sd_remove()
-> sd_shutdown()
-> sd_sync_cache()In other words, it is guaranteed that scsi_device_set_state(sdev, SDEV_CANCEL) has been called before sd_remove() if it is called by scsi_remove_host().
Quoted 11 lines
I think we should investigate this further though, to make sure that we can always safely call
sd_shutdown().__scsi_remove_device()has this comment:/*
* If blocked, we go straight to DEL and restart the queue so
* any commands issued during driver shutdown (like sync
* cache) are errored immediately.
*/Which kind of give a hint that we should probably not blindy always try to call
sd_shutdown().
Does that comment perhaps refer to the SDEV_BLOCK / SDEV_CREATED_BLOCK states? Anyway, I'm wondering whether there are better ways to prevent that it is attempted to queue SCSI commands if a SCSI device is suspended, e.g. by checking the suspended state from inside scsi_device_state_check() or scsi_dispatch_cmd().
Thanks,
Bart.
Damien Le Moal
maintainerRe: [PATCH v5 09/23] scsi: sd: Do not issue commands to suspended disks on shutdown
Quoted 34 lines · Bart Van Assche
Quoted 5 lines · Damien Le Moal
Looking at the code,
scsi_remove_host()callsscsi_forget_host()which calls__scsi_remove_device()for any device that is not in theSDEV_DELstate.__scsi_remove_device()then sets the state toSDEV_CANCEL. So it appears that the state should always be CANCEL and not running. However, my tests showed it to be running. I am not fully understanding howsd_remove()end up being called...I think this is how
sd_sync_cache()gets called from insidescsi_remove_host():
scsi_remove_host()
->scsi_forget_host()-> __scsi_remove_device() -> device_del(&sdev->sdev_gendev) -> bus_remove_device() -> device_release_driver() -> __device_release_driver() -> sd_remove() -> sd_shutdown() -> sd_sync_cache()In other words, it is guaranteed that
scsi_device_set_state(sdev, SDEV_CANCEL)has been called beforesd_remove()if it is called byscsi_remove_host().Quoted 9 lines
I think we should investigate this further though, to make sure that we can always safely call
sd_shutdown().__scsi_remove_device()has this comment:/*
* If blocked, we go straight to DEL and restart the queue so
* any commands issued during driver shutdown (like sync
* cache) are errored immediately.
*/
Quoted 8 lines
Quoted 2 lines
Which kind of give a hint that we should probably not blindy always try to call
sd_shutdown().Does that comment perhaps refer to the
SDEV_BLOCK/SDEV_CREATED_BLOCKstates? Anyway, I'm wondering whether there are better ways to prevent that it is attempted to queue SCSI commands if a SCSI device is suspended, e.g. by checking the suspended state from insidescsi_device_state_check()orscsi_dispatch_cmd().
Using information in the device ->power structure is not reliable without holding the device lock(), so we should not do that. But we can add a "suspended" scsi_device flag that we maintain on execution of sd_suspend_system() and sd_resume_system(). Many drivers do that... Thoughts ?
Signature · 2 lines
Damien Le Moal Western Digital Research
Re: [PATCH v5 09/23] scsi: sd: Do not issue commands to suspended disks on shutdown
Quoted 7 lines · Damien Le Moal
Quoted 5 lines · Bart Van Assche
Does that comment perhaps refer to the
SDEV_BLOCK/SDEV_CREATED_BLOCKstates? Anyway, I'm wondering whether there are better ways to prevent that it is attempted to queue SCSI commands if a SCSI device is suspended, e.g. by checking the suspended state from insidescsi_device_state_check()orscsi_dispatch_cmd().
Quoted 5 lines
Using information in the device ->power structure is not reliable without holding the device
lock(), so we should not do that. But we can add a "suspended"scsi_deviceflag that we maintain on execution ofsd_suspend_system()andsd_resume_system(). Many drivers do that... Thoughts ?
That sounds good to me.
Thanks,
Bart.
Damien Le Moal
maintainerPATCH v5 10/23 ata: libata-core: Fix compilation warning in ata_dev_config_ncq()
In reply to Damien Le MoalThe 24 bytes length allocated to the ncq_desc string in ata_dev_config_lba() for ata_dev_config_ncq() to use is too short, causing the following gcc compilation warnings when compiling with W=1:
drivers/ata/libata-core.c: In function ‘ata_dev_configure’:drivers/ata/libata-core.c:2378:56: warning: ‘%d’ directive output may be truncated writing between 1 and 2 bytes into a region of size between 1 and 11 [-Wformat-truncation=]
2378 | snprintf(desc, desc_sz, "NCQ (depth %d/%d)%s", hdepth,
| ^~
In function ‘ata_dev_config_ncq’,
inlined from ‘ata_dev_config_lba’ at drivers/ata/libata-core.c:2649:8,
inlined from ‘ata_dev_configure’ at drivers/ata/libata-core.c:2952:9:drivers/ata/libata-core.c:2378:41: note: directive argument in the range [1, 32]
2378 | snprintf(desc, desc_sz, "NCQ (depth %d/%d)%s", hdepth,
| ^~~~~~~~~~~~~~~~~~~~~
drivers/ata/libata-core.c:2378:17: note: ‘snprintf’ output between 16 and 31 bytes into a destination of size 24
2378 | snprintf(desc, desc_sz, "NCQ (depth %d/%d)%s", hdepth,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2379 | ddepth, aa_desc);
| ~~~~~~~~~~~~~~~~Avoid these warnings and the potential truncation by changing the size of the ncq_desc string to 32 characters.
--- drivers/ata/libata-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ata/libata-core.c +1 -1
@@ -2619,7 +2619,7 @@ static int ata_dev_config_lba(struct ata_device *dev) {const u16 *id = dev->id;const char *lba_desc; -char ncq_desc[24 ]; +char ncq_desc[32 ];int ret; dev->flags |=ATA_DFLAG_LBA ;
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 11/23 ata: libata-eh: Fix compilation warning in ata_eh_link_report()
In reply to Damien Le MoalThe 6 bytes length of the tries_buf string in ata_eh_link_report() is too short and results in a gcc compilation warning with W-!:
drivers/ata/libata-eh.c: In function ‘ata_eh_link_report’:drivers/ata/libata-eh.c:2371:59: warning: ‘%d’ directive output may be truncated writing between 1 and 11 bytes into a region of size 4 [-Wformat-truncation=]
2371 | snprintf(tries_buf, sizeof(tries_buf), " t%d",
| ^~drivers/ata/libata-eh.c:2371:56: note: directive argument in the range [-2147483648, 4]
2371 | snprintf(tries_buf, sizeof(tries_buf), " t%d",
| ^~~~~~drivers/ata/libata-eh.c:2371:17: note: ‘snprintf’ output between 4 and 14 bytes into a destination of size 6
2371 | snprintf(tries_buf, sizeof(tries_buf), " t%d",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2372 | ap->eh_tries);
| ~~~~~~~~~~~~~Avoid this warning by increasing the string size to 16B.
--- drivers/ata/libata-eh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ata/libata-eh.c +1 -1
@@ -2332,7 +2332,7 @@ static void ata_eh_link_report(struct ata_link *link)struct ata_eh_context *ehc = &link->eh_context;struct ata_queued_cmd *qc;const char *frozen, *desc; -char tries_buf[6 ] ="" ; +char tries_buf[16 ] ="" ;int tag, nr_failed =0 ;if (ehc->i.flags &ATA_EHI_QUIET )
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 12/23 scsi: Remove scsi device no_start_on_resume flag
In reply to Damien Le MoalThe scsi device flag no_start_on_resume is not set by any scsi low level driver. Remove it. This reverts the changes introduced by commit 0a8589055936 ("ata,scsi: do not issue START STOP UNIT on resume").
--- drivers/scsi/sd.c | 7 ++----- include/scsi/scsi_device.h | 1 - 2 files changed, 2 insertions(+), 6 deletions(-)
drivers/scsi/sd.c +2 -5
@@ -3886,11 +3886,8 @@ static int sd_resume(struct device *dev, bool runtime)if (!sd_do_start_stop (sdkp->device, runtime))return 0 ; -if (!sdkp->device->no_start_on_resume) { -sd_printk (KERN_NOTICE , sdkp,"Starting disk\n" ); - ret =sd_start_stop_device (sdkp,1 ); - } - +sd_printk (KERN_NOTICE , sdkp,"Starting disk\n" ); + ret =sd_start_stop_device (sdkp,1 );if (!ret)opal_unlock_from_suspend (sdkp->opal_dev);return ret;
include/scsi/scsi_device.h +0 -1
@@ -195,7 +195,6 @@ struct scsi_device { unsigned allow_restart:1; /* issue START_UNIT in error handler */ unsigned manage_system_start_stop:1; /* Let HLD (sd) manage system start/stop */ unsigned manage_runtime_start_stop:1; /* Let HLD (sd) manage runtime start/stop */ - unsigned no_start_on_resume:1; /* Do not issue START_STOP_UNIT on resume */ unsigned start_stop_pwr_cond:1; /* Set power cond. in START_STOP_UNIT */ unsigned no_uld_attach:1; /* disable connecting to upper level drivers */ unsigned select_no_atn:1;
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 13/23 ata: libata-scsi: Cleanup ata_scsi_start_stop_xlat()
In reply to Damien Le MoalNow that libata does its own internal device power mode management through libata EH, the scsi disk driver will not issue START STOP UNIT commands anymore. We can receive this command only from user passthrough operations. So there is no need to consider the system state and ATA port flags for suspend to translate the command.
Since setting up the taskfile for the verify and standby immediate commands is the same as done in ata_dev_power_set_active() and ata_dev_power_set_standby(), factor out this code into the helper function ata_dev_power_init_tf() to simplify ata_scsi_start_stop_xlat() as well as ata_dev_power_set_active() and ata_dev_power_set_standby().
--- drivers/ata/libata-core.c | 55 +++++++++++++++++++++++---------------- drivers/ata/libata-scsi.c | 53 +++++++------------------------------ drivers/ata/libata.h | 2 ++ 3 files changed, 44 insertions(+), 66 deletions(-)
drivers/ata/libata-core.c +33 -22
@@ -1972,6 +1972,35 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,return rc; } +bool ata_dev_power_init_tf (struct ata_device *dev,struct ata_taskfile *tf, +bool set_active) +{ +/* Only applies to ATA and ZAC devices */ +if (dev->class !=ATA_DEV_ATA && dev->class !=ATA_DEV_ZAC ) +return false; + +ata_tf_init (dev, tf); + tf->flags |=ATA_TFLAG_DEVICE |ATA_TFLAG_ISADDR ; + tf->protocol =ATA_PROT_NODATA ; + +if (set_active) { +/* VERIFY for 1 sector at lba=0 */ + tf->command =ATA_CMD_VERIFY ; + tf->nsect =1 ; +if (dev->flags &ATA_DFLAG_LBA ) { + tf->flags |=ATA_TFLAG_LBA ; + tf->device |=ATA_LBA ; + }else { +/* CHS */ + tf->lbal =0x1 ;/* sect */ + } + }else { + tf->command =ATA_CMD_STANDBYNOW1 ; + } + +return true; +} +/** * ata_dev_power_set_standby - Set a device power mode to standby * @dev: target device
@@ -1988,10 +2017,6 @@ void ata_dev_power_set_standby(struct ata_device *dev)struct ata_taskfile tf;unsigned int err_mask; -/* Issue STANDBY IMMEDIATE command only if supported by the device */ -if (dev->class !=ATA_DEV_ATA && dev->class !=ATA_DEV_ZAC ) -return ; -/* * Some odd clown BIOSes issue spindown on power off (ACPI S4 or S5) * causing some drives to spin up and down again. For these, do nothing
@@ -2005,10 +2030,9 @@ void ata_dev_power_set_standby(struct ata_device *dev) system_entering_hibernation())return ; -ata_tf_init (dev, &tf); - tf.flags |=ATA_TFLAG_DEVICE |ATA_TFLAG_ISADDR ; - tf.protocol =ATA_PROT_NODATA ; - tf.command =ATA_CMD_STANDBYNOW1 ; +/* Issue STANDBY IMMEDIATE command only if supported by the device */ +if (!ata_dev_power_init_tf (dev, &tf, false)) +return ;ata_dev_notice (dev,"Entering standby power mode\n" );
@@ -2038,22 +2062,9 @@ void ata_dev_power_set_active(struct ata_device *dev)* Issue READ VERIFY SECTORS command for 1 sector at lba=0 only * if supported by the device. */ -if (dev->class !=ATA_DEV_ATA && dev->class !=ATA_DEV_ZAC ) +if (!ata_dev_power_init_tf (dev, &tf, true))return ; -ata_tf_init (dev, &tf); - tf.flags |=ATA_TFLAG_DEVICE |ATA_TFLAG_ISADDR ; - tf.protocol =ATA_PROT_NODATA ; - tf.command =ATA_CMD_VERIFY ; - tf.nsect =1 ; -if (dev->flags &ATA_DFLAG_LBA ) { - tf.flags |=ATA_TFLAG_LBA ; - tf.device |=ATA_LBA ; - }else { -/* CHS */ - tf.lbal =0x1 ;/* sect */ - } -ata_dev_notice (dev,"Entering active power mode\n" ); err_mask =ata_exec_internal (dev, &tf,NULL ,DMA_NONE ,NULL ,0 ,0 );
drivers/ata/libata-scsi.c +9 -44
82 lines not on this page · raw message
drivers/ata/libata.h +2 -0
@@ -62,6 +62,8 @@ extern int ata_dev_reread_id(struct ata_device *dev, unsigned int readid_flags);extern int ata_dev_revalidate (struct ata_device *dev,unsigned int new_class,unsigned int readid_flags);extern int ata_dev_configure (struct ata_device *dev); +extern bool ata_dev_power_init_tf (struct ata_device *dev, +struct ata_taskfile *tf,bool set_active);extern void ata_dev_power_set_standby (struct ata_device *dev);extern void ata_dev_power_set_active (struct ata_device *dev);extern int sata_down_spd_limit (struct ata_link *link,u32 spd_limit);
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 14/23 ata: libata-core: Synchronize ata_port_detach() with hotplug
In reply to Damien Le MoalThe call to async_synchronize_cookie() to synchronize a port removal and hotplug probe is done in ata_host_detach() right before calling ata_port_detach(). Move this call at the beginning of ata_port_detach() to ensure that this operation is always synchronized with probe.
--- drivers/ata/libata-core.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/ata/libata-core.c +4 -4
@@ -6065,6 +6065,9 @@ static void ata_port_detach(struct ata_port *ap)struct ata_link *link;struct ata_device *dev; +/* Ensure ata_port probe has completed */ +async_synchronize_cookie (ap->cookie +1 ); +/* Wait for any ongoing EH */ ata_port_wait_eh (ap);
@@ -6129,11 +6132,8 @@ void ata_host_detach(struct ata_host *host) {int i; -for (i =0 ; i < host->n_ports; i++) { -/* Ensure ata_port probe has completed */ -async_synchronize_cookie (host->ports[i]->cookie +1 ); +for (i =0 ; i < host->n_ports; i++)ata_port_detach (host->ports[i]); - }/* the host is dead now, dissociate ACPI */ ata_acpi_dissociate (host);
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 15/23 ata: libata-core: Detach a port devices on shutdown
In reply to Damien Le MoalModify ata_pci_shutdown_one() to schedule EH to unload a port devices before freezing and thawing the port. This ensures that drives are cleanly disabled and transitioned to standby power mode when a PCI adapter is removed or the system is powered off.
--- drivers/ata/libata-core.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/ata/libata-core.c +15 -1
@@ -6164,10 +6164,24 @@ EXPORT_SYMBOL_GPL(ata_pci_remove_one);void ata_pci_shutdown_one (struct pci_dev *pdev) {struct ata_host *host =pci_get_drvdata (pdev); +struct ata_port *ap; +unsigned long flags;int i; +/* Tell EH to disable all devices */ for (i =0 ; i < host->n_ports; i++) { -struct ata_port *ap = host->ports[i]; + ap = host->ports[i]; +spin_lock_irqsave (ap->lock, flags); + ap->pflags |=ATA_PFLAG_UNLOADING ; +ata_port_schedule_eh (ap); +spin_unlock_irqrestore (ap->lock, flags); + } + +for (i =0 ; i < host->n_ports; i++) { + ap = host->ports[i]; + +/* Wait for EH to complete before freezing the port */ +ata_port_wait_eh (ap); ap->pflags |=ATA_PFLAG_FROZEN ;
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 16/23 ata: libata-core: Remove ata_port_suspend_async()
In reply to Damien Le Moalata_port_suspend_async() is only called by ata_sas_port_suspend(). Modify ata_port_suspend() with an additional bool argument indicating an asynchronous or synchronous suspend to allow removing that helper function. With this change, the variable ata_port_resume_ehi can also be removed and its value (ATA_EHI_XXX flags passed directly to ata_port_request_pm().
--- drivers/ata/libata-core.c | 46 +++++++++++++++------------------------ 1 file changed, 17 insertions(+), 29 deletions(-)
drivers/ata/libata-core.c +17 -29
@@ -5166,18 +5166,8 @@ static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,ata_port_wait_eh (ap); } -/* -* On some hardware, device fails to respond after spun down for suspend. As -* the device won't be used before being resumed, we don't need to touch the -* device. Ask EH to skip the usual stuff and proceed directly to suspend. -* -* http://thread.gmane.org/gmane.linux.ide/46764 -*/ -static const unsigned int ata_port_suspend_ehi =ATA_EHI_QUIET - |ATA_EHI_NO_AUTOPSY - |ATA_EHI_NO_RECOVERY ; - -static void ata_port_suspend (struct ata_port *ap,pm_message_t mesg) +static void ata_port_suspend (struct ata_port *ap,pm_message_t mesg, +bool async) {/* * We are about to suspend the port, so we do not care about
@@ -5187,20 +5177,18 @@ static void ata_port_suspend(struct ata_port *ap, pm_message_t mesg)*/ cancel_delayed_work_sync (&ap->scsi_rescan_task); -ata_port_request_pm (ap, mesg,0 , ata_port_suspend_ehi, false); -} - -static void ata_port_suspend_async (struct ata_port *ap,pm_message_t mesg) -{/* -* We are about to suspend the port, so we do not care about -* scsi_rescan_device() calls scheduled by previous resume operations. -* The next resume will schedule the rescan again. So cancel any rescan -* that is not done yet. +* On some hardware, device fails to respond after spun down for +* suspend. As the device wil not t be used until being resumed, we +* do not need to touch the device. Ask EH to skip the usual stuff +* and proceed directly to suspend. +* +* http://thread.gmane.org/gmane.linux.ide/46764 */ -cancel_delayed_work_sync (&ap->scsi_rescan_task); - -ata_port_request_pm (ap, mesg,0 , ata_port_suspend_ehi, true); +ata_port_request_pm (ap, mesg,0 , +ATA_EHI_QUIET |ATA_EHI_NO_AUTOPSY | +ATA_EHI_NO_RECOVERY , + async); }static int ata_port_pm_suspend (struct device *dev)
@@ -5210,7 +5198,7 @@ static int ata_port_pm_suspend(struct device *dev)if (pm_runtime_suspended (dev))return 0 ; -ata_port_suspend (ap,PMSG_SUSPEND ); +ata_port_suspend (ap,PMSG_SUSPEND , false);return 0 ; }
@@ -5221,13 +5209,13 @@ static int ata_port_pm_freeze(struct device *dev)if (pm_runtime_suspended (dev))return 0 ; -ata_port_suspend (ap,PMSG_FREEZE ); +ata_port_suspend (ap,PMSG_FREEZE , false);return 0 ; }static int ata_port_pm_poweroff (struct device *dev) { -ata_port_suspend (to_ata_port (dev),PMSG_HIBERNATE ); +ata_port_suspend (to_ata_port (dev),PMSG_HIBERNATE , false);return 0 ; }
@@ -5279,7 +5267,7 @@ static int ata_port_runtime_idle(struct device *dev)static int ata_port_runtime_suspend (struct device *dev) { -ata_port_suspend (to_ata_port (dev),PMSG_AUTO_SUSPEND ); +ata_port_suspend (to_ata_port (dev),PMSG_AUTO_SUSPEND , false);return 0 ; }
@@ -5309,7 +5297,7 @@ static const struct dev_pm_ops ata_port_pm_ops = {*/ void ata_sas_port_suspend (struct ata_port *ap) { -ata_port_suspend_async (ap,PMSG_SUSPEND ); +ata_port_suspend (ap,PMSG_SUSPEND , true); }EXPORT_SYMBOL_GPL (ata_sas_port_suspend);
Signature · 1 line
2.41.0
Re: [PATCH v5 16/23] ata: libata-core: Remove ata_port_suspend_async()
Quoted 19 lines · Damien Le Moal
ata_port_suspend_async()is only called byata_sas_port_suspend(). Modifyata_port_suspend()with an additional bool argument indicating an asynchronous or synchronous suspend to allow removing that helper function. With this change, the variableata_port_resume_ehican also be removed and its value (ATA_EHI_XXXflags passed directly toata_port_request_pm().Signed-off-by Damien Le Moal <[email protected]>Reviewed-by Hannes Reinecke <[email protected]>Tested-by Geert Uytterhoeven <[email protected]>--- drivers/ata/libata-core.c | 46 +++++++++++++++------------------------ 1 file changed, 17 insertions(+), 29 deletions(-)drivers/ata/libata-core.c +0
-0
[...]
Quoted 6 lines
drivers/ata/libata-core.c +0
-2@@ -5187,20 +5177,18 @@ static void ata_port_suspend(struct ata_port *ap, pm_message_t mesg)*/ cancel_delayed_work_sync (&ap->scsi_rescan_task); -ata_port_request_pm (ap, mesg,0 , ata_port_suspend_ehi, false); -}
drivers/ata/libata-core.c static void ata_port_suspend(struct ata_port *ap, pm_message_t mesg) · line 5182
Quoted 10 lines
drivers/ata/libata-core.c +2
-7- -static void ata_port_suspend_async (struct ata_port *ap,pm_message_t mesg) -{/* -* We are about to suspend the port, so we do not care about -* scsi_rescan_device() calls scheduled by previous resume operations. -* The next resume will schedule the rescan again. So cancel any rescan -* that is not done yet. +* On some hardware, device fails to respond after spun down for +* suspend. As the device wil not t be used until being resumed, we
Will not be used?
There were no typos in the original comment... :-)
[...]
MBR, Sergey
Damien Le Moal
maintainerPATCH v5 17/23 ata: libata-core: Remove ata_port_resume_async()
In reply to Damien Le MoalRemove ata_port_resume_async() and replace it with a modified ata_port_resume() taking an additional bool argument indicating if ata EH resume operation should be executed synchronously or asynchronously. With this change, the variable ata_port_resume_ehi is not longer necessary and its value (ATA_EHI_XXX flags) passed directly to ata_port_request_pm().
--- drivers/ata/libata-core.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)
drivers/ata/libata-core.c +8 -13
@@ -5219,22 +5219,17 @@ static int ata_port_pm_poweroff(struct device *dev)return 0 ; } -static const unsigned int ata_port_resume_ehi =ATA_EHI_NO_AUTOPSY - |ATA_EHI_QUIET ; - -static void ata_port_resume (struct ata_port *ap,pm_message_t mesg) +static void ata_port_resume (struct ata_port *ap,pm_message_t mesg, +bool async) { -ata_port_request_pm (ap, mesg,ATA_EH_RESET , ata_port_resume_ehi, false); -} - -static void ata_port_resume_async (struct ata_port *ap,pm_message_t mesg) -{ -ata_port_request_pm (ap, mesg,ATA_EH_RESET , ata_port_resume_ehi, true); +ata_port_request_pm (ap, mesg,ATA_EH_RESET , +ATA_EHI_NO_AUTOPSY |ATA_EHI_QUIET , + async); }static int ata_port_pm_resume (struct device *dev) { -ata_port_resume_async (to_ata_port (dev),PMSG_RESUME ); +ata_port_resume (to_ata_port (dev),PMSG_RESUME , true);pm_runtime_disable (dev);pm_runtime_set_active (dev);pm_runtime_enable (dev);
@@ -5273,7 +5268,7 @@ static int ata_port_runtime_suspend(struct device *dev)static int ata_port_runtime_resume (struct device *dev) { -ata_port_resume (to_ata_port (dev),PMSG_AUTO_RESUME ); +ata_port_resume (to_ata_port (dev),PMSG_AUTO_RESUME , false);return 0 ; }
@@ -5303,7 +5298,7 @@ EXPORT_SYMBOL_GPL(ata_sas_port_suspend);void ata_sas_port_resume (struct ata_port *ap) { -ata_port_resume_async (ap,PMSG_RESUME ); +ata_port_resume (ap,PMSG_RESUME , true); }EXPORT_SYMBOL_GPL (ata_sas_port_resume);
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 18/23 ata: libata-core: Do not poweroff runtime suspended ports
In reply to Damien Le MoalWhen powering off, there is no need to suspend a port that has already been runtime suspended. Skip the EH PM request in ata_port_pm_poweroff() in this case.
--- drivers/ata/libata-core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/ata/libata-core.c +2 -1
@@ -5215,7 +5215,8 @@ static int ata_port_pm_freeze(struct device *dev)static int ata_port_pm_poweroff (struct device *dev) { -ata_port_suspend (to_ata_port (dev),PMSG_HIBERNATE , false); +if (!pm_runtime_suspended (dev)) +ata_port_suspend (to_ata_port (dev),PMSG_HIBERNATE , false);return 0 ; }
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 19/23 ata: libata-core: Do not resume runtime suspended ports
In reply to Damien Le MoalThe scsi disk driver does not resume disks that have been runtime suspended by the user. To be consistent with this behavior, do the same for ata ports and skip the PM request in ata_port_pm_resume() if the port was already runtime suspended. With this change, it is no longer necessary to force the PM state of the port to ACTIVE as the PM core code will take care of that when handling runtime resume.
--- drivers/ata/libata-core.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/ata/libata-core.c +2 -4
@@ -5230,10 +5230,8 @@ static void ata_port_resume(struct ata_port *ap, pm_message_t mesg,static int ata_port_pm_resume (struct device *dev) { -ata_port_resume (to_ata_port (dev),PMSG_RESUME , true); -pm_runtime_disable (dev); -pm_runtime_set_active (dev); -pm_runtime_enable (dev); +if (!pm_runtime_suspended (dev)) +ata_port_resume (to_ata_port (dev),PMSG_RESUME , true);return 0 ; }
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 20/23 ata: libata-sata: Improve ata_sas_slave_configure()
In reply to Damien Le MoalChange ata_sas_slave_configure() to return the return value of ata_scsi_dev_config() to ensure that any error from that function is propagated to libsas.
--- drivers/ata/libata-sata.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/ata/libata-sata.c +2 -2
@@ -1182,8 +1182,8 @@ EXPORT_SYMBOL_GPL(ata_sas_tport_delete);int ata_sas_slave_configure (struct scsi_device *sdev,struct ata_port *ap) {ata_scsi_sdev_config (sdev); -ata_scsi_dev_config (sdev, ap->link.device); -return 0 ; + +return ata_scsi_dev_config (sdev, ap->link.device); }EXPORT_SYMBOL_GPL (ata_sas_slave_configure);
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 21/23 ata: libata-eh: Improve reset error messages
In reply to Damien Le MoalSome drives are really slow to spinup on resume, resulting is a very slow response to COMRESET and to error messages such as:
ata1: COMRESET failed (errno=-16)
ata1: link is slow to respond, please be patient (ready=0)
ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300) ata1.00: configured for UDMA/133
Given that the slowness of the response is indicated with the message "link is slow to respond..." and that resets are retried until the device is detected as online after up to 1min (ata_eh_reset_timeouts), there is no point in printing the "COMRESET failed" error message. Let's not scare the user with non fatal errors and only warn about reset failures in ata_eh_reset() when all reset retries have been exhausted.
--- drivers/ata/libata-eh.c | 2 ++ drivers/ata/libata-sata.c | 1 - 2 files changed, 2 insertions(+), 1 deletion(-)
drivers/ata/libata-eh.c +2 -0
@@ -2909,6 +2909,8 @@ int ata_eh_reset(struct ata_link *link, int classify,*/ if (ata_is_host_link (link))ata_eh_thaw_port (ap); +ata_link_warn (link,"%s failed\n" , + reset == hardreset ?"hardreset" :"softreset" ); goto out; }
drivers/ata/libata-sata.c +0 -1
@@ -621,7 +621,6 @@ int sata_link_hardreset(struct ata_link *link, const unsigned int *timing,/* online is set iff link is online && reset succeeded */ if (online) *online = false; -ata_link_err (link,"COMRESET failed (errno=%d)\n" , rc); }return rc; }
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 22/23 ata: libata-eh: Reduce "disable device" message verbosity
In reply to Damien Le MoalThere is no point in warning about a device being disabled when we expect it to be, that is, on suspend, shutdown or when detaching the device.
Suppress the message "disable device" for these cases by introducing the EH static function ata_eh_dev_disable() and by using it in ata_eh_unload() and ata_eh_detach_dev(). ata_dev_disable() code is modified to call this new function after printing the "disable device" message.
--- drivers/ata/libata-eh.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)
drivers/ata/libata-eh.c +19 -13
@@ -494,6 +494,18 @@ void ata_eh_release(struct ata_port *ap)mutex_unlock (&ap->host->eh_mutex); } +static void ata_eh_dev_disable (struct ata_device *dev) +{ +ata_acpi_on_disable (dev); +ata_down_xfermask_limit (dev,ATA_DNXFER_FORCE_PIO0 |ATA_DNXFER_QUIET ); + dev->class++; + +/* From now till the next successful probe, ering is used to +* track probe failures. Clear accumulated device error info. +*/ +ata_ering_clear (&dev->ering); +} +static void ata_eh_unload (struct ata_port *ap) {struct ata_link *link;
@@ -517,8 +529,8 @@ static void ata_eh_unload(struct ata_port *ap)*/ ata_for_each_link (link, ap,PMP_FIRST ) {sata_scr_write (link,SCR_CONTROL , link->saved_scontrol &0xff0 ); -ata_for_each_dev (dev, link,ALL ) -ata_dev_disable (dev); +ata_for_each_dev (dev, link,ENABLED ) +ata_eh_dev_disable (dev); }/* freeze and set UNLOADED */
@@ -1211,14 +1223,8 @@ void ata_dev_disable(struct ata_device *dev)return ;ata_dev_warn (dev,"disable device\n" ); -ata_acpi_on_disable (dev); -ata_down_xfermask_limit (dev,ATA_DNXFER_FORCE_PIO0 |ATA_DNXFER_QUIET ); - dev->class++; -/* From now till the next successful probe, ering is used to -* track probe failures. Clear accumulated device error info. -*/ -ata_ering_clear (&dev->ering); +ata_eh_dev_disable (dev); }EXPORT_SYMBOL_GPL (ata_dev_disable);
@@ -1240,12 +1246,12 @@ void ata_eh_detach_dev(struct ata_device *dev)/* * If the device is still enabled, transition it to standby power mode -* (i.e. spin down HDDs). +* (i.e. spin down HDDs) and disable it. */ -if (ata_dev_enabled (dev)) +if (ata_dev_enabled (dev)) {ata_dev_power_set_standby (dev); - -ata_dev_disable (dev); +ata_eh_dev_disable (dev); + }spin_lock_irqsave (ap->lock, flags);
Signature · 1 line
2.41.0
Damien Le Moal
maintainerPATCH v5 23/23 ata: libata: Cleanup inline DMA helper functions
In reply to Damien Le MoalSimplify the inline DMA helper functions ata_using_mwdma(), ata_using_udma() and ata_dma_enabled() to directly return as a boolean the result of their test condition.
--- include/linux/libata.h | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)
include/linux/libata.h +8 -10
@@ -1881,23 +1881,21 @@ static inline unsigned long ata_deadline(unsigned long from_jiffies,change in future hardware and specs, secondly 0xFF means 'no DMA' but is > UDMA_0. Dyma ddreigiau */ -static inline int ata_using_mwdma (struct ata_device *adev) +static inline bool ata_using_mwdma (struct ata_device *adev) { -if (adev->dma_mode >=XFER_MW_DMA_0 && adev->dma_mode <=XFER_MW_DMA_4 ) -return 1 ; -return 0 ; +return adev->dma_mode >=XFER_MW_DMA_0 && + adev->dma_mode <=XFER_MW_DMA_4 ; } -static inline int ata_using_udma (struct ata_device *adev) +static inline bool ata_using_udma (struct ata_device *adev) { -if (adev->dma_mode >=XFER_UDMA_0 && adev->dma_mode <=XFER_UDMA_7 ) -return 1 ; -return 0 ; +return adev->dma_mode >=XFER_UDMA_0 && + adev->dma_mode <=XFER_UDMA_7 ; } -static inline int ata_dma_enabled (struct ata_device *adev) +static inline bool ata_dma_enabled (struct ata_device *adev) { -return (adev->dma_mode ==0xFF ?0 :1 ); +return adev->dma_mode !=0xFF ; }/**************************************************************************
Signature · 1 line
2.41.0
Damien,
Quoted 8 lines
The first 9 patches of this series fix several issues with suspend/resume power management operations in scsi and libata. The most significant changes introduced are in patch 4 and 5, where the
manage_start_stopflag of scsi devices is split into themanage_system_start_stopandmanage_runtime_start_stopflags to allow keeping scsi runtime power operations for spining up/down ATA devices but have libata do its own system suspend/resume device power state management using EH.
Cosmetic nits aside, no objections from me. Glad to finally see the ATA-SCSI binding established!
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering