Patchwork [2/5] scsi: sr: runtime pm when ODD is open/closed

login
register
mail settings
Submitter Aaron Lu
Date July 23, 2012, 6:49 a.m.
Message ID <1343026180-22236-3-git-send-email-aaron.lu@amd.com>
Download mbox | patch
Permalink /patch/172551/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Aaron Lu - July 23, 2012, 6:49 a.m.
The ODD can either be runtime resumed by the user or by a software
request. And for the latter part, we only support runtime resume the ODD
when the eject request is received. We did this in sr's block ioctl
function, this looks ugly.

Change this by runtime resuming the ODD in its open function and runtime
suspending it in its release function.

The downside of this approach is that in old distros with old udisk
daemon, the ODD will be polled by udisk daemon so open/close will
constantly be called, which will cause the ODD frequently resume from
suspend state, breaking the effect of power saving. User with such a
distro is advised to issue a udisk command to inhibit polling of the
disk like this:
$ udisks --inhibit-polling /dev/sr0
And since newer kernel has in kernel polling, there is no problem
regarding ODD's event report.

Signed-off-by: Aaron Lu <aaron.lu@amd.com>
---
 drivers/scsi/sr.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
Alan Stern - July 23, 2012, 2:57 p.m.
On Mon, 23 Jul 2012, Aaron Lu wrote:

> The ODD can either be runtime resumed by the user or by a software
> request. And for the latter part, we only support runtime resume the ODD
> when the eject request is received. We did this in sr's block ioctl
> function, this looks ugly.
> 
> Change this by runtime resuming the ODD in its open function and runtime
> suspending it in its release function.
> 
> The downside of this approach is that in old distros with old udisk
> daemon, the ODD will be polled by udisk daemon so open/close will
> constantly be called, which will cause the ODD frequently resume from
> suspend state, breaking the effect of power saving. User with such a
> distro is advised to issue a udisk command to inhibit polling of the
> disk like this:
> $ udisks --inhibit-polling /dev/sr0
> And since newer kernel has in kernel polling, there is no problem
> regarding ODD's event report.
> 
> Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> ---
>  drivers/scsi/sr.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 5f4d19a..f7a7635 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -152,8 +152,15 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
>  	kref_get(&cd->kref);
>  	if (scsi_device_get(cd->device))
>  		goto out_put;
> +	if (pm_runtime_get_sync(&cd->device->sdev_gendev) < 0) {
> +		pm_runtime_put_noidle(&cd->device->sdev_gendev);
> +		goto out_pm;
> +	}

You should use scsi_autopm_get_device instead of bypassing the SCSI 
layer.  Similarly for the _put call.

I know the existing calls do this already.  They shouldn't.

Alan Stern

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

Patch

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 5f4d19a..f7a7635 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -152,8 +152,15 @@  static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 	kref_get(&cd->kref);
 	if (scsi_device_get(cd->device))
 		goto out_put;
+	if (pm_runtime_get_sync(&cd->device->sdev_gendev) < 0) {
+		pm_runtime_put_noidle(&cd->device->sdev_gendev);
+		goto out_pm;
+	}
 	goto out;
 
+ out_pm:
+	scsi_device_put(cd->device);
+
  out_put:
 	kref_put(&cd->kref, sr_kref_release);
 	cd = NULL;
@@ -169,6 +176,8 @@  static void scsi_cd_put(struct scsi_cd *cd)
 	mutex_lock(&sr_ref_mutex);
 	kref_put(&cd->kref, sr_kref_release);
 	scsi_device_put(sdev);
+	pm_runtime_mark_last_busy(&cd->device->sdev_gendev);
+	pm_runtime_put_autosuspend(&cd->device->sdev_gendev);
 	mutex_unlock(&sr_ref_mutex);
 }
 
@@ -654,13 +663,6 @@  static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 	void __user *argp = (void __user *)arg;
 	int ret;
 
-	/* Make sure the ODD is not suspended */
-	ret = pm_runtime_get_sync(&sdev->sdev_gendev);
-	if (ret < 0) {
-		pm_runtime_put_noidle(&sdev->sdev_gendev);
-		return -EACCES;
-	}
-
 	mutex_lock(&sr_mutex);
 
 	/*
@@ -692,8 +694,6 @@  static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 
 out:
 	mutex_unlock(&sr_mutex);
-	pm_runtime_mark_last_busy(&cd->device->sdev_gendev);
-	pm_runtime_put_autosuspend(&cd->device->sdev_gendev);
 	return ret;
 }