On Wed, 4 Jan 2012, Matthew Dharm wrote:
> On Wed, Jan 4, 2012 at 1:03 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > On Wed, 4 Jan 2012, Oliver Neukum wrote:
> >
> >> Looking at the following code from scsi.c:
> >>
> >> int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
> >> {
> >
> > ...
> >
> >> � � � � /*
> >> � � � � �* If SCSI-2 or lower, store the LUN value in cmnd.
> >> � � � � �*/
> >> � � � � if (cmd->device->scsi_level <= SCSI_2 &&
> >> � � � � � � cmd->device->scsi_level != SCSI_UNKNOWN) {
> >> � � � � � � � � cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
> >> � � � � � � � � � � � � � � � �(cmd->device->lun << 5 & 0xe0);
> >>
> >>
> >> I am really unsure whether this change is really safe.
> >> In particular I doubt it is material for the stable tree.
> >
> > Well, this change just makes us more compatible with what devices are
> > supposed to expect. �But if you think it's too chancy, it can be kept
> > out of the stable trees.
> >
> > Matt, what do you think? �The change Oliver refers to removes the
> > code that forces every USB device to have its scsi_level set to 2, so
> > now we'll use the actual level reported by the device. �At the same
> > time, it adds a new mechanism to prevent the use of REPORT LUNS, which
> > was the reason for forcing the level to 2 in the first place.
>
> This is actually a bit of a complicated issue.
>
> Historically, there have been at least 3 observed behavior modes for
> the reported SCSI level:
>
> 1) Reasonably correct (i.e. 1 or 2)
> 2) Reports 0 (which, when -1'ed, yields a rev of 0xff). I actually
> had an argument with a vendor who insisted this was "correct" because
> the device didn't really support all the required SCSI commands; it's
> difficult to say he was wrong.
I'm not sure what the story is here... But note that the SCSI layer
doesn't increment the reported revision value if it indicates
pre-SCSI-2, i.e., SCSI_UNKNOWN or SCSI_1 (0 or 1 without CCS).
> 3) Reports 0xff (yada yada to 0xfe).
>
> When this 'fixup' code to force rev 2 was added, there were actually 3
> separate issues:
>
> a) REPORT_LUNS command sent to devices that didn't support it, causing
> device firmare crash
> b) 16-byte command usage used to be keyed off of this, causing
> firmware crash (since changed, I *THINK* -- needs verification)
> c) Confusing userspace tools (cdrecord, I think, maybe others) into
> doing (a) or (b)
>
> If we're pretty certain that the only two things that SCSI revision
> are used for is REPORT_LUNS and determining where to stuff the LUN in
> the command bytes, that still leaves issue (c); we could always call
> that a case of "bad userspace", but users will complain.
Hmmm. It's also used for a couple of other things. Most are pretty
minor, like deciding whether or not to scan for LUNs above 7. But two
uses are more significant: Whether to issue READ CAPACITY(16) (probably
what you were thinking of above -- this caused problems for some buggy
card readers but we have an unusual_devs flag for it) and whether to
read various VPD pages related to block device limits, characteristics,
and provisioning. The SCSI tape driver also uses scsi_level for some
decisions; I don't know how important they are for us.
> The real problem, however, is the number of devices which exhibit bad
> behavior (2) or (3). The vast majority of them need the LUN stuffed
> into the command bytes (i.e. SCSI-2 behavior). How do be guarantee
> functionality to those devices?
>
> Related question: How do we identify those devices? This bad
> behavior was so common in the earlier era of usb-storage that we never
> created a separate UNUSUAL_DEV flag for it; we just overrode the value
> and went on with our lives. Maybe we write it such that the SCSI
> level is set to 2 if the reported value is 0 or obviously stupid (i.e.
> >3).
Well, > 3 is not obviously stupid. Perry's device reports 6 (the raw
value), which is the next value beyond SCSI_SPC_3 and isn't currently
defined in the SCSI header files, and the drive clearly doesn't want
to see LUN bits stuffed into the command bytes.
As for the buggy devices which claim to be SCSI-3 but don't support
REPORT LUNS... We don't really know how they feel about the LUN bits
because we haven't received any reports about devices with more than
one LUN.
> I don't *recall* any devices reporting SCSI-3 that needed to be set to
> SCSI-2.... but it's been many years since I wrote this bit.
I remember a couple -- they did crash upon receiving REPORT LUNS. But
it has been a long time, and in addition, we've got a little
miscommunication going on here. I had to go back and look through the
git history to see what you were referring to.
All that stuff about forcing the level to be at least SCSI-2 was
removed in 2007, by commit f3f4906516a084bbd9aa3da7592e6b029fe78f5b
(usb-storage: SCSI level fixes). What Oliver and I have been talking
about is rather the code added by commit
a4e628328ec60873fec9d506d682155391f589ce (USB Storage: wedge SCSI
revision at 2 for usb-storage devices) in 2005 and later modified by
that other commit, which prevents the level from ever going higher than
SCSI-2.
> Curious observation: I wonder how this sort of SCSI-3 multi-LUN device
> fares on WinXP? As far as I know, WinXP also assumes SCSI-2 when
> sending commands to USB devices...
Hard to tell without having both the hardware and the OS on hand to
test. SCSI-3 came about long before XP was terminated, so it should
have been supported. But maybe that support was never added to the USB
mass-storage driver...
> So, in short, I think Alan's patch is a Bad Idea(tm). While it fixes
> the problem described in the comments, there are other issues lurking
> out there. As I think about it, I kinda like my "force rev to 2 if
> obviously wrong" approach... what do you guys think?
Go look at those commits; they are likely to affect your decision to
some extent.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
