Patchwork [v3,7/7,SCSI] sr: adds Zero-power ODD support

login
register
mail settings
Submitter Aaron Lu
Date April 5, 2012, 8:32 a.m.
Message ID <20120405083245.GA19508@localhost.amd.com>
Download mbox | patch
Permalink /patch/150892/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Aaron Lu - April 5, 2012, 8:32 a.m.
Hi,

On Thu, Apr 05, 2012 at 02:00:57PM +0800, Lin Ming wrote:
> >
> > Another thing to consider is, user might want to eject the tray by
> > software like the
> > eject /dev/sr0 command or some UI mouse clicks against the cdrom icon. I'm still
> > thinking how to do this correctly.
> 
> Assume eject /dev/sr0 is implemented as:
> 
> int fd = open("/dev/sr0", ...)
> ioctl(fd, CDROMEJECT)
> 

Indeed, it is implemented as this :-)

> We may need to resume ODD in the ioctl handler(scsi_cmd_ioctl).
> 

I prefer we do this in sr_block_ioctl.
Suppose the ODD is now runtime suspended and received an ioctl:
if the ioctl's cmd is CDROMEJECT, resume it.
For other cases, return an error code like EPERM.
When done, according to the result of ioctl: if success, leave it resumed;
if failed, put it back to sleep.

Something like this:


Does this work?

-Aaron


--
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
Lin Ming - April 5, 2012, 8:48 a.m.
On Thu, Apr 5, 2012 at 4:32 PM, Aaron Lu <aaron.lu@amd.com> wrote:
> Hi,
>
> On Thu, Apr 05, 2012 at 02:00:57PM +0800, Lin Ming wrote:
>> >
>> > Another thing to consider is, user might want to eject the tray by
>> > software like the
>> > eject /dev/sr0 command or some UI mouse clicks against the cdrom icon. I'm still
>> > thinking how to do this correctly.
>>
>> Assume eject /dev/sr0 is implemented as:
>>
>> int fd = open("/dev/sr0", ...)
>> ioctl(fd, CDROMEJECT)
>>
>
> Indeed, it is implemented as this :-)
>
>> We may need to resume ODD in the ioctl handler(scsi_cmd_ioctl).
>>
>
> I prefer we do this in sr_block_ioctl.

That's better.

> Suppose the ODD is now runtime suspended and received an ioctl:
> if the ioctl's cmd is CDROMEJECT, resume it.
> For other cases, return an error code like EPERM.
> When done, according to the result of ioctl: if success, leave it resumed;
> if failed, put it back to sleep.
>
> Something like this:
>
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 5fc97d2..aa6e920 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>
> @@ -538,10 +539,21 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
>        struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
>        struct scsi_device *sdev = cd->device;
>        void __user *argp = (void __user *)arg;
> -       int ret;
> +       int ret, rpm_resumed = 0;
>
>        mutex_lock(&sr_mutex);
>
> +       if (pm_runtime_suspended(&sdev->sdev_gendev)) {
> +               if (cmd == CDROMEJECT) {
> +                       scsi_autopm_get_device(sdev);
> +                       rpm_resumed = 1;
> +               }
> +               else {
> +                       ret = -EPERM;
> +                       goto out;
> +               }
> +       }
> +
>        /*
>         * Send SCSI addressing ioctls directly to mid level, send other
>         * ioctls to cdrom/block level.
> @@ -570,6 +582,9 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
>        ret = scsi_ioctl(sdev, cmd, argp);
>
>  out:
> +       if (rpm_resumed && ret)
> +               scsi_autopm_put_device(sdev);
> +
>        mutex_unlock(&sr_mutex);
>        return ret;
>  }
>
> Does this work?

I think so.
Will add this patch.

Thanks,
Lin Ming

>
> -Aaron
--
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
Alan Stern - April 5, 2012, 2:03 p.m.
On Thu, 5 Apr 2012, Aaron Lu wrote:

> Hi,
> 
> On Thu, Apr 05, 2012 at 02:00:57PM +0800, Lin Ming wrote:
> > >
> > > Another thing to consider is, user might want to eject the tray by
> > > software like the
> > > eject /dev/sr0 command or some UI mouse clicks against the cdrom icon. I'm still
> > > thinking how to do this correctly.
> > 
> > Assume eject /dev/sr0 is implemented as:
> > 
> > int fd = open("/dev/sr0", ...)
> > ioctl(fd, CDROMEJECT)
> > 
> 
> Indeed, it is implemented as this :-)
> 
> > We may need to resume ODD in the ioctl handler(scsi_cmd_ioctl).
> > 
> 
> I prefer we do this in sr_block_ioctl.
> Suppose the ODD is now runtime suspended and received an ioctl:
> if the ioctl's cmd is CDROMEJECT, resume it.
> For other cases, return an error code like EPERM.
> When done, according to the result of ioctl: if success, leave it resumed;
> if failed, put it back to sleep.

Alternatively, you may want to do the runtime resume in sr_open and 
sr_block_open and the suspend in the corresponding release routines.  
It's up to you.

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
Lin Ming - April 6, 2012, 6:10 a.m.
On Thu, Apr 5, 2012 at 4:32 PM, Aaron Lu <aaron.lu@amd.com> wrote:
> Hi,
>
> On Thu, Apr 05, 2012 at 02:00:57PM +0800, Lin Ming wrote:
>> >
>> > Another thing to consider is, user might want to eject the tray by
>> > software like the
>> > eject /dev/sr0 command or some UI mouse clicks against the cdrom icon. I'm still
>> > thinking how to do this correctly.
>>
>> Assume eject /dev/sr0 is implemented as:
>>
>> int fd = open("/dev/sr0", ...)
>> ioctl(fd, CDROMEJECT)
>>
>
> Indeed, it is implemented as this :-)
>
>> We may need to resume ODD in the ioctl handler(scsi_cmd_ioctl).
>>
>
> I prefer we do this in sr_block_ioctl.
> Suppose the ODD is now runtime suspended and received an ioctl:
> if the ioctl's cmd is CDROMEJECT, resume it.
> For other cases, return an error code like EPERM.
> When done, according to the result of ioctl: if success, leave it resumed;
> if failed, put it back to sleep.
>
> Something like this:
>
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 5fc97d2..aa6e920 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>
> @@ -538,10 +539,21 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
>        struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
>        struct scsi_device *sdev = cd->device;
>        void __user *argp = (void __user *)arg;
> -       int ret;
> +       int ret, rpm_resumed = 0;
>
>        mutex_lock(&sr_mutex);
>
> +       if (pm_runtime_suspended(&sdev->sdev_gendev)) {
> +               if (cmd == CDROMEJECT) {
> +                       scsi_autopm_get_device(sdev);
> +                       rpm_resumed = 1;
> +               }
> +               else {
> +                       ret = -EPERM;
> +                       goto out;
> +               }
> +       }
> +
>        /*
>         * Send SCSI addressing ioctls directly to mid level, send other
>         * ioctls to cdrom/block level.
> @@ -570,6 +582,9 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
>        ret = scsi_ioctl(sdev, cmd, argp);
>
>  out:
> +       if (rpm_resumed && ret)
> +               scsi_autopm_put_device(sdev);
> +
>        mutex_unlock(&sr_mutex);
>        return ret;
>  }
>
> Does this work?

I have tested this patch.
Unfortunately, this does not work.

int fd = open("/dev/sr0", ...)
ioctl(fd, COMMAND)

Actually, the open() code path requires CDROM to be in active state, for example

sr_block_open
  cdrom_open
    open_for_data
       cdo->drive_status
         scsi_test_unit_ready
           scsi_execute_req

I'm thinking 3 solutions.

Solution 1:
sr_block_open() always return failure in ZPODD state.
But "eject /dev/sr0" won't work in this solution.

Solution 2:
sr_block_open() always return success in ZPODD state.
Then we have to insert ODD status check in many entries accessing it.
For example, sr_drive_status(), sr_check_events(),  sr_lock_door() etc.
And only runtime resume ODD for some special case, for example,
CDROMEJECT ioctl.

Solution 3:
Runtime resume ODD in sr_block_open(), as Alan suggested.
But the big problem is userspace will open ODD every seconds, so ODD
is frequently
resumed and we lose the expected power savings from ZPODD.

Any other idea?

Thanks,
Lin Ming
--
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
Lin Ming - April 6, 2012, 6:13 a.m.
On Thu, Apr 5, 2012 at 10:03 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 5 Apr 2012, Aaron Lu wrote:
>
>> Hi,
>>
>> On Thu, Apr 05, 2012 at 02:00:57PM +0800, Lin Ming wrote:
>> > >
>> > > Another thing to consider is, user might want to eject the tray by
>> > > software like the
>> > > eject /dev/sr0 command or some UI mouse clicks against the cdrom icon. I'm still
>> > > thinking how to do this correctly.
>> >
>> > Assume eject /dev/sr0 is implemented as:
>> >
>> > int fd = open("/dev/sr0", ...)
>> > ioctl(fd, CDROMEJECT)
>> >
>>
>> Indeed, it is implemented as this :-)
>>
>> > We may need to resume ODD in the ioctl handler(scsi_cmd_ioctl).
>> >
>>
>> I prefer we do this in sr_block_ioctl.
>> Suppose the ODD is now runtime suspended and received an ioctl:
>> if the ioctl's cmd is CDROMEJECT, resume it.
>> For other cases, return an error code like EPERM.
>> When done, according to the result of ioctl: if success, leave it resumed;
>> if failed, put it back to sleep.
>
> Alternatively, you may want to do the runtime resume in sr_open and
> sr_block_open and the suspend in the corresponding release routines.
> It's up to you.

GNOME opens cdrom every seconds to check status.
So we won't get the desired power savings if we do the runtime resume
in sr_block_open.
See my previous mail.

>
> 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
Aaron Lu - April 6, 2012, 4:01 p.m.
Hi,

On Fri, Apr 06, 2012 at 02:10:31PM +0800, Lin Ming wrote:
> >
> > I prefer we do this in sr_block_ioctl.
> > Suppose the ODD is now runtime suspended and received an ioctl:
> > if the ioctl's cmd is CDROMEJECT, resume it.
> > For other cases, return an error code like EPERM.
> > When done, according to the result of ioctl: if success, leave it resumed;
> > if failed, put it back to sleep.
> >
> > Something like this:
> >
> > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > index 5fc97d2..aa6e920 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>
> > @@ -538,10 +539,21 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
> >        struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
> >        struct scsi_device *sdev = cd->device;
> >        void __user *argp = (void __user *)arg;
> > -       int ret;
> > +       int ret, rpm_resumed = 0;
> >
> >        mutex_lock(&sr_mutex);
> >
> > +       if (pm_runtime_suspended(&sdev->sdev_gendev)) {
> > +               if (cmd == CDROMEJECT) {
> > +                       scsi_autopm_get_device(sdev);
> > +                       rpm_resumed = 1;
> > +               }
> > +               else {
> > +                       ret = -EPERM;
> > +                       goto out;
> > +               }
> > +       }
> > +
> >        /*
> >         * Send SCSI addressing ioctls directly to mid level, send other
> >         * ioctls to cdrom/block level.
> > @@ -570,6 +582,9 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
> >        ret = scsi_ioctl(sdev, cmd, argp);
> >
> >  out:
> > +       if (rpm_resumed && ret)
> > +               scsi_autopm_put_device(sdev);
> > +
> >        mutex_unlock(&sr_mutex);
> >        return ret;
> >  }
> >
> > Does this work?
> 
> I have tested this patch.
> Unfortunately, this does not work.
> 
> int fd = open("/dev/sr0", ...)
> ioctl(fd, COMMAND)
> 
> Actually, the open() code path requires CDROM to be in active state, for example
> 
> sr_block_open
>   cdrom_open
>     open_for_data
>        cdo->drive_status
>          scsi_test_unit_ready
>            scsi_execute_req
> 
Hmm... This is not the case on my Linux box.
eject will use O_NONBLOCK when open, which would results in the
following code path:
sr_block_open
  cdrom_open
    cdi->ops->open(sr_open)
      done

Maybe your eject is old and doesn't use the O_NONBLOCK flag?
If it is the case, then we may need to avoid this solution since we
can't depend on the user space tools.

> I'm thinking 3 solutions.
> 
> Solution 1:
> sr_block_open() always return failure in ZPODD state.
> But "eject /dev/sr0" won't work in this solution.
> 
Right, so, not an option.

> Solution 2:
> sr_block_open() always return success in ZPODD state.
> Then we have to insert ODD status check in many entries accessing it.
> For example, sr_drive_status(), sr_check_events(),  sr_lock_door() etc.
> And only runtime resume ODD for some special case, for example,
> CDROMEJECT ioctl.
Sounds like pretty complicated :-)

> 
> Solution 3:
> Runtime resume ODD in sr_block_open(), as Alan suggested.
> But the big problem is userspace will open ODD every seconds, so ODD
> is frequently
> resumed and we lose the expected power savings from ZPODD.
Yes, the udisks-daemon will constantly open the ODD's block device,
dunno why it would do this, need to check its code later.

Another problem I can think of doing suspend/resume in open/release is,
it's not that easy to determine if we should suspend the ODD in release
function. We may need to do some extra house keeping to achieve this
based on information like if the door is open and/or if there is media
inside, etc.

Thanks,
Aaron

--
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 5fc97d2..aa6e920 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>
@@ -538,10 +539,21 @@  static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
        struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
        struct scsi_device *sdev = cd->device;
        void __user *argp = (void __user *)arg;
-       int ret;
+       int ret, rpm_resumed = 0;
 
        mutex_lock(&sr_mutex);
 
+       if (pm_runtime_suspended(&sdev->sdev_gendev)) {
+               if (cmd == CDROMEJECT) {
+                       scsi_autopm_get_device(sdev);
+                       rpm_resumed = 1;
+               }
+               else {
+                       ret = -EPERM;
+                       goto out;
+               }
+       }
+
        /*
         * Send SCSI addressing ioctls directly to mid level, send other
         * ioctls to cdrom/block level.
@@ -570,6 +582,9 @@  static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
        ret = scsi_ioctl(sdev, cmd, argp);
 
 out:
+       if (rpm_resumed && ret)
+               scsi_autopm_put_device(sdev);
+
        mutex_unlock(&sr_mutex);
        return ret;
 }