| Message ID | 20191217180840.9414-1-msuchanek@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: blacklist: add VMware ESXi cdrom - broken tray emulation | expand |
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.
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
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.
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
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
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) \
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(-)