| Message ID | 20120405083245.GA19508@localhost.amd.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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; }