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

["Michal Suchánek" <msuchanek@suse.de>]

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