Fix cdrom autoclose
Subsystems (6)
cover 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(-)
Signature · 1 line
2.13.6
PATCH 1/6 delay: add poll_event_interruptible
Add convenience macro for polling an event that does not have a waitqueue.
--- include/linux/delay.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
include/linux/delay.h +12 -0
@@ -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) */
Signature · 1 line
2.13.6
PATCH 2/6 cdrom: factor out common open_for_* code
In reply to Michal SuchanekThe open_for_audio and open_for_data copies are bitrotten in different ways already and will need to update the autoclose logic in both.
--- drivers/cdrom/cdrom.c | 100 ++++++++++++++++++-------------------------------- 1 file changed, 36 insertions(+), 64 deletions(-)
drivers/cdrom/cdrom.c +36 -64
@@ -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 );
Signature · 1 line
2.13.6
PATCH 3/6 cdrom: wait for tray to close
In reply to Michal SuchanekThe 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.
--- drivers/cdrom/cdrom.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)
drivers/cdrom/cdrom.c +19 -2
@@ -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,
Signature · 1 line
2.13.6
Re: [PATCH 3/6] cdrom: wait for tray to close
drivers/cdrom/cdrom.c static int cdrom_ioctl_closetray(struct · line 2332
Quoted 7 lines · On Thu, 14 Dec 2017 16:13:52 +0100 Michal Suchanek <[email protected]> wrote:
drivers/cdrom/cdrom.c +2
-1@@ -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
PATCH 4/6 cdrom: introduce CDS_DRIVE_ERROR
In reply to Michal SuchanekCDS_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.
--- 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(-)
drivers/block/paride/pcd.c +1 -1
@@ -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 ;
drivers/cdrom/gdrom.c +1 -1
@@ -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 ; }
drivers/ide/ide-cd_ioctl.c +8 -4
@@ -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 ; }/*
drivers/scsi/sr_ioctl.c +1 -1
@@ -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)
include/uapi/linux/cdrom.h +1 -0
@@ -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 */
Signature · 1 line
2.13.6
PATCH 5/6 Documentetion: cdrom: introduce CDS_DRIVE_ERROR
In reply to Michal SuchanekCDS_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.
--- Documentation/cdrom/cdrom-standard.tex | 8 +++++++- Documentation/cdrom/ide-cd | 6 ++++++ Documentation/ioctl/cdrom.txt | 1 + 3 files changed, 14 insertions(+), 1 deletion(-)
Documentation/cdrom/cdrom-standard.tex +7 -1
@@ -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\
Documentation/cdrom/ide-cd +6 -0
@@ -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;
Documentation/ioctl/cdrom.txt +1 -0
@@ -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:
Signature · 1 line
2.13.6
PATCH 6/6 cdrom: wait for drive to become ready
In reply to Michal SuchanekWhen 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.
--- drivers/cdrom/cdrom.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/cdrom/cdrom.c +9 -0
@@ -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 ; }
Signature · 1 line
2.13.6
cover Fix cdrom autoclose
In reply to Michal SuchanekHello,
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(-)
Signature · 1 line
2.13.6
PATCH RESEND 1/6 delay: add poll_event_interruptible
Add convenience macro for polling an event that does not have a waitqueue.
--- include/linux/delay.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
include/linux/delay.h +12 -0
@@ -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) */
Signature · 1 line
2.13.6
Re: [PATCH resend 1/6] delay: add poll_event_interruptible
Quoted 19 lines · Michal Suchanek
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(+)include/linux/delay.h +3
-0@@ -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)) { \
include/linux/delay.h static inline void ssleep(unsigned int seconds) · line 79
Quoted 10 lines
include/linux/delay.h +9
-0+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.
PATCH RESEND 2/6 cdrom: factor out common open_for_* code
In reply to Michal SuchanekThe open_for_audio and open_for_data copies are bitrotten in different ways already and will need to update the autoclose logic in both.
--- drivers/cdrom/cdrom.c | 100 ++++++++++++++++++-------------------------------- 1 file changed, 36 insertions(+), 64 deletions(-)
drivers/cdrom/cdrom.c +36 -64
@@ -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 );
Signature · 1 line
2.13.6
Re: [PATCH resend 2/6] cdrom: factor out common open_for_* code
drivers/cdrom/cdrom.c int open_for_data(struct cdrom_device_info *cdi) · line 1051
Quoted 2 lines · Michal Suchanek
drivers/cdrom/cdrom.c +1
-1- 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.
PATCH RESEND 3/6 cdrom: wait for tray to close
In reply to Michal SuchanekThe 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.
---
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(-)
drivers/cdrom/cdrom.c +19 -2
@@ -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,
Signature · 1 line
2.13.6
Re: [PATCH resend 3/6] cdrom: wait for tray to close
Quoted 27 lines · Michal Suchanek
drivers/cdrom/cdrom.c +15
-1+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
drivers/cdrom/cdrom.c static int cdrom_ioctl_closetray(struct cdrom_device_info *cdi) · line 2333
Quoted 7 lines
drivers/cdrom/cdrom.c +2
-1@@ -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.
Re: [PATCH resend 3/6] cdrom: wait for tray to close
Quoted 33 lines · On Mon, 29 Jan 2018 17:05:47 +0000 Bart Van Assche <[email protected]>…
Quoted 32 lines · Michal Suchanek
drivers/cdrom/cdrom.c +10
-0+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(structcdrom_device_info*cdi, tracktype
*tracks) {drivers/cdrom/cdrom.c +3
-1@@ -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)drivers/cdrom/cdrom.c +0
-1if (!CDROM_CAN (CDC_CLOSE_TRAY ))return -ENOSYS ; -return cdi->ops->tray_move (cdi,0 );
Quoted 8 lines
drivers/cdrom/cdrom.c static int cdrom_ioctl_closetray(struct cdrom_device_info *cdi) · line 2333
Quoted 3 lines
drivers/cdrom/cdrom.c +2
-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
Re: [PATCH resend 3/6] cdrom: wait for tray to close
In reply to Bart Van AsscheQuoted 29 lines · Bart Van Assche
Quoted 28 lines · Michal Suchanek
drivers/cdrom/cdrom.c +15
-1+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)
Quoted 10 lines
drivers/cdrom/cdrom.c static int cdrom_ioctl_closetray(struct cdrom_device_info *cdi) · line 2333
Quoted 6 lines
drivers/cdrom/cdrom.c +2
-1if (!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
PATCH RESEND 4/6 cdrom: introduce CDS_DRIVE_ERROR
In reply to Michal SuchanekCDS_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.
--- 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(-)
drivers/block/paride/pcd.c +1 -1
@@ -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 ;
drivers/cdrom/gdrom.c +1 -1
@@ -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 ; }
drivers/ide/ide-cd_ioctl.c +8 -4
@@ -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 ; }/*
drivers/scsi/sr_ioctl.c +1 -1
@@ -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)
include/uapi/linux/cdrom.h +1 -0
@@ -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 */
Signature · 1 line
2.13.6
PATCH RESEND 5/6 Documentetion: cdrom: introduce CDS_DRIVE_ERROR
In reply to Michal SuchanekCDS_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.
--- Documentation/cdrom/cdrom-standard.tex | 8 +++++++- Documentation/cdrom/ide-cd | 6 ++++++ Documentation/ioctl/cdrom.txt | 1 + 3 files changed, 14 insertions(+), 1 deletion(-)
Documentation/cdrom/cdrom-standard.tex +7 -1
@@ -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\
Documentation/cdrom/ide-cd +6 -0
@@ -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;
Documentation/ioctl/cdrom.txt +1 -0
@@ -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:
Signature · 1 line
2.13.6
PATCH RESEND 6/6 cdrom: wait for drive to become ready
In reply to Michal SuchanekWhen 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.
--- drivers/cdrom/cdrom.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/cdrom/cdrom.c +9 -0
@@ -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 ; }
Signature · 1 line
2.13.6
Re: [PATCH resend 6/6] cdrom: wait for drive to become ready
Quoted 18 lines · Michal Suchanek
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(+)drivers/cdrom/cdrom.c +2
-0@@ -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 ) {
drivers/cdrom/cdrom.c int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks) · line 1101
Quoted 10 lines
drivers/cdrom/cdrom.c +7
-0+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.
Quoted 1 line · Michal Suchanek
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
Quoted 6 lines · On Fri, 26 Jan 2018 12:04:56 -0800 James Bottomley <[email protected]>…
Quoted 1 line · Michal Suchanek
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