2017-12-14 15:14:13

by Michal Suchánek

Subject: [PATCH 0/6] Fix cdrom autoclose

Hello,

there is cdrom autoclose feature that is supposed to close the tray, wait for
the disc to become ready, and then open the device.

This used to work in ancient times. Then in old times there was a hack in
util-linux which worked around the breakage which probably resulted from
switching to scsi emulation.

Currently util-linux maintainer refuses to merge another hack on the basis that
kernel still has the feature so it should be fixed there. Indeed, to implement
this feature effectively from userspace one would need to know when the CD-ROM
is in the "drive becoming ready" state which is knowledge that never leaves the
hardware-specific driver and is passed neither to userspace nor the generic
cdrom driver.

So this patchset fixes the kernel autoclose implementation in cdrom.c and to
do so reports the "drive becoming ready" state from the harware specific
drivers.

Michal Suchanek (6):
delay: add poll_event_interruptible
cdrom: factor out common open_for_* code
cdrom: wait for tray to close
cdrom: introduce CDS_DRIVE_ERROR
Documentetion: cdrom: introduce CDS_DRIVE_ERROR
cdrom: wait for drive to become ready

Documentation/cdrom/cdrom-standard.tex | 8 ++-
Documentation/cdrom/ide-cd | 6 ++
Documentation/ioctl/cdrom.txt | 1 +
drivers/block/paride/pcd.c | 2 +-
drivers/cdrom/cdrom.c | 124 ++++++++++++++++-----------------
drivers/cdrom/gdrom.c | 2 +-
drivers/ide/ide-cd_ioctl.c | 12 ++--
drivers/scsi/sr_ioctl.c | 2 +-
include/linux/delay.h | 12 ++++
include/uapi/linux/cdrom.h | 1 +
10 files changed, 99 insertions(+), 71 deletions(-)

--
2.13.6

2017-12-14 15:14:42

by Michal Suchánek

Subject: [PATCH 3/6] cdrom: wait for tray to close

The scsi command to close tray only starts the motor and does not wait
for the tray to close. Wait until the state chages from TRAY_OPEN so
users do not race with the tray closing.

This looks like inifinte wait but unless the drive is broken it either
closes the tray within a few seconds or reports an error when it detects
the tray is blocked. At worst the wait can be interrupted by user.

Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/cdrom/cdrom.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index e976d3d0180d..040d3d466cd7 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -281,7 +281,9 @@
#include <linux/fcntl.h>
#include <linux/blkdev.h>
#include <linux/times.h>
+#include <linux/delay.h>
#include <linux/uaccess.h>
+#include <linux/sched/signal.h>
#include <scsi/scsi_request.h>

/* used to tell the module to turn on full debugging messages */
@@ -1030,6 +1032,18 @@ static void cdrom_count_tracks(struct cdrom_device_info *cdi, tracktype *tracks)
tracks->cdi, tracks->xa);
}

+static int tray_close(struct cdrom_device_info *cdi)
+{
+ int ret;
+
+ ret = cdi->ops->tray_move(cdi, 0);
+ if (ret)
+ return ret;
+
+ return poll_event_interruptible(CDS_TRAY_OPEN !=
+ cdi->ops->drive_status(cdi, CDSL_CURRENT), 500);
+}
+
static
int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
{
@@ -1048,7 +1062,9 @@ int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
if (CDROM_CAN(CDC_CLOSE_TRAY) &&
cdi->options & CDO_AUTO_CLOSE) {
cd_dbg(CD_OPEN, "trying to close the tray\n");
- ret = cdo->tray_move(cdi, 0);
+ ret = tray_close(cdi);
+ if (ret == -ERESTARTSYS)
+ return ret;
if (ret) {
cd_dbg(CD_OPEN, "bummer. tried to close the tray but failed.\n");
/* Ignore the error from the low
@@ -2312,7 +2328,8 @@ static int cdrom_ioctl_closetray(struct cdrom_device_info *cdi)

if (!CDROM_CAN(CDC_CLOSE_TRAY))
return -ENOSYS;
- return cdi->ops->tray_move(cdi, 0);
+
+ return tray_close(cdi);
}

static int cdrom_ioctl_eject_sw(struct cdrom_device_info *cdi,
--
2.13.6

2017-12-14 15:14:41

by Michal Suchánek

Subject: [PATCH 4/6] cdrom: introduce CDS_DRIVE_ERROR

CDS_DRIVE_NOT_READY is used for the state in which CDROM is 'becoming
ready' (typically analyzing the disc) but also as the fallback when
nothing else applies. Introduce CDS_DRIVE_ERROR for the fallback case.

Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/block/paride/pcd.c | 2 +-
drivers/cdrom/gdrom.c | 2 +-
drivers/ide/ide-cd_ioctl.c | 12 ++++++++----
drivers/scsi/sr_ioctl.c | 2 +-
include/uapi/linux/cdrom.h | 1 +
5 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c
index 7b8c6368beb7..6e00093ff34e 100644
--- a/drivers/block/paride/pcd.c
+++ b/drivers/block/paride/pcd.c
@@ -605,7 +605,7 @@ static int pcd_drive_status(struct cdrom_device_info *cdi, int slot_nr)
struct pcd_unit *cd = cdi->handle;

if (pcd_ready_wait(cd, PCD_READY_TMO))
- return CDS_DRIVE_NOT_READY;
+ return CDS_DRIVE_ERROR;
if (pcd_atapi(cd, rc_cmd, 8, pcd_scratch, DBMSG("check media")))
return CDS_NO_DISC;
return CDS_DISC_OK;
diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
index 6495b03f576c..702f255bbe42 100644
--- a/drivers/cdrom/gdrom.c
+++ b/drivers/cdrom/gdrom.c
@@ -390,7 +390,7 @@ static int gdrom_drivestatus(struct cdrom_device_info *cd_info, int ignore)
if (sense == 0)
return CDS_DISC_OK;
if (sense == 0x20)
- return CDS_DRIVE_NOT_READY;
+ return CDS_DRIVE_ERROR;
/* default */
return CDS_NO_INFO;
}
diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
index 2acca12b9c94..9a26f50a2092 100644
--- a/drivers/ide/ide-cd_ioctl.c
+++ b/drivers/ide/ide-cd_ioctl.c
@@ -62,9 +62,13 @@ int ide_cdrom_drive_status(struct cdrom_device_info *cdi, int slot_nr)
return CDS_NO_DISC;
}

- if (sense.sense_key == NOT_READY && sense.asc == 0x04
- && sense.ascq == 0x04)
- return CDS_DISC_OK;
+ if (sense.sense_key == NOT_READY && sense.asc == 0x04)
+ switch (sense.ascq) {
+ case 0x01:
+ return CDS_DRIVE_NOT_READY;
+ case 0x04:
+ return CDS_DISC_OK;
+ }

/*
* If not using Mt Fuji extended media tray reports,
@@ -77,7 +81,7 @@ int ide_cdrom_drive_status(struct cdrom_device_info *cdi, int slot_nr)
else
return CDS_TRAY_OPEN;
}
- return CDS_DRIVE_NOT_READY;
+ return CDS_DRIVE_ERROR;
}

/*
diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
index 2a21f2d48592..7c93f12a9cb8 100644
--- a/drivers/scsi/sr_ioctl.c
+++ b/drivers/scsi/sr_ioctl.c
@@ -333,7 +333,7 @@ int sr_drive_status(struct cdrom_device_info *cdi, int slot)
else
return CDS_TRAY_OPEN;

- return CDS_DRIVE_NOT_READY;
+ return CDS_DRIVE_ERROR;
}

int sr_disk_status(struct cdrom_device_info *cdi)
diff --git a/include/uapi/linux/cdrom.h b/include/uapi/linux/cdrom.h
index 2817230148fd..339b1435f44e 100644
--- a/include/uapi/linux/cdrom.h
+++ b/include/uapi/linux/cdrom.h
@@ -398,6 +398,7 @@ struct cdrom_generic_command
#define CDS_TRAY_OPEN 2
#define CDS_DRIVE_NOT_READY 3
#define CDS_DISC_OK 4
+#define CDS_DRIVE_ERROR 5

/* return values for the CDROM_DISC_STATUS ioctl */
/* can also return CDS_NO_[INFO|DISC], from above */
--
2.13.6

2017-12-14 15:14:39

by Michal Suchánek

Subject: [PATCH 2/6] cdrom: factor out common open_for_* code

The open_for_audio and open_for_data copies are bitrotten in different
ways already and will need to update the autoclose logic in both.

Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/cdrom/cdrom.c | 100 ++++++++++++++++++--------------------------------
1 file changed, 36 insertions(+), 64 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index e36d160c458f..e976d3d0180d 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -1031,12 +1031,12 @@ static void cdrom_count_tracks(struct cdrom_device_info *cdi, tracktype *tracks)
}

static
-int open_for_data(struct cdrom_device_info *cdi)
+int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
{
int ret;
const struct cdrom_device_ops *cdo = cdi->ops;
- tracktype tracks;
- cd_dbg(CD_OPEN, "entering open_for_data\n");
+
+ cd_dbg(CD_OPEN, "entering " __func__ "\n");
/* Check if the driver can report drive status. If it can, we
can do clever things. If it can't, well, we at least tried! */
if (cdo->drive_status != NULL) {
@@ -1048,7 +1048,7 @@ int open_for_data(struct cdrom_device_info *cdi)
if (CDROM_CAN(CDC_CLOSE_TRAY) &&
cdi->options & CDO_AUTO_CLOSE) {
cd_dbg(CD_OPEN, "trying to close the tray\n");
- ret=cdo->tray_move(cdi,0);
+ ret = cdo->tray_move(cdi, 0);
if (ret) {
cd_dbg(CD_OPEN, "bummer. tried to close the tray but failed.\n");
/* Ignore the error from the low
@@ -1056,37 +1056,45 @@ int open_for_data(struct cdrom_device_info *cdi)
couldn't close the tray. We only care
that there is no disc in the drive,
since that is the _REAL_ problem here.*/
- ret=-ENOMEDIUM;
- goto clean_up_and_return;
+ return -ENOMEDIUM;
}
} else {
cd_dbg(CD_OPEN, "bummer. this drive can't close the tray.\n");
- ret=-ENOMEDIUM;
- goto clean_up_and_return;
+ return -ENOMEDIUM;
}
/* Ok, the door should be closed now.. Check again */
ret = cdo->drive_status(cdi, CDSL_CURRENT);
- if ((ret == CDS_NO_DISC) || (ret==CDS_TRAY_OPEN)) {
+ if ((ret == CDS_NO_DISC) || (ret == CDS_TRAY_OPEN)) {
cd_dbg(CD_OPEN, "bummer. the tray is still not closed.\n");
cd_dbg(CD_OPEN, "tray might not contain a medium\n");
- ret=-ENOMEDIUM;
- goto clean_up_and_return;
+ return -ENOMEDIUM;
}
cd_dbg(CD_OPEN, "the tray is now closed\n");
}
- /* the door should be closed now, check for the disc */
- ret = cdo->drive_status(cdi, CDSL_CURRENT);
- if (ret!=CDS_DISC_OK) {
- ret = -ENOMEDIUM;
- goto clean_up_and_return;
- }
+ if (ret != CDS_DISC_OK)
+ return -ENOMEDIUM;
}
- cdrom_count_tracks(cdi, &tracks);
- if (tracks.error == CDS_NO_DISC) {
+ cdrom_count_tracks(cdi, tracks);
+ if (tracks->error == CDS_NO_DISC) {
cd_dbg(CD_OPEN, "bummer. no disc.\n");
- ret=-ENOMEDIUM;
- goto clean_up_and_return;
+ return -ENOMEDIUM;
}
+
+ return 0;
+}
+
+static
+int open_for_data(struct cdrom_device_info *cdi)
+{
+ int ret;
+ const struct cdrom_device_ops *cdo = cdi->ops;
+ tracktype tracks;
+
+ cd_dbg(CD_OPEN, "entering " __func__ "\n");
+ ret = open_for_common(cdi, &tracks);
+ if (ret)
+ goto clean_up_and_return;
+
/* CD-Players which don't use O_NONBLOCK, workman
* for example, need bit CDO_CHECK_TYPE cleared! */
if (tracks.data==0) {
@@ -1196,53 +1204,17 @@ int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev,
/* This code is similar to that in open_for_data. The routine is called
whenever an audio play operation is requested.
*/
-static int check_for_audio_disc(struct cdrom_device_info *cdi,
- const struct cdrom_device_ops *cdo)
+static int check_for_audio_disc(struct cdrom_device_info *cdi)
{
int ret;
tracktype tracks;
cd_dbg(CD_OPEN, "entering check_for_audio_disc\n");
if (!(cdi->options & CDO_CHECK_TYPE))
return 0;
- if (cdo->drive_status != NULL) {
- ret = cdo->drive_status(cdi, CDSL_CURRENT);
- cd_dbg(CD_OPEN, "drive_status=%d\n", ret);
- if (ret == CDS_TRAY_OPEN) {
- cd_dbg(CD_OPEN, "the tray is open...\n");
- /* can/may i close it? */
- if (CDROM_CAN(CDC_CLOSE_TRAY) &&
- cdi->options & CDO_AUTO_CLOSE) {
- cd_dbg(CD_OPEN, "trying to close the tray\n");
- ret=cdo->tray_move(cdi,0);
- if (ret) {
- cd_dbg(CD_OPEN, "bummer. tried to close tray but failed.\n");
- /* Ignore the error from the low
- level driver. We don't care why it
- couldn't close the tray. We only care
- that there is no disc in the drive,
- since that is the _REAL_ problem here.*/
- return -ENOMEDIUM;
- }
- } else {
- cd_dbg(CD_OPEN, "bummer. this driver can't close the tray.\n");
- return -ENOMEDIUM;
- }
- /* Ok, the door should be closed now.. Check again */
- ret = cdo->drive_status(cdi, CDSL_CURRENT);
- if ((ret == CDS_NO_DISC) || (ret==CDS_TRAY_OPEN)) {
- cd_dbg(CD_OPEN, "bummer. the tray is still not closed.\n");
- return -ENOMEDIUM;
- }
- if (ret!=CDS_DISC_OK) {
- cd_dbg(CD_OPEN, "bummer. disc isn't ready.\n");
- return -EIO;
- }
- cd_dbg(CD_OPEN, "the tray is now closed\n");
- }
- }
- cdrom_count_tracks(cdi, &tracks);
- if (tracks.error)
- return(tracks.error);
+
+ ret = open_for_common(cdi, &tracks);
+ if (ret)
+ return ret;

if (tracks.audio==0)
return -EMEDIUMTYPE;
@@ -2710,7 +2682,7 @@ static int cdrom_ioctl_play_trkind(struct cdrom_device_info *cdi,
if (copy_from_user(&ti, argp, sizeof(ti)))
return -EFAULT;

- ret = check_for_audio_disc(cdi, cdi->ops);
+ ret = check_for_audio_disc(cdi);
if (ret)
return ret;
return cdi->ops->audio_ioctl(cdi, CDROMPLAYTRKIND, &ti);
@@ -2758,7 +2730,7 @@ static int cdrom_ioctl_audioctl(struct cdrom_device_info *cdi,

if (!CDROM_CAN(CDC_PLAY_AUDIO))
return -ENOSYS;
- ret = check_for_audio_disc(cdi, cdi->ops);
+ ret = check_for_audio_disc(cdi);
if (ret)
return ret;
return cdi->ops->audio_ioctl(cdi, cmd, NULL);
--
2.13.6

2017-12-14 15:15:21

by Michal Suchánek

Subject: [PATCH 6/6] cdrom: wait for drive to become ready

When the drive closes it can take tens of seconds until the disc is
analyzed. Wait for the drive to become ready or report an error.

Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/cdrom/cdrom.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 040d3d466cd7..a483f34b7648 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -1087,6 +1087,15 @@ int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
}
cd_dbg(CD_OPEN, "the tray is now closed\n");
}
+ /* the door should be closed now, check for the disc */
+ if (ret == CDS_DRIVE_NOT_READY) {
+ int poll_res = poll_event_interruptible(
+ CDS_DRIVE_NOT_READY !=
+ (ret = cdo->drive_status(cdi, CDSL_CURRENT)),
+ 500);
+ if (poll_res == -ERESTARTSYS)
+ return poll_res;
+ }
if (ret != CDS_DISC_OK)
return -ENOMEDIUM;
}
--
2.13.6

2017-12-14 15:15:20

by Michal Suchánek

Subject: [PATCH 5/6] Documentetion: cdrom: introduce CDS_DRIVE_ERROR

CDS_DRIVE_NOT_READY is used for the state in which CDROM is 'becoming
ready' (typically analyzing the disc) but also as the fallback when
nothing else applies. Introduce CDS_DRIVE_ERROR for the fallback case.

Signed-off-by: Michal Suchanek <[email protected]>
---
Documentation/cdrom/cdrom-standard.tex | 8 +++++++-
Documentation/cdrom/ide-cd | 6 ++++++
Documentation/ioctl/cdrom.txt | 1 +
3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/Documentation/cdrom/cdrom-standard.tex b/Documentation/cdrom/cdrom-standard.tex
index 8f85b0e41046..018284ba696a 100644
--- a/Documentation/cdrom/cdrom-standard.tex
+++ b/Documentation/cdrom/cdrom-standard.tex
@@ -371,11 +371,17 @@ $$
CDS_NO_INFO& no information available\cr
CDS_NO_DISC& no disc is inserted, tray is closed\cr
CDS_TRAY_OPEN& tray is opened\cr
-CDS_DRIVE_NOT_READY& something is wrong, tray is moving?\cr
+CDS_DRIVE_NOT_READY& tray just closed?\cr
CDS_DISC_OK& a disc is loaded and everything is fine\cr
+CDS_DRIVE_ERROR& something is wrong\cr
}
$$

+Note: The IDE and SCSI cdroms have a status code 'drive becoming ready' which
+is typically returned when the drive has just closed and is analyzing the disc.
+For other cdrom types this state is not reported by the hardware or not
+implemented by the driver.
+
\subsection{$Int\ media_changed(struct\ cdrom_device_info * cdi, int\ disc_nr)$}

This function is very similar to the original function in $struct\
diff --git a/Documentation/cdrom/ide-cd b/Documentation/cdrom/ide-cd
index a5f2a7f1ff46..9324a8fd9a39 100644
--- a/Documentation/cdrom/ide-cd
+++ b/Documentation/cdrom/ide-cd
@@ -455,6 +455,9 @@ main (int argc, char **argv)
case CDS_DRIVE_NOT_READY:
printf ("Drive Not Ready.\n");
break;
+ case CDS_DRIVE_ERROR:
+ printf ("Drive problem.\n");
+ break;
default:
printf ("This Should not happen!\n");
break;
@@ -481,6 +484,9 @@ main (int argc, char **argv)
case CDS_NO_INFO:
printf ("No Information available.");
break;
+ case CDS_DRIVE_ERROR:
+ printf ("Drive problem.\n");
+ break;
default:
printf ("This Should not happen!\n");
break;
diff --git a/Documentation/ioctl/cdrom.txt b/Documentation/ioctl/cdrom.txt
index a4d62a9d6771..7720d11807c3 100644
--- a/Documentation/ioctl/cdrom.txt
+++ b/Documentation/ioctl/cdrom.txt
@@ -700,6 +700,7 @@ CDROM_DRIVE_STATUS Get tray position, etc.
CDS_TRAY_OPEN
CDS_DRIVE_NOT_READY
CDS_DISC_OK
+ CDS_DRIVE_ERROR
-1 error

error returns:
--
2.13.6

2017-12-14 15:14:30

by Michal Suchánek

Subject: [PATCH 1/6] delay: add poll_event_interruptible

Add convenience macro for polling an event that does not have a
waitqueue.

Signed-off-by: Michal Suchanek <[email protected]>
---
include/linux/delay.h | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/include/linux/delay.h b/include/linux/delay.h
index b78bab4395d8..3ae9fa395628 100644
--- a/include/linux/delay.h
+++ b/include/linux/delay.h
@@ -64,4 +64,16 @@ static inline void ssleep(unsigned int seconds)
msleep(seconds * 1000);
}

+#define poll_event_interruptible(event, interval) ({ \
+ int ret = 0; \
+ while (!(event)) { \
+ if (signal_pending(current)) { \
+ ret = -ERESTARTSYS; \
+ break; \
+ } \
+ msleep_interruptible(interval); \
+ } \
+ ret; \
+})
+
#endif /* defined(_LINUX_DELAY_H) */
--
2.13.6

2017-12-15 17:20:11

by Michal Suchánek

Subject: Re: [PATCH 3/6] cdrom: wait for tray to close

On Thu, 14 Dec 2017 16:13:52 +0100
Michal Suchanek <[email protected]> wrote:

> @@ -2312,7 +2328,8 @@ static int cdrom_ioctl_closetray(struct
> cdrom_device_info *cdi)
> if (!CDROM_CAN(CDC_CLOSE_TRAY))
> return -ENOSYS;
> - return cdi->ops->tray_move(cdi, 0);
> +
> + return tray_close(cdi);

This change should be skipped. drive_status is optional and tray_close
relies on it. In open_for_* functions tray_close is called based on
reading the status so it's ok there.

Thanks

Michal

2018-01-26 19:51:40

by Michal Suchánek

Subject: [PATCH resend 0/6] Fix cdrom autoclose

Hello,

there is cdrom autoclose feature that is supposed to close the tray, wait for
the disc to become ready, and then open the device.

This used to work in ancient times. Then in old times there was a hack in
util-linux which worked around the breakage which probably resulted from
switching to scsi emulation.

Currently util-linux maintainer refuses to merge another hack on the basis that
kernel still has the feature so it should be fixed there. Indeed, to implement
this feature effectively from userspace one would need to know when the CD-ROM
is in the "drive becoming ready" state which is knowledge that never leaves the
hardware-specific driver and is passed neither to userspace nor the generic
cdrom driver.

So this patchset fixes the kernel autoclose implementation in cdrom.c and to
do so reports the "drive becoming ready" state from the harware specific
drivers.

First time I did not get any feedback for the patches. I found a defect in
tray_close - it used status function without checking it exists. So resending
with the defect corrected.

Michal Suchanek (6):
delay: add poll_event_interruptible
cdrom: factor out common open_for_* code
cdrom: wait for tray to close
cdrom: introduce CDS_DRIVE_ERROR
Documentetion: cdrom: introduce CDS_DRIVE_ERROR
cdrom: wait for drive to become ready

Documentation/cdrom/cdrom-standard.tex | 8 ++-
Documentation/cdrom/ide-cd | 6 ++
Documentation/ioctl/cdrom.txt | 1 +
drivers/block/paride/pcd.c | 2 +-
drivers/cdrom/cdrom.c | 124 ++++++++++++++++-----------------
drivers/cdrom/gdrom.c | 2 +-
drivers/ide/ide-cd_ioctl.c | 12 ++--
drivers/scsi/sr_ioctl.c | 2 +-
include/linux/delay.h | 12 ++++
include/uapi/linux/cdrom.h | 1 +
10 files changed, 99 insertions(+), 71 deletions(-)

--
2.13.6

2018-01-26 20:06:36

by James Bottomley

Subject: Re: [PATCH resend 0/6] Fix cdrom autoclose

On Fri, 2018-01-26 at 17:58 +0100, Michal Suchanek wrote:
> First time I did not get any feedback for the patches.

This is likely because no-one who might inspect the code saw the
patches ... what list are they going to?  I'm on the block, scsi and
ide mailing lists and I only saw a doc patch the last time.

James

2018-01-27 05:01:21

by Michal Suchánek

Subject: [PATCH resend 2/6] cdrom: factor out common open_for_* code

The open_for_audio and open_for_data copies are bitrotten in different
ways already and will need to update the autoclose logic in both.

Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/cdrom/cdrom.c | 100 ++++++++++++++++++--------------------------------
1 file changed, 36 insertions(+), 64 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index e36d160c458f..89746b3d193f 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -1031,12 +1031,12 @@ static void cdrom_count_tracks(struct cdrom_device_info *cdi, tracktype *tracks)
}

static
-int open_for_data(struct cdrom_device_info *cdi)
+int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
{
int ret;
const struct cdrom_device_ops *cdo = cdi->ops;
- tracktype tracks;
- cd_dbg(CD_OPEN, "entering open_for_data\n");
+
+ cd_dbg(CD_OPEN, "entering open_for_common\n");
/* Check if the driver can report drive status. If it can, we
can do clever things. If it can't, well, we at least tried! */
if (cdo->drive_status != NULL) {
@@ -1048,7 +1048,7 @@ int open_for_data(struct cdrom_device_info *cdi)
if (CDROM_CAN(CDC_CLOSE_TRAY) &&
cdi->options & CDO_AUTO_CLOSE) {
cd_dbg(CD_OPEN, "trying to close the tray\n");
- ret=cdo->tray_move(cdi,0);
+ ret = cdo->tray_move(cdi, 0);
if (ret) {
cd_dbg(CD_OPEN, "bummer. tried to close the tray but failed.\n");
/* Ignore the error from the low
@@ -1056,37 +1056,45 @@ int open_for_data(struct cdrom_device_info *cdi)
couldn't close the tray. We only care
that there is no disc in the drive,
since that is the _REAL_ problem here.*/
- ret=-ENOMEDIUM;
- goto clean_up_and_return;
+ return -ENOMEDIUM;
}
} else {
cd_dbg(CD_OPEN, "bummer. this drive can't close the tray.\n");
- ret=-ENOMEDIUM;
- goto clean_up_and_return;
+ return -ENOMEDIUM;
}
/* Ok, the door should be closed now.. Check again */
ret = cdo->drive_status(cdi, CDSL_CURRENT);
- if ((ret == CDS_NO_DISC) || (ret==CDS_TRAY_OPEN)) {
+ if ((ret == CDS_NO_DISC) || (ret == CDS_TRAY_OPEN)) {
cd_dbg(CD_OPEN, "bummer. the tray is still not closed.\n");
cd_dbg(CD_OPEN, "tray might not contain a medium\n");
- ret=-ENOMEDIUM;
- goto clean_up_and_return;
+ return -ENOMEDIUM;
}
cd_dbg(CD_OPEN, "the tray is now closed\n");
}
- /* the door should be closed now, check for the disc */
- ret = cdo->drive_status(cdi, CDSL_CURRENT);
- if (ret!=CDS_DISC_OK) {
- ret = -ENOMEDIUM;
- goto clean_up_and_return;
- }
+ if (ret != CDS_DISC_OK)
+ return -ENOMEDIUM;
}
- cdrom_count_tracks(cdi, &tracks);
- if (tracks.error == CDS_NO_DISC) {
+ cdrom_count_tracks(cdi, tracks);
+ if (tracks->error == CDS_NO_DISC) {
cd_dbg(CD_OPEN, "bummer. no disc.\n");
- ret=-ENOMEDIUM;
- goto clean_up_and_return;
+ return -ENOMEDIUM;
}
+
+ return 0;
+}
+
+static
+int open_for_data(struct cdrom_device_info *cdi)
+{
+ int ret;
+ const struct cdrom_device_ops *cdo = cdi->ops;
+ tracktype tracks;
+
+ cd_dbg(CD_OPEN, "entering open_for_data\n");
+ ret = open_for_common(cdi, &tracks);
+ if (ret)
+ goto clean_up_and_return;
+
/* CD-Players which don't use O_NONBLOCK, workman
* for example, need bit CDO_CHECK_TYPE cleared! */
if (tracks.data==0) {
@@ -1196,53 +1204,17 @@ int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev,
/* This code is similar to that in open_for_data. The routine is called
whenever an audio play operation is requested.
*/
-static int check_for_audio_disc(struct cdrom_device_info *cdi,
- const struct cdrom_device_ops *cdo)
+static int check_for_audio_disc(struct cdrom_device_info *cdi)
{
int ret;
tracktype tracks;
cd_dbg(CD_OPEN, "entering check_for_audio_disc\n");
if (!(cdi->options & CDO_CHECK_TYPE))
return 0;
- if (cdo->drive_status != NULL) {
- ret = cdo->drive_status(cdi, CDSL_CURRENT);
- cd_dbg(CD_OPEN, "drive_status=%d\n", ret);
- if (ret == CDS_TRAY_OPEN) {
- cd_dbg(CD_OPEN, "the tray is open...\n");
- /* can/may i close it? */
- if (CDROM_CAN(CDC_CLOSE_TRAY) &&
- cdi->options & CDO_AUTO_CLOSE) {
- cd_dbg(CD_OPEN, "trying to close the tray\n");
- ret=cdo->tray_move(cdi,0);
- if (ret) {
- cd_dbg(CD_OPEN, "bummer. tried to close tray but failed.\n");
- /* Ignore the error from the low
- level driver. We don't care why it
- couldn't close the tray. We only care
- that there is no disc in the drive,
- since that is the _REAL_ problem here.*/
- return -ENOMEDIUM;
- }
- } else {
- cd_dbg(CD_OPEN, "bummer. this driver can't close the tray.\n");
- return -ENOMEDIUM;
- }
- /* Ok, the door should be closed now.. Check again */
- ret = cdo->drive_status(cdi, CDSL_CURRENT);
- if ((ret == CDS_NO_DISC) || (ret==CDS_TRAY_OPEN)) {
- cd_dbg(CD_OPEN, "bummer. the tray is still not closed.\n");
- return -ENOMEDIUM;
- }
- if (ret!=CDS_DISC_OK) {
- cd_dbg(CD_OPEN, "bummer. disc isn't ready.\n");
- return -EIO;
- }
- cd_dbg(CD_OPEN, "the tray is now closed\n");
- }
- }
- cdrom_count_tracks(cdi, &tracks);
- if (tracks.error)
- return(tracks.error);
+
+ ret = open_for_common(cdi, &tracks);
+ if (ret)
+ return ret;

if (tracks.audio==0)
return -EMEDIUMTYPE;
@@ -2710,7 +2682,7 @@ static int cdrom_ioctl_play_trkind(struct cdrom_device_info *cdi,
if (copy_from_user(&ti, argp, sizeof(ti)))
return -EFAULT;

- ret = check_for_audio_disc(cdi, cdi->ops);
+ ret = check_for_audio_disc(cdi);
if (ret)
return ret;
return cdi->ops->audio_ioctl(cdi, CDROMPLAYTRKIND, &ti);
@@ -2758,7 +2730,7 @@ static int cdrom_ioctl_audioctl(struct cdrom_device_info *cdi,

if (!CDROM_CAN(CDC_PLAY_AUDIO))
return -ENOSYS;
- ret = check_for_audio_disc(cdi, cdi->ops);
+ ret = check_for_audio_disc(cdi);
if (ret)
return ret;
return cdi->ops->audio_ioctl(cdi, cmd, NULL);
--
2.13.6

2018-01-27 12:49:09

by Michal Suchánek

Subject: [PATCH resend 6/6] cdrom: wait for drive to become ready

When the drive closes it can take tens of seconds until the disc is
analyzed. Wait for the drive to become ready or report an error.

Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/cdrom/cdrom.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 69e85c902373..9994441f5041 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -1087,6 +1087,15 @@ int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
}
cd_dbg(CD_OPEN, "the tray is now closed\n");
}
+ /* the door should be closed now, check for the disc */
+ if (ret == CDS_DRIVE_NOT_READY) {
+ int poll_res = poll_event_interruptible(
+ CDS_DRIVE_NOT_READY !=
+ (ret = cdo->drive_status(cdi, CDSL_CURRENT)),
+ 500);
+ if (poll_res == -ERESTARTSYS)
+ return poll_res;
+ }
if (ret != CDS_DISC_OK)
return -ENOMEDIUM;
}
--
2.13.6

2018-01-27 13:55:15

by Michal Suchánek

Subject: [PATCH resend 4/6] cdrom: introduce CDS_DRIVE_ERROR

CDS_DRIVE_NOT_READY is used for the state in which CDROM is 'becoming
ready' (typically analyzing the disc) but also as the fallback when
nothing else applies. Introduce CDS_DRIVE_ERROR for the fallback case.

Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/block/paride/pcd.c | 2 +-
drivers/cdrom/gdrom.c | 2 +-
drivers/ide/ide-cd_ioctl.c | 12 ++++++++----
drivers/scsi/sr_ioctl.c | 2 +-
include/uapi/linux/cdrom.h | 1 +
5 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c
index 7b8c6368beb7..6e00093ff34e 100644
--- a/drivers/block/paride/pcd.c
+++ b/drivers/block/paride/pcd.c
@@ -605,7 +605,7 @@ static int pcd_drive_status(struct cdrom_device_info *cdi, int slot_nr)
struct pcd_unit *cd = cdi->handle;

if (pcd_ready_wait(cd, PCD_READY_TMO))
- return CDS_DRIVE_NOT_READY;
+ return CDS_DRIVE_ERROR;
if (pcd_atapi(cd, rc_cmd, 8, pcd_scratch, DBMSG("check media")))
return CDS_NO_DISC;
return CDS_DISC_OK;
diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
index 6495b03f576c..702f255bbe42 100644
--- a/drivers/cdrom/gdrom.c
+++ b/drivers/cdrom/gdrom.c
@@ -390,7 +390,7 @@ static int gdrom_drivestatus(struct cdrom_device_info *cd_info, int ignore)
if (sense == 0)
return CDS_DISC_OK;
if (sense == 0x20)
- return CDS_DRIVE_NOT_READY;
+ return CDS_DRIVE_ERROR;
/* default */
return CDS_NO_INFO;
}
diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
index 2acca12b9c94..9a26f50a2092 100644
--- a/drivers/ide/ide-cd_ioctl.c
+++ b/drivers/ide/ide-cd_ioctl.c
@@ -62,9 +62,13 @@ int ide_cdrom_drive_status(struct cdrom_device_info *cdi, int slot_nr)
return CDS_NO_DISC;
}

- if (sense.sense_key == NOT_READY && sense.asc == 0x04
- && sense.ascq == 0x04)
- return CDS_DISC_OK;
+ if (sense.sense_key == NOT_READY && sense.asc == 0x04)
+ switch (sense.ascq) {
+ case 0x01:
+ return CDS_DRIVE_NOT_READY;
+ case 0x04:
+ return CDS_DISC_OK;
+ }

/*
* If not using Mt Fuji extended media tray reports,
@@ -77,7 +81,7 @@ int ide_cdrom_drive_status(struct cdrom_device_info *cdi, int slot_nr)
else
return CDS_TRAY_OPEN;
}
- return CDS_DRIVE_NOT_READY;
+ return CDS_DRIVE_ERROR;
}

/*
diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
index 2a21f2d48592..7c93f12a9cb8 100644
--- a/drivers/scsi/sr_ioctl.c
+++ b/drivers/scsi/sr_ioctl.c
@@ -333,7 +333,7 @@ int sr_drive_status(struct cdrom_device_info *cdi, int slot)
else
return CDS_TRAY_OPEN;

- return CDS_DRIVE_NOT_READY;
+ return CDS_DRIVE_ERROR;
}

int sr_disk_status(struct cdrom_device_info *cdi)
diff --git a/include/uapi/linux/cdrom.h b/include/uapi/linux/cdrom.h
index 2817230148fd..339b1435f44e 100644
--- a/include/uapi/linux/cdrom.h
+++ b/include/uapi/linux/cdrom.h
@@ -398,6 +398,7 @@ struct cdrom_generic_command
#define CDS_TRAY_OPEN 2
#define CDS_DRIVE_NOT_READY 3
#define CDS_DISC_OK 4
+#define CDS_DRIVE_ERROR 5

/* return values for the CDROM_DISC_STATUS ioctl */
/* can also return CDS_NO_[INFO|DISC], from above */
--
2.13.6

2018-01-27 15:49:16

by Michal Suchánek

Subject: [PATCH resend 3/6] cdrom: wait for tray to close

The scsi command to close tray only starts the motor and does not wait
for the tray to close. Wait until the state chages from TRAY_OPEN so
users do not race with the tray closing.

This looks like inifinte wait but unless the drive is broken it either
closes the tray within a few seconds or reports an error when it detects
the tray is blocked. At worst the wait can be interrupted by user.

Signed-off-by: Michal Suchanek <[email protected]>
---
v2:
- check drive_status exists before using it
- rename tray_close -> cdrom_tray_close
---
drivers/cdrom/cdrom.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 89746b3d193f..69e85c902373 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -281,7 +281,9 @@
#include <linux/fcntl.h>
#include <linux/blkdev.h>
#include <linux/times.h>
+#include <linux/delay.h>
#include <linux/uaccess.h>
+#include <linux/sched/signal.h>
#include <scsi/scsi_request.h>

/* used to tell the module to turn on full debugging messages */
@@ -1030,6 +1032,18 @@ static void cdrom_count_tracks(struct cdrom_device_info *cdi, tracktype *tracks)
tracks->cdi, tracks->xa);
}

+static int cdrom_tray_close(struct cdrom_device_info *cdi)
+{
+ int ret;
+
+ ret = cdi->ops->tray_move(cdi, 0);
+ if (ret || !cdi->ops->drive_status)
+ return ret;
+
+ return poll_event_interruptible(CDS_TRAY_OPEN !=
+ cdi->ops->drive_status(cdi, CDSL_CURRENT), 500);
+}
+
static
int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
{
@@ -1048,7 +1062,9 @@ int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
if (CDROM_CAN(CDC_CLOSE_TRAY) &&
cdi->options & CDO_AUTO_CLOSE) {
cd_dbg(CD_OPEN, "trying to close the tray\n");
- ret = cdo->tray_move(cdi, 0);
+ ret = cdrom_tray_close(cdi);
+ if (ret == -ERESTARTSYS)
+ return ret;
if (ret) {
cd_dbg(CD_OPEN, "bummer. tried to close the tray but failed.\n");
/* Ignore the error from the low
@@ -2312,7 +2328,8 @@ static int cdrom_ioctl_closetray(struct cdrom_device_info *cdi)

if (!CDROM_CAN(CDC_CLOSE_TRAY))
return -ENOSYS;
- return cdi->ops->tray_move(cdi, 0);
+
+ return cdrom_tray_close(cdi);
}

static int cdrom_ioctl_eject_sw(struct cdrom_device_info *cdi,
--
2.13.6

2018-01-27 16:46:13

by Michal Suchánek

Subject: [PATCH resend 1/6] delay: add poll_event_interruptible

Add convenience macro for polling an event that does not have a
waitqueue.

Signed-off-by: Michal Suchanek <[email protected]>
---
include/linux/delay.h | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/include/linux/delay.h b/include/linux/delay.h
index b78bab4395d8..3ae9fa395628 100644
--- a/include/linux/delay.h
+++ b/include/linux/delay.h
@@ -64,4 +64,16 @@ static inline void ssleep(unsigned int seconds)
msleep(seconds * 1000);
}

+#define poll_event_interruptible(event, interval) ({ \
+ int ret = 0; \
+ while (!(event)) { \
+ if (signal_pending(current)) { \
+ ret = -ERESTARTSYS; \
+ break; \
+ } \
+ msleep_interruptible(interval); \
+ } \
+ ret; \
+})
+
#endif /* defined(_LINUX_DELAY_H) */
--
2.13.6

2018-01-27 18:59:07

by Michal Suchánek

Subject: [PATCH resend 5/6] Documentetion: cdrom: introduce CDS_DRIVE_ERROR

CDS_DRIVE_NOT_READY is used for the state in which CDROM is 'becoming
ready' (typically analyzing the disc) but also as the fallback when
nothing else applies. Introduce CDS_DRIVE_ERROR for the fallback case.

Signed-off-by: Michal Suchanek <[email protected]>
---
Documentation/cdrom/cdrom-standard.tex | 8 +++++++-
Documentation/cdrom/ide-cd | 6 ++++++
Documentation/ioctl/cdrom.txt | 1 +
3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/Documentation/cdrom/cdrom-standard.tex b/Documentation/cdrom/cdrom-standard.tex
index 8f85b0e41046..018284ba696a 100644
--- a/Documentation/cdrom/cdrom-standard.tex
+++ b/Documentation/cdrom/cdrom-standard.tex
@@ -371,11 +371,17 @@ $$
CDS_NO_INFO& no information available\cr
CDS_NO_DISC& no disc is inserted, tray is closed\cr
CDS_TRAY_OPEN& tray is opened\cr
-CDS_DRIVE_NOT_READY& something is wrong, tray is moving?\cr
+CDS_DRIVE_NOT_READY& tray just closed?\cr
CDS_DISC_OK& a disc is loaded and everything is fine\cr
+CDS_DRIVE_ERROR& something is wrong\cr
}
$$

+Note: The IDE and SCSI cdroms have a status code 'drive becoming ready' which
+is typically returned when the drive has just closed and is analyzing the disc.
+For other cdrom types this state is not reported by the hardware or not
+implemented by the driver.
+
\subsection{$Int\ media_changed(struct\ cdrom_device_info * cdi, int\ disc_nr)$}

This function is very similar to the original function in $struct\
diff --git a/Documentation/cdrom/ide-cd b/Documentation/cdrom/ide-cd
index a5f2a7f1ff46..9324a8fd9a39 100644
--- a/Documentation/cdrom/ide-cd
+++ b/Documentation/cdrom/ide-cd
@@ -455,6 +455,9 @@ main (int argc, char **argv)
case CDS_DRIVE_NOT_READY:
printf ("Drive Not Ready.\n");
break;
+ case CDS_DRIVE_ERROR:
+ printf ("Drive problem.\n");
+ break;
default:
printf ("This Should not happen!\n");
break;
@@ -481,6 +484,9 @@ main (int argc, char **argv)
case CDS_NO_INFO:
printf ("No Information available.");
break;
+ case CDS_DRIVE_ERROR:
+ printf ("Drive problem.\n");
+ break;
default:
printf ("This Should not happen!\n");
break;
diff --git a/Documentation/ioctl/cdrom.txt b/Documentation/ioctl/cdrom.txt
index a4d62a9d6771..7720d11807c3 100644
--- a/Documentation/ioctl/cdrom.txt
+++ b/Documentation/ioctl/cdrom.txt
@@ -700,6 +700,7 @@ CDROM_DRIVE_STATUS Get tray position, etc.
CDS_TRAY_OPEN
CDS_DRIVE_NOT_READY
CDS_DISC_OK
+ CDS_DRIVE_ERROR
-1 error

error returns:
--
2.13.6

2018-01-27 23:01:30

by Michal Suchánek

Subject: Re: [PATCH resend 0/6] Fix cdrom autoclose

On Fri, 26 Jan 2018 12:04:56 -0800
James Bottomley <[email protected]> wrote:

> On Fri, 2018-01-26 at 17:58 +0100, Michal Suchanek wrote:
> > First time I did not get any feedback for the patches.
>
> This is likely because no-one who might inspect the code saw the
> patches ... what list are they going to?  I'm on the block, scsi and
> ide mailing lists and I only saw a doc patch the last time.

This is what the MAINTAINERS file says:

UNIFORM CDROM DRIVER
M: Jens Axboe <[email protected]>
W: http://www.kernel.dk
S: Maintained
F: Documentation/cdrom/
F: drivers/cdrom/cdrom.c
F: include/linux/cdrom.h
F: include/uapi/linux/cdrom.h

Maybe adding some mainling lists there would be useful?

Thanks

Michal

2018-01-29 17:01:10

by Bart Van Assche

Subject: Re: [PATCH resend 1/6] delay: add poll_event_interruptible

On Fri, 2018-01-26 at 17:58 +0100, Michal Suchanek wrote:
> Add convenience macro for polling an event that does not have a
> waitqueue.
>
> Signed-off-by: Michal Suchanek <[email protected]>
> ---
> include/linux/delay.h | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/include/linux/delay.h b/include/linux/delay.h
> index b78bab4395d8..3ae9fa395628 100644
> --- a/include/linux/delay.h
> +++ b/include/linux/delay.h
> @@ -64,4 +64,16 @@ static inline void ssleep(unsigned int seconds)
> msleep(seconds * 1000);
> }
>
> +#define poll_event_interruptible(event, interval) ({ \
> + int ret = 0; \
> + while (!(event)) { \
> + if (signal_pending(current)) { \
> + ret = -ERESTARTSYS; \
> + break; \
> + } \
> + msleep_interruptible(interval); \
> + } \
> + ret; \
> +})
> +
> #endif /* defined(_LINUX_DELAY_H) */

Sorry but I'm not sure we should encourage other kernel developers to use
busy-waiting by adding the poll_event_interruptible() macro to a system-wide
header file. Can that macro be moved into a CDROM-specific .c or .h file?

Thanks,

Bart.

2018-01-29 17:03:58

by Bart Van Assche

Subject: Re: [PATCH resend 2/6] cdrom: factor out common open_for_* code

On Fri, 2018-01-26 at 17:58 +0100, Michal Suchanek wrote:
> - ret=cdo->tray_move(cdi,0);
> + ret = cdo->tray_move(cdi, 0);

Please separate whitespace-only changes from functional changes such that
this patch series becomes easier to review.

Thanks,

Bart.

2018-01-29 17:08:08

by Bart Van Assche

Subject: Re: [PATCH resend 3/6] cdrom: wait for tray to close

On Fri, 2018-01-26 at 17:58 +0100, Michal Suchanek wrote:
> +static int cdrom_tray_close(struct cdrom_device_info *cdi)
> +{
> + int ret;
> +
> + ret = cdi->ops->tray_move(cdi, 0);
> + if (ret || !cdi->ops->drive_status)
> + return ret;
> +
> + return poll_event_interruptible(CDS_TRAY_OPEN !=
> + cdi->ops->drive_status(cdi, CDSL_CURRENT), 500);
> +}
> +
> static
> int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
> {
> @@ -1048,7 +1062,9 @@ int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
> if (CDROM_CAN(CDC_CLOSE_TRAY) &&
> cdi->options & CDO_AUTO_CLOSE) {
> cd_dbg(CD_OPEN, "trying to close the tray\n");
> - ret = cdo->tray_move(cdi, 0);
> + ret = cdrom_tray_close(cdi);
> + if (ret == -ERESTARTSYS)
> + return ret;
> if (ret) {
> cd_dbg(CD_OPEN, "bummer. tried to close the tray but failed.\n");
> /* Ignore the error from the low
> @@ -2312,7 +2328,8 @@ static int cdrom_ioctl_closetray(struct cdrom_device_info *cdi)
>
> if (!CDROM_CAN(CDC_CLOSE_TRAY))
> return -ENOSYS;
> - return cdi->ops->tray_move(cdi, 0);
> +
> + return cdrom_tray_close(cdi);
> }

So this patch changes code that does not wait into code that potentially waits
forever? Sorry but I don't think that's ideal. Please make sure that after a
certain time (a few seconds?) the loop finishes.

Thanks,

Bart.

2018-01-29 17:12:12

by Bart Van Assche

Subject: Re: [PATCH resend 6/6] cdrom: wait for drive to become ready

On Fri, 2018-01-26 at 17:58 +0100, Michal Suchanek wrote:
> When the drive closes it can take tens of seconds until the disc is
> analyzed. Wait for the drive to become ready or report an error.
>
> Signed-off-by: Michal Suchanek <[email protected]>
> ---
> drivers/cdrom/cdrom.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 69e85c902373..9994441f5041 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -1087,6 +1087,15 @@ int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
> }
> cd_dbg(CD_OPEN, "the tray is now closed\n");
> }
> + /* the door should be closed now, check for the disc */
> + if (ret == CDS_DRIVE_NOT_READY) {
> + int poll_res = poll_event_interruptible(
> + CDS_DRIVE_NOT_READY !=
> + (ret = cdo->drive_status(cdi, CDSL_CURRENT)),
> + 500);
> + if (poll_res == -ERESTARTSYS)
> + return poll_res;
> + }
> if (ret != CDS_DISC_OK)
> return -ENOMEDIUM;
> }

Same comment here as for a previous patch: although interruptible by a signal,
I'm not sure potentially infinite loops inside the kernel are really welcome.

Thanks,

Bart.

2018-01-31 18:22:41

by Michal Suchánek

Subject: Re: [PATCH resend 3/6] cdrom: wait for tray to close

On Mon, 29 Jan 2018 17:05:47 +0000
Bart Van Assche <[email protected]> wrote:

> On Fri, 2018-01-26 at 17:58 +0100, Michal Suchanek wrote:
> > +static int cdrom_tray_close(struct cdrom_device_info *cdi)
> > +{
> > + int ret;
> > +
> > + ret = cdi->ops->tray_move(cdi, 0);
> > + if (ret || !cdi->ops->drive_status)
> > + return ret;
> > +
> > + return poll_event_interruptible(CDS_TRAY_OPEN !=
> > + cdi->ops->drive_status(cdi, CDSL_CURRENT),
> > 500); +}
> > +
> > static
> > int open_for_common(struct cdrom_device_info *cdi, tracktype
> > *tracks) {
> > @@ -1048,7 +1062,9 @@ int open_for_common(struct cdrom_device_info
> > *cdi, tracktype *tracks) if (CDROM_CAN(CDC_CLOSE_TRAY) &&
> > cdi->options & CDO_AUTO_CLOSE) {
> > cd_dbg(CD_OPEN, "trying to close
> > the tray\n");
> > - ret = cdo->tray_move(cdi, 0);
> > + ret = cdrom_tray_close(cdi);
> > + if (ret == -ERESTARTSYS)
> > + return ret;
> > if (ret) {
> > cd_dbg(CD_OPEN, "bummer.
> > tried to close the tray but failed.\n"); /* Ignore the error from
> > the low @@ -2312,7 +2328,8 @@ static int
> > cdrom_ioctl_closetray(struct cdrom_device_info *cdi)
> > if (!CDROM_CAN(CDC_CLOSE_TRAY))
> > return -ENOSYS;
> > - return cdi->ops->tray_move(cdi, 0);
> > +
> > + return cdrom_tray_close(cdi);
> > }
>
> So this patch changes code that does not wait into code that
> potentially waits forever? Sorry but I don't think that's ideal.
> Please make sure that after a certain time (a few seconds?) the loop
> finishes.

The problem is that few seconds does not cut it. We are waiting for a
mechanical tray or CD changer to move. On non-broken hardware the tray
either closes or an error is reported within a few seconds. For the
timeout to not race with the event we are waiting for it must be much
longer, though.

Also note that this code is only invoked when the user specifically
requested that the tray gets closed automatically which is off by
default.

Thanks

Michal

2019-10-23 16:41:41

by Michal Suchánek

Subject: Re: [PATCH resend 3/6] cdrom: wait for tray to close

On Mon, Jan 29, 2018 at 05:05:47PM +0000, Bart Van Assche wrote:
> On Fri, 2018-01-26 at 17:58 +0100, Michal Suchanek wrote:
> > +static int cdrom_tray_close(struct cdrom_device_info *cdi)
> > +{
> > + int ret;
> > +
> > + ret = cdi->ops->tray_move(cdi, 0);
> > + if (ret || !cdi->ops->drive_status)
> > + return ret;
> > +
> > + return poll_event_interruptible(CDS_TRAY_OPEN !=
> > + cdi->ops->drive_status(cdi, CDSL_CURRENT), 500);
> > +}
> > +
> > static
> > int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
> > {
> > @@ -1048,7 +1062,9 @@ int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks)
> > if (CDROM_CAN(CDC_CLOSE_TRAY) &&
> > cdi->options & CDO_AUTO_CLOSE) {
> > cd_dbg(CD_OPEN, "trying to close the tray\n");
> > - ret = cdo->tray_move(cdi, 0);
> > + ret = cdrom_tray_close(cdi);
> > + if (ret == -ERESTARTSYS)
> > + return ret;
> > if (ret) {
> > cd_dbg(CD_OPEN, "bummer. tried to close the tray but failed.\n");
> > /* Ignore the error from the low
> > @@ -2312,7 +2328,8 @@ static int cdrom_ioctl_closetray(struct cdrom_device_info *cdi)
> >
> > if (!CDROM_CAN(CDC_CLOSE_TRAY))
> > return -ENOSYS;
> > - return cdi->ops->tray_move(cdi, 0);
> > +
> > + return cdrom_tray_close(cdi);
> > }
>
> So this patch changes code that does not wait into code that potentially waits
> forever? Sorry but I don't think that's ideal. Please make sure that after a
> certain time (a few seconds?) the loop finishes.

Unfortunately, a few seconds is NOT sufficinet. I have no idea what is
the upper bound on the time it can take to close the tray taking into
account all hardware implementations like media changers. For the usual
desktop units it takes tens of seconds so you would need to wait minutes
to give some headroom - basically near-eternity.

Thanks

Michal