[PATCH] scsi: blacklist: add VMware ESXi cdrom - broken tray emulation

[PATCH] scsi: blacklist: add VMware ESXi cdrom - broken tray emulation

[Michal Suchanek]

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 <msuchanek@suse.de>
---
v2: new patch
v3: change into a blacklist flag
v4: fix vendor match condition
---
 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..923f54b88d24 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

Re: [PATCH] scsi: blacklist: add VMware ESXi cdrom - broken tray emulation

[Martin K. Petersen]

Michal,

> +	{"VMware", "VMware", NULL, BLIST_NO_MATCH_VENDOR | BLIST_NO_TRAY},

Please don't introduce a blist flag to work around deficiencies in the
matching interface. I suggest you tweak the matching functions so they
handle a NULL vendor string correctly.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: blacklist: add VMware ESXi cdrom - broken tray emulation

[Michal Suchánek]

On Tue, Dec 17, 2019 at 05:28:28PM -0500, Martin K. Petersen wrote:
> 
> Michal,
> 
> > +	{"VMware", "VMware", NULL, BLIST_NO_MATCH_VENDOR | BLIST_NO_TRAY},
> 
> Please don't introduce a blist flag to work around deficiencies in the
> matching interface. I suggest you tweak the matching functions so they
> handle a NULL vendor string correctly.

I don't think that will work with the interface for dynamically adding
entries through sysfs.

Thanks

Michal

Re: [PATCH] scsi: blacklist: add VMware ESXi cdrom - broken tray emulation

[Martin K. Petersen]

Michal,

>> Please don't introduce a blist flag to work around deficiencies in the
>> matching interface. I suggest you tweak the matching functions so they
>> handle a NULL vendor string correctly.
>
> I don't think that will work with the interface for dynamically adding
> entries through sysfs.

Please make it work :)

There's nothing conceptually wrong with being able to do:

        echo ":Model:Flags" > /proc/scsi/device_info

We keep running into issues where the same device needs to be listed
many times because it gets branded by different vendors.

Brownie points for making all this less clunky. The libata globbing
blacklist works much better, fwiw.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: blacklist: add VMware ESXi cdrom - broken tray emulation

[Michal Suchánek]

On Thu, Dec 19, 2019 at 06:31:12PM -0500, Martin K. Petersen wrote:
> 
> Michal,
> 
> >> Please don't introduce a blist flag to work around deficiencies in the
> >> matching interface. I suggest you tweak the matching functions so they
> >> handle a NULL vendor string correctly.
> >
> > I don't think that will work with the interface for dynamically adding
> > entries through sysfs.
> 
> Please make it work :)
> 
> There's nothing conceptually wrong with being able to do:
> 
>         echo ":Model:Flags" > /proc/scsi/device_info
> 
> We keep running into issues where the same device needs to be listed
> many times because it gets branded by different vendors.
> 
Is there any description of what the format is supposed to be?

From the current code it looks like comma separated list of blacklist
entries that may be optionally quoted in some way.

The quoting is basically ignored but it is not clear if the inidividual
entries are supposed to be quoted or the whole thing.

Thanks

Michal

Re: [PATCH] scsi: blacklist: add VMware ESXi cdrom - broken tray emulation

[Michal Suchánek]

On Thu, Dec 19, 2019 at 06:31:12PM -0500, Martin K. Petersen wrote:
> 
> Michal,
> 
> >> Please don't introduce a blist flag to work around deficiencies in the
> >> matching interface. I suggest you tweak the matching functions so they
> >> handle a NULL vendor string correctly.
> >
> > I don't think that will work with the interface for dynamically adding
> > entries through sysfs.
> 
> Please make it work :)
> 
> There's nothing conceptually wrong with being able to do:
> 
>         echo ":Model:Flags" > /proc/scsi/device_info
> 
> We keep running into issues where the same device needs to be listed
> many times because it gets branded by different vendors.

Actually the flag is really needed. The vendor string is a field of
specific length, not a pointer. This makes sense because the vendor
string is fixed length. The code adding the blacklist entries  cannot
handle NULL, and the matching code works with char array already.

What can be done differently is stop space-padding the values and
instead match the length of the string as provided. This will however
cause API break. Currently short entries are space-padded to match
exactly the provided vendor string. If we change the match to only the
provided string length it will potentially match and blacklist
additional devices.

If a flag is required to trigger the prefix matching it can be used
instead of the flag that disables vendor matching with empty vendor
string.

Thanks

Michal