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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF1ivSaAm4pTTKpjipYZxfvCrPhxnpH6WtxpDoaHFhC5kwA5Lw@mail.gmail.com>
Date:	Fri, 6 Apr 2012 14:10:31 +0800
From:	Lin Ming <ming.m.lin@...el.com>
To:	Aaron Lu <aaron.lu@....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

On Thu, Apr 5, 2012 at 4:32 PM, Aaron Lu <aaron.lu@....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-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/