diff mbox

[RFC] runtime PM for sd

Message ID 41990912.yaYtS9KD7e@linux-lqwf.site (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Oliver Neukum July 26, 2012, 7:26 p.m. UTC
Hi,

this is the first draft of a patch to allow runtime power management
for SCSI drives and thence USB storage devices with mounted media.
I am looking foward to comments and especially suggestions on how
to fix the race between sd_prep_fn() and sd_suspend() regarding device_busy.

	Regards
		Oliver

From 59c5b66bc4b84e361cc965cee2d6447a69bd6769 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oliver@neukum.org>
Date: Thu, 26 Jul 2012 21:20:32 +0200
Subject: [PATCH] sd: runtime PM for scsi drives with mounted media

---
 drivers/scsi/scsi_lib.c    |    1 -
 drivers/scsi/scsi_pm.c     |   13 ++++++++
 drivers/scsi/sd.c          |   70 +++++++++++++++++++++++++++++++++++++------
 include/scsi/scsi_device.h |    4 ++
 4 files changed, 77 insertions(+), 11 deletions(-)

Comments

Alan Stern July 26, 2012, 7:43 p.m. UTC | #1
On Thu, 26 Jul 2012, Oliver Neukum wrote:

> Hi,
> 
> this is the first draft of a patch to allow runtime power management
> for SCSI drives and thence USB storage devices with mounted media.
> I am looking foward to comments and especially suggestions on how
> to fix the race between sd_prep_fn() and sd_suspend() regarding device_busy.

How is this related to Lin Ming's patch set?  On the face of it they 
seem to do the same thing, except that his work applies to all block 
drivers that use request structures (as opposed to bio-oriented 
drivers).

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Neukum July 30, 2012, 12:43 p.m. UTC | #2
On Thursday 26 July 2012 15:43:59 Alan Stern wrote:
> On Thu, 26 Jul 2012, Oliver Neukum wrote:
> 
> > Hi,
> > 
> > this is the first draft of a patch to allow runtime power management
> > for SCSI drives and thence USB storage devices with mounted media.
> > I am looking foward to comments and especially suggestions on how
> > to fix the race between sd_prep_fn() and sd_suspend() regarding device_busy.
> 
> How is this related to Lin Ming's patch set?  On the face of it they 
> seem to do the same thing, except that his work applies to all block 
> drivers that use request structures (as opposed to bio-oriented 
> drivers).

As far as I can tell Lin Ming addressed sr. So I don't see a connection.

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern July 30, 2012, 2:44 p.m. UTC | #3
On Mon, 30 Jul 2012, Oliver Neukum wrote:

> On Thursday 26 July 2012 15:43:59 Alan Stern wrote:
> > On Thu, 26 Jul 2012, Oliver Neukum wrote:
> > 
> > > Hi,
> > > 
> > > this is the first draft of a patch to allow runtime power management
> > > for SCSI drives and thence USB storage devices with mounted media.
> > > I am looking foward to comments and especially suggestions on how
> > > to fix the race between sd_prep_fn() and sd_suspend() regarding device_busy.
> > 
> > How is this related to Lin Ming's patch set?  On the face of it they 
> > seem to do the same thing, except that his work applies to all block 
> > drivers that use request structures (as opposed to bio-oriented 
> > drivers).
> 
> As far as I can tell Lin Ming addressed sr. So I don't see a connection.

I was talking about the patch series that starts here:

	http://marc.info/?l=linux-pm&m=134154770423893&w=2

That appears to be the most recent iteration of the patches.  James 
Bottomley hasn't approved it, though, and didn't respond to the last 
message I posted on the subject.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Neukum July 30, 2012, 3:18 p.m. UTC | #4
On Monday 30 July 2012 10:44:31 Alan Stern wrote:
> On Mon, 30 Jul 2012, Oliver Neukum wrote:
> 
> > On Thursday 26 July 2012 15:43:59 Alan Stern wrote:
> > > On Thu, 26 Jul 2012, Oliver Neukum wrote:
> > > 
> > > > Hi,
> > > > 
> > > > this is the first draft of a patch to allow runtime power management
> > > > for SCSI drives and thence USB storage devices with mounted media.
> > > > I am looking foward to comments and especially suggestions on how
> > > > to fix the race between sd_prep_fn() and sd_suspend() regarding device_busy.
> > > 
> > > How is this related to Lin Ming's patch set?  On the face of it they 
> > > seem to do the same thing, except that his work applies to all block 
> > > drivers that use request structures (as opposed to bio-oriented 
> > > drivers).
> > 
> > As far as I can tell Lin Ming addressed sr. So I don't see a connection.
> 
> I was talking about the patch series that starts here:
> 
> 	http://marc.info/?l=linux-pm&m=134154770423893&w=2
> 
> That appears to be the most recent iteration of the patches.  James 
> Bottomley hasn't approved it, though, and didn't respond to the last 
> message I posted on the subject.

Hi,

I indeed overlooked this series. I'd bite my own posterior were I flexible
enough.

I'd say that the block layer is too generic to use for this. In fact, even
sd may be too high in the stack. For example iSCSI might, if the network
card is sufficiently advanced, suspend the network card while commands
are being executed.

That said in many details Min Ling's patches are clearly better than mine.
His patch 06149135407e0cbf943fe955e06418afff222fcc is very good stuff.
d1e0768738946d0ed19a145218f9789f4f0b6685 changes sd in a manner
that is no good. Because of check_disk_change() sd_open() must resume
devices.

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b583277..fb1bf7d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -254,7 +254,6 @@  int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 }
 EXPORT_SYMBOL(scsi_execute);
 
-
 int scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd,
 		     int data_direction, void *buffer, unsigned bufflen,
 		     struct scsi_sense_hdr *sshdr, int timeout, int retries,
diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index dc0ad85..edab892 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -201,6 +201,19 @@  void scsi_autopm_put_device(struct scsi_device *sdev)
 }
 EXPORT_SYMBOL_GPL(scsi_autopm_put_device);
 
+void scsi_autopm_mark_busy(struct scsi_device *sdev)
+{
+	pm_runtime_mark_last_busy(&sdev->sdev_gendev);
+}
+EXPORT_SYMBOL_GPL(scsi_autopm_mark_busy);
+
+void scsi_autopm_demand_wakeup(struct scsi_device *sdev)
+{
+	pm_runtime_get(&sdev->sdev_gendev);
+	pm_runtime_put_noidle(&sdev->sdev_gendev);
+}
+EXPORT_SYMBOL_GPL(scsi_autopm_demand_wakeup);
+
 void scsi_autopm_get_target(struct scsi_target *starget)
 {
 	pm_runtime_get_sync(&starget->dev);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4df73e5..68ca2e8 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -664,6 +664,11 @@  static void sd_unprep_fn(struct request_queue *q, struct request *rq)
 	}
 }
 
+static bool sd_relevant_for_pm(struct request *rq)
+{
+	return (rq->cmd[0] != SYNCHRONIZE_CACHE) && (rq->cmd[6] != START_STOP);
+}
+
 /**
  *	sd_prep_fn - build a scsi (read or write) command from
  *	information in the request structure.
@@ -683,6 +688,7 @@  static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	unsigned int this_count = blk_rq_sectors(rq);
 	int ret, host_dif;
 	unsigned char protect;
+	bool pmrelevant;
 
 	/*
 	 * Discard request come in as REQ_TYPE_FS but we turn them into
@@ -707,6 +713,17 @@  static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	SCpnt = rq->special;
 	sdkp = scsi_disk(disk);
 
+	pmrelevant = sd_relevant_for_pm(rq);
+	if (sdp->sdev_gendev.power.runtime_status) {
+		if (pmrelevant) {
+			scsi_autopm_demand_wakeup(sdp);
+			ret = BLKPREP_DEFER;
+			goto out;
+		}
+	}
+	if (pmrelevant)
+		scsi_autopm_mark_busy(sdp);
+
 	/* from here on until we're complete, any goto out
 	 * is used for a killable error condition */
 	ret = BLKPREP_KILL;
@@ -1011,6 +1028,7 @@  static int sd_open(struct block_device *bdev, fmode_t mode)
 		if (scsi_block_when_processing_errors(sdev))
 			scsi_set_medium_removal(sdev, SCSI_REMOVAL_PREVENT);
 	}
+	scsi_autopm_put_device(sdev);
 
 	return 0;
 
@@ -1038,9 +1056,14 @@  static int sd_release(struct gendisk *disk, fmode_t mode)
 {
 	struct scsi_disk *sdkp = scsi_disk(disk);
 	struct scsi_device *sdev = sdkp->device;
+	int retval;
 
 	SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp, "sd_release\n"));
 
+	retval = scsi_autopm_get_device(sdev);
+	if (retval)
+		goto error_autopm;
+
 	if (atomic_dec_return(&sdkp->openers) == 0 && sdev->removable) {
 		if (scsi_block_when_processing_errors(sdev))
 			scsi_set_medium_removal(sdev, SCSI_REMOVAL_ALLOW);
@@ -1052,6 +1075,7 @@  static int sd_release(struct gendisk *disk, fmode_t mode)
 	 */
 
 	scsi_autopm_put_device(sdev);
+error_autopm:
 	scsi_disk_put(sdkp);
 	return 0;
 }
@@ -1110,6 +1134,11 @@  static int sd_ioctl(struct block_device *bdev, fmode_t mode,
 	if (error < 0)
 		return error;
 
+	error = scsi_autopm_get_device(sdp);
+	if (error < 0) {
+		error = -EIO;
+		goto out;
+	}
 	/*
 	 * If we are in the middle of error recovery, don't let anyone
 	 * else try and use this device.  Also, if error recovery fails, it
@@ -1138,6 +1167,7 @@  static int sd_ioctl(struct block_device *bdev, fmode_t mode,
 			error = scsi_ioctl(sdp, cmd, p);
 			break;
 	}
+	scsi_autopm_put_device(sdp);
 out:
 	return error;
 }
@@ -1313,25 +1343,30 @@  static int sd_compat_ioctl(struct block_device *bdev, fmode_t mode,
 	if (ret < 0)
 		return ret;
 
+	ret = scsi_autopm_get_device(sdev);
+	if (ret < 0)
+		return -EIO;
+
+	ret = -ENOIOCTLCMD;
 	/*
 	 * If we are in the middle of error recovery, don't let anyone
 	 * else try and use this device.  Also, if error recovery fails, it
 	 * may try and take the device offline, in which case all further
 	 * access to the device is prohibited.
 	 */
-	if (!scsi_block_when_processing_errors(sdev))
-		return -ENODEV;
+	if (!scsi_block_when_processing_errors(sdev)) {
+		ret = -ENODEV;
+		goto out;
+	}
 	       
-	if (sdev->host->hostt->compat_ioctl) {
+	if (sdev->host->hostt->compat_ioctl)
 		ret = sdev->host->hostt->compat_ioctl(sdev, cmd, (void __user *)arg);
-
-		return ret;
-	}
-
+out:
+	scsi_autopm_put_device(sdev);
 	/* 
 	 * Let the static ioctl translation table take care of it.
 	 */
-	return -ENOIOCTLCMD; 
+	return ret; 
 }
 #endif
 
@@ -2459,6 +2494,7 @@  static int sd_revalidate_disk(struct gendisk *disk)
 	struct scsi_device *sdp = sdkp->device;
 	unsigned char *buffer;
 	unsigned flush = 0;
+	int retval = 0;
 
 	SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp,
 				      "sd_revalidate_disk\n"));
@@ -2477,6 +2513,12 @@  static int sd_revalidate_disk(struct gendisk *disk)
 		goto out;
 	}
 
+	retval = scsi_autopm_get_device(sdp);
+	if (retval < 0) {
+		retval = -EIO;
+		goto out;
+	}
+
 	sd_spinup_disk(sdkp);
 
 	/*
@@ -2512,10 +2554,11 @@  static int sd_revalidate_disk(struct gendisk *disk)
 	blk_queue_flush(sdkp->disk->queue, flush);
 
 	set_capacity(disk, sdkp->capacity);
+	scsi_autopm_put_device(sdp);
 	kfree(buffer);
 
  out:
-	return 0;
+	return retval;
 }
 
 /**
@@ -2870,16 +2913,23 @@  static int sd_suspend(struct device *dev, pm_message_t mesg)
 	if (!sdkp)
 		return 0;	/* this can happen */
 
+	if (PMSG_IS_AUTO(mesg) && sdkp->device->busy)
+		return -EBUSY;
+
 	if (sdkp->WCE) {
 		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
 		ret = sd_sync_cache(sdkp);
-		if (ret)
+		if (ret < 0) {
 			goto done;
+		}
 	}
 
 	if ((mesg.event & PM_EVENT_SLEEP) && sdkp->device->manage_start_stop) {
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
 		ret = sd_start_stop_device(sdkp, 0);
+		if (ret < 0) {
+			goto done;
+		}
 	}
 
 done:
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 7539f52..f7e7072 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -394,9 +394,13 @@  extern int scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd,
 #ifdef CONFIG_PM_RUNTIME
 extern int scsi_autopm_get_device(struct scsi_device *);
 extern void scsi_autopm_put_device(struct scsi_device *);
+extern void scsi_autopm_mark_busy(struct scsi_device *);
+extern void scsi_autopm_demand_wakeup(struct scsi_device *);
 #else
 static inline int scsi_autopm_get_device(struct scsi_device *d) { return 0; }
 static inline void scsi_autopm_put_device(struct scsi_device *d) {}
+static inline void scsi_autopm_mark_busy(struct scsi_device *d) {}
+static inline void scsi_autopm_demand_wakeup(struct scsi_device *d) {}
 #endif /* CONFIG_PM_RUNTIME */
 
 static inline int __must_check scsi_device_reprobe(struct scsi_device *sdev)