linux-kernel - Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20120406160126.GA4835@localhost.amd.com>
Date:	Sat, 7 Apr 2012 00:01:27 +0800
From:	Aaron Lu <aaron.lu@....com>
To:	Lin Ming <ming.m.lin@...el.com>
CC:	Alan Stern <stern@...land.harvard.edu>,
	Zhang Rui <rui.zhang@...el.com>,
	Jeff Garzik <jgarzik@...ox.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>, Tejun Heo <tj@...nel.org>,
	<linux-kernel@...r.kernel.org>, <linux-ide@...r.kernel.org>,
	<linux-scsi@...r.kernel.org>, <linux-pm@...r.kernel.org>,
	linux-acpi <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH v3 7/7] [SCSI] sr: adds Zero-power ODD support

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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/