Patchwork [v13,1/9] scsi: sr: support runtime pm

login
register
mail settings
Submitter Aaron Lu
Date Jan. 15, 2013, 9:20 a.m.
Message ID <1358241665-2156-2-git-send-email-aaron.lu@intel.com>
Download mbox | patch
Permalink /patch/212048/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Aaron Lu - Jan. 15, 2013, 9:20 a.m.
This patch adds runtime pm support for sr.

It did this by increasing the runtime usage_count of the device when:
- its block device is opened;
- the events checking is to run.

And decreasing the runtime usage_count of the device when:
- its block device is closed;
- After the events checking is done.

The idea is discussed in this mail thread:
http://thread.gmane.org/gmane.linux.acpi.devel/55243/focus=52703

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/scsi/sr.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)
James Bottomley - Jan. 16, 2013, 3:45 p.m.
On Tue, 2013-01-15 at 17:20 +0800, Aaron Lu wrote:
> This patch adds runtime pm support for sr.
> 
> It did this by increasing the runtime usage_count of the device when:
> - its block device is opened;
> - the events checking is to run.
> 
> And decreasing the runtime usage_count of the device when:
> - its block device is closed;
> - After the events checking is done.
> 
> The idea is discussed in this mail thread:
> http://thread.gmane.org/gmane.linux.acpi.devel/55243/focus=52703
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>

I'd like an ack from Alan Stern as well, please, but with that, you can
add my acked-by.

James


--
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 - Jan. 16, 2013, 4:31 p.m.
On Wed, 16 Jan 2013, James Bottomley wrote:

> On Tue, 2013-01-15 at 17:20 +0800, Aaron Lu wrote:
> > This patch adds runtime pm support for sr.
> > 
> > It did this by increasing the runtime usage_count of the device when:
> > - its block device is opened;
> > - the events checking is to run.
> > 
> > And decreasing the runtime usage_count of the device when:
> > - its block device is closed;
> > - After the events checking is done.
> > 
> > The idea is discussed in this mail thread:
> > http://thread.gmane.org/gmane.linux.acpi.devel/55243/focus=52703
> > 
> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> 
> I'd like an ack from Alan Stern as well, please, but with that, you can
> add my acked-by.

Aaron, have you checked whether this patch works okay when you play a
track on an audio-only CD on the computer?  The block interface looks 
okay but I'm not sure about the cdrom_device interface.

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 - Jan. 18, 2013, 7:42 a.m.
On Wed, Jan 16, 2013 at 11:31:31AM -0500, Alan Stern wrote:
> On Wed, 16 Jan 2013, James Bottomley wrote:
> 
> > On Tue, 2013-01-15 at 17:20 +0800, Aaron Lu wrote:
> > > This patch adds runtime pm support for sr.
> > > 
> > > It did this by increasing the runtime usage_count of the device when:
> > > - its block device is opened;
> > > - the events checking is to run.
> > > 
> > > And decreasing the runtime usage_count of the device when:
> > > - its block device is closed;
> > > - After the events checking is done.
> > > 
> > > The idea is discussed in this mail thread:
> > > http://thread.gmane.org/gmane.linux.acpi.devel/55243/focus=52703
> > > 
> > > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > 
> > I'd like an ack from Alan Stern as well, please, but with that, you can
> > add my acked-by.
> 
> Aaron, have you checked whether this patch works okay when you play a
> track on an audio-only CD on the computer?  The block interface looks 
> okay but I'm not sure about the cdrom_device interface.

Just verified it works OK with the whole patchset applied using 2 audio
CDs.

After the ODD has been put into zero power state, insert an audio cd.
ODD will be resumed, gvfs will automatically mount the disc, and a
dialog titled "Audio Disc" asks me what to do. Press OK, the rhythmbox
application will get started. Press the Play button of rhythmbox, songs
will start to play, and runtime_usage is 2. Eject the disc, the ODD will
be put to zero power state some time later.

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
Alan Stern - Jan. 18, 2013, 3:24 p.m.
On Fri, 18 Jan 2013, Aaron Lu wrote:

> > Aaron, have you checked whether this patch works okay when you play a
> > track on an audio-only CD on the computer?  The block interface looks 
> > okay but I'm not sure about the cdrom_device interface.
> 
> Just verified it works OK with the whole patchset applied using 2 audio
> CDs.
> 
> After the ODD has been put into zero power state, insert an audio cd.
> ODD will be resumed, gvfs will automatically mount the disc, and a
> dialog titled "Audio Disc" asks me what to do. Press OK, the rhythmbox
> application will get started. Press the Play button of rhythmbox, songs
> will start to play, and runtime_usage is 2. Eject the disc, the ODD will
> be put to zero power state some time later.

What happens if you use a non-ZP drive?

What happens if you're not running a desktop graphical environment, so
gvfs doesn't mount the disc?  Basically, I'm worried that the drive may
remain suspended after sr_open() returns.

What happens if you use a program other than rhythmbox?  There are (or
used to be) programs which would issue the PLAY AUDIO command and then
exit.  The drive would continue playing even while the device file was
closed.  Do we want to drop support for that kind of behavior?

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 - Jan. 19, 2013, 8:55 a.m.
On 01/18/2013 11:24 PM, Alan Stern wrote:
> On Fri, 18 Jan 2013, Aaron Lu wrote:
>
>>> Aaron, have you checked whether this patch works okay when you play a
>>> track on an audio-only CD on the computer?  The block interface looks
>>> okay but I'm not sure about the cdrom_device interface.
>>
>> Just verified it works OK with the whole patchset applied using 2 audio
>> CDs.
>>
>> After the ODD has been put into zero power state, insert an audio cd.
>> ODD will be resumed, gvfs will automatically mount the disc, and a
>> dialog titled "Audio Disc" asks me what to do. Press OK, the rhythmbox
>> application will get started. Press the Play button of rhythmbox, songs
>> will start to play, and runtime_usage is 2. Eject the disc, the ODD will
>> be put to zero power state some time later.
>
> What happens if you use a non-ZP drive?
>
> What happens if you're not running a desktop graphical environment, so
> gvfs doesn't mount the disc?  Basically, I'm worried that the drive may
> remain suspended after sr_open() returns.

Tried on my notebook with a normal ODD, using mplayer under console
mode, no GUI environment running, works fine.

The usage count will be incremented by the app, and get decreased
only when it exits. So the ODD will not be in runtime suspended state
when the app is running.

>
> What happens if you use a program other than rhythmbox?  There are (or
> used to be) programs which would issue the PLAY AUDIO command and then
> exit.  The drive would continue playing even while the device file was

Then we indeed have a problem. But I didn't find any such app in
Fedora's repo or by searching the internet. But of course such apps can
exist since the Mount Fuji spec defined such a command.

> closed.  Do we want to drop support for that kind of behavior?

I don't think we should drop such support.
And the safest way to avoid such break is we refine the suspend
condition for ODD, and using what ZPODD defined condition isn't that
bad to me:
- for tray type, no media inside and tray close;
- for slot type, no media inside.
While whether tray is closed or not may not be that important, but at
least we should make sure there is no media inside.

Thoughts?

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
Alan Stern - Jan. 19, 2013, 6:46 p.m.
On Sat, 19 Jan 2013, Aaron Lu wrote:

> > What happens if you're not running a desktop graphical environment, so
> > gvfs doesn't mount the disc?  Basically, I'm worried that the drive may
> > remain suspended after sr_open() returns.
> 
> Tried on my notebook with a normal ODD, using mplayer under console
> mode, no GUI environment running, works fine.
> 
> The usage count will be incremented by the app, and get decreased
> only when it exits. So the ODD will not be in runtime suspended state
> when the app is running.

You missed my point.  What happens when sr_open() gets called?  It 
doesn't try to resume the device.  Will we run into trouble then?

> > What happens if you use a program other than rhythmbox?  There are (or
> > used to be) programs which would issue the PLAY AUDIO command and then
> > exit.  The drive would continue playing even while the device file was
> 
> Then we indeed have a problem. But I didn't find any such app in
> Fedora's repo or by searching the internet.

http://rpm.pbone.net/index.php3/stat/4/idpl/2392183/dir/redhat_5.x/com/cdp-0.33-10.i386.rpm.html

> > closed.  Do we want to drop support for that kind of behavior?
> 
> I don't think we should drop such support.
> And the safest way to avoid such break is we refine the suspend
> condition for ODD, and using what ZPODD defined condition isn't that
> bad to me:
> - for tray type, no media inside and tray close;
> - for slot type, no media inside.
> While whether tray is closed or not may not be that important, but at
> least we should make sure there is no media inside.
> 
> Thoughts?

That sounds reasonable to me, at least as a first step.  If people want 
their CD drive to suspend, they can eject the disc.

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
Julian Calaby - Jan. 21, 2013, 3:31 a.m.
Hi Alan,

On Sun, Jan 20, 2013 at 5:46 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, 19 Jan 2013, Aaron Lu wrote:
>> > closed.  Do we want to drop support for that kind of behavior?
>>
>> I don't think we should drop such support.
>> And the safest way to avoid such break is we refine the suspend
>> condition for ODD, and using what ZPODD defined condition isn't that
>> bad to me:
>> - for tray type, no media inside and tray close;
>> - for slot type, no media inside.
>> While whether tray is closed or not may not be that important, but at
>> least we should make sure there is no media inside.
>>
>> Thoughts?
>
> That sounds reasonable to me, at least as a first step.  If people want
> their CD drive to suspend, they can eject the disc.

Stupid question: does the kernel know if a CD has audio tracks?

(I'm assuming that nobody will access a data track without mounting it
or holding the device open)

Thanks,
Aaron Lu - Jan. 21, 2013, 8:04 a.m.
On Sat, Jan 19, 2013 at 01:46:15PM -0500, Alan Stern wrote:
> On Sat, 19 Jan 2013, Aaron Lu wrote:
> 
> > > What happens if you're not running a desktop graphical environment, so
> > > gvfs doesn't mount the disc?  Basically, I'm worried that the drive may
> > > remain suspended after sr_open() returns.
> > 
> > Tried on my notebook with a normal ODD, using mplayer under console
> > mode, no GUI environment running, works fine.
> > 
> > The usage count will be incremented by the app, and get decreased
> > only when it exits. So the ODD will not be in runtime suspended state
> > when the app is running.
> 
> You missed my point.  What happens when sr_open() gets called?  It 
> doesn't try to resume the device.  Will we run into trouble then?

Oh yes, you mean the cdo->open gets called without going through sr's
block interface.

I just checked the code, it looks to me the cdrom interface code is the
code common to cdrom functionality, to be used by various underlying
block drivers like sr, ide-cd, etc. All the code cdrom.c has provided
requires a cdi parameter, which can only be provided by the underlying
block driver, so I don't think those cdrom_device_ops functions will be
called without going through the block drivers' interface.

But the functions defined in block_device_operations by sr can be called
by other kernel components as long as they have access to the gendisk
structure of the block device, so I'll add get/sync pair to all those
functions(i.e. sr_block_revalidate_disk, sr_block_ioctl in addition to
the open/release/check_events calls).

> 
> > > What happens if you use a program other than rhythmbox?  There are (or
> > > used to be) programs which would issue the PLAY AUDIO command and then
> > > exit.  The drive would continue playing even while the device file was
> > 
> > Then we indeed have a problem. But I didn't find any such app in
> > Fedora's repo or by searching the internet.
> 
> http://rpm.pbone.net/index.php3/stat/4/idpl/2392183/dir/redhat_5.x/com/cdp-0.33-10.i386.rpm.html
> 
> > > closed.  Do we want to drop support for that kind of behavior?
> > 
> > I don't think we should drop such support.
> > And the safest way to avoid such break is we refine the suspend
> > condition for ODD, and using what ZPODD defined condition isn't that
> > bad to me:
> > - for tray type, no media inside and tray close;
> > - for slot type, no media inside.
> > While whether tray is closed or not may not be that important, but at
> > least we should make sure there is no media inside.
> > 
> > Thoughts?
> 
> That sounds reasonable to me, at least as a first step.  If people want 
> their CD drive to suspend, they can eject the disc.

I'm gonna add the cd->media_present check in sr's runtime suspend
callback, if sr thinks there is media inside, suspend will not proceed.

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
Aaron Lu - Jan. 21, 2013, 8:14 a.m.
On Mon, Jan 21, 2013 at 02:31:50PM +1100, Julian Calaby wrote:
> Hi Alan,
> 
> On Sun, Jan 20, 2013 at 5:46 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Sat, 19 Jan 2013, Aaron Lu wrote:
> >> > closed.  Do we want to drop support for that kind of behavior?
> >>
> >> I don't think we should drop such support.
> >> And the safest way to avoid such break is we refine the suspend
> >> condition for ODD, and using what ZPODD defined condition isn't that
> >> bad to me:
> >> - for tray type, no media inside and tray close;
> >> - for slot type, no media inside.
> >> While whether tray is closed or not may not be that important, but at
> >> least we should make sure there is no media inside.
> >>
> >> Thoughts?
> >
> > That sounds reasonable to me, at least as a first step.  If people want
> > their CD drive to suspend, they can eject the disc.
> 
> Stupid question: does the kernel know if a CD has audio tracks?

Yes, cdrom module knows that, but the block driver(e.g. sr) doesn't.
See cdrom_count_tracks in cdrom.c.

May I know if you have an use case that you want to runtime suspend the
cd drive with a disc inside? That would help us to refine the runtime
suspend condition.

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
Julian Calaby - Jan. 21, 2013, 8:55 a.m.
Hi Aaron,

On Mon, Jan 21, 2013 at 7:14 PM, Aaron Lu <aaron.lu@intel.com> wrote:
> On Mon, Jan 21, 2013 at 02:31:50PM +1100, Julian Calaby wrote:
>> Hi Alan,
>>
>> On Sun, Jan 20, 2013 at 5:46 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Sat, 19 Jan 2013, Aaron Lu wrote:
>> >> > closed.  Do we want to drop support for that kind of behavior?
>> >>
>> >> I don't think we should drop such support.
>> >> And the safest way to avoid such break is we refine the suspend
>> >> condition for ODD, and using what ZPODD defined condition isn't that
>> >> bad to me:
>> >> - for tray type, no media inside and tray close;
>> >> - for slot type, no media inside.
>> >> While whether tray is closed or not may not be that important, but at
>> >> least we should make sure there is no media inside.
>> >>
>> >> Thoughts?
>> >
>> > That sounds reasonable to me, at least as a first step.  If people want
>> > their CD drive to suspend, they can eject the disc.
>>
>> Stupid question: does the kernel know if a CD has audio tracks?
>
> Yes, cdrom module knows that, but the block driver(e.g. sr) doesn't.
> See cdrom_count_tracks in cdrom.c.
>
> May I know if you have an use case that you want to runtime suspend the
> cd drive with a disc inside? That would help us to refine the runtime
> suspend condition.

The discussion seemed to be restricting when the drive would be
suspended to only occasions where the drive is empty and, where
applicable, closed. I'll admit that I hadn't followed the discussion
enough to know what the restrictions were before that, but this seemed
to be a further restriction, therefore I assumed that you were
originally planning to be able to suspend the drive with a disk
inside.

I asked my question in the hope of someone setting up the compromise:
"we could suspend the drive only if the drive is empty and closed, or
a data disc is inside and nobody's using it."

Personally, providing nobody's using it, and relevant state is stored,
I can't see any reason why you can't suspend a drive with a disc in
it.

Thanks,
Aaron Lu - Jan. 21, 2013, 9:11 a.m.
Hi Julian,

On Mon, Jan 21, 2013 at 07:55:20PM +1100, Julian Calaby wrote:
> Hi Aaron,
> 
> On Mon, Jan 21, 2013 at 7:14 PM, Aaron Lu <aaron.lu@intel.com> wrote:
> > On Mon, Jan 21, 2013 at 02:31:50PM +1100, Julian Calaby wrote:
> >> Hi Alan,
> >>
> >> On Sun, Jan 20, 2013 at 5:46 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> > On Sat, 19 Jan 2013, Aaron Lu wrote:
> >> >> > closed.  Do we want to drop support for that kind of behavior?
> >> >>
> >> >> I don't think we should drop such support.
> >> >> And the safest way to avoid such break is we refine the suspend
> >> >> condition for ODD, and using what ZPODD defined condition isn't that
> >> >> bad to me:
> >> >> - for tray type, no media inside and tray close;
> >> >> - for slot type, no media inside.
> >> >> While whether tray is closed or not may not be that important, but at
> >> >> least we should make sure there is no media inside.
> >> >>
> >> >> Thoughts?
> >> >
> >> > That sounds reasonable to me, at least as a first step.  If people want
> >> > their CD drive to suspend, they can eject the disc.
> >>
> >> Stupid question: does the kernel know if a CD has audio tracks?
> >
> > Yes, cdrom module knows that, but the block driver(e.g. sr) doesn't.
> > See cdrom_count_tracks in cdrom.c.
> >
> > May I know if you have an use case that you want to runtime suspend the
> > cd drive with a disc inside? That would help us to refine the runtime
> > suspend condition.
> 
> The discussion seemed to be restricting when the drive would be
> suspended to only occasions where the drive is empty and, where
> applicable, closed. I'll admit that I hadn't followed the discussion
> enough to know what the restrictions were before that, but this seemed
> to be a further restriction, therefore I assumed that you were
> originally planning to be able to suspend the drive with a disk
> inside.

Right, that was the original implementation to allow the cd drive to be
runtime suspended even with a disc inside.

> 
> I asked my question in the hope of someone setting up the compromise:
> "we could suspend the drive only if the drive is empty and closed, or
> a data disc is inside and nobody's using it."
> 
> Personally, providing nobody's using it, and relevant state is stored,
> I can't see any reason why you can't suspend a drive with a disc in
> it.

It is not easy for the OS to tell if the drive is being used or not
sometimes :-)

Alan has reminded me it is possible for an app to open the block device
file(/dev/sr0), issue a command(play audio), then close the device file.
From the OS' point of view, we think nobody is using it. But actually,
the drive is playing cd for the user, so we can't suspend the device.

I think we can always refine the condition check, but as a first step, I
want to be safe and avoid breaking existing functionalities.

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
Aaron Lu - Jan. 21, 2013, 1:36 p.m.
On 01/20/2013 02:46 AM, Alan Stern wrote:
> On Sat, 19 Jan 2013, Aaron Lu wrote:
>> Then we indeed have a problem. But I didn't find any such app in
>> Fedora's repo or by searching the internet.
>
> http://rpm.pbone.net/index.php3/stat/4/idpl/2392183/dir/redhat_5.x/com/cdp-0.33-10.i386.rpm.html

Now that with the RFC PATCH, the cd drive's runtime status will always
be active if there is disc inside, so we don't have this problem any
more. Tested on two systems, one with a ZP-drive and the other with a
normal one.

BTW, I can't get the above app run, as it requires an old ncurses lib.
And compiling it is not easy...

For an amusement, I wrote the following stupid simple app intended for
test:

#include <sys/types.h>
#include <sys/stat.h>
#include <sys/ioctl.h>
#include <fcntl.h>
#include <linux/cdrom.h>

int main(void)
{
	int ret = 0;
	struct cdrom_msf msf = { 1, 0, 0, 2, 0, 0 };
	int fd;
	
	fd = open("/dev/sr0", O_RDONLY | O_NONBLOCK);
	if (fd == -1) {
		perror("open");
		return -1;
	}

	ret = ioctl(fd, CDROMPLAYMSF, &msf);
	if (ret) {
		perror("ioctl");
		goto out;
	}
out:
	close(fd);

	return ret;
}

Insert an audio disc, make sure no app is accessing it, run the above
app, then the app will exit and the led on the drive will be blinking
for roughly one minute, as the app has set, but I didn't hear any sound...

And the runtime status is of course active all the time, no matter if
there is app accessing it or not as expected.

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
Oliver Neukum - Jan. 21, 2013, 2:56 p.m.
On Monday 21 January 2013 17:11:04 Aaron Lu wrote:
> It is not easy for the OS to tell if the drive is being used or not
> sometimes 
> 
> Alan has reminded me it is possible for an app to open the block device
> file(/dev/sr0), issue a command(play audio), then close the device file.
> From the OS' point of view, we think nobody is using it. But actually,
> the drive is playing cd for the user, so we can't suspend the device.

Are there drives that support ZPODD and have an audio output?

	Regards
		Oliver

--
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 - Jan. 22, 2013, 2:25 a.m.
On Mon, Jan 21, 2013 at 03:56:43PM +0100, Oliver Neukum wrote:
> On Monday 21 January 2013 17:11:04 Aaron Lu wrote:
> > It is not easy for the OS to tell if the drive is being used or not
> > sometimes 
> > 
> > Alan has reminded me it is possible for an app to open the block device
> > file(/dev/sr0), issue a command(play audio), then close the device file.
> > From the OS' point of view, we think nobody is using it. But actually,
> > the drive is playing cd for the user, so we can't suspend the device.
> 
> Are there drives that support ZPODD and have an audio output?

I'm afraid I don't know, since there are so many ODD makers.
But at least we can say, the SPEC doesn't forbid it.

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
Oliver Neukum - Jan. 22, 2013, 9:13 a.m.
On Tuesday 22 January 2013 10:25:31 Aaron Lu wrote:
> On Mon, Jan 21, 2013 at 03:56:43PM +0100, Oliver Neukum wrote:
> > On Monday 21 January 2013 17:11:04 Aaron Lu wrote:
> > > It is not easy for the OS to tell if the drive is being used or not
> > > sometimes 
> > > 
> > > Alan has reminded me it is possible for an app to open the block device
> > > file(/dev/sr0), issue a command(play audio), then close the device file.
> > > From the OS' point of view, we think nobody is using it. But actually,
> > > the drive is playing cd for the user, so we can't suspend the device.
> > 
> > Are there drives that support ZPODD and have an audio output?
> 
> I'm afraid I don't know, since there are so many ODD makers.
> But at least we can say, the SPEC doesn't forbid it.

Well, then we have to handle it.

	Regards
		Oliver

--
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 - Jan. 22, 2013, 9:20 a.m.
On 01/22/2013 05:13 PM, Oliver Neukum wrote:
> On Tuesday 22 January 2013 10:25:31 Aaron Lu wrote:
>> On Mon, Jan 21, 2013 at 03:56:43PM +0100, Oliver Neukum wrote:
>>> On Monday 21 January 2013 17:11:04 Aaron Lu wrote:
>>>> It is not easy for the OS to tell if the drive is being used or not
>>>> sometimes 
>>>>
>>>> Alan has reminded me it is possible for an app to open the block device
>>>> file(/dev/sr0), issue a command(play audio), then close the device file.
>>>> From the OS' point of view, we think nobody is using it. But actually,
>>>> the drive is playing cd for the user, so we can't suspend the device.
>>>
>>> Are there drives that support ZPODD and have an audio output?
>>
>> I'm afraid I don't know, since there are so many ODD makers.
>> But at least we can say, the SPEC doesn't forbid it.
> 
> Well, then we have to handle it.

Yes, and the way we handle it is by checking the cd->media_present: if
it is true, we will not allow runtime suspend as shown in the RFC patch.
http://marc.info/?l=linux-scsi&m=135876099800714&w=2

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..4d1a610 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>
@@ -146,7 +147,8 @@  static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 	kref_get(&cd->kref);
 	if (scsi_device_get(cd->device))
 		goto out_put;
-	goto out;
+	if (!scsi_autopm_get_device(cd->device))
+		goto out;
 
  out_put:
 	kref_put(&cd->kref, sr_kref_release);
@@ -162,6 +164,7 @@  static void scsi_cd_put(struct scsi_cd *cd)
 
 	mutex_lock(&sr_ref_mutex);
 	kref_put(&cd->kref, sr_kref_release);
+	scsi_autopm_put_device(sdev);
 	scsi_device_put(sdev);
 	mutex_unlock(&sr_ref_mutex);
 }
@@ -211,7 +214,7 @@  static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 				    unsigned int clearing, int slot)
 {
 	struct scsi_cd *cd = cdi->handle;
-	bool last_present;
+	bool last_present = cd->media_present;
 	struct scsi_sense_hdr sshdr;
 	unsigned int events;
 	int ret;
@@ -220,6 +223,8 @@  static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	if (CDSL_CURRENT != slot)
 		return 0;
 
+	scsi_autopm_get_device(cd->device);
+
 	events = sr_get_events(cd->device);
 	cd->get_event_changed |= events & DISK_EVENT_MEDIA_CHANGE;
 
@@ -246,10 +251,9 @@  static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	}
 
 	if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
-		return events;
+		goto out;
 do_tur:
 	/* let's see whether the media is there with TUR */
-	last_present = cd->media_present;
 	ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
 
 	/*
@@ -270,7 +274,7 @@  do_tur:
 	}
 
 	if (cd->ignore_get_event)
-		return events;
+		goto out;
 
 	/* check whether GET_EVENT is reporting spurious MEDIA_CHANGE */
 	if (!cd->tur_changed) {
@@ -287,6 +291,18 @@  do_tur:
 	cd->tur_changed = false;
 	cd->get_event_changed = false;
 
+out:
+	/*
+	 * If there is no medium detected or the medium has been there
+	 * since last poll, try to suspend the device. Otherwise, keep
+	 * it active for one more poll interval so that if user space
+	 * application opens the block device, we can avoid a runtime
+	 * status change.
+	 */
+	pm_runtime_put_noidle(&cd->device->sdev_gendev);
+	if (!cd->media_present || last_present)
+		pm_runtime_suspend(&cd->device->sdev_gendev);
+
 	return events;
 }
 
@@ -718,6 +734,8 @@  static int sr_probe(struct device *dev)
 
 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
+	scsi_autopm_put_device(cd->device);
+
 	return 0;
 
 fail_put:
@@ -965,6 +983,8 @@  static int sr_remove(struct device *dev)
 {
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 
+	scsi_autopm_get_device(cd->device);
+
 	blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn);
 	del_gendisk(cd->disk);