Hello,
I had some time yesterday and decided to help Jens out by rediffing the
now-infamous SATA suspend-to-ram patch [1] against current git and
test-building it.
For posterity,
This patch adds the ata_scsi_device_resume and ata_scsi_device_suspend
functions (along with helpers) to put to sleep and wake up Serial ATA
controllers when entering sleep states, and hooks the functions into
each SATA controller driver so that suspend-to-RAM is possible.
Note that this patch is a holdover patch until it is possible to
generalize this concept for all SCSI devices, which requires more data
on which devices need to be put to sleep and which don't.
Signed-off-by: Joshua Kwan <[email protected]>
--
Joshua Kwan
[1] http://seclists.org/lists/linux-kernel/2005/Sep/6195.html
On Fri, Sep 23 2005, Joshua Kwan wrote:
> Hello,
>
> I had some time yesterday and decided to help Jens out by rediffing the
> now-infamous SATA suspend-to-ram patch [1] against current git and
> test-building it.
>
> For posterity,
>
> This patch adds the ata_scsi_device_resume and ata_scsi_device_suspend
> functions (along with helpers) to put to sleep and wake up Serial ATA
> controllers when entering sleep states, and hooks the functions into
> each SATA controller driver so that suspend-to-RAM is possible.
>
> Note that this patch is a holdover patch until it is possible to
> generalize this concept for all SCSI devices, which requires more data
> on which devices need to be put to sleep and which don't.
Port looks fine, thanks. The only problem I've seen with the base patch
is that sometimes ata_do_simple_cmd() seems to be invoked right before a
previous command has completed. So I needed this addon to work around
that issue.
From: Jens Axboe <[email protected]>
Subject: Wait for current command to finish in ata_do_simple_cmd()
Patch-mainline:
References: 114648
A hack to wait a little while for the current command to complete, before
issuing a new one.
Acked-by:
Signed-off-by:
--- linux-2.6.13/drivers/scsi/libata-core.c~ 2005-09-01 12:22:19.000000000 +0200
+++ linux-2.6.13/drivers/scsi/libata-core.c 2005-09-01 12:24:38.000000000 +0200
@@ -3738,8 +3738,8 @@
unsigned long flags;
int rc;
- qc = ata_qc_new_init(ap, dev);
- BUG_ON(qc == NULL);
+ while ((qc = ata_qc_new_init(ap, dev)) == NULL)
+ msleep(10);
qc->tf.command = cmd;
qc->tf.flags |= ATA_TFLAG_DEVICE;
--
Jens Axboe
On Fri, 23 Sep 2005, Jens Axboe wrote:
>
> Port looks fine, thanks. The only problem I've seen with the base patch
> is that sometimes ata_do_simple_cmd() seems to be invoked right before a
> previous command has completed. So I needed this addon to work around
> that issue.
Ok. Can we have this in -mm for a few days just to shake out anything
interesting, and then merge it into mainline?
Linus
Jens Axboe wrote:
> On Fri, Sep 23 2005, Joshua Kwan wrote:
>>I had some time yesterday and decided to help Jens out by rediffing the
>>now-infamous SATA suspend-to-ram patch [1] against current git and
>>test-building it.
Very strange. I cannot find this patch at all in my email folders.
Can someone resend it to me?
> --- linux-2.6.13/drivers/scsi/libata-core.c~ 2005-09-01 12:22:19.000000000 +0200
> +++ linux-2.6.13/drivers/scsi/libata-core.c 2005-09-01 12:24:38.000000000 +0200
> @@ -3738,8 +3738,8 @@
> unsigned long flags;
> int rc;
>
> - qc = ata_qc_new_init(ap, dev);
> - BUG_ON(qc == NULL);
> + while ((qc = ata_qc_new_init(ap, dev)) == NULL)
> + msleep(10);
>
> qc->tf.command = cmd;
> qc->tf.flags |= ATA_TFLAG_DEVICE;
Worried now!
If this patch is needed, something VERY VERY WRONG is going on. This
patch indicates that the queueing state machine has been violated, and
something is trying to IGNORE the command synchronization :(
Further, you cannot always assume that msleep() is valid in that
context. It should be the caller that waits (libata suspend code), not
ata_do_simple_cmd() itself.
Does anyone have a link to James Bottomley's proposed patch? That one
seemed to do what was necessary -- send a SYNCHRONIZE_CACHE command then
turn it over to the LLD for further suspend.
Linus wrote:
> Ok. Can we have this in -mm for a few days just to shake out anything
> interesting, and then merge it into mainline?
Once we get a decent patch, I can merge it into my libata-dev.git
repository, which is automatically propagated to -mm.
Jeff
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -214,6 +214,8 @@ static Scsi_Host_Template ahci_sht = {
.slave_configure = ata_scsi_slave_config,
.bios_param = ata_std_bios_param,
.ordered_flush = 1,
+ .resume = ata_scsi_device_resume,
+ .suspend = ata_scsi_device_suspend,
};
static struct ata_port_operations ahci_ops = {
@@ -286,6 +288,8 @@ static struct pci_driver ahci_pci_driver
.id_table = ahci_pci_tbl,
.probe = ahci_init_one,
.remove = ahci_remove_one,
+ .suspend = ata_scsi_device_suspend,
+ .resume = ata_scsi_device_resume,
};
diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -125,6 +125,8 @@ static struct pci_driver piix_pci_driver
.id_table = piix_pci_tbl,
.probe = piix_init_one,
.remove = ata_pci_remove_one,
+ .suspend = ata_pci_device_suspend,
+ .resume = ata_pci_device_resume,
};
static Scsi_Host_Template piix_sht = {
@@ -145,6 +147,8 @@ static Scsi_Host_Template piix_sht = {
.slave_configure = ata_scsi_slave_config,
.bios_param = ata_std_bios_param,
.ordered_flush = 1,
+ .resume = ata_scsi_device_resume,
+ .suspend = ata_scsi_device_suspend,
};
static struct ata_port_operations piix_pata_ops = {
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3829,6 +3829,104 @@ err_out:
* LOCKING:
*/
+/*
+ * Execute a 'simple' command, that only consists of the opcode 'cmd' itself,
+ * without filling any other registers
+ */
+static int ata_do_simple_cmd(struct ata_port *ap, struct ata_device *dev,
+ u8 cmd)
+{
+ DECLARE_COMPLETION(wait);
+ struct ata_queued_cmd *qc;
+ unsigned long flags;
+ int rc;
+
+ qc = ata_qc_new_init(ap, dev);
+ BUG_ON(qc == NULL);
+
+ qc->tf.command = cmd;
+ qc->tf.flags |= ATA_TFLAG_DEVICE;
+ qc->tf.protocol = ATA_PROT_NODATA;
+
+ qc->waiting = &wait;
+ qc->complete_fn = ata_qc_complete_noop;
+
+ spin_lock_irqsave(&ap->host_set->lock, flags);
+ rc = ata_qc_issue(qc);
+ spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+ if (!rc)
+ wait_for_completion(&wait);
+
+ return rc;
+}
+
+static int ata_flush_cache(struct ata_port *ap, struct ata_device *dev)
+{
+ u8 cmd;
+
+ if (!ata_try_flush_cache(dev))
+ return 0;
+
+ if (ata_id_has_flush_ext(dev->id))
+ cmd = ATA_CMD_FLUSH_EXT;
+ else
+ cmd = ATA_CMD_FLUSH;
+
+ return ata_do_simple_cmd(ap, dev, cmd);
+}
+
+static int ata_standby_drive(struct ata_port *ap, struct ata_device *dev)
+{
+ return ata_do_simple_cmd(ap, dev, ATA_CMD_STANDBYNOW1);
+}
+
+static int ata_start_drive(struct ata_port *ap, struct ata_device *dev)
+{
+ return ata_do_simple_cmd(ap, dev, ATA_CMD_IDLEIMMEDIATE);
+}
+
+/**
+ * ata_device_resume - wakeup a previously suspended devices
+ *
+ * Kick the drive back into action, by sending it an idle immediate
+ * command and making sure its transfer mode matches between drive
+ * and host.
+ *
+ */
+int ata_device_resume(struct ata_port *ap, struct ata_device *dev)
+{
+ if (ap->flags & ATA_FLAG_SUSPENDED) {
+ ap->flags &= ~ATA_FLAG_SUSPENDED;
+ ata_set_mode(ap);
+ }
+ if (!ata_dev_present(dev))
+ return 0;
+ if (dev->class == ATA_DEV_ATA)
+ ata_start_drive(ap, dev);
+
+ return 0;
+}
+
+/**
+ * ata_device_suspend - prepare a device for suspend
+ *
+ * Flush the cache on the drive, if appropriate, then issue a
+ * standbynow command.
+ *
+ */
+int ata_device_suspend(struct ata_port *ap, struct ata_device *dev)
+{
+ if (!ata_dev_present(dev))
+ return 0;
+ if (dev->class == ATA_DEV_ATA)
+ ata_flush_cache(ap, dev);
+
+ ata_standby_drive(ap, dev);
+ ap->flags |= ATA_FLAG_SUSPENDED;
+ return 0;
+}
+
int ata_port_start (struct ata_port *ap)
{
struct device *dev = ap->host_set->dev;
@@ -4542,6 +4640,23 @@ int pci_test_config_bits(struct pci_dev
return (tmp == bits->val) ? 1 : 0;
}
+
+int ata_pci_device_suspend(struct pci_dev *pdev, pm_message_t state)
+{
+ pci_save_state(pdev);
+ pci_disable_device(pdev);
+ pci_set_power_state(pdev, PCI_D3hot);
+ return 0;
+}
+
+int ata_pci_device_resume(struct pci_dev *pdev)
+{
+ pci_set_power_state(pdev, PCI_D0);
+ pci_restore_state(pdev);
+ pci_enable_device(pdev);
+ pci_set_master(pdev);
+ return 0;
+}
#endif /* CONFIG_PCI */
@@ -4620,4 +4735,11 @@ EXPORT_SYMBOL_GPL(ata_pci_host_stop);
EXPORT_SYMBOL_GPL(ata_pci_init_native_mode);
EXPORT_SYMBOL_GPL(ata_pci_init_one);
EXPORT_SYMBOL_GPL(ata_pci_remove_one);
+EXPORT_SYMBOL_GPL(ata_pci_device_suspend);
+EXPORT_SYMBOL_GPL(ata_pci_device_resume);
#endif /* CONFIG_PCI */
+
+EXPORT_SYMBOL_GPL(ata_device_suspend);
+EXPORT_SYMBOL_GPL(ata_device_resume);
+EXPORT_SYMBOL_GPL(ata_scsi_device_suspend);
+EXPORT_SYMBOL_GPL(ata_scsi_device_resume);
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -402,6 +402,22 @@ int ata_scsi_error(struct Scsi_Host *hos
return 0;
}
+int ata_scsi_device_resume(struct scsi_device *sdev)
+{
+ struct ata_port *ap = (struct ata_port *) &sdev->host->hostdata[0];
+ struct ata_device *dev = &ap->device[sdev->id];
+
+ return ata_device_resume(ap, dev);
+}
+
+int ata_scsi_device_suspend(struct scsi_device *sdev)
+{
+ struct ata_port *ap = (struct ata_port *) &sdev->host->hostdata[0];
+ struct ata_device *dev = &ap->device[sdev->id];
+
+ return ata_device_suspend(ap, dev);
+}
+
/**
* ata_scsi_start_stop_xlat - Translate SCSI START STOP UNIT command
* @qc: Storage for translated ATA taskfile
diff --git a/drivers/scsi/sata_mv.c b/drivers/scsi/sata_mv.c
--- a/drivers/scsi/sata_mv.c
+++ b/drivers/scsi/sata_mv.c
@@ -208,6 +208,8 @@ static Scsi_Host_Template mv_sht = {
.slave_configure = ata_scsi_slave_config,
.bios_param = ata_std_bios_param,
.ordered_flush = 1,
+ .suspend = ata_scsi_device_suspend,
+ .resume = ata_scsi_device_resume,
};
static struct ata_port_operations mv_ops = {
@@ -294,6 +296,8 @@ static struct pci_driver mv_pci_driver =
.id_table = mv_pci_tbl,
.probe = mv_init_one,
.remove = ata_pci_remove_one,
+ .suspend = ata_scsi_device_suspend,
+ .resume = ata_scsi_device_resume,
};
/*
diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
--- a/drivers/scsi/sata_nv.c
+++ b/drivers/scsi/sata_nv.c
@@ -214,6 +214,8 @@ static struct pci_driver nv_pci_driver =
.id_table = nv_pci_tbl,
.probe = nv_init_one,
.remove = ata_pci_remove_one,
+ .suspend = ata_pci_device_suspend,
+ .resume = ata_pci_device_resume,
};
static Scsi_Host_Template nv_sht = {
@@ -234,6 +236,8 @@ static Scsi_Host_Template nv_sht = {
.slave_configure = ata_scsi_slave_config,
.bios_param = ata_std_bios_param,
.ordered_flush = 1,
+ .resume = ata_scsi_device_resume,
+ .suspend = ata_scsi_device_suspend,
};
static struct ata_port_operations nv_ops = {
diff --git a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
--- a/drivers/scsi/sata_promise.c
+++ b/drivers/scsi/sata_promise.c
@@ -111,6 +111,8 @@ static Scsi_Host_Template pdc_ata_sht =
.slave_configure = ata_scsi_slave_config,
.bios_param = ata_std_bios_param,
.ordered_flush = 1,
+ .resume = ata_scsi_device_resume,
+ .suspend = ata_scsi_device_suspend,
};
static struct ata_port_operations pdc_sata_ops = {
@@ -231,6 +233,8 @@ static struct pci_driver pdc_ata_pci_dri
.id_table = pdc_ata_pci_tbl,
.probe = pdc_ata_init_one,
.remove = ata_pci_remove_one,
+ .suspend = ata_pci_device_suspend,
+ .resume = ata_pci_device_resume,
};
diff --git a/drivers/scsi/sata_qstor.c b/drivers/scsi/sata_qstor.c
--- a/drivers/scsi/sata_qstor.c
+++ b/drivers/scsi/sata_qstor.c
@@ -143,6 +143,8 @@ static Scsi_Host_Template qs_ata_sht = {
.dma_boundary = QS_DMA_BOUNDARY,
.slave_configure = ata_scsi_slave_config,
.bios_param = ata_std_bios_param,
+ .resume = ata_scsi_device_resume,
+ .suspend = ata_scsi_device_suspend,
};
static struct ata_port_operations qs_ata_ops = {
@@ -194,6 +196,8 @@ static struct pci_driver qs_ata_pci_driv
.id_table = qs_ata_pci_tbl,
.probe = qs_ata_init_one,
.remove = ata_pci_remove_one,
+ .suspend = ata_pci_device_suspend,
+ .resume = ata_pci_device_resume,
};
static int qs_check_atapi_dma(struct ata_queued_cmd *qc)
diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -128,6 +128,8 @@ static struct pci_driver sil_pci_driver
.id_table = sil_pci_tbl,
.probe = sil_init_one,
.remove = ata_pci_remove_one,
+ .suspend = ata_pci_device_suspend,
+ .resume = ata_pci_device_resume,
};
static Scsi_Host_Template sil_sht = {
@@ -148,6 +150,8 @@ static Scsi_Host_Template sil_sht = {
.slave_configure = ata_scsi_slave_config,
.bios_param = ata_std_bios_param,
.ordered_flush = 1,
+ .resume = ata_scsi_device_resume,
+ .suspend = ata_scsi_device_suspend,
};
static struct ata_port_operations sil_ops = {
diff --git a/drivers/scsi/sata_sis.c b/drivers/scsi/sata_sis.c
--- a/drivers/scsi/sata_sis.c
+++ b/drivers/scsi/sata_sis.c
@@ -80,6 +80,8 @@ static struct pci_driver sis_pci_driver
.id_table = sis_pci_tbl,
.probe = sis_init_one,
.remove = ata_pci_remove_one,
+ .suspend = ata_pci_device_suspend,
+ .resume = ata_pci_device_resume,
};
static Scsi_Host_Template sis_sht = {
@@ -100,6 +102,8 @@ static Scsi_Host_Template sis_sht = {
.slave_configure = ata_scsi_slave_config,
.bios_param = ata_std_bios_param,
.ordered_flush = 1,
+ .resume = ata_scsi_device_resume,
+ .suspend = ata_scsi_device_suspend,
};
static struct ata_port_operations sis_ops = {
diff --git a/drivers/scsi/sata_svw.c b/drivers/scsi/sata_svw.c
--- a/drivers/scsi/sata_svw.c
+++ b/drivers/scsi/sata_svw.c
@@ -294,6 +294,8 @@ static Scsi_Host_Template k2_sata_sht =
#endif
.bios_param = ata_std_bios_param,
.ordered_flush = 1,
+ .resume = ata_scsi_device_resume,
+ .suspend = ata_scsi_device_suspend,
};
@@ -470,6 +472,8 @@ static struct pci_driver k2_sata_pci_dri
.id_table = k2_sata_pci_tbl,
.probe = k2_sata_init_one,
.remove = ata_pci_remove_one,
+ .suspend = ata_pci_device_suspend,
+ .resume = ata_pci_device_resume,
};
diff --git a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
--- a/drivers/scsi/sata_sx4.c
+++ b/drivers/scsi/sata_sx4.c
@@ -194,6 +194,8 @@ static Scsi_Host_Template pdc_sata_sht =
.slave_configure = ata_scsi_slave_config,
.bios_param = ata_std_bios_param,
.ordered_flush = 1,
+ .resume = ata_scsi_device_resume,
+ .suspend = ata_scsi_device_suspend,
};
static struct ata_port_operations pdc_20621_ops = {
@@ -240,6 +242,8 @@ static struct pci_driver pdc_sata_pci_dr
.id_table = pdc_sata_pci_tbl,
.probe = pdc_sata_init_one,
.remove = ata_pci_remove_one,
+ .suspend = ata_pci_device_suspend,
+ .resume = ata_pci_device_resume,
};
diff --git a/drivers/scsi/sata_uli.c b/drivers/scsi/sata_uli.c
--- a/drivers/scsi/sata_uli.c
+++ b/drivers/scsi/sata_uli.c
@@ -68,6 +68,8 @@ static struct pci_driver uli_pci_driver
.id_table = uli_pci_tbl,
.probe = uli_init_one,
.remove = ata_pci_remove_one,
+ .suspend = ata_pci_device_suspend,
+ .resume = ata_pci_device_resume,
};
static Scsi_Host_Template uli_sht = {
@@ -88,6 +90,8 @@ static Scsi_Host_Template uli_sht = {
.slave_configure = ata_scsi_slave_config,
.bios_param = ata_std_bios_param,
.ordered_flush = 1,
+ .resume = ata_scsi_device_resume,
+ .suspend = ata_scsi_device_suspend,
};
static struct ata_port_operations uli_ops = {
diff --git a/drivers/scsi/sata_via.c b/drivers/scsi/sata_via.c
--- a/drivers/scsi/sata_via.c
+++ b/drivers/scsi/sata_via.c
@@ -87,6 +87,8 @@ static struct pci_driver svia_pci_driver
.id_table = svia_pci_tbl,
.probe = svia_init_one,
.remove = ata_pci_remove_one,
+ .suspend = ata_pci_device_suspend,
+ .resume = ata_pci_device_resume,
};
static Scsi_Host_Template svia_sht = {
@@ -107,6 +109,8 @@ static Scsi_Host_Template svia_sht = {
.slave_configure = ata_scsi_slave_config,
.bios_param = ata_std_bios_param,
.ordered_flush = 1,
+ .resume = ata_scsi_device_resume,
+ .suspend = ata_scsi_device_suspend,
};
static struct ata_port_operations svia_sata_ops = {
diff --git a/drivers/scsi/sata_vsc.c b/drivers/scsi/sata_vsc.c
--- a/drivers/scsi/sata_vsc.c
+++ b/drivers/scsi/sata_vsc.c
@@ -228,6 +228,8 @@ static Scsi_Host_Template vsc_sata_sht =
.slave_configure = ata_scsi_slave_config,
.bios_param = ata_std_bios_param,
.ordered_flush = 1,
+ .resume = ata_scsi_device_resume,
+ .suspend = ata_scsi_device_suspend,
};
@@ -404,6 +406,8 @@ static struct pci_driver vsc_sata_pci_dr
.id_table = vsc_sata_pci_tbl,
.probe = vsc_sata_init_one,
.remove = ata_pci_remove_one,
+ .suspend = ata_pci_device_suspend,
+ .resume = ata_pci_device_resume,
};
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1944,8 +1944,9 @@ EXPORT_SYMBOL(scsi_device_quiesce);
void
scsi_device_resume(struct scsi_device *sdev)
{
- if(scsi_device_set_state(sdev, SDEV_RUNNING))
+ if (scsi_device_set_state(sdev, SDEV_RUNNING))
return;
+
scsi_run_queue(sdev->request_queue);
}
EXPORT_SYMBOL(scsi_device_resume);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -263,9 +263,40 @@ static int scsi_bus_match(struct device
return (sdp->inq_periph_qual == SCSI_INQ_PQ_CON)? 1: 0;
}
+static int scsi_bus_suspend(struct device * dev, pm_message_t state)
+{
+ struct scsi_device *sdev = to_scsi_device(dev);
+ struct scsi_host_template *sht = sdev->host->hostt;
+ int err;
+
+ err = scsi_device_quiesce(sdev);
+ if (err)
+ return err;
+
+ if (sht->suspend)
+ err = sht->suspend(sdev);
+
+ return err;
+}
+
+static int scsi_bus_resume(struct device * dev)
+{
+ struct scsi_device *sdev = to_scsi_device(dev);
+ struct scsi_host_template *sht = sdev->host->hostt;
+ int err = 0;
+
+ if (sht->resume)
+ err = sht->resume(sdev);
+
+ scsi_device_resume(sdev);
+ return err;
+}
+
struct bus_type scsi_bus_type = {
.name = "scsi",
.match = scsi_bus_match,
+ .suspend = scsi_bus_suspend,
+ .resume = scsi_bus_resume,
};
int scsi_sysfs_register(void)
diff --git a/include/linux/ata.h b/include/linux/ata.h
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -132,6 +132,8 @@ enum {
ATA_CMD_PACKET = 0xA0,
ATA_CMD_VERIFY = 0x40,
ATA_CMD_VERIFY_EXT = 0x42,
+ ATA_CMD_STANDBYNOW1 = 0xE0,
+ ATA_CMD_IDLEIMMEDIATE = 0xE1,
/* SETFEATURES stuff */
SETFEATURES_XFER = 0x03,
diff --git a/include/linux/libata.h b/include/linux/libata.h
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -117,6 +117,7 @@ enum {
ATA_FLAG_PIO_DMA = (1 << 8), /* PIO cmds via DMA */
ATA_FLAG_NOINTR = (1 << 9), /* FIXME: Remove this once
* proper HSM is in place. */
+ ATA_FLAG_SUSPENDED = (1 << 10), /* port is suspended */
ATA_QCFLAG_ACTIVE = (1 << 1), /* cmd not yet ack'd to scsi lyer */
ATA_QCFLAG_SG = (1 << 3), /* have s/g table? */
@@ -391,6 +392,8 @@ extern void ata_std_ports(struct ata_iop
extern int ata_pci_init_one (struct pci_dev *pdev, struct ata_port_info **port_info,
unsigned int n_ports);
extern void ata_pci_remove_one (struct pci_dev *pdev);
+extern int ata_pci_device_suspend(struct pci_dev *pdev, pm_message_t state);
+extern int ata_pci_device_resume(struct pci_dev *pdev);
#endif /* CONFIG_PCI */
extern int ata_device_add(struct ata_probe_ent *ent);
extern int ata_scsi_detect(Scsi_Host_Template *sht);
@@ -399,6 +402,10 @@ extern int ata_scsi_queuecmd(struct scsi
extern int ata_scsi_error(struct Scsi_Host *host);
extern int ata_scsi_release(struct Scsi_Host *host);
extern unsigned int ata_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc);
+extern int ata_scsi_device_resume(struct scsi_device *);
+extern int ata_scsi_device_suspend(struct scsi_device *);
+extern int ata_device_resume(struct ata_port *, struct ata_device *);
+extern int ata_device_suspend(struct ata_port *, struct ata_device *);
/*
* Default driver ops implementations
*/
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -295,6 +295,12 @@ struct scsi_host_template {
int (*proc_info)(struct Scsi_Host *, char *, char **, off_t, int, int);
/*
+ * suspend support
+ */
+ int (*resume)(struct scsi_device *);
+ int (*suspend)(struct scsi_device *);
+
+ /*
* Name of proc directory
*/
char *proc_name;
Jeff Garzik wrote:
> Very strange. I cannot find this patch at all in my email folders.
>
> Can someone resend it to me?
The original is at
http://seclists.org/lists/linux-kernel/2005/May/0447.html, as indicated
by the footnote in my original message.
That said, it's identical in content to the one I've just sent (attached
as a text/plain MIME attachment.)
> Worried now!
>
> If this patch is needed, something VERY VERY WRONG is going on. This
> patch indicates that the queueing state machine has been violated, and
> something is trying to IGNORE the command synchronization :(
>
> Further, you cannot always assume that msleep() is valid in that
> context. It should be the caller that waits (libata suspend code), not
> ata_do_simple_cmd() itself.
>
> Does anyone have a link to James Bottomley's proposed patch? That one
> seemed to do what was necessary -- send a SYNCHRONIZE_CACHE command then
> turn it over to the LLD for further suspend.
I don't think James sent any patch in the original thread with Jens'
patch. But that sounds somewhat cleaner than msleep().
At any rate, the current patch (with or without Jens' new addition)
works for me on Thinkpad T43. So the net result is better anyway, IMO.
>> Ok. Can we have this in -mm for a few days just to shake out anything
>> interesting, and then merge it into mainline?
>
> Once we get a decent patch, I can merge it into my libata-dev.git
> repository, which is automatically propagated to -mm.
Is the kludginess of Jens' addition the only problem you have with the
current patch? (If it is, then fixing this is out of the range of my
current ability...)
Thanks for responding.
--
Joshua Kwan
Bad news regarding sata suspend/resume, thought it may not
specifically be a problem with the patch but something more
fundamental.
I have a tecra M3 which uses the i915m and ich6m chips and
as such should be very similar to various dells and ibms which
this patch works great on.
However, for my particular machine, the step of putting the
sata controller pci device into D3 locks the machine up
completely. If I skip this part of the suspend process, it
doesn't lock up. but something is clearly not right with it
as the hd will fail to wake up if I leave it suspended too
long (if it's suspended and then quickly resumed like this,
it seems to work). Putting the PCI device into D1 leads to
problems after resume regardless of the amount of time
suspended.
I disassembled the acpi DSDT and checked it for errors but
didn't find anything of note, so I'm not sure why it does
this. It's obviously not a fundamental problem because windows
can suspend fine on it, but there's clearly a piece of the
puzzle missing. It's all very frustrating because this is
the only part of the machine that can't handle suspending yet.
I tried the reformatted patch with and without Jens' workaround
but the behavour is unchanged.
--phil
On Fri, Sep 23 2005, Jeff Garzik wrote:
> >--- linux-2.6.13/drivers/scsi/libata-core.c~ 2005-09-01
> >12:22:19.000000000 +0200
> >+++ linux-2.6.13/drivers/scsi/libata-core.c 2005-09-01
> >12:24:38.000000000 +0200
> >@@ -3738,8 +3738,8 @@
> > unsigned long flags;
> > int rc;
> >
> >- qc = ata_qc_new_init(ap, dev);
> >- BUG_ON(qc == NULL);
> >+ while ((qc = ata_qc_new_init(ap, dev)) == NULL)
> >+ msleep(10);
> >
> > qc->tf.command = cmd;
> > qc->tf.flags |= ATA_TFLAG_DEVICE;
>
> Worried now!
>
> If this patch is needed, something VERY VERY WRONG is going on. This
> patch indicates that the queueing state machine has been violated, and
> something is trying to IGNORE the command synchronization :(
I haven't diagnosed this further and it only ever happened in the SUSE
kernel to my knowledge (no one has reported it to me for the vanilla
kernels + suspend patch).
So lets just keep this patch out of the equation for now, it could be
that other SUSE patches broke something in this area :/
> Further, you cannot always assume that msleep() is valid in that
> context. It should be the caller that waits (libata suspend code), not
> ata_do_simple_cmd() itself.
ata_do_simple_cmd() always blocks anyways, so I don't see the point.
Perhaps rename the function to ata_execute_and_wait_simple_cmd().
--
Jens Axboe
Joshua Kwan wrote:
> Hello,
>
> I had some time yesterday and decided to help Jens out by rediffing the
> now-infamous SATA suspend-to-ram patch [1] against current git and
> test-building it.
>
> For posterity,
>
> This patch adds the ata_scsi_device_resume and ata_scsi_device_suspend
> functions (along with helpers) to put to sleep and wake up Serial ATA
> controllers when entering sleep states, and hooks the functions into
> each SATA controller driver so that suspend-to-RAM is possible.
>
> Note that this patch is a holdover patch until it is possible to
> generalize this concept for all SCSI devices, which requires more data
> on which devices need to be put to sleep and which don't.
>
> Signed-off-by: Joshua Kwan <[email protected]>
Ah hah! I found the other SCSI suspend patch:
http://lwn.net/Articles/97453/
Anybody (Joshua?) up for reconciling and testing the two?
The main change from Jens/Joshua's patch is that we use SCSI's
sd_shutdown() to call sync cache, eliminating the need for
ata_flush_cache(), since the SCSI layer would now perform that.
For bonus points,
1) sd should call START STOP UNIT on suspend, which eliminates the need
for ata_standby_drive(), and completely encompasses the suspend process
in the SCSI layer.
2) sd should call START STOP UNIT on resume -- and as a SUPER BONUS, the
combination of these two changes ensures that there are no queue
synchronization issues, the likes of which would require hacks like
Jens' while-loop patch.
None of these are huge changes requiring a lot of thinking/planning...
Finally, ideally, we should be issuing a hardware or software reset on
suspend.
Jeff
On Thu, Sep 29, 2005 at 01:21:28AM -0400, Jeff Garzik wrote:
> Ah hah! I found the other SCSI suspend patch:
>
> http://lwn.net/Articles/97453/
>
> Anybody (Joshua?) up for reconciling and testing the two?
>
> The main change from Jens/Joshua's patch is that we use SCSI's
> sd_shutdown() to call sync cache, eliminating the need for
> ata_flush_cache(), since the SCSI layer would now perform that.
>
> For bonus points,
>
> 1) sd should call START STOP UNIT on suspend, which eliminates the need
> for ata_standby_drive(), and completely encompasses the suspend process
> in the SCSI layer.
>
> 2) sd should call START STOP UNIT on resume -- and as a SUPER BONUS, the
> combination of these two changes ensures that there are no queue
> synchronization issues, the likes of which would require hacks like
> Jens' while-loop patch.
>
> None of these are huge changes requiring a lot of thinking/planning...
>
> Finally, ideally, we should be issuing a hardware or software reset on
> suspend.
I like this one much more than the other patch aswell, because suspsending
is an ULDD operation, not an LLDD one, and this fits the layering model
much better. The only complaints here are cosmetics:
- generic_scsi_suspend/generic_scsi_resume are misnamed, they should
probably be scsi_device_suspend/resume.
- while we're at it they could probably move to scsi_sysfs.c to keep
them static in one file - they're just a tiny bit of glue anyway.
- get rid of all the CONFIG_PM ifdefs - it just clutters thing up far
too much.
On Thu, Sep 29, 2005 at 08:34:37AM +0100, Christoph Hellwig wrote:
> is an ULDD operation, not an LLDD one, and this fits the layering model
> much better. The only complaints here are cosmetics:
>
> - generic_scsi_suspend/generic_scsi_resume are misnamed, they should
> probably be scsi_device_suspend/resume.
> - while we're at it they could probably move to scsi_sysfs.c to keep
> them static in one file - they're just a tiny bit of glue anyway.
> - get rid of all the CONFIG_PM ifdefs - it just clutters thing up far
> too much.
Actually one important thing is missing, that is a way to avoid spinning
down external disks. As a start a sysfs-controlable flag should do it,
later we can add transport-specific ways to find out whether a device
is external.
In other times when I implemented this....
You need to be careful on the power-up. Many JBODs share a single
"enclosure" and that enclosure has a limited power supply. If all
drives were spun up in parallel (and a drive may take 10-15seconds
to spin up), then they can overload the enclosure's power limit.
This issue, which normally occurs when an enclosure is first powered
on, was solved by injecting sequence delays based on either jumpers
or delays based on address/slot. But this won't help the software
suspend/resume.
There were not a lot of great answers on how to solve this as it usually
required knowledge of how the hardware was packaged. What we defaulted
to was limiting spin up to never concurrently start more than N drives
on a scsi bus. N defaulted to 1, but allowed the admin to tune it.
-- james s
> -----Original Message-----
> From: [email protected]
> [mailto:[email protected]]On Behalf Of
> Christoph Hellwig
> Sent: Thursday, September 29, 2005 4:12 AM
> To: Christoph Hellwig; Jeff Garzik; Joshua Kwan;
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> randy_dunlap
> Subject: Re: SATA suspend/resume (was Re: [PATCH] updated version of
> Jens' SATA suspend-to-ram patch)
>
>
> On Thu, Sep 29, 2005 at 08:34:37AM +0100, Christoph Hellwig wrote:
> > is an ULDD operation, not an LLDD one, and this fits the
> layering model
> > much better. The only complaints here are cosmetics:
> >
> > - generic_scsi_suspend/generic_scsi_resume are misnamed,
> they should
> > probably be scsi_device_suspend/resume.
> > - while we're at it they could probably move to
> scsi_sysfs.c to keep
> > them static in one file - they're just a tiny bit of glue anyway.
> > - get rid of all the CONFIG_PM ifdefs - it just clutters
> thing up far
> > too much.
>
> Actually one important thing is missing, that is a way to
> avoid spinning
> down external disks. As a start a sysfs-controlable flag
> should do it,
> later we can add transport-specific ways to find out whether a device
> is external.
> -
> To unsubscribe from this list: send the line "unsubscribe
> linux-scsi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
I admit I haven't studied the patches. Anyway, here is what I have to
say about what I (mis?)understood from your posts:
James Smart wrote:
> You need to be careful on the power-up. Many JBODs share a single
> "enclosure" and that enclosure has a limited power supply. If all
> drives were spun up in parallel (and a drive may take 10-15seconds
> to spin up), then they can overload the enclosure's power limit.
[...]
> There were not a lot of great answers on how to solve this as it usually
> required knowledge of how the hardware was packaged.
[...]
>>On Thu, Sep 29, 2005 at 08:34:37AM +0100, Christoph Hellwig wrote:
>>>is an ULDD operation, not an LLDD one, and this fits the layering model
>>>much better.
The operation _involves_ the high level, yes. But whether it may be/
must not be performed has to be controlled by the transport layer. Take
FireWire as an example: IEEE 1394(a,b) has power management
specifications. (NB: Its indeed in IEEE 1394, not in the SBP-2 spec.)
One rule is that only one node on a FireWire bus may perform power
management; the node which is allowed to do this is determined by a
special protocol.
>>Actually one important thing is missing, that is a way to avoid spinning
>>down external disks. As a start a sysfs-controlable flag should do it,
>>later we can add transport-specific ways to find out whether a device
>>is external.
It is not a question of external vs. internal, at least not if you
consider more than SATA. Power management is the genuine task of
transport layers (specifically, of transport management layers). These
layers might need assistence from SCSI high-level protocol layer though.
IOW it's certainly correct to provide suspend/resume helpers in SCSI
high level (probably abstracted through SCSI core), but whether these
helpers are called or not has to be decided down in the SCSI low level,
or even further beneath that level.
--
Stefan Richter
-=====-=-=-= =--= ===-=
http://arcgraph.de/sr/
On Thu, 29 Sep 2005, Jeff Garzik wrote:
> Joshua Kwan wrote:
> > Hello,
> >
> > I had some time yesterday and decided to help Jens out by rediffing the
> > now-infamous SATA suspend-to-ram patch [1] against current git and
> > test-building it.
> >
> > For posterity,
> >
> > This patch adds the ata_scsi_device_resume and ata_scsi_device_suspend
> > functions (along with helpers) to put to sleep and wake up Serial ATA
> > controllers when entering sleep states, and hooks the functions into
> > each SATA controller driver so that suspend-to-RAM is possible.
> >
> > Note that this patch is a holdover patch until it is possible to
> > generalize this concept for all SCSI devices, which requires more data
> > on which devices need to be put to sleep and which don't.
> >
> > Signed-off-by: Joshua Kwan <[email protected]>
>
> Ah hah! I found the other SCSI suspend patch:
>
> http://lwn.net/Articles/97453/
>
> Anybody (Joshua?) up for reconciling and testing the two?
>
> The main change from Jens/Joshua's patch is that we use SCSI's
> sd_shutdown() to call sync cache, eliminating the need for
> ata_flush_cache(), since the SCSI layer would now perform that.
>
> For bonus points,
>
> 1) sd should call START STOP UNIT on suspend, which eliminates the need
> for ata_standby_drive(), and completely encompasses the suspend process
> in the SCSI layer.
>
> 2) sd should call START STOP UNIT on resume -- and as a SUPER BONUS, the
> combination of these two changes ensures that there are no queue
> synchronization issues, the likes of which would require hacks like
> Jens' while-loop patch.
>
> None of these are huge changes requiring a lot of thinking/planning...
>
> Finally, ideally, we should be issuing a hardware or software reset on
> suspend.
Here's Nathan Bryant's patch (from the lwn.ne article) updated
to 2.6.14-rc2-git7 + changes that Christoph suggested (except
that 'scsi_device_resume' name was already used, so I changed it
to 'scsi_device_wakeup' instead).
I'll get back to Jeff's suggestion(s) and the sysfs flag next,
but others can use this as a basis if wanted.
(also available from
http://www.xenotime.net/linux/scsi/scsi-suspend-resume.patch
)
--
~Randy
drivers/scsi/scsi_priv.h | 2 +
drivers/scsi/scsi_sysfs.c | 39 +++++++++++++++++++++++++++++++++--
drivers/scsi/sd.c | 18 +++++++++++++++-
include/scsi/scsi_driver.h | 2 +
4 files changed, 58 insertions(+), 3 deletions(-)
diff -Naurp -X linux-2614-rc2/Documentation/dontdiff linux-2614-rc2-git7-clean/drivers/scsi/scsi_priv.h linux-2614-rc2-git7-scsi/drivers/scsi/scsi_priv.h
--- linux-2614-rc2-git7-clean/drivers/scsi/scsi_priv.h 2005-09-29 12:58:21.000000000 -0700
+++ linux-2614-rc2-git7-scsi/drivers/scsi/scsi_priv.h 2005-09-29 12:48:00.000000000 -0700
@@ -55,6 +55,8 @@ static inline void scsi_log_send(struct
static inline void scsi_log_completion(struct scsi_cmnd *cmd, int disposition)
{ };
#endif
+extern int scsi_device_suspend(struct device *dev, pm_message_t state);
+extern int scsi_device_wakeup(struct device *dev);
/* scsi_devinfo.c */
extern int scsi_get_device_flags(struct scsi_device *sdev,
diff -Naurp -X linux-2614-rc2/Documentation/dontdiff linux-2614-rc2-git7-clean/drivers/scsi/scsi_sysfs.c linux-2614-rc2-git7-scsi/drivers/scsi/scsi_sysfs.c
--- linux-2614-rc2-git7-clean/drivers/scsi/scsi_sysfs.c 2005-09-29 12:58:25.000000000 -0700
+++ linux-2614-rc2-git7-scsi/drivers/scsi/scsi_sysfs.c 2005-09-29 12:56:16.000000000 -0700
@@ -14,6 +14,7 @@
#include <scsi/scsi.h>
#include <scsi/scsi_device.h>
+#include <scsi/scsi_driver.h>
#include <scsi/scsi_host.h>
#include <scsi/scsi_tcq.h>
#include <scsi/scsi_transport.h>
@@ -264,8 +265,10 @@ static int scsi_bus_match(struct device
}
struct bus_type scsi_bus_type = {
- .name = "scsi",
- .match = scsi_bus_match,
+ .name = "scsi",
+ .match = scsi_bus_match,
+ .suspend = scsi_device_suspend,
+ .resume = scsi_device_wakeup,
};
int scsi_sysfs_register(void)
@@ -770,6 +773,38 @@ void scsi_remove_target(struct device *d
}
EXPORT_SYMBOL(scsi_remove_target);
+int scsi_device_suspend(struct device *dev, pm_message_t state)
+{
+ int err;
+ struct scsi_device *sdev = to_scsi_device(dev);
+ struct scsi_driver *drv = to_scsi_driver(dev->driver);
+
+ err = scsi_device_quiesce(sdev);
+ if (err)
+ return err;
+
+ if (drv->suspend)
+ return drv->suspend(dev, state);
+
+ return 0;
+}
+
+int scsi_device_wakeup(struct device *dev)
+{
+ int err;
+ struct scsi_device *sdev = to_scsi_device(dev);
+ struct scsi_driver *drv = to_scsi_driver(dev->driver);
+
+ if (drv->resume) {
+ err = drv->resume(dev);
+ if (err)
+ return err;
+ }
+
+ scsi_device_resume(sdev);
+ return 0;
+}
+
int scsi_register_driver(struct device_driver *drv)
{
drv->bus = &scsi_bus_type;
diff -Naurp -X linux-2614-rc2/Documentation/dontdiff linux-2614-rc2-git7-clean/drivers/scsi/sd.c linux-2614-rc2-git7-scsi/drivers/scsi/sd.c
--- linux-2614-rc2-git7-clean/drivers/scsi/sd.c 2005-09-29 12:58:25.000000000 -0700
+++ linux-2614-rc2-git7-scsi/drivers/scsi/sd.c 2005-09-29 13:02:10.000000000 -0700
@@ -117,6 +117,8 @@ static void sd_rw_intr(struct scsi_cmnd
static int sd_probe(struct device *);
static int sd_remove(struct device *);
+static int sd_suspend(struct device *, pm_message_t);
+static int sd_resume(struct device *);
static void sd_shutdown(struct device *dev);
static void sd_rescan(struct device *);
static int sd_init_command(struct scsi_cmnd *);
@@ -136,6 +138,8 @@ static struct scsi_driver sd_template =
},
.rescan = sd_rescan,
.init_command = sd_init_command,
+ .suspend = sd_suspend,
+ .resume = sd_resume,
.issue_flush = sd_issue_flush,
.prepare_flush = sd_prepare_flush,
.end_flush = sd_end_flush,
@@ -1691,7 +1695,19 @@ static void sd_shutdown(struct device *d
printk(KERN_NOTICE "Synchronizing SCSI cache for disk %s: \n",
sdkp->disk->disk_name);
sd_sync_cache(sdp);
-}
+}
+
+static int sd_suspend(struct device *dev, pm_message_t state)
+{
+ sd_shutdown(dev);
+ return 0;
+}
+
+static int sd_resume(struct device *dev)
+{
+ sd_rescan(dev);
+ return 0;
+}
/**
* init_sd - entry point for this driver (both when built in or when
diff -Naurp -X linux-2614-rc2/Documentation/dontdiff linux-2614-rc2-git7-clean/include/scsi/scsi_driver.h linux-2614-rc2-git7-scsi/include/scsi/scsi_driver.h
--- linux-2614-rc2-git7-clean/include/scsi/scsi_driver.h 2005-08-28 16:41:01.000000000 -0700
+++ linux-2614-rc2-git7-scsi/include/scsi/scsi_driver.h 2005-09-29 12:47:43.000000000 -0700
@@ -13,6 +13,8 @@ struct scsi_driver {
int (*init_command)(struct scsi_cmnd *);
void (*rescan)(struct device *);
+ int (*suspend)(struct device *dev, pm_message_t state);
+ int (*resume)(struct device *dev);
int (*issue_flush)(struct device *, sector_t *);
int (*prepare_flush)(struct request_queue *, struct request *);
void (*end_flush)(struct request_queue *, struct request *);
On Thu, 29 Sep 2005, Randy.Dunlap wrote:
> Here's Nathan Bryant's patch (from the lwn.ne article) updated
> to 2.6.14-rc2-git7 + changes that Christoph suggested (except
> that 'scsi_device_resume' name was already used, so I changed it
> to 'scsi_device_wakeup' instead).
>
> I'll get back to Jeff's suggestion(s) and the sysfs flag next,
> but others can use this as a basis if wanted.
>
> (also available from
> http://www.xenotime.net/linux/scsi/scsi-suspend-resume.patch
> )
The inline version of this patch is whitespace-damaged :(
(thanx to pine).
It's ok for review, but please download the one above
for applying or patching.
--
~Randy
Jeff Garzik wrote:
..
> Ah hah! I found the other SCSI suspend patch:
> http://lwn.net/Articles/97453/
> Anybody (Joshua?) up for reconciling and testing the two?
I just now tried out *only* "the other SCSI suspend patch",
and by itself it hangs on resume. Laptop computer, blank screen,
no serial ports, no printk()s visible.
And there's one minor bug in that patch: it uses GFP_KERNEL to
alloc a buffer, but on resume it really should use GFP_ATOMIC instead,
since the swap device is the same drive we're trying to resume..
> 2) sd should call START STOP UNIT on resume
That's probably why it hangs when used as-is by itself.
I may do some further testing.
Anyone else out there playing with this yet?
On Sat, 01 Oct 2005 14:24:12 -0400 Mark Lord wrote:
> Jeff Garzik wrote:
> ..
> > Ah hah! I found the other SCSI suspend patch:
> > http://lwn.net/Articles/97453/
> > Anybody (Joshua?) up for reconciling and testing the two?
>
> I just now tried out *only* "the other SCSI suspend patch",
> and by itself it hangs on resume. Laptop computer, blank screen,
> no serial ports, no printk()s visible.
>
> And there's one minor bug in that patch: it uses GFP_KERNEL to
> alloc a buffer, but on resume it really should use GFP_ATOMIC instead,
> since the swap device is the same drive we're trying to resume..
>
> > 2) sd should call START STOP UNIT on resume
>
> That's probably why it hangs when used as-is by itself.
> I may do some further testing.
>
> Anyone else out there playing with this yet?
Not playing with it yet, just making some changes as suggested
by Jeff and Christoph. Patches are in
http://www.xenotime.net/linux/scsi/
1. http://www.xenotime.net/linux/scsi/scsi-suspend-resume.patch
2. http://www.xenotime.net/linux/scsi/scsi-susres-startstop2.patch
and work-in-progress: adding <spindown> ok/allowed to scsi
targets: http://www.xenotime.net/linux/scsi/scsi-susres-stst-spin.patch
(this patch file includes 1. and 2. above)
---
~Randy