| Message ID | 03915a2e64f50ec04ea6d8e6f80e36ecf16e4f0f.1516985620.git.msuchanek@suse.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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.
On Mon, 29 Jan 2018 17:05:47 +0000 Bart Van Assche <Bart.VanAssche@wdc.com> 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
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
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,
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 <msuchanek@suse.de> --- 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(-)