scsi: sd: Fix TCG OPAL unlock on system resume
Damien Le Moal
maintainerPATCH 1/1 scsi: sd: Fix TCG OPAL unlock on system resume
Commit 3cc2ffe5c16d introduced the manage_system_start_stop scsi device flag to allow libata to indicate to the scsi disk driver that nothing should be done when resuming a disk on system resume. This change turned the execution of sd_resume() into a no-op for ATA devices on system resume. While this solved deadlock issues during device resume, this change also wrongly removed the execution of opal_unlock_from_suspend(). As a result, devices with TCG OPAL locking enabled remain locked and inaccessible after a system resume from sleep.
To fix this issue, introduce the scsi driver resume method and implement it with the sd_resume() function calling opal_unlock_from_suspend(). The former sd_resume() function is renamed to sd_resume_common() and modified to call the new sd_resume() function. For non-ata devices, this result in no functional changes.
Inorder for libata to explicitly execute sd_resume() when a device is resumed during system re-start, the function scsi_resume_device() is introduced. libata calls this function from the revalidation work executed on devie resume, a state that is indicated with the new device flag ATA_DFLAG_RESUMING. Doing so, locked TCG OPAL enabled devices are unlocked on resume, allowing normal operation.
--- drivers/ata/libata-eh.c | 5 ++++- drivers/ata/libata-scsi.c | 9 +++++++++ drivers/scsi/scsi_scan.c | 34 ++++++++++++++++++++++++++++++++++ drivers/scsi/sd.c | 23 +++++++++++++++++++---- include/linux/libata.h | 1 + include/scsi/scsi_driver.h | 1 + include/scsi/scsi_host.h | 1 + 7 files changed, 69 insertions(+), 5 deletions(-)
drivers/ata/libata-eh.c +4 -1
@@ -712,8 +712,10 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap) ehc->saved_ncq_enabled |=1 << devno;/* If we are resuming, wake up the device */ -if (ap->pflags &ATA_PFLAG_RESUMING ) +if (ap->pflags &ATA_PFLAG_RESUMING ) { + dev->flags |=ATA_DFLAG_RESUMING ; ehc->i.dev_action[devno] |=ATA_EH_SET_ACTIVE ; + } } }
@@ -3169,6 +3171,7 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,return 0 ; err: + dev->flags &= ~ATA_DFLAG_RESUMING ; *r_failed_dev = dev;return rc; }
drivers/ata/libata-scsi.c +9 -0
@@ -4730,6 +4730,7 @@ void ata_scsi_dev_rescan(struct work_struct *work)struct ata_link *link;struct ata_device *dev;unsigned long flags; +bool do_resume;int ret =0 ;mutex_lock (&ap->scsi_scan_mutex);
@@ -4751,7 +4752,15 @@ void ata_scsi_dev_rescan(struct work_struct *work)if (scsi_device_get (sdev))continue ; + do_resume = dev->flags &ATA_DFLAG_RESUMING ; +spin_unlock_irqrestore (ap->lock, flags); +if (do_resume) { + ret =scsi_resume_device (sdev); +if (ret == -EWOULDBLOCK ) + goto unlock; + dev->flags &= ~ATA_DFLAG_RESUMING ; + } ret =scsi_rescan_device (sdev);scsi_device_put (sdev);spin_lock_irqsave (ap->lock, flags);
drivers/scsi/scsi_scan.c +34 -0
@@ -1642,6 +1642,40 @@ int scsi_add_device(struct Scsi_Host *host, uint channel, }EXPORT_SYMBOL (scsi_add_device); +int scsi_resume_device (struct scsi_device *sdev) +{ +struct device *dev = &sdev->sdev_gendev; +int ret =0 ; + +device_lock (dev); + +/* +* Bail out if the device or its queue are 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 || +blk_queue_pm_only (sdev->request_queue)) { + ret = -EWOULDBLOCK ; + goto unlock; + } + +if (dev->driver &&try_module_get (dev->driver->owner)) { +struct scsi_driver *drv =to_scsi_driver (dev->driver); + +if (drv->resume) + ret = drv->resume (dev); +module_put (dev->driver->owner); + } + +unlock: +device_unlock (dev); + +return ret; +} +EXPORT_SYMBOL (scsi_resume_device); +int scsi_rescan_device (struct scsi_device *sdev) {struct device *dev = &sdev->sdev_gendev;
drivers/scsi/sd.c +19 -4
@@ -4003,7 +4003,21 @@ static int sd_suspend_runtime(struct device *dev)return sd_suspend_common (dev, true); } -static int sd_resume (struct device *dev,bool runtime) +static int sd_resume (struct device *dev) +{ +struct scsi_disk *sdkp =dev_get_drvdata (dev); + +sd_printk (KERN_NOTICE , sdkp,"Starting disk\n" ); + +if (opal_unlock_from_suspend (sdkp->opal_dev)) { +sd_printk (KERN_NOTICE , sdkp,"OPAL unlock failed\n" ); +return -EIO ; + } + +return 0 ; +} + +static int sd_resume_common (struct device *dev,bool runtime) {struct scsi_disk *sdkp =dev_get_drvdata (dev);int ret;
@@ -4019,7 +4033,7 @@ static int sd_resume(struct device *dev, bool runtime)sd_printk (KERN_NOTICE , sdkp,"Starting disk\n" ); ret =sd_start_stop_device (sdkp,1 );if (!ret) { -opal_unlock_from_suspend (sdkp->opal_dev); +sd_resume (dev); sdkp->suspended = false; }
@@ -4038,7 +4052,7 @@ static int sd_resume_system(struct device *dev)return 0 ; } -return sd_resume (dev, false); +return sd_resume_common (dev, false); }static int sd_resume_runtime (struct device *dev)
@@ -4065,7 +4079,7 @@ static int sd_resume_runtime(struct device *dev) "Failed to clear sense data\n"); } -return sd_resume (dev, true); +return sd_resume_common (dev, true); }static const struct dev_pm_ops sd_pm_ops = {
@@ -4088,6 +4102,7 @@ static struct scsi_driver sd_template = { .pm = &sd_pm_ops, }, .rescan = sd_rescan, + .resume = sd_resume, .init_command = sd_init_command, .uninit_command = sd_uninit_command, .done = sd_done,
include/linux/libata.h +1 -0
@@ -107,6 +107,7 @@ enum {ATA_DFLAG_NCQ_PRIO_ENABLED = (1 <<20 ),/* Priority cmds sent to dev */ ATA_DFLAG_CDL_ENABLED = (1 <<21 ),/* cmd duration limits is enabled */ +ATA_DFLAG_RESUMING = (1 <<22 ),/* Device is resuming */ ATA_DFLAG_DETACH = (1 <<24 ),ATA_DFLAG_DETACHED = (1 <<25 ), ATA_DFLAG_DA = (1 << 26), /* device supports Device Attention */
include/scsi/scsi_driver.h +1 -0
@@ -12,6 +12,7 @@ struct request;struct scsi_driver {struct device_driver gendrv; +int (*resume)(struct device *);void (*rescan)(struct device *);blk_status_t (*init_command)(struct scsi_cmnd *);void (*uninit_command)(struct scsi_cmnd *);
include/scsi/scsi_host.h +1 -0
@@ -767,6 +767,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 int scsi_resume_device (struct scsi_device *sdev);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 *);
Signature · 1 line
2.44.0
Damien Le Moal
maintainerQuoted 22 lines · Damien Le Moal
Commit
3cc2ffe5c16dintroduced themanage_system_start_stopscsi device flag to allow libata to indicate to the scsi disk driver that nothing should be done when resuming a disk on system resume. This change turned the execution ofsd_resume()into a no-op for ATA devices on system resume. While this solved deadlock issues during device resume, this change also wrongly removed the execution ofopal_unlock_from_suspend(). As a result, devices with TCG OPAL locking enabled remain locked and inaccessible after a system resume from sleep.To fix this issue, introduce the scsi driver resume method and implement it with the
sd_resume()function callingopal_unlock_from_suspend(). The formersd_resume()function is renamed tosd_resume_common()and modified to call the newsd_resume()function. For non-ata devices, this result in no functional changes.Inorder for libata to explicitly execute
sd_resume()when a device is resumed during system re-start, the functionscsi_resume_device()is introduced. libata calls this function from the revalidation work executed on devie resume, a state that is indicated with the new device flagATA_DFLAG_RESUMING. Doing so, locked TCG OPAL enabled devices are unlocked on resume, allowing normal operation.
Quoted 4 lines
Fixes 3cc2ffe5c16d ("scsi: sd: Differentiate system and runtime start/stop management")Signed-off-by Damien Le Moal <[email protected]>
Martin,
Ping ?
Quoted 205 lines
--- drivers/ata/libata-eh.c | 5 ++++- drivers/ata/libata-scsi.c | 9 +++++++++ drivers/scsi/scsi_scan.c | 34 ++++++++++++++++++++++++++++++++++ drivers/scsi/sd.c | 23 +++++++++++++++++++---- include/linux/libata.h | 1 + include/scsi/scsi_driver.h | 1 + include/scsi/scsi_host.h | 1 + 7 files changed, 69 insertions(+), 5 deletions(-)drivers/ata/libata-eh.c +4
-1@@ -712,8 +712,10 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap) ehc->saved_ncq_enabled |=1 << devno;/* If we are resuming, wake up the device */ -if (ap->pflags &ATA_PFLAG_RESUMING ) +if (ap->pflags &ATA_PFLAG_RESUMING ) { + dev->flags |=ATA_DFLAG_RESUMING ; ehc->i.dev_action[devno] |=ATA_EH_SET_ACTIVE ; + } } }@@ -3169,6 +3171,7 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,return 0 ; err: + dev->flags &= ~ATA_DFLAG_RESUMING ; *r_failed_dev = dev;return rc; }drivers/ata/libata-scsi.c +9
-0@@ -4730,6 +4730,7 @@ void ata_scsi_dev_rescan(struct work_struct *work)struct ata_link *link;struct ata_device *dev;unsigned long flags; +bool do_resume;int ret =0 ;mutex_lock (&ap->scsi_scan_mutex);@@ -4751,7 +4752,15 @@ void ata_scsi_dev_rescan(struct work_struct *work)if (scsi_device_get (sdev))continue ; + do_resume = dev->flags &ATA_DFLAG_RESUMING ; +spin_unlock_irqrestore (ap->lock, flags); +if (do_resume) { + ret =scsi_resume_device (sdev); +if (ret == -EWOULDBLOCK ) + goto unlock; + dev->flags &= ~ATA_DFLAG_RESUMING ; + } ret =scsi_rescan_device (sdev);scsi_device_put (sdev);spin_lock_irqsave (ap->lock, flags);drivers/scsi/scsi_scan.c +34
-0@@ -1642,6 +1642,40 @@ int scsi_add_device(struct Scsi_Host *host, uint channel, }EXPORT_SYMBOL (scsi_add_device); +int scsi_resume_device (struct scsi_device *sdev) +{ +struct device *dev = &sdev->sdev_gendev; +int ret =0 ; + +device_lock (dev); + +/* +* Bail out if the device or its queue are 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 || +blk_queue_pm_only (sdev->request_queue)) { + ret = -EWOULDBLOCK ; + goto unlock; + } + +if (dev->driver &&try_module_get (dev->driver->owner)) { +struct scsi_driver *drv =to_scsi_driver (dev->driver); + +if (drv->resume) + ret = drv->resume (dev); +module_put (dev->driver->owner); + } + +unlock: +device_unlock (dev); + +return ret; +} +EXPORT_SYMBOL (scsi_resume_device); +int scsi_rescan_device (struct scsi_device *sdev) {struct device *dev = &sdev->sdev_gendev;drivers/scsi/sd.c +19
-4@@ -4003,7 +4003,21 @@ static int sd_suspend_runtime(struct device *dev)return sd_suspend_common (dev, true); } -static int sd_resume (struct device *dev,bool runtime) +static int sd_resume (struct device *dev) +{ +struct scsi_disk *sdkp =dev_get_drvdata (dev); + +sd_printk (KERN_NOTICE , sdkp,"Starting disk\n" ); + +if (opal_unlock_from_suspend (sdkp->opal_dev)) { +sd_printk (KERN_NOTICE , sdkp,"OPAL unlock failed\n" ); +return -EIO ; + } + +return 0 ; +} + +static int sd_resume_common (struct device *dev,bool runtime) {struct scsi_disk *sdkp =dev_get_drvdata (dev);int ret;@@ -4019,7 +4033,7 @@ static int sd_resume(struct device *dev, bool runtime)sd_printk (KERN_NOTICE , sdkp,"Starting disk\n" ); ret =sd_start_stop_device (sdkp,1 );if (!ret) { -opal_unlock_from_suspend (sdkp->opal_dev); +sd_resume (dev); sdkp->suspended = false; }@@ -4038,7 +4052,7 @@ static int sd_resume_system(struct device *dev)return 0 ; } -return sd_resume (dev, false); +return sd_resume_common (dev, false); }static int sd_resume_runtime (struct device *dev)@@ -4065,7 +4079,7 @@ static int sd_resume_runtime(struct device *dev) "Failed to clear sense data\n"); } -return sd_resume (dev, true); +return sd_resume_common (dev, true); }static const struct dev_pm_ops sd_pm_ops = {@@ -4088,6 +4102,7 @@ static struct scsi_driver sd_template = { .pm = &sd_pm_ops, }, .rescan = sd_rescan, + .resume = sd_resume, .init_command = sd_init_command, .uninit_command = sd_uninit_command, .done = sd_done,include/linux/libata.h +1
-0@@ -107,6 +107,7 @@ enum {ATA_DFLAG_NCQ_PRIO_ENABLED = (1 <<20 ),/* Priority cmds sent to dev */ ATA_DFLAG_CDL_ENABLED = (1 <<21 ),/* cmd duration limits is enabled */ +ATA_DFLAG_RESUMING = (1 <<22 ),/* Device is resuming */ ATA_DFLAG_DETACH = (1 <<24 ),ATA_DFLAG_DETACHED = (1 <<25 ), ATA_DFLAG_DA = (1 << 26), /* device supports Device Attention */include/scsi/scsi_driver.h +1
-0@@ -12,6 +12,7 @@ struct request;struct scsi_driver {struct device_driver gendrv; +int (*resume)(struct device *);void (*rescan)(struct device *);blk_status_t (*init_command)(struct scsi_cmnd *);void (*uninit_command)(struct scsi_cmnd *);include/scsi/scsi_host.h +1
-0@@ -767,6 +767,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 int scsi_resume_device (struct scsi_device *sdev);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 *);
Signature · 2 lines
Damien Le Moal Western Digital Research
Martin K. Petersen
AppliedQuoted 10 lines · Damien Le Moal
Commit
3cc2ffe5c16dintroduced themanage_system_start_stopscsi device flag to allow libata to indicate to the scsi disk driver that nothing should be done when resuming a disk on system resume. This change turned the execution ofsd_resume()into a no-op for ATA devices on system resume. While this solved deadlock issues during device resume, this change also wrongly removed the execution ofopal_unlock_from_suspend(). As a result, devices with TCG OPAL locking enabled remain locked and inaccessible after a system resume from sleep.[...]
Applied to 6.9/scsi-fixes, thanks!
[1/1] scsi: sd: Fix TCG OPAL unlock on system resume
https://git.kernel.org/mkp/scsi/c/0c76106cb975
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering