On Wednesday, September 12, 2012, Aaron Lu wrote:
> Place the ODD into runtime suspend state as soon as there is nobody
> using it.
OK, so how is ODD related to the sr driver?
> The only exception is, if we just find that a new medium is
> inserted, we wait for the next events checking to idle it.
What exactly do you mean by "to idle it"?
Does this patch have any functional effect without the following patches?
> Based on ideas of Alan Stern and Oliver Neukum.
>
> Signed-off-by: Aaron Lu <aaron...@intel.com>
> ---
> drivers/scsi/sr.c | 29 +++++++++++++++++++++++++----
> 1 file changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 5fc97d2..7a8222f 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -45,6 +45,7 @@
> #include <linux/blkdev.h>
> #include <linux/mutex.h>
> #include <linux/slab.h>
> +#include <linux/pm_runtime.h>
> #include <asm/uaccess.h>
>
> #include <scsi/scsi.h>
> @@ -146,8 +147,12 @@ 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 (scsi_autopm_get_device(cd->device))
> + goto out_pm;
> goto out;
Why don't you do
> + if (!scsi_autopm_get_device(cd->device))
> + goto out;
without the new label?
>
> + out_pm:
> + scsi_device_put(cd->device);
> out_put:
> kref_put(&cd->kref, sr_kref_release);
> cd = NULL;
> @@ -163,6 +168,7 @@ 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);
> + scsi_autopm_put_device(sdev);
> mutex_unlock(&sr_ref_mutex);
> }
>
> @@ -211,7 +217,7 @@ static unsigned int sr_check_events(struct
> cdrom_device_info *cdi,
> unsigned int clearing, int slot)
> {
> struct scsi_cd *cd = cdi->handle;
> - bool last_present;
> + bool last_present = cd->media_present;
> struct scsi_sense_hdr sshdr;
> unsigned int events;
> int ret;
> @@ -220,6 +226,8 @@ static unsigned int sr_check_events(struct
> cdrom_device_info *cdi,
> if (CDSL_CURRENT != slot)
> return 0;
>
> + scsi_autopm_get_device(cd->device);
> +
> events = sr_get_events(cd->device);
> cd->get_event_changed |= events & DISK_EVENT_MEDIA_CHANGE;
>
> @@ -246,10 +254,9 @@ static unsigned int sr_check_events(struct
> cdrom_device_info *cdi,
> }
>
> if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
> - return events;
> + goto out;
> do_tur:
> /* let's see whether the media is there with TUR */
> - last_present = cd->media_present;
> ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
>
> /*
> @@ -270,7 +277,7 @@ do_tur:
> }
>
> if (cd->ignore_get_event)
> - return events;
> + goto out;
>
> /* check whether GET_EVENT is reporting spurious MEDIA_CHANGE */
> if (!cd->tur_changed) {
> @@ -287,6 +294,12 @@ do_tur:
> cd->tur_changed = false;
> cd->get_event_changed = false;
>
> +out:
> + if (cd->media_present && !last_present)
> + pm_runtime_put_noidle(&cd->device->sdev_gendev);
> + else
> + scsi_autopm_put_device(cd->device);
> +
This thing is asking for a comment.
It looks like you're kind of avoiding to call _idle() for the device, but why?
What might go wrong if pm_runtime_put() is used instead of the whole
conditional,
among other things?
> return events;
> }
>
> @@ -715,9 +728,14 @@ static int sr_probe(struct device *dev)
> dev_set_drvdata(dev, cd);
> disk->flags |= GENHD_FL_REMOVABLE;
> add_disk(disk);
> + disk_events_set_poll_msecs(disk, 5000);
Why do you need this and why is the poll interval universally suitable?
>
> sdev_printk(KERN_DEBUG, sdev,
> "Attached scsi CD-ROM %s\n", cd->cdi.name);
> +
> + /* enable runtime pm */
Not really. What it does is to enable the device to be suspended.
> + scsi_autopm_put_device(cd->device);
> +
> return 0;
>
> fail_put:
> @@ -965,6 +983,9 @@ static int sr_remove(struct device *dev)
> {
> struct scsi_cd *cd = dev_get_drvdata(dev);
>
> + /* disable runtime pm */
And that prevents the device from being suspended (which means that it's
going to be resumed at this point in case it was suspended before).
> + scsi_autopm_get_device(cd->device);
> +
> blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn);
> del_gendisk(cd->disk);
>
>
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
