2019-10-25 19:52:46

by Michal Suchánek

[permalink] [raw]
Subject: [PATCH v3 0/7] 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.
The code needs not be replicated in every userspace utility like mount
or dd which has no business knowing which devices are CD-roms and where
the autoclose setting is in the kernel.

Link: https://lore.kernel.org/lkml/[email protected]/
Link: https://lore.kernel.org/lkml/[email protected]/

v3:
- change the VMware workaround to use blacklist flag
- use exported function instead of ioctl

Michal Suchanek (7):
cdrom: add poll_event_interruptible
cdrom: factor out common open_for_* code
cdrom: wait for the tray to close
cdrom: export autoclose logic as a separate function
bdev: add open_finish.
scsi: blacklist: add VMware ESXi cdrom - broken tray emulation.
scsi: sr: wait for the medium to become ready

Documentation/filesystems/locking.rst | 2 +
drivers/cdrom/cdrom.c | 192 ++++++++++++++------------
drivers/scsi/scsi_devinfo.c | 15 +-
drivers/scsi/sr.c | 60 ++++++--
fs/block_dev.c | 21 ++-
include/linux/blkdev.h | 1 +
include/linux/cdrom.h | 1 +
include/scsi/scsi_devinfo.h | 7 +-
8 files changed, 191 insertions(+), 108 deletions(-)

--
2.23.0


2019-10-25 19:53:58

by Michal Suchánek

[permalink] [raw]
Subject: [PATCH v3 6/7] scsi: blacklist: add VMware ESXi cdrom - broken tray emulation.

The WMware ESXi cdrom identifies itself as:
sr 0:0:0:0: [sr0] scsi3-mmc drive: vendor: "NECVMWarVMware SATA CD001.00"
model: "VMware SATA CD001.00"
with the following get_capabilities print in sr.c:
sr_printk(KERN_INFO, cd,
"scsi3-mmc drive: vendor: \"%s\" model: \"%s\"\n",
cd->device->vendor, cd->device->model);

The model looks like reliable identification while vendor does not.

The drive claims to have a tray and claims to be able to close it.
However, the UI has no notion of a tray - when medium is ejected it is
dropped in the floor and the user must select a medium again before the
drive can be re-loaded. On the kernel side the tray_move call to close
the tray succeeds but the drive state does not change as a result of the
call.

The drive does not in fact emulate the tray state. There are two ways to
get the medium state. One is the SCSI status:

Physical drive:

Fixed format, current; Sense key: Not Ready
Additional sense: Medium not present - tray open
Raw sense data (in hex):
70 00 02 00 00 00 00 0a 00 00 00 00 3a 02 00 00
00 00

Fixed format, current; Sense key: Not Ready
Additional sense: Medium not present - tray closed
Raw sense data (in hex):
70 00 02 00 00 00 00 0a 00 00 00 00 3a 01 00 00
00 00

VMware ESXi:

Fixed format, current; Sense key: Not Ready
Additional sense: Medium not present
Info fld=0x0 [0]
Raw sense data (in hex):
f0 00 02 00 00 00 00 0a 00 00 00 00 3a 00 00 00
00 00

So the tray state is not reported here. Other is medium status which the
kernel prefers if available. Adding a print here gives:

cdrom: get_media_event success: code = 0, door_open = 1, medium_present = 0

door_open is interpreted as open tray. This is fine so long as tray_move
would close the tray when requested or report an error which never
happens on VMware ESXi servers (5.5 and 6.5 tested).

This is a popular virtualization platform so a workaround is worthwhile.

Signed-off-by: Michal Suchanek <[email protected]>
---
v2: new patch
v3: change into a blacklist flag - needs testing it still matches, will
try to patch qemu to provide same ID string
---
drivers/scsi/scsi_devinfo.c | 15 +++++++++------
drivers/scsi/sr.c | 6 ++++++
include/scsi/scsi_devinfo.h | 7 ++++++-
3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index df14597752ec..f5f51ec527aa 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -252,6 +252,7 @@ static struct {
{"TOSHIBA", "CD-ROM", NULL, BLIST_ISROM},
{"Traxdata", "CDR4120", NULL, BLIST_NOLUN}, /* locks up */
{"USB2.0", "SMARTMEDIA/XD", NULL, BLIST_FORCELUN | BLIST_INQUIRY_36},
+ {"VMware", "VMware", NULL, BLIST_NO_MATCH_VENDOR | BLIST_NO_TRAY},
{"WangDAT", "Model 2600", "01.7", BLIST_SELECT_NO_ATN},
{"WangDAT", "Model 3200", "02.2", BLIST_SELECT_NO_ATN},
{"WangDAT", "Model 1300", "02.4", BLIST_SELECT_NO_ATN},
@@ -454,10 +455,11 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
/*
* vendor strings must be an exact match
*/
- if (vmax != strnlen(devinfo->vendor,
- sizeof(devinfo->vendor)) ||
- memcmp(devinfo->vendor, vskip, vmax))
- continue;
+ if (!devinfo->flags & BLIST_NO_MATCH_VENDOR)
+ if (vmax != strnlen(devinfo->vendor,
+ sizeof(devinfo->vendor)) ||
+ memcmp(devinfo->vendor, vskip, vmax))
+ continue;

/*
* @model specifies the full string, and
@@ -468,8 +470,9 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
continue;
return devinfo;
} else {
- if (!memcmp(devinfo->vendor, vendor,
- sizeof(devinfo->vendor)) &&
+ if ((!memcmp(devinfo->vendor, vendor,
+ sizeof(devinfo->vendor))
+ || (devinfo->flags & BLIST_NO_MATCH_VENDOR)) &&
!memcmp(devinfo->model, model,
sizeof(devinfo->model)))
return devinfo;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 4664fdf75c0f..07c319494bf4 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -58,6 +58,7 @@
#include <scsi/scsi_eh.h>
#include <scsi/scsi_host.h>
#include <scsi/scsi_ioctl.h> /* For the door lock/unlock commands */
+#include <scsi/scsi_devinfo.h>

#include "scsi_logging.h"
#include "sr.h"
@@ -922,6 +923,11 @@ static void get_capabilities(struct scsi_cd *cd)
buffer[n + 4] & 0x20 ? "xa/form2 " : "", /* can read xa/from2 */
buffer[n + 5] & 0x01 ? "cdda " : "", /* can read audio data */
loadmech[buffer[n + 6] >> 5]);
+ if (cd->device->sdev_bflags & BLIST_NO_TRAY) {
+ buffer[n + 6] &= ~(0xff << 5);
+ sr_printk(KERN_INFO, cd,
+ "Tray emulation bug workaround: tray -> caddy\n");
+ }
if ((buffer[n + 6] >> 5) == 0)
/* caddy drives can't close tray... */
cd->cdi.mask |= CDC_CLOSE_TRAY;
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 3fdb322d4c4b..17ea96936cc6 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -67,8 +67,13 @@
#define BLIST_RETRY_ITF ((__force blist_flags_t)(1ULL << 32))
/* Always retry ABORTED_COMMAND with ASC 0xc1 */
#define BLIST_RETRY_ASC_C1 ((__force blist_flags_t)(1ULL << 33))
+/* Device reports to have a tray but it cannot be operated reliably */
+#define BLIST_NO_TRAY ((__force blist_flags_t)(1ULL << 34))
+/* Vendor string is bogus */
+#define BLIST_NO_MATCH_VENDOR ((__force blist_flags_t)(1ULL << 35))

-#define __BLIST_LAST_USED BLIST_RETRY_ASC_C1
+
+#define __BLIST_LAST_USED BLIST_NO_MATCH_VENDOR

#define __BLIST_HIGH_UNUSED (~(__BLIST_LAST_USED | \
(__force blist_flags_t) \
--
2.23.0

2019-10-25 19:54:55

by Michal Suchánek

[permalink] [raw]
Subject: [PATCH v3 7/7] scsi: sr: wait for the medium to become ready

Use the autoclose function provided by cdrom driver to wait for drive to
close in open_finish, and attempt to open once more after the door
closes.

Signed-off-by: Michal Suchanek <[email protected]>
---
v3: use function call rather than IOCTL
---
drivers/scsi/sr.c | 54 ++++++++++++++++++++++++++++++++++++-----------
1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 07c319494bf4..d144ad085b35 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -522,29 +522,58 @@ static blk_status_t sr_init_command(struct scsi_cmnd *SCpnt)
return ret;
}

-static int sr_block_open(struct block_device *bdev, fmode_t mode)
+static int __sr_block_open(struct block_device *bdev, fmode_t mode)
{
- struct scsi_cd *cd;
- struct scsi_device *sdev;
- int ret = -ENXIO;
-
- cd = scsi_cd_get(bdev->bd_disk);
- if (!cd)
- goto out;
+ struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
+ int ret;

- sdev = cd->device;
- scsi_autopm_get_device(sdev);
check_disk_change(bdev);

mutex_lock(&sr_mutex);
ret = cdrom_open(&cd->cdi, bdev, mode);
mutex_unlock(&sr_mutex);

+ return ret;
+}
+
+static int sr_block_open(struct block_device *bdev, fmode_t mode)
+{
+ struct scsi_cd *cd = scsi_cd_get(bdev->bd_disk);
+ struct scsi_device *sdev;
+ int ret;
+
+ if (!cd)
+ return -ENXIO;
+
+ sdev = cd->device;
+ scsi_autopm_get_device(sdev);
+ ret = __sr_block_open(bdev, mode);
scsi_autopm_put_device(sdev);
- if (ret)
+
+ if (ret == -ERESTARTSYS)
scsi_cd_put(cd);

-out:
+ return ret;
+}
+
+static int sr_block_open_finish(struct block_device *bdev, fmode_t mode,
+ int ret)
+{
+ struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
+
+ /* wait for drive to get ready */
+ if ((ret == -ENOMEDIUM) && !(mode & FMODE_NDELAY)) {
+ struct scsi_device *sdev = cd->device;
+ /*
+ * Cannot use sr_block_ioctl because it locks sr_mutex blocking
+ * out any processes trying to access the drive
+ */
+ scsi_autopm_get_device(sdev);
+ cdrom_autoclose(&cd->cdi);
+ ret = __sr_block_open(bdev, mode);
+ scsi_autopm_put_device(sdev);
+ }
+
return ret;
}

@@ -640,6 +669,7 @@ static const struct block_device_operations sr_bdops =
{
.owner = THIS_MODULE,
.open = sr_block_open,
+ .open_finish = sr_block_open_finish,
.release = sr_block_release,
.ioctl = sr_block_ioctl,
.check_events = sr_block_check_events,
--
2.23.0

2019-10-25 22:33:19

by Michal Suchánek

[permalink] [raw]
Subject: [PATCH v3 2/7] 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]>
---
v2: do not add unrelated whitespace changes
v3: fix declaration style
---
drivers/cdrom/cdrom.c | 96 +++++++++++++++----------------------------
1 file changed, 33 insertions(+), 63 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 2ad6b73482fe..87db82489bf8 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -1045,13 +1045,12 @@ static void cdrom_count_tracks(struct cdrom_device_info *cdi, tracktype *tracks)
tracks->cdi, tracks->xa);
}

-static
-int open_for_data(struct cdrom_device_info *cdi)
+static 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) {
@@ -1071,37 +1070,44 @@ 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)) {
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) {
@@ -1208,53 +1214,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;
@@ -2725,7 +2695,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);
@@ -2773,7 +2743,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.23.0

2019-10-25 22:34:53

by Michal Suchánek

[permalink] [raw]
Subject: [PATCH v3 3/7] cdrom: wait for the tray to close

The scsi command to close the 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 shortly or reports an error when it detects the tray is
blocked. At worst the wait can be interrupted by the user.

Also wait for the drive to become ready once the tray closes.

Signed-off-by: Michal Suchanek <[email protected]>
---
v2:
- check drive_status exists before using it
- rename tray_close -> cdrom_tray_close
- also wait for drive to become ready after tray closes
- do not wait in cdrom_ioctl_closetray
---
drivers/cdrom/cdrom.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 87db82489bf8..27f4fce22781 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -1045,6 +1045,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)
{
int ret;
@@ -1062,7 +1074,9 @@ static 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
@@ -1085,6 +1099,15 @@ static 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.23.0