Re: [PATCH v13 1/9] scsi: sr: support runtime pm
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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 1/9] scsi: sr: support runtime pm
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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 1/9] scsi: sr: support runtime pm
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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 1/9] scsi: sr: support runtime pm
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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 1/9] scsi: sr: support runtime pm
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
#include
#include
#include
#include
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-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 1/9] scsi: sr: support runtime pm
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 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 > >> 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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 1/9] scsi: sr: support runtime pm
Hi Aaron, On Mon, Jan 21, 2013 at 7:14 PM, Aaron Lu 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 >> 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, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/ .Plan: http://sites.google.com/site/juliancalaby/ -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 1/9] scsi: sr: support runtime pm
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 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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 1/9] scsi: sr: support runtime pm
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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 1/9] scsi: sr: support runtime pm
Hi Alan, On Sun, Jan 20, 2013 at 5:46 AM, Alan Stern 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, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/ .Plan: http://sites.google.com/site/juliancalaby/ -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 1/9] scsi: sr: support runtime pm
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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 1/9] scsi: sr: support runtime pm
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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 1/9] scsi: sr: support runtime pm
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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 1/9] scsi: sr: support runtime pm
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 > > > > 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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 1/9] scsi: sr: support runtime pm
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 > > 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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 1/9] scsi: sr: support runtime pm
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 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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
