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 for Android: free password hash cracker in your pocket

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

Message-ID: <Pine.LNX.4.44L0.1203291050580.1417-100000@iolanthe.rowland.org>
Date:	Thu, 29 Mar 2012 11:00:18 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Aaron Lu <aaron.lu@....com>
cc:	Lin Ming <ming.m.lin@...el.com>, 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, 29 Mar 2012, Aaron Lu wrote:

> >>> @@ -216,6 +244,11 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
> >>>       unsigned int events;
> >>>       int ret;
> >>>
> >>> +     /* Not necessary to check events if enter ZPODD state */
> >>> +     if (cd->device->power_off &&
> >>> +                     pm_runtime_suspended(&cd->device->sdev_gendev))
> >>> +             return 0;
> >>
> >> The comment is wrong and the new code does the wrong thing.  You _do_
> >> have to check for events even in the ZPODD state, which means
> >> sr_check_events must power-up the device if necessary.
> >> sd_check_events in James Bottomley's scsi-misc tree now does the right
> >> thing; see commit 4e2247b2bd289f079349d6c69755f8cff4e31f2b.
> >
> > The problem is userspace(GNOME, for example) will check for events
> > every seconds.
> > If sr is power up so frequently then we lost the expected power
> > savings from ZPODD.

That's true.  There's nothing you can do about it in the kernel; it's 
up to userspace to change the frequency of event polling.

> Agreed.
> BTW, it's udevd on my Linux box that changed the default poll msecs kernel param
> which caused sr_check_events being called every 2 seconds.

Indeed.

> > There are 2 events:
> >
> > DISK_EVENT_MEDIA_CHANGE
> > DISK_EVENT_EJECT_REQUEST
> >
> > In current implementation, if sr is in ZPODD state, then it means
> > there is no disk in the CDROM.
> > So if sr is in ZPODD state, MEDIA_CHANGE would never happen.

Sure it will: when the user inserts a disc.

> > EJECT_REQUEST seems can be ignored, since there is no disk in the CDROM at all.
> >
> 
> For the ODD to be put into suspend state, the conditions should be:
> 1 tray closed
> 2 no media inside
> I think we missed the condition 1 check now.
> 
> And if we follow the two conditions, the events can be safely ignored.

No, they can't.  Otherwise the device won't power back up when the user 
inserts a new disc.

> What do you think of blocking events for it when going to suspend and unblocking
> when resume? This could erase the unnecessary calls of the check events function
> when ODD is suspended.
> But disk_(un)block_events are not exported and can't be used in sr
> module. So I'm
> not sure how to do this.
> 
> 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.

What's the problem?  Can't the user already eject the tray via 
software?

> >>> @@ -260,6 +293,11 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
> >>>       cd->media_present = scsi_status_is_good(ret) ||
> >>>               (scsi_sense_valid(&sshdr) && sshdr.asc != 0x3a);
> >>>
> >>> +     if (!cd->media_present && cd->device->power_off && !cd->poweroff_event) {
> >>> +             scsi_autopm_put_device(cd->device);
> >>
> >> You can see your mistake here.  You call scsi_autopm_put_device here
> >> without calling scsi_autopm_get_device earlier.
> >
> > Let me check this.
> 
> I guess the earlier call of get device is this?
> 
>  869 int scsi_sysfs_add_sdev(struct scsi_device *sdev)
> ... ...
>  891
>  892        /* The following call will keep sdev active indefinitely, until
>  893         * its driver does a corresponding scsi_autopm_pm_device().  Only
>  894         * drivers supporting autosuspend will do this.
>  895         */
>  896        scsi_autopm_get_device(sdev);
>  897

You have to do this correctly.  Currently sr doesn't support runtime PM 
at all.  If you do want to support it, then the driver should call 
scsi_autopm_put_device at the end of the probe routine (or whenever it 
is ready to allow runtime suspends) and scsi_autopm_get_device at the 
start of the remove routine (or whenever it wants to prevent runtime 
suspends).

The idea is that the driver is probed with the PM usage counter set to
1, so a runtime suspend won't occur until you do a put.  Similarly, in
the remove routine, you must make sure that your gets and puts balance
out.

Alan Stern

--
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/