[Bug 80711] New: SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Bug 80711] New: SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

            Bug ID: 80711
           Summary: SG_FLAG_LUN_INHIBIT is no longer implemented and
                    there's not way to prevent the kernel from using the
                    2nd cdb byte for the LUN
           Product: IO/Storage
           Version: 2.5
    Kernel Version: 3.16-rc1
          Hardware: All
                OS: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: SCSI
          Assignee: linux-scsi@vger.kernel.org
          Reporter: tiziano.bacocco@gmail.com
        Regression: No

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #1 from Tiziano Bacocco <tiziano.bacocco@gmail.com> ---
Created attachment 143541
  --> https://bugzilla.kernel.org/attachment.cgi?id=143541&action=edit
This patch allows using SG_FLAG_LUN_INHIBIT

I hope i've not broken anything else , this patch allows using
SG_FLAG_LUN_INHIBIT

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

Tiziano Bacocco <tiziano.bacocco@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #143541|0                           |1
        is obsolete|                            |

--- Comment #2 from Tiziano Bacocco <tiziano.bacocco@gmail.com> ---
Created attachment 143611
  --> https://bugzilla.kernel.org/attachment.cgi?id=143611&action=edit
This patch allows using SG_FLAG_LUN_INHIBIT

Removed useless patch to blkdev.h

-- 
You are receiving this mail because:
You are the assignee for the bug.

Re: [Bug 80711] SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Christoph Hellwig]

This is going in the wrong direction.  If your device doesn't like
the LUN field in the CDB it misreports the standard compliance level.

We'll need to fix that root cause, probably by a black list entry
instead of hacking around it in userspace.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

Alan <alan@lxorguk.ukuu.org.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |alan@lxorguk.ukuu.org.uk
            Summary|SG_FLAG_LUN_INHIBIT is no   |[PATCH]SG_FLAG_LUN_INHIBIT
                   |longer implemented and      |is no longer implemented
                   |there's not way to prevent  |and there's not way to
                   |the kernel from using the   |prevent the kernel from
                   |2nd cdb byte for the LUN    |using the 2nd cdb byte for
                   |                            |the LUN

--- Comment #3 from Alan <alan@lxorguk.ukuu.org.uk> ---
Patches need to go to the mailing list and/or maintainers. See
Documentation/SubmittingPatches. 

This is needed as we need a Signed-off-by: and valid email address.

-- 
You are receiving this mail because:
You are the assignee for the bug.

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Douglas Gilbert]

On 14-07-29 05:57 PM, bugzilla-daemon@bugzilla.kernel.org wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=80711
>
> Alan <alan@lxorguk.ukuu.org.uk> changed:
>
>             What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                   CC|                            |alan@lxorguk.ukuu.org.uk
>              Summary|SG_FLAG_LUN_INHIBIT is no   |[PATCH]SG_FLAG_LUN_INHIBIT
>                     |longer implemented and      |is no longer implemented
>                     |there's not way to prevent  |and there's not way to
>                     |the kernel from using the   |prevent the kernel from
>                     |2nd cdb byte for the LUN    |using the 2nd cdb byte for
>                     |                            |the LUN
>
> --- Comment #3 from Alan <alan@lxorguk.ukuu.org.uk> ---
> Patches need to go to the mailing list and/or maintainers. See
> Documentation/SubmittingPatches.

In my original rework of the sg driver (1998) it did what
was being requested in this patch. Then around 10 years ago
changes were added to the SCSI subsystem to strip this
capability from the sg driver. Since then about 5 attempts (my
guess) have been made to re-instate this capability, this
being the most recent.

The use case is almost always the same: specialized USB
storage devices. USB is not going away. Does anyone see
a change in "company" policy in this regard?


If not and since I'm told black lists and the like won't
work, my advice for the record is to use FreeBSD or Windows
for tools that need this capability.

Doug Gilbert

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Christoph Hellwig]

On Wed, Aug 06, 2014 at 03:29:47PM +0200, Douglas Gilbert wrote:
> If not and since I'm told black lists and the like won't
> work, my advice for the record is to use FreeBSD or Windows
> for tools that need this capability.

I doubt either of them forces users to hack up flags for these cases.

At least for windows I suspect it just never sends the LUN encoded
in the CDB and treats USB devices special instead of our insistance
on pretending they are SCSI-2.

Maybe some of the USB people have on the wire traces or access to
device or windows documentation on this?

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Alan Stern]

On Wed, 6 Aug 2014, Christoph Hellwig wrote:

> On Wed, Aug 06, 2014 at 03:29:47PM +0200, Douglas Gilbert wrote:
> > If not and since I'm told black lists and the like won't
> > work, my advice for the record is to use FreeBSD or Windows
> > for tools that need this capability.
> 
> I doubt either of them forces users to hack up flags for these cases.

Why was this change needed in the first place?  There's no explanation 
in the patch itself.

> At least for windows I suspect it just never sends the LUN encoded
> in the CDB and treats USB devices special instead of our insistance
> on pretending they are SCSI-2.

We no longer pretend that USB mass-storage devices have any particular 
SCSI level.  See commit 09b6b51b0b6c.

> Maybe some of the USB people have on the wire traces or access to
> device or windows documentation on this?

Most likely it varies with the version of Windows and the INQUIRY data
returned by the device.

I can obtain hardware traces for the kinds of devices and computers 
lying around here.  But what sort of combinations should I test?

Alan Stern

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Alan Stern]

Please don't remove names from the CC: list; use Reply-To-All.  I had 
to go back and add all the names back in.

On Wed, 6 Aug 2014, Tiziano Bacocco wrote:

> Test with alcor based USB flash drives, linux 3.16 will remove the 3 msb of
> the CDB byte when using SG raw

Sure, but isn't that what you want it to do?  Doesn't the flash drive 
use those bits to hold the LUN number?

Alan Stern


> Il 06/ago/2014 22:02 "Alan Stern" <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> ha scritto:
>
> > On Wed, 6 Aug 2014, Christoph Hellwig wrote:
> >
> > > On Wed, Aug 06, 2014 at 03:29:47PM +0200, Douglas Gilbert wrote:
> > > > If not and since I'm told black lists and the like won't
> > > > work, my advice for the record is to use FreeBSD or Windows
> > > > for tools that need this capability.
> > >
> > > I doubt either of them forces users to hack up flags for these cases.
> >
> > Why was this change needed in the first place?  There's no explanation
> > in the patch itself.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Christoph Hellwig]

On Wed, Aug 06, 2014 at 04:02:22PM -0400, Alan Stern wrote:
> > I doubt either of them forces users to hack up flags for these cases.
> 
> Why was this change needed in the first place?  There's no explanation 
> in the patch itself.

Which chance?  The one to not support SG_FLAG_LUN_INHIBIT?

> 
> > At least for windows I suspect it just never sends the LUN encoded
> > in the CDB and treats USB devices special instead of our insistance
> > on pretending they are SCSI-2.
> 
> We no longer pretend that USB mass-storage devices have any particular 
> SCSI level.  See commit 09b6b51b0b6c.

So the origina reported device must report SCSI-2 all by itself if he's
running a recent kernel, ok.

> > Maybe some of the USB people have on the wire traces or access to
> > device or windows documentation on this?
> 
> Most likely it varies with the version of Windows and the INQUIRY data
> returned by the device.
> 
> I can obtain hardware traces for the kinds of devices and computers 
> lying around here.  But what sort of combinations should I test?

I'd mostly be interested to see if it actualy encodes the LUN in the CDB
for any USB multi-LUN device.

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Alan Stern]

On Wed, 6 Aug 2014, Christoph Hellwig wrote:

> On Wed, Aug 06, 2014 at 04:02:22PM -0400, Alan Stern wrote:
> > > I doubt either of them forces users to hack up flags for these cases.
> > 
> > Why was this change needed in the first place?  There's no explanation 
> > in the patch itself.
> 
> Which chance?  The one to not support SG_FLAG_LUN_INHIBIT?

No, the patch that started this Bugzilla entry.  Tiziano says it is 
needed in order to send vendor-specific commands that use the LUN bits 
in CDB[1].

> > > At least for windows I suspect it just never sends the LUN encoded
> > > in the CDB and treats USB devices special instead of our insistance
> > > on pretending they are SCSI-2.
> > 
> > We no longer pretend that USB mass-storage devices have any particular 
> > SCSI level.  See commit 09b6b51b0b6c.
> 
> So the origina reported device must report SCSI-2 all by itself if he's
> running a recent kernel, ok.
> 
> > > Maybe some of the USB people have on the wire traces or access to
> > > device or windows documentation on this?
> > 
> > Most likely it varies with the version of Windows and the INQUIRY data
> > returned by the device.
> > 
> > I can obtain hardware traces for the kinds of devices and computers 
> > lying around here.  But what sort of combinations should I test?
> 
> I'd mostly be interested to see if it actualy encodes the LUN in the CDB
> for any USB multi-LUN device.

I tried connecting a Linux mass-storage gadget with two logical units
to a host PC running Windows 7.  The host scanned the first logical
unit and completely ignored the second!  Didn't even send an INQUIRY 
command.

So the question remains unanswered...

Can someone tell me if anything special is needed to make Windows
recognize logical units beyond the first?

Alan Stern

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Christoph Hellwig]

On Thu, Aug 07, 2014 at 11:58:37AM -0400, Alan Stern wrote:
> > On Wed, Aug 06, 2014 at 04:02:22PM -0400, Alan Stern wrote:
> > > > I doubt either of them forces users to hack up flags for these cases.
> > > 
> > > Why was this change needed in the first place?  There's no explanation 
> > > in the patch itself.
> > 
> > Which chance?  The one to not support SG_FLAG_LUN_INHIBIT?
> 
> No, the patch that started this Bugzilla entry.  Tiziano says it is 
> needed in order to send vendor-specific commands that use the LUN bits 
> in CDB[1].

Yes, I'd really like to know the exact scenario.  What kind of command
is this and who sends it?

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Alan Stern]

On Tue, 19 Aug 2014, Christoph Hellwig wrote:

> On Thu, Aug 07, 2014 at 11:58:37AM -0400, Alan Stern wrote:
> > > On Wed, Aug 06, 2014 at 04:02:22PM -0400, Alan Stern wrote:
> > > > > I doubt either of them forces users to hack up flags for these cases.
> > > > 
> > > > Why was this change needed in the first place?  There's no explanation 
> > > > in the patch itself.
> > > 
> > > Which chance?  The one to not support SG_FLAG_LUN_INHIBIT?
> > 
> > No, the patch that started this Bugzilla entry.  Tiziano says it is 
> > needed in order to send vendor-specific commands that use the LUN bits 
> > in CDB[1].
> 
> Yes, I'd really like to know the exact scenario.  What kind of command
> is this and who sends it?

I don't know what the command is, but Tiziano is sending it via the
SCSI-generic (sg) interface.

In the meantime, I have made a little progress with Windows.  It turns
out there are two reasons my earlier test didn't work:

	I didn't bother to set up a valid serial number for the test
	device, and Windows wants to see the serial number.

	Windows wants at least one of the logical units to be
	removable.

After those issues were fixed, the host did recognize both logical 
units.  I tested with two devices, one reporting an ANSI SCSI level of 
0 and the other 2.  In both cases, Windows 7 does _not_ stick the LUN 
value into the high bits of CDB[1].

This suggests that we shouldn't be doing that either, for USB
mass-storage devices.  But I'm reluctant to change it because of the
possibility of regressions, not to mention the fact that it would
violate the SCSI spec.

Suggestions?

Alan Stern

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Douglas Gilbert]

On 14-08-20 03:15 PM, Alan Stern wrote:
> On Tue, 19 Aug 2014, Christoph Hellwig wrote:
>
>> On Thu, Aug 07, 2014 at 11:58:37AM -0400, Alan Stern wrote:
>>>> On Wed, Aug 06, 2014 at 04:02:22PM -0400, Alan Stern wrote:
>>>>>> I doubt either of them forces users to hack up flags for these cases.
>>>>>
>>>>> Why was this change needed in the first place?  There's no explanation
>>>>> in the patch itself.
>>>>
>>>> Which chance?  The one to not support SG_FLAG_LUN_INHIBIT?
>>>
>>> No, the patch that started this Bugzilla entry.  Tiziano says it is
>>> needed in order to send vendor-specific commands that use the LUN bits
>>> in CDB[1].
>>
>> Yes, I'd really like to know the exact scenario.  What kind of command
>> is this and who sends it?
>
> I don't know what the command is, but Tiziano is sending it via the
> SCSI-generic (sg) interface.
>
> In the meantime, I have made a little progress with Windows.  It turns
> out there are two reasons my earlier test didn't work:
>
> 	I didn't bother to set up a valid serial number for the test
> 	device, and Windows wants to see the serial number.
>
> 	Windows wants at least one of the logical units to be
> 	removable.
>
> After those issues were fixed, the host did recognize both logical
> units.  I tested with two devices, one reporting an ANSI SCSI level of
> 0 and the other 2.  In both cases, Windows 7 does _not_ stick the LUN
> value into the high bits of CDB[1].
>
> This suggests that we shouldn't be doing that either, for USB
> mass-storage devices.  But I'm reluctant to change it because of the
> possibility of regressions, not to mention the fact that it would
> violate the SCSI spec.
>
> Suggestions?

Perhaps we could add another bit flag in struct
scsi_host_template such as:
     unsigned int transport_says_dont_scsi2_lun_cmd:1;

then drivers/usb/storage/scsiglue.c could set that
bit in its usb_stor_host_template and
drivers/scsi/scsi.c could take heed (and not mask
cmd->cmnd[1] with the LUN).

Doug Gilbert


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Christoph Hellwig]

On Thu, Aug 21, 2014 at 10:41:02AM -0400, Douglas Gilbert wrote:
> Perhaps we could add another bit flag in struct
> scsi_host_template such as:
>     unsigned int transport_says_dont_scsi2_lun_cmd:1;
> 
> then drivers/usb/storage/scsiglue.c could set that
> bit in its usb_stor_host_template and
> drivers/scsi/scsi.c could take heed (and not mask
> cmd->cmnd[1] with the LUN).

Fully agreed.

(except that I'd shorten the flag name to .no_scsi2lun :))

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Alan Stern]

On Thu, 21 Aug 2014, Christoph Hellwig wrote:

> On Thu, Aug 21, 2014 at 10:41:02AM -0400, Douglas Gilbert wrote:
> > Perhaps we could add another bit flag in struct
> > scsi_host_template such as:
> >     unsigned int transport_says_dont_scsi2_lun_cmd:1;
> > 
> > then drivers/usb/storage/scsiglue.c could set that
> > bit in its usb_stor_host_template and
> > drivers/scsi/scsi.c could take heed (and not mask
> > cmd->cmnd[1] with the LUN).
> 
> Fully agreed.
> 
> (except that I'd shorten the flag name to .no_scsi2lun :))

Okay, here's a patch that implements the suggestion, except that I put
the flag in the Scsi_Host structure instead of the template.  This was
to minimize the impact of the change.  Among the various SCSI-over-USB
transports, only the Bulk-Only transport gives the LUN separately from
the CDB.  I don't know if there are any multi-LUN USB devices that
don't use the Bulk-Only transport, but if there are then they won't 
work if the LUN isn't stored in CDB[1].

Tiziano, does this do what you want?

Alan Stern



Index: usb-3.16/include/scsi/scsi_host.h
===================================================================
--- usb-3.16.orig/include/scsi/scsi_host.h
+++ usb-3.16/include/scsi/scsi_host.h
@@ -695,6 +695,9 @@ struct Scsi_Host {
 	/* The controller does not support WRITE SAME */
 	unsigned no_write_same:1;
 
+	/* The transport requires the LUN bits NOT to be stored in CDB[1] */
+	unsigned no_scsi2_lun:1;
+
 	/*
 	 * Optional work queue to be utilized by the transport
 	 */
Index: usb-3.16/drivers/scsi/scsi.c
===================================================================
--- usb-3.16.orig/drivers/scsi/scsi.c
+++ usb-3.16/drivers/scsi/scsi.c
@@ -678,7 +678,8 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
 	 * 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->device->scsi_level != SCSI_UNKNOWN &&
+	    !host->no_scsi2_lun) {
 		cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
 			       (cmd->device->lun << 5 & 0xe0);
 	}
Index: usb-3.16/drivers/usb/storage/usb.c
===================================================================
--- usb-3.16.orig/drivers/usb/storage/usb.c
+++ usb-3.16/drivers/usb/storage/usb.c
@@ -981,6 +981,14 @@ int usb_stor_probe2(struct us_data *us)
 	if (!(us->fflags & US_FL_SCM_MULT_TARG))
 		us_to_host(us)->max_id = 1;
 
+	/*
+	 * Like Windows, we won't store the LUN bits in CDB[1] for SCSI-2
+	 * devices using the Bulk-Only transport (even though this violates
+	 * the SCSI spec).
+	 */
+	if (us->transport == usb_stor_Bulk_transport)
+		us_to_host(us)->no_scsi2_lun = 1;
+
 	/* Find the endpoints and calculate pipe values */
 	result = get_pipes(us);
 	if (result)

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Martin K. Petersen]

>>>>> "Alan" == Alan Stern <stern@rowland.harvard.edu> writes:

Alan> Okay, here's a patch that implements the suggestion, except that I
Alan> put the flag in the Scsi_Host structure instead of the template.
Alan> This was to minimize the impact of the change.  Among the various
Alan> SCSI-over-USB transports, only the Bulk-Only transport gives the
Alan> LUN separately from the CDB.  I don't know if there are any
Alan> multi-LUN USB devices that don't use the Bulk-Only transport, but
Alan> if there are then they won't work if the LUN isn't stored in
Alan> CDB[1].

I'm in agreement with this approach.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Christoph Hellwig]

On Thu, Aug 21, 2014 at 05:43:41PM -0400, Martin K. Petersen wrote:
> Alan> Okay, here's a patch that implements the suggestion, except that I
> Alan> put the flag in the Scsi_Host structure instead of the template.
> Alan> This was to minimize the impact of the change.  Among the various
> Alan> SCSI-over-USB transports, only the Bulk-Only transport gives the
> Alan> LUN separately from the CDB.  I don't know if there are any
> Alan> multi-LUN USB devices that don't use the Bulk-Only transport, but
> Alan> if there are then they won't work if the LUN isn't stored in
> Alan> CDB[1].
> 
> I'm in agreement with this approach.

I like it too. One idea to unclutter the fastpath would be to have a
single flag that controls if the LUN is set which is based on the
host(-template) flag and the scsi level, which would allow us to remove
all the clutter around that area.

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Alan Stern]

On Thu, 21 Aug 2014, Christoph Hellwig wrote:

> On Thu, Aug 21, 2014 at 05:43:41PM -0400, Martin K. Petersen wrote:
> > Alan> Okay, here's a patch that implements the suggestion, except that I
> > Alan> put the flag in the Scsi_Host structure instead of the template.
> > Alan> This was to minimize the impact of the change.  Among the various
> > Alan> SCSI-over-USB transports, only the Bulk-Only transport gives the
> > Alan> LUN separately from the CDB.  I don't know if there are any
> > Alan> multi-LUN USB devices that don't use the Bulk-Only transport, but
> > Alan> if there are then they won't work if the LUN isn't stored in
> > Alan> CDB[1].
> > 
> > I'm in agreement with this approach.
> 
> I like it too. One idea to unclutter the fastpath would be to have a
> single flag that controls if the LUN is set which is based on the
> host(-template) flag and the scsi level, which would allow us to remove
> all the clutter around that area.

Good idea.  An enhanced patch is below.  If I can get a Tested-By: from
Tiziano and one or two Acked-By: responses, I'll submit this for the
current and stable kernels.

Sending the initial INQUIRY command to LUNs larger than 0 involves a
chicken-and-egg problem -- we don't know whether to fill in the LUN
bits in the command until we know the SCSI level, and we don't know the
SCSI level until the INQUIRY response is received.  The solution we 
have been using is to store the most recently discovered level in the 
target structure, and use it as a default.  If probing starts with LUN 
0, and if all the LUNs have similar levels, this ought to work.

Except for one thing: The code does store the default level in the
scsi_target, but forgets to copy it back into each newly allocated
scsi_device!  I added a line to do that into the patch.

Alan Stern



Index: usb-3.16/include/scsi/scsi_host.h
===================================================================
--- usb-3.16.orig/include/scsi/scsi_host.h
+++ usb-3.16/include/scsi/scsi_host.h
@@ -695,6 +695,9 @@ struct Scsi_Host {
 	/* The controller does not support WRITE SAME */
 	unsigned no_write_same:1;
 
+	/* The transport requires the LUN bits NOT to be stored in CDB[1] */
+	unsigned no_scsi2_lun_in_cdb:1;
+
 	/*
 	 * Optional work queue to be utilized by the transport
 	 */
Index: usb-3.16/include/scsi/scsi_device.h
===================================================================
--- usb-3.16.orig/include/scsi/scsi_device.h
+++ usb-3.16/include/scsi/scsi_device.h
@@ -174,6 +174,7 @@ struct scsi_device {
 	unsigned wce_default_on:1;	/* Cache is ON by default */
 	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
 	unsigned broken_fua:1;		/* Don't set FUA bit */
+	unsigned lun_in_cdb:1;		/* Store LUN bits in CDB[1] */
 
 	atomic_t disk_events_disable_depth; /* disable depth for disk events */
 
Index: usb-3.16/drivers/scsi/scsi.c
===================================================================
--- usb-3.16.orig/drivers/scsi/scsi.c
+++ usb-3.16/drivers/scsi/scsi.c
@@ -674,14 +674,10 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
 		goto out;
 	}
 
-	/* 
-	 * If SCSI-2 or lower, store the LUN value in cmnd.
-	 */
-	if (cmd->device->scsi_level <= SCSI_2 &&
-	    cmd->device->scsi_level != SCSI_UNKNOWN) {
+	/* Store the LUN value in cmnd, if needed. */
+	if (cmd->device->lun_in_cdb)
 		cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
 			       (cmd->device->lun << 5 & 0xe0);
-	}
 
 	scsi_log_send(cmd);
 
Index: usb-3.16/drivers/scsi/scsi_scan.c
===================================================================
--- usb-3.16.orig/drivers/scsi/scsi_scan.c
+++ usb-3.16/drivers/scsi/scsi_scan.c
@@ -257,6 +257,7 @@ static struct scsi_device *scsi_alloc_sd
 
 	sdev->sdev_gendev.parent = get_device(&starget->dev);
 	sdev->sdev_target = starget;
+	sdev->scsi_level = starget->scsi_level;
 
 	/* usually NULL and set by ->slave_alloc instead */
 	sdev->hostdata = hostdata;
@@ -735,6 +736,15 @@ static int scsi_probe_lun(struct scsi_de
 		sdev->scsi_level++;
 	sdev->sdev_target->scsi_level = sdev->scsi_level;
 
+	/*
+	 * If SCSI-2 or lower, and if the transport requires it,
+	 * store the LUN value in CDB[1].
+	 */
+	if (sdev->scsi_level <= SCSI_2 &&
+	    sdev->scsi_level != SCSI_UNKNOWN &&
+	    !sdev->host->no_scsi2_lun_in_cdb)
+		sdev->lun_in_cdb = 1;
+
 	return 0;
 }
 
Index: usb-3.16/drivers/usb/storage/usb.c
===================================================================
--- usb-3.16.orig/drivers/usb/storage/usb.c
+++ usb-3.16/drivers/usb/storage/usb.c
@@ -981,6 +981,14 @@ int usb_stor_probe2(struct us_data *us)
 	if (!(us->fflags & US_FL_SCM_MULT_TARG))
 		us_to_host(us)->max_id = 1;
 
+	/*
+	 * Like Windows, we won't store the LUN bits in CDB[1] for SCSI-2
+	 * devices using the Bulk-Only transport (even though this violates
+	 * the SCSI spec).
+	 */
+	if (us->transport == usb_stor_Bulk_transport)
+		us_to_host(us)->no_scsi2_lun_in_cdb = 1;
+
 	/* Find the endpoints and calculate pipe values */
 	result = get_pipes(us);
 	if (result)

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Christoph Hellwig]

On Fri, Aug 22, 2014 at 10:53:42AM -0400, Alan Stern wrote:
> Good idea.  An enhanced patch is below.  If I can get a Tested-By: from
> Tiziano and one or two Acked-By: responses, I'll submit this for the
> current and stable kernels.
> 
> Sending the initial INQUIRY command to LUNs larger than 0 involves a
> chicken-and-egg problem -- we don't know whether to fill in the LUN
> bits in the command until we know the SCSI level, and we don't know the
> SCSI level until the INQUIRY response is received.  The solution we 
> have been using is to store the most recently discovered level in the 
> target structure, and use it as a default.  If probing starts with LUN 
> 0, and if all the LUNs have similar levels, this ought to work.
> 
> Except for one thing: The code does store the default level in the
> scsi_target, but forgets to copy it back into each newly allocated
> scsi_device!  I added a line to do that into the patch.

Looks good to me,

Acked-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>

Do you want to queue this up in the USB tree?  From the looks of what I
have on my plate so far it seems like we could avoid conflicts with it
in the SCSI tree.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Alan Stern]

On Fri, 22 Aug 2014, Christoph Hellwig wrote:

> On Fri, Aug 22, 2014 at 10:53:42AM -0400, Alan Stern wrote:
> > Good idea.  An enhanced patch is below.  If I can get a Tested-By: from
> > Tiziano and one or two Acked-By: responses, I'll submit this for the
> > current and stable kernels.
> > 
> > Sending the initial INQUIRY command to LUNs larger than 0 involves a
> > chicken-and-egg problem -- we don't know whether to fill in the LUN
> > bits in the command until we know the SCSI level, and we don't know the
> > SCSI level until the INQUIRY response is received.  The solution we 
> > have been using is to store the most recently discovered level in the 
> > target structure, and use it as a default.  If probing starts with LUN 
> > 0, and if all the LUNs have similar levels, this ought to work.
> > 
> > Except for one thing: The code does store the default level in the
> > scsi_target, but forgets to copy it back into each newly allocated
> > scsi_device!  I added a line to do that into the patch.
> 
> Looks good to me,
> 
> Acked-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> 
> Do you want to queue this up in the USB tree?  From the looks of what I
> have on my plate so far it seems like we could avoid conflicts with it
> in the SCSI tree.

After hearing from Tiziano, I'll submit the patch to the SCSI tree.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Martin K. Petersen]

>>>>> "Alan" == Alan Stern <stern@rowland.harvard.edu> writes:

Alan> Sending the initial INQUIRY command to LUNs larger than 0 involves
Alan> a chicken-and-egg problem -- we don't know whether to fill in the
Alan> LUN bits in the command until we know the SCSI level, and we don't
Alan> know the SCSI level until the INQUIRY response is received.  The
Alan> solution we have been using is to store the most recently
Alan> discovered level in the target structure, and use it as a default.
Alan> If probing starts with LUN 0, and if all the LUNs have similar
Alan> levels, this ought to work.

Looks good, Alan.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[James Bottomley]

On Fri, 2014-08-22 at 10:53 -0400, Alan Stern wrote:
> On Thu, 21 Aug 2014, Christoph Hellwig wrote:
> 
> > On Thu, Aug 21, 2014 at 05:43:41PM -0400, Martin K. Petersen wrote:
> > > Alan> Okay, here's a patch that implements the suggestion, except that I
> > > Alan> put the flag in the Scsi_Host structure instead of the template.
> > > Alan> This was to minimize the impact of the change.  Among the various
> > > Alan> SCSI-over-USB transports, only the Bulk-Only transport gives the
> > > Alan> LUN separately from the CDB.  I don't know if there are any
> > > Alan> multi-LUN USB devices that don't use the Bulk-Only transport, but
> > > Alan> if there are then they won't work if the LUN isn't stored in
> > > Alan> CDB[1].
> > > 
> > > I'm in agreement with this approach.
> > 
> > I like it too. One idea to unclutter the fastpath would be to have a
> > single flag that controls if the LUN is set which is based on the
> > host(-template) flag and the scsi level, which would allow us to remove
> > all the clutter around that area.
> 
> Good idea.  An enhanced patch is below.  If I can get a Tested-By: from
> Tiziano and one or two Acked-By: responses, I'll submit this for the
> current and stable kernels.
> 
> Sending the initial INQUIRY command to LUNs larger than 0 involves a
> chicken-and-egg problem -- we don't know whether to fill in the LUN
> bits in the command until we know the SCSI level, and we don't know the
> SCSI level until the INQUIRY response is received.  The solution we 
> have been using is to store the most recently discovered level in the 
> target structure, and use it as a default.  If probing starts with LUN 
> 0, and if all the LUNs have similar levels, this ought to work.
> 
> Except for one thing: The code does store the default level in the
> scsi_target, but forgets to copy it back into each newly allocated
> scsi_device!  I added a line to do that into the patch.
> 
> Alan Stern
> 
> 
> 
> Index: usb-3.16/include/scsi/scsi_host.h
> ===================================================================
> --- usb-3.16.orig/include/scsi/scsi_host.h
> +++ usb-3.16/include/scsi/scsi_host.h
> @@ -695,6 +695,9 @@ struct Scsi_Host {
>  	/* The controller does not support WRITE SAME */
>  	unsigned no_write_same:1;
>  
> +	/* The transport requires the LUN bits NOT to be stored in CDB[1] */
> +	unsigned no_scsi2_lun_in_cdb:1;

I think Christoph mentioned shortening this flag length, but personally
I'm not that bothered.

>  	/*
>  	 * Optional work queue to be utilized by the transport
>  	 */
> Index: usb-3.16/include/scsi/scsi_device.h
> ===================================================================
> --- usb-3.16.orig/include/scsi/scsi_device.h
> +++ usb-3.16/include/scsi/scsi_device.h
> @@ -174,6 +174,7 @@ struct scsi_device {
>  	unsigned wce_default_on:1;	/* Cache is ON by default */
>  	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
>  	unsigned broken_fua:1;		/* Don't set FUA bit */
> +	unsigned lun_in_cdb:1;		/* Store LUN bits in CDB[1] */
>  
>  	atomic_t disk_events_disable_depth; /* disable depth for disk events */
>  
> Index: usb-3.16/drivers/scsi/scsi.c
> ===================================================================
> --- usb-3.16.orig/drivers/scsi/scsi.c
> +++ usb-3.16/drivers/scsi/scsi.c
> @@ -674,14 +674,10 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
>  		goto out;
>  	}
>  
> -	/* 
> -	 * If SCSI-2 or lower, store the LUN value in cmnd.
> -	 */
> -	if (cmd->device->scsi_level <= SCSI_2 &&
> -	    cmd->device->scsi_level != SCSI_UNKNOWN) {
> +	/* Store the LUN value in cmnd, if needed. */
> +	if (cmd->device->lun_in_cdb)
>  		cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
>  			       (cmd->device->lun << 5 & 0xe0);
> -	}
>  
>  	scsi_log_send(cmd);
>  
> Index: usb-3.16/drivers/scsi/scsi_scan.c
> ===================================================================
> --- usb-3.16.orig/drivers/scsi/scsi_scan.c
> +++ usb-3.16/drivers/scsi/scsi_scan.c
> @@ -257,6 +257,7 @@ static struct scsi_device *scsi_alloc_sd
>  
>  	sdev->sdev_gendev.parent = get_device(&starget->dev);
>  	sdev->sdev_target = starget;
> +	sdev->scsi_level = starget->scsi_level;

Why is this necessary?  Isn't this set correctly in scsi_probe_lun?  The
target level is actually set from the device level.

Other than this, I'm fine with the code ... you can add the acked by
from me when we resolve the above question.

James


>  	/* usually NULL and set by ->slave_alloc instead */
>  	sdev->hostdata = hostdata;
> @@ -735,6 +736,15 @@ static int scsi_probe_lun(struct scsi_de
>  		sdev->scsi_level++;
>  	sdev->sdev_target->scsi_level = sdev->scsi_level;
> +	/*
> +	 * If SCSI-2 or lower, and if the transport requires it,
> +	 * store the LUN value in CDB[1].
> +	 */
> +	if (sdev->scsi_level <= SCSI_2 &&
> +	    sdev->scsi_level != SCSI_UNKNOWN &&
> +	    !sdev->host->no_scsi2_lun_in_cdb)
> +		sdev->lun_in_cdb = 1;
> +
>  	return 0;
>  }
>  
> Index: usb-3.16/drivers/usb/storage/usb.c
> ===================================================================
> --- usb-3.16.orig/drivers/usb/storage/usb.c
> +++ usb-3.16/drivers/usb/storage/usb.c
> @@ -981,6 +981,14 @@ int usb_stor_probe2(struct us_data *us)
>  	if (!(us->fflags & US_FL_SCM_MULT_TARG))
>  		us_to_host(us)->max_id = 1;
>  
> +	/*
> +	 * Like Windows, we won't store the LUN bits in CDB[1] for SCSI-2
> +	 * devices using the Bulk-Only transport (even though this violates
> +	 * the SCSI spec).
> +	 */
> +	if (us->transport == usb_stor_Bulk_transport)
> +		us_to_host(us)->no_scsi2_lun_in_cdb = 1;
> +
>  	/* Find the endpoints and calculate pipe values */
>  	result = get_pipes(us);
>  	if (result)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Alan Stern]

On Fri, 22 Aug 2014, James Bottomley wrote:

> On Fri, 2014-08-22 at 10:53 -0400, Alan Stern wrote:

> > Sending the initial INQUIRY command to LUNs larger than 0 involves a
> > chicken-and-egg problem -- we don't know whether to fill in the LUN
> > bits in the command until we know the SCSI level, and we don't know the
> > SCSI level until the INQUIRY response is received.  The solution we 
> > have been using is to store the most recently discovered level in the 
> > target structure, and use it as a default.  If probing starts with LUN 
> > 0, and if all the LUNs have similar levels, this ought to work.
> > 
> > Except for one thing: The code does store the default level in the
> > scsi_target, but forgets to copy it back into each newly allocated
> > scsi_device!  I added a line to do that into the patch.

> > --- usb-3.16.orig/include/scsi/scsi_host.h
> > +++ usb-3.16/include/scsi/scsi_host.h
> > @@ -695,6 +695,9 @@ struct Scsi_Host {
> >  	/* The controller does not support WRITE SAME */
> >  	unsigned no_write_same:1;
> >  
> > +	/* The transport requires the LUN bits NOT to be stored in CDB[1] */
> > +	unsigned no_scsi2_lun_in_cdb:1;
> 
> I think Christoph mentioned shortening this flag length, but personally
> I'm not that bothered.

It was shorter in the original version of the patch, but I decided to
make it a little longer to match the name of the new scsi_device flag
added in this version.

> > --- usb-3.16.orig/drivers/scsi/scsi_scan.c
> > +++ usb-3.16/drivers/scsi/scsi_scan.c
> > @@ -257,6 +257,7 @@ static struct scsi_device *scsi_alloc_sd
> >  
> >  	sdev->sdev_gendev.parent = get_device(&starget->dev);
> >  	sdev->sdev_target = starget;
> > +	sdev->scsi_level = starget->scsi_level;
> 
> Why is this necessary?  Isn't this set correctly in scsi_probe_lun?  The
> target level is actually set from the device level.

The reason was mentioned in the quoted text at the start of this email.

In greater detail: Yes, sdev->scsi_level is set correctly in
scsi_probe_lun, but only _after_ the INQUIRY data has been received
(because the level is part of the INQUIRY data).  So how can the
INQUIRY CDB be set up correctly before we know the device's level?

(When the current code sends an INQUIRY for LUN 1, it will _not_ put 
the LUN bits in CDB[1] -- even if LUN 0 reported SCSI-2.  That doesn't 
seem right.)

My answer is to use a default level copied from the target.  The
target's level in turn was set from the previously probed device...  
which means that the first device to be probed gets a default level of
SCSI-2.  That's okay as long as the first device probed is LUN 0.

Hmmm, but now I see the patch doesn't set a default value for the new
sdev->lun_in_cdb flag.  That needs to be fixed...

> Other than this, I'm fine with the code ... you can add the acked by
> from me when we resolve the above question.

Okay.  It's true that this issue is only tangentially related to the 
main point of the patch.  It could be removed and addressed later.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Christoph Hellwig]

On Fri, Aug 22, 2014 at 01:29:32PM -0400, Alan Stern wrote:
> > Other than this, I'm fine with the code ... you can add the acked by
> > from me when we resolve the above question.
> 
> Okay.  It's true that this issue is only tangentially related to the 
> main point of the patch.  It could be removed and addressed later.

Just make it a separate patch and send it along..
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Alan Stern]

On Sun, 24 Aug 2014, Christoph Hellwig wrote:

> On Fri, Aug 22, 2014 at 01:29:32PM -0400, Alan Stern wrote:
> > > Other than this, I'm fine with the code ... you can add the acked by
> > > from me when we resolve the above question.
> > 
> > Okay.  It's true that this issue is only tangentially related to the 
> > main point of the patch.  It could be removed and addressed later.
> 
> Just make it a separate patch and send it along..

All right.  But I still want to know first whether the patch really
fixes the original problem.

Tiziano, do you intend to test this patch?

James, can you explain how the INQUIRY command in scsi_probe_lun()  
managed to work back in the days when multi-lun SCSI-2 devices were
common?  sdev->scsi_level doesn't get set when sdev is allocated, so it
initially contains 0, so the LUN bits won't get filled in when the
first INQUIRY command is sent.  Then how could the target know which
logical unit the INQUIRY was meant for?

Alan Stern

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[James Bottomley]

On Mon, 2014-08-25 at 10:44 -0400, Alan Stern wrote:

> James, can you explain how the INQUIRY command in scsi_probe_lun()  
> managed to work back in the days when multi-lun SCSI-2 devices were
> common?  sdev->scsi_level doesn't get set when sdev is allocated, so it
> initially contains 0, so the LUN bits won't get filled in when the
> first INQUIRY command is sent.  Then how could the target know which
> logical unit the INQUIRY was meant for?

Best guess, some patches over the course of time altered the way we do
this and no-one noticed.  I think it was probably the introduction of
the unknown SCSI data level that caused the breakage.

Historically, the LUN in CMD bits is left over from SCSI-1; it was
incorporated into SCSI-2 for backward compatibility (even though SCSI-2
moved the LUN specification to the identify message).  In SCSI-3 and
beyond, those bits were obsoleted and transports took sole
responsibility for LUN handling.  I'm fairly certain all the SCSI-1
devices relying on this behaviour have long ago migrated to the great
data centre in the sky.

Alan's fix looks reasonable because we probe LUN 0 first (for SCSI-1 and
2 which has parallel scanning), which is why it doesn't matter (the bits
are set to zero) and once we have LUN 0 we propagate the data to the
target and make it the basis for future checks.  I'd like to see a
comment explaining this in the code, though ...
James

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Alan Stern]

On Mon, 25 Aug 2014, James Bottomley wrote:

> On Mon, 2014-08-25 at 10:44 -0400, Alan Stern wrote:
> 
> > James, can you explain how the INQUIRY command in scsi_probe_lun()  
> > managed to work back in the days when multi-lun SCSI-2 devices were
> > common?  sdev->scsi_level doesn't get set when sdev is allocated, so it
> > initially contains 0, so the LUN bits won't get filled in when the
> > first INQUIRY command is sent.  Then how could the target know which
> > logical unit the INQUIRY was meant for?
> 
> Best guess, some patches over the course of time altered the way we do
> this and no-one noticed.  I think it was probably the introduction of
> the unknown SCSI data level that caused the breakage.

Heh.  The change was made by commit 4d7db04a7a69 ([SCSI] add
SCSI_UNKNOWN and LUN transfer limit restrictions) back in 2006 -- the
2.6.17 kernel.  If nobody has complained in all this time then it's
probably not worth changing.

> Historically, the LUN in CMD bits is left over from SCSI-1; it was
> incorporated into SCSI-2 for backward compatibility (even though SCSI-2
> moved the LUN specification to the identify message).  In SCSI-3 and
> beyond, those bits were obsoleted and transports took sole
> responsibility for LUN handling.  I'm fairly certain all the SCSI-1
> devices relying on this behaviour have long ago migrated to the great
> data centre in the sky.
> 
> Alan's fix looks reasonable because we probe LUN 0 first (for SCSI-1 and
> 2 which has parallel scanning), which is why it doesn't matter (the bits
> are set to zero) and once we have LUN 0 we propagate the data to the
> target and make it the basis for future checks.  I'd like to see a
> comment explaining this in the code, though ...

If you think it would be a good idea, I could put it into a separate 
patch with an explanatory comment.  At the moment, I'm inclined to 
forget about it.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[Alan Stern]

On Mon, 25 Aug 2014, Alan Stern wrote:

> On Mon, 25 Aug 2014, James Bottomley wrote:
> 
> > On Mon, 2014-08-25 at 10:44 -0400, Alan Stern wrote:
> > 
> > > James, can you explain how the INQUIRY command in scsi_probe_lun()  
> > > managed to work back in the days when multi-lun SCSI-2 devices were
> > > common?  sdev->scsi_level doesn't get set when sdev is allocated, so it
> > > initially contains 0, so the LUN bits won't get filled in when the
> > > first INQUIRY command is sent.  Then how could the target know which
> > > logical unit the INQUIRY was meant for?
> > 
> > Best guess, some patches over the course of time altered the way we do
> > this and no-one noticed.  I think it was probably the introduction of
> > the unknown SCSI data level that caused the breakage.
> 
> Heh.  The change was made by commit 4d7db04a7a69 ([SCSI] add
> SCSI_UNKNOWN and LUN transfer limit restrictions) back in 2006 -- the
> 2.6.17 kernel.  If nobody has complained in all this time then it's
> probably not worth changing.

It turns out the code is already there, and I didn't realize because I
was looking at the wrong source file.  scsi_sysfs_device_initialize()
already does:

	sdev->scsi_level = starget->scsi_level;

Here's the update to the patch, adding an appropriate comment and
setting the new sdev->lun_in_sdb flag properly:

Alan Stern


Index: usb-3.16/drivers/scsi/scsi_sysfs.c
===================================================================
--- usb-3.16.orig/drivers/scsi/scsi_sysfs.c
+++ usb-3.16/drivers/scsi/scsi_sysfs.c
@@ -1238,7 +1238,19 @@ void scsi_sysfs_device_initialize(struct
 	sdev->sdev_dev.class = &sdev_class;
 	dev_set_name(&sdev->sdev_dev, "%d:%d:%d:%d",
 		     sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
+	/*
+	 * Get a default scsi_level from the target (derived from sibling
+	 * devices).  This is the best we can do for guessing how to set
+	 * sdev->lun_in_cdb for the initial INQUIRY command.  For LUN 0 the
+	 * setting doesn't matter, because all the bits are zero anyway.
+	 * But it does matter for higher LUNs.
+	 */
 	sdev->scsi_level = starget->scsi_level;
+	if (sdev->scsi_level <= SCSI_2 &&
+			sdev->scsi_level != SCSI_UNKNOWN &&
+			!shost->no_scsi2_lun_in_cdb)
+		sdev->lun_in_cdb = 1;
+
 	transport_setup_device(&sdev->sdev_gendev);
 	spin_lock_irqsave(shost->host_lock, flags);
 	list_add_tail(&sdev->same_target_siblings, &starget->devices);

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[James Bottomley]

On Mon, 2014-08-25 at 17:19 -0400, Alan Stern wrote:
> On Mon, 25 Aug 2014, Alan Stern wrote:
> 
> > On Mon, 25 Aug 2014, James Bottomley wrote:
> > 
> > > On Mon, 2014-08-25 at 10:44 -0400, Alan Stern wrote:
> > > 
> > > > James, can you explain how the INQUIRY command in scsi_probe_lun()  
> > > > managed to work back in the days when multi-lun SCSI-2 devices were
> > > > common?  sdev->scsi_level doesn't get set when sdev is allocated, so it
> > > > initially contains 0, so the LUN bits won't get filled in when the
> > > > first INQUIRY command is sent.  Then how could the target know which
> > > > logical unit the INQUIRY was meant for?
> > > 
> > > Best guess, some patches over the course of time altered the way we do
> > > this and no-one noticed.  I think it was probably the introduction of
> > > the unknown SCSI data level that caused the breakage.
> > 
> > Heh.  The change was made by commit 4d7db04a7a69 ([SCSI] add
> > SCSI_UNKNOWN and LUN transfer limit restrictions) back in 2006 -- the
> > 2.6.17 kernel.  If nobody has complained in all this time then it's
> > probably not worth changing.
> 
> It turns out the code is already there, and I didn't realize because I
> was looking at the wrong source file.  scsi_sysfs_device_initialize()
> already does:
> 
> 	sdev->scsi_level = starget->scsi_level;
> 
> Here's the update to the patch, adding an appropriate comment and
> setting the new sdev->lun_in_sdb flag properly:

Looks good.  Add your signed-off-by and you can add my acked-by as well.

James

> 
> Index: usb-3.16/drivers/scsi/scsi_sysfs.c
> ===================================================================
> --- usb-3.16.orig/drivers/scsi/scsi_sysfs.c
> +++ usb-3.16/drivers/scsi/scsi_sysfs.c
> @@ -1238,7 +1238,19 @@ void scsi_sysfs_device_initialize(struct
>  	sdev->sdev_dev.class = &sdev_class;
>  	dev_set_name(&sdev->sdev_dev, "%d:%d:%d:%d",
>  		     sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
> +	/*
> +	 * Get a default scsi_level from the target (derived from sibling
> +	 * devices).  This is the best we can do for guessing how to set
> +	 * sdev->lun_in_cdb for the initial INQUIRY command.  For LUN 0 the
> +	 * setting doesn't matter, because all the bits are zero anyway.
> +	 * But it does matter for higher LUNs.
> +	 */
>  	sdev->scsi_level = starget->scsi_level;
> +	if (sdev->scsi_level <= SCSI_2 &&
> +			sdev->scsi_level != SCSI_UNKNOWN &&
> +			!shost->no_scsi2_lun_in_cdb)
> +		sdev->lun_in_cdb = 1;
> +
>  	transport_setup_device(&sdev->sdev_gendev);
>  	spin_lock_irqsave(shost->host_lock, flags);
>  	list_add_tail(&sdev->same_target_siblings, &starget->devices);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #4 from d gilbert <dgilbert@interlog.com> ---
On 14-07-29 05:57 PM, bugzilla-daemon@bugzilla.kernel.org wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=80711
>
> Alan <alan@lxorguk.ukuu.org.uk> changed:
>
>             What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                   CC|                            |alan@lxorguk.ukuu.org.uk
>              Summary|SG_FLAG_LUN_INHIBIT is no   |[PATCH]SG_FLAG_LUN_INHIBIT
>                     |longer implemented and      |is no longer implemented
>                     |there's not way to prevent  |and there's not way to
>                     |the kernel from using the   |prevent the kernel from
>                     |2nd cdb byte for the LUN    |using the 2nd cdb byte for
>                     |                            |the LUN
>
> --- Comment #3 from Alan <alan@lxorguk.ukuu.org.uk> ---
> Patches need to go to the mailing list and/or maintainers. See
> Documentation/SubmittingPatches.

In my original rework of the sg driver (1998) it did what
was being requested in this patch. Then around 10 years ago
changes were added to the SCSI subsystem to strip this
capability from the sg driver. Since then about 5 attempts (my
guess) have been made to re-instate this capability, this
being the most recent.

The use case is almost always the same: specialized USB
storage devices. USB is not going away. Does anyone see
a change in "company" policy in this regard?


If not and since I'm told black lists and the like won't
work, my advice for the record is to use FreeBSD or Windows
for tools that need this capability.

Doug Gilbert

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #5 from Alan Stern <stern@rowland.harvard.edu> ---
On Wed, 6 Aug 2014, Christoph Hellwig wrote:

> On Wed, Aug 06, 2014 at 03:29:47PM +0200, Douglas Gilbert wrote:
> > If not and since I'm told black lists and the like won't
> > work, my advice for the record is to use FreeBSD or Windows
> > for tools that need this capability.
> 
> I doubt either of them forces users to hack up flags for these cases.

Why was this change needed in the first place?  There's no explanation 
in the patch itself.

> At least for windows I suspect it just never sends the LUN encoded
> in the CDB and treats USB devices special instead of our insistance
> on pretending they are SCSI-2.

We no longer pretend that USB mass-storage devices have any particular 
SCSI level.  See commit 09b6b51b0b6c.

> Maybe some of the USB people have on the wire traces or access to
> device or windows documentation on this?

Most likely it varies with the version of Windows and the INQUIRY data
returned by the device.

I can obtain hardware traces for the kinds of devices and computers 
lying around here.  But what sort of combinations should I test?

Alan Stern

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #6 from Alan Stern <stern@rowland.harvard.edu> ---
Please don't remove names from the CC: list; use Reply-To-All.  I had 
to go back and add all the names back in.

On Wed, 6 Aug 2014, Tiziano Bacocco wrote:

> Test with alcor based USB flash drives, linux 3.16 will remove the 3 msb of
> the CDB byte when using SG raw

Sure, but isn't that what you want it to do?  Doesn't the flash drive 
use those bits to hold the LUN number?

Alan Stern


> Il 06/ago/2014 22:02 "Alan Stern" <stern@rowland.harvard.edu> ha scritto:
>
> > On Wed, 6 Aug 2014, Christoph Hellwig wrote:
> >
> > > On Wed, Aug 06, 2014 at 03:29:47PM +0200, Douglas Gilbert wrote:
> > > > If not and since I'm told black lists and the like won't
> > > > work, my advice for the record is to use FreeBSD or Windows
> > > > for tools that need this capability.
> > >
> > > I doubt either of them forces users to hack up flags for these cases.
> >
> > Why was this change needed in the first place?  There's no explanation
> > in the patch itself.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #7 from Tiziano Bacocco <tiziano.bacocco@gmail.com> ---
Not when issuing vendor specific commands , even if the flash drive has only 1
LUN , there's the need of using these bits with LUN numbers higher than the
reported number of LUNs

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #8 from Alan Stern <stern@rowland.harvard.edu> ---
On Wed, 6 Aug 2014, Christoph Hellwig wrote:

> On Wed, Aug 06, 2014 at 04:02:22PM -0400, Alan Stern wrote:
> > > I doubt either of them forces users to hack up flags for these cases.
> > 
> > Why was this change needed in the first place?  There's no explanation 
> > in the patch itself.
> 
> Which chance?  The one to not support SG_FLAG_LUN_INHIBIT?

No, the patch that started this Bugzilla entry.  Tiziano says it is 
needed in order to send vendor-specific commands that use the LUN bits 
in CDB[1].

> > > At least for windows I suspect it just never sends the LUN encoded
> > > in the CDB and treats USB devices special instead of our insistance
> > > on pretending they are SCSI-2.
> > 
> > We no longer pretend that USB mass-storage devices have any particular 
> > SCSI level.  See commit 09b6b51b0b6c.
> 
> So the origina reported device must report SCSI-2 all by itself if he's
> running a recent kernel, ok.
> 
> > > Maybe some of the USB people have on the wire traces or access to
> > > device or windows documentation on this?
> > 
> > Most likely it varies with the version of Windows and the INQUIRY data
> > returned by the device.
> > 
> > I can obtain hardware traces for the kinds of devices and computers 
> > lying around here.  But what sort of combinations should I test?
> 
> I'd mostly be interested to see if it actualy encodes the LUN in the CDB
> for any USB multi-LUN device.

I tried connecting a Linux mass-storage gadget with two logical units
to a host PC running Windows 7.  The host scanned the first logical
unit and completely ignored the second!  Didn't even send an INQUIRY 
command.

So the question remains unanswered...

Can someone tell me if anything special is needed to make Windows
recognize logical units beyond the first?

Alan Stern

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #9 from Tiziano Bacocco <tiziano.bacocco@gmail.com> ---
(In reply to Alan Stern from comment #8)
> On Wed, 6 Aug 2014, Christoph Hellwig wrote:
> 
> > On Wed, Aug 06, 2014 at 04:02:22PM -0400, Alan Stern wrote:
> > > > I doubt either of them forces users to hack up flags for these cases.
> > > 
> > > Why was this change needed in the first place?  There's no explanation 
> > > in the patch itself.
> > 
> > Which chance?  The one to not support SG_FLAG_LUN_INHIBIT?
> 
> No, the patch that started this Bugzilla entry.  Tiziano says it is 
> needed in order to send vendor-specific commands that use the LUN bits 
> in CDB[1].
> 
> > > > At least for windows I suspect it just never sends the LUN encoded
> > > > in the CDB and treats USB devices special instead of our insistance
> > > > on pretending they are SCSI-2.
> > > 
> > > We no longer pretend that USB mass-storage devices have any particular 
> > > SCSI level.  See commit 09b6b51b0b6c.
> > 
> > So the origina reported device must report SCSI-2 all by itself if he's
> > running a recent kernel, ok.
> > 
> > > > Maybe some of the USB people have on the wire traces or access to
> > > > device or windows documentation on this?
> > > 
> > > Most likely it varies with the version of Windows and the INQUIRY data
> > > returned by the device.
> > > 
> > > I can obtain hardware traces for the kinds of devices and computers 
> > > lying around here.  But what sort of combinations should I test?
> > 
> > I'd mostly be interested to see if it actualy encodes the LUN in the CDB
> > for any USB multi-LUN device.
> 
> I tried connecting a Linux mass-storage gadget with two logical units
> to a host PC running Windows 7.  The host scanned the first logical
> unit and completely ignored the second!  Didn't even send an INQUIRY 
> command.
> 
> So the question remains unanswered...
> 
> Can someone tell me if anything special is needed to make Windows
> recognize logical units beyond the first?
> 
> Alan Stern

That's weird , old android phones which use multiple LUNs and Linux usb gadgets
, had internal memory and SD card , and they worked on windows

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #10 from Alan Stern <stern@rowland.harvard.edu> ---
On Tue, 19 Aug 2014, Christoph Hellwig wrote:

> On Thu, Aug 07, 2014 at 11:58:37AM -0400, Alan Stern wrote:
> > > On Wed, Aug 06, 2014 at 04:02:22PM -0400, Alan Stern wrote:
> > > > > I doubt either of them forces users to hack up flags for these cases.
> > > > 
> > > > Why was this change needed in the first place?  There's no explanation 
> > > > in the patch itself.
> > > 
> > > Which chance?  The one to not support SG_FLAG_LUN_INHIBIT?
> > 
> > No, the patch that started this Bugzilla entry.  Tiziano says it is 
> > needed in order to send vendor-specific commands that use the LUN bits 
> > in CDB[1].
> 
> Yes, I'd really like to know the exact scenario.  What kind of command
> is this and who sends it?

I don't know what the command is, but Tiziano is sending it via the
SCSI-generic (sg) interface.

In the meantime, I have made a little progress with Windows.  It turns
out there are two reasons my earlier test didn't work:

    I didn't bother to set up a valid serial number for the test
    device, and Windows wants to see the serial number.

    Windows wants at least one of the logical units to be
    removable.

After those issues were fixed, the host did recognize both logical 
units.  I tested with two devices, one reporting an ANSI SCSI level of 
0 and the other 2.  In both cases, Windows 7 does _not_ stick the LUN 
value into the high bits of CDB[1].

This suggests that we shouldn't be doing that either, for USB
mass-storage devices.  But I'm reluctant to change it because of the
possibility of regressions, not to mention the fact that it would
violate the SCSI spec.

Suggestions?

Alan Stern

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #11 from d gilbert <dgilbert@interlog.com> ---
On 14-08-20 03:15 PM, Alan Stern wrote:
> On Tue, 19 Aug 2014, Christoph Hellwig wrote:
>
>> On Thu, Aug 07, 2014 at 11:58:37AM -0400, Alan Stern wrote:
>>>> On Wed, Aug 06, 2014 at 04:02:22PM -0400, Alan Stern wrote:
>>>>>> I doubt either of them forces users to hack up flags for these cases.
>>>>>
>>>>> Why was this change needed in the first place?  There's no explanation
>>>>> in the patch itself.
>>>>
>>>> Which chance?  The one to not support SG_FLAG_LUN_INHIBIT?
>>>
>>> No, the patch that started this Bugzilla entry.  Tiziano says it is
>>> needed in order to send vendor-specific commands that use the LUN bits
>>> in CDB[1].
>>
>> Yes, I'd really like to know the exact scenario.  What kind of command
>> is this and who sends it?
>
> I don't know what the command is, but Tiziano is sending it via the
> SCSI-generic (sg) interface.
>
> In the meantime, I have made a little progress with Windows.  It turns
> out there are two reasons my earlier test didn't work:
>
> 	I didn't bother to set up a valid serial number for the test
> 	device, and Windows wants to see the serial number.
>
> 	Windows wants at least one of the logical units to be
> 	removable.
>
> After those issues were fixed, the host did recognize both logical
> units.  I tested with two devices, one reporting an ANSI SCSI level of
> 0 and the other 2.  In both cases, Windows 7 does _not_ stick the LUN
> value into the high bits of CDB[1].
>
> This suggests that we shouldn't be doing that either, for USB
> mass-storage devices.  But I'm reluctant to change it because of the
> possibility of regressions, not to mention the fact that it would
> violate the SCSI spec.
>
> Suggestions?

Perhaps we could add another bit flag in struct
scsi_host_template such as:
     unsigned int transport_says_dont_scsi2_lun_cmd:1;

then drivers/usb/storage/scsiglue.c could set that
bit in its usb_stor_host_template and
drivers/scsi/scsi.c could take heed (and not mask
cmd->cmnd[1] with the LUN).

Doug Gilbert

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #12 from Alan Stern <stern@rowland.harvard.edu> ---
On Thu, 21 Aug 2014, Christoph Hellwig wrote:

> On Thu, Aug 21, 2014 at 10:41:02AM -0400, Douglas Gilbert wrote:
> > Perhaps we could add another bit flag in struct
> > scsi_host_template such as:
> >     unsigned int transport_says_dont_scsi2_lun_cmd:1;
> > 
> > then drivers/usb/storage/scsiglue.c could set that
> > bit in its usb_stor_host_template and
> > drivers/scsi/scsi.c could take heed (and not mask
> > cmd->cmnd[1] with the LUN).
> 
> Fully agreed.
> 
> (except that I'd shorten the flag name to .no_scsi2lun :))

Okay, here's a patch that implements the suggestion, except that I put
the flag in the Scsi_Host structure instead of the template.  This was
to minimize the impact of the change.  Among the various SCSI-over-USB
transports, only the Bulk-Only transport gives the LUN separately from
the CDB.  I don't know if there are any multi-LUN USB devices that
don't use the Bulk-Only transport, but if there are then they won't 
work if the LUN isn't stored in CDB[1].

Tiziano, does this do what you want?

Alan Stern



Index: usb-3.16/include/scsi/scsi_host.h
===================================================================
--- usb-3.16.orig/include/scsi/scsi_host.h
+++ usb-3.16/include/scsi/scsi_host.h
@@ -695,6 +695,9 @@ struct Scsi_Host {
     /* The controller does not support WRITE SAME */
     unsigned no_write_same:1;

+    /* The transport requires the LUN bits NOT to be stored in CDB[1] */
+    unsigned no_scsi2_lun:1;
+
     /*
      * Optional work queue to be utilized by the transport
      */
Index: usb-3.16/drivers/scsi/scsi.c
===================================================================
--- usb-3.16.orig/drivers/scsi/scsi.c
+++ usb-3.16/drivers/scsi/scsi.c
@@ -678,7 +678,8 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
      * 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->device->scsi_level != SCSI_UNKNOWN &&
+        !host->no_scsi2_lun) {
         cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
                    (cmd->device->lun << 5 & 0xe0);
     }
Index: usb-3.16/drivers/usb/storage/usb.c
===================================================================
--- usb-3.16.orig/drivers/usb/storage/usb.c
+++ usb-3.16/drivers/usb/storage/usb.c
@@ -981,6 +981,14 @@ int usb_stor_probe2(struct us_data *us)
     if (!(us->fflags & US_FL_SCM_MULT_TARG))
         us_to_host(us)->max_id = 1;

+    /*
+     * Like Windows, we won't store the LUN bits in CDB[1] for SCSI-2
+     * devices using the Bulk-Only transport (even though this violates
+     * the SCSI spec).
+     */
+    if (us->transport == usb_stor_Bulk_transport)
+        us_to_host(us)->no_scsi2_lun = 1;
+
     /* Find the endpoints and calculate pipe values */
     result = get_pipes(us);
     if (result)

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #13 from Martin K. Petersen <martin.petersen@oracle.com> ---
>>>>> "Alan" == Alan Stern <stern@rowland.harvard.edu> writes:

Alan> Okay, here's a patch that implements the suggestion, except that I
Alan> put the flag in the Scsi_Host structure instead of the template.
Alan> This was to minimize the impact of the change.  Among the various
Alan> SCSI-over-USB transports, only the Bulk-Only transport gives the
Alan> LUN separately from the CDB.  I don't know if there are any
Alan> multi-LUN USB devices that don't use the Bulk-Only transport, but
Alan> if there are then they won't work if the LUN isn't stored in
Alan> CDB[1].

I'm in agreement with this approach.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #14 from Alan Stern <stern@rowland.harvard.edu> ---
On Thu, 21 Aug 2014, Christoph Hellwig wrote:

> On Thu, Aug 21, 2014 at 05:43:41PM -0400, Martin K. Petersen wrote:
> > Alan> Okay, here's a patch that implements the suggestion, except that I
> > Alan> put the flag in the Scsi_Host structure instead of the template.
> > Alan> This was to minimize the impact of the change.  Among the various
> > Alan> SCSI-over-USB transports, only the Bulk-Only transport gives the
> > Alan> LUN separately from the CDB.  I don't know if there are any
> > Alan> multi-LUN USB devices that don't use the Bulk-Only transport, but
> > Alan> if there are then they won't work if the LUN isn't stored in
> > Alan> CDB[1].
> > 
> > I'm in agreement with this approach.
> 
> I like it too. One idea to unclutter the fastpath would be to have a
> single flag that controls if the LUN is set which is based on the
> host(-template) flag and the scsi level, which would allow us to remove
> all the clutter around that area.

Good idea.  An enhanced patch is below.  If I can get a Tested-By: from
Tiziano and one or two Acked-By: responses, I'll submit this for the
current and stable kernels.

Sending the initial INQUIRY command to LUNs larger than 0 involves a
chicken-and-egg problem -- we don't know whether to fill in the LUN
bits in the command until we know the SCSI level, and we don't know the
SCSI level until the INQUIRY response is received.  The solution we 
have been using is to store the most recently discovered level in the 
target structure, and use it as a default.  If probing starts with LUN 
0, and if all the LUNs have similar levels, this ought to work.

Except for one thing: The code does store the default level in the
scsi_target, but forgets to copy it back into each newly allocated
scsi_device!  I added a line to do that into the patch.

Alan Stern



Index: usb-3.16/include/scsi/scsi_host.h
===================================================================
--- usb-3.16.orig/include/scsi/scsi_host.h
+++ usb-3.16/include/scsi/scsi_host.h
@@ -695,6 +695,9 @@ struct Scsi_Host {
     /* The controller does not support WRITE SAME */
     unsigned no_write_same:1;

+    /* The transport requires the LUN bits NOT to be stored in CDB[1] */
+    unsigned no_scsi2_lun_in_cdb:1;
+
     /*
      * Optional work queue to be utilized by the transport
      */
Index: usb-3.16/include/scsi/scsi_device.h
===================================================================
--- usb-3.16.orig/include/scsi/scsi_device.h
+++ usb-3.16/include/scsi/scsi_device.h
@@ -174,6 +174,7 @@ struct scsi_device {
     unsigned wce_default_on:1;    /* Cache is ON by default */
     unsigned no_dif:1;    /* T10 PI (DIF) should be disabled */
     unsigned broken_fua:1;        /* Don't set FUA bit */
+    unsigned lun_in_cdb:1;        /* Store LUN bits in CDB[1] */

     atomic_t disk_events_disable_depth; /* disable depth for disk events */

Index: usb-3.16/drivers/scsi/scsi.c
===================================================================
--- usb-3.16.orig/drivers/scsi/scsi.c
+++ usb-3.16/drivers/scsi/scsi.c
@@ -674,14 +674,10 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
         goto out;
     }

-    /* 
-     * If SCSI-2 or lower, store the LUN value in cmnd.
-     */
-    if (cmd->device->scsi_level <= SCSI_2 &&
-        cmd->device->scsi_level != SCSI_UNKNOWN) {
+    /* Store the LUN value in cmnd, if needed. */
+    if (cmd->device->lun_in_cdb)
         cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
                    (cmd->device->lun << 5 & 0xe0);
-    }

     scsi_log_send(cmd);

Index: usb-3.16/drivers/scsi/scsi_scan.c
===================================================================
--- usb-3.16.orig/drivers/scsi/scsi_scan.c
+++ usb-3.16/drivers/scsi/scsi_scan.c
@@ -257,6 +257,7 @@ static struct scsi_device *scsi_alloc_sd

     sdev->sdev_gendev.parent = get_device(&starget->dev);
     sdev->sdev_target = starget;
+    sdev->scsi_level = starget->scsi_level;

     /* usually NULL and set by ->slave_alloc instead */
     sdev->hostdata = hostdata;
@@ -735,6 +736,15 @@ static int scsi_probe_lun(struct scsi_de
         sdev->scsi_level++;
     sdev->sdev_target->scsi_level = sdev->scsi_level;

+    /*
+     * If SCSI-2 or lower, and if the transport requires it,
+     * store the LUN value in CDB[1].
+     */
+    if (sdev->scsi_level <= SCSI_2 &&
+        sdev->scsi_level != SCSI_UNKNOWN &&
+        !sdev->host->no_scsi2_lun_in_cdb)
+        sdev->lun_in_cdb = 1;
+
     return 0;
 }

Index: usb-3.16/drivers/usb/storage/usb.c
===================================================================
--- usb-3.16.orig/drivers/usb/storage/usb.c
+++ usb-3.16/drivers/usb/storage/usb.c
@@ -981,6 +981,14 @@ int usb_stor_probe2(struct us_data *us)
     if (!(us->fflags & US_FL_SCM_MULT_TARG))
         us_to_host(us)->max_id = 1;

+    /*
+     * Like Windows, we won't store the LUN bits in CDB[1] for SCSI-2
+     * devices using the Bulk-Only transport (even though this violates
+     * the SCSI spec).
+     */
+    if (us->transport == usb_stor_Bulk_transport)
+        us_to_host(us)->no_scsi2_lun_in_cdb = 1;
+
     /* Find the endpoints and calculate pipe values */
     result = get_pipes(us);
     if (result)

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #15 from Martin K. Petersen <martin.petersen@oracle.com> ---
>>>>> "Alan" == Alan Stern <stern@rowland.harvard.edu> writes:

Alan> Sending the initial INQUIRY command to LUNs larger than 0 involves
Alan> a chicken-and-egg problem -- we don't know whether to fill in the
Alan> LUN bits in the command until we know the SCSI level, and we don't
Alan> know the SCSI level until the INQUIRY response is received.  The
Alan> solution we have been using is to store the most recently
Alan> discovered level in the target structure, and use it as a default.
Alan> If probing starts with LUN 0, and if all the LUNs have similar
Alan> levels, this ought to work.

Looks good, Alan.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #16 from Alan Stern <stern@rowland.harvard.edu> ---
On Fri, 22 Aug 2014, Christoph Hellwig wrote:

> On Fri, Aug 22, 2014 at 10:53:42AM -0400, Alan Stern wrote:
> > Good idea.  An enhanced patch is below.  If I can get a Tested-By: from
> > Tiziano and one or two Acked-By: responses, I'll submit this for the
> > current and stable kernels.
> > 
> > Sending the initial INQUIRY command to LUNs larger than 0 involves a
> > chicken-and-egg problem -- we don't know whether to fill in the LUN
> > bits in the command until we know the SCSI level, and we don't know the
> > SCSI level until the INQUIRY response is received.  The solution we 
> > have been using is to store the most recently discovered level in the 
> > target structure, and use it as a default.  If probing starts with LUN 
> > 0, and if all the LUNs have similar levels, this ought to work.
> > 
> > Except for one thing: The code does store the default level in the
> > scsi_target, but forgets to copy it back into each newly allocated
> > scsi_device!  I added a line to do that into the patch.
> 
> Looks good to me,
> 
> Acked-by: Christoph Hellwig <hch@lst.de>
> 
> Do you want to queue this up in the USB tree?  From the looks of what I
> have on my plate so far it seems like we could avoid conflicts with it
> in the SCSI tree.

After hearing from Tiziano, I'll submit the patch to the SCSI tree.

Alan Stern

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #17 from Alan Stern <stern@rowland.harvard.edu> ---
On Fri, 22 Aug 2014, James Bottomley wrote:

> On Fri, 2014-08-22 at 10:53 -0400, Alan Stern wrote:

> > Sending the initial INQUIRY command to LUNs larger than 0 involves a
> > chicken-and-egg problem -- we don't know whether to fill in the LUN
> > bits in the command until we know the SCSI level, and we don't know the
> > SCSI level until the INQUIRY response is received.  The solution we 
> > have been using is to store the most recently discovered level in the 
> > target structure, and use it as a default.  If probing starts with LUN 
> > 0, and if all the LUNs have similar levels, this ought to work.
> > 
> > Except for one thing: The code does store the default level in the
> > scsi_target, but forgets to copy it back into each newly allocated
> > scsi_device!  I added a line to do that into the patch.

> > --- usb-3.16.orig/include/scsi/scsi_host.h
> > +++ usb-3.16/include/scsi/scsi_host.h
> > @@ -695,6 +695,9 @@ struct Scsi_Host {
> >  	/* The controller does not support WRITE SAME */
> >  	unsigned no_write_same:1;
> >  
> > +	/* The transport requires the LUN bits NOT to be stored in CDB[1] */
> > +	unsigned no_scsi2_lun_in_cdb:1;
> 
> I think Christoph mentioned shortening this flag length, but personally
> I'm not that bothered.

It was shorter in the original version of the patch, but I decided to
make it a little longer to match the name of the new scsi_device flag
added in this version.

> > --- usb-3.16.orig/drivers/scsi/scsi_scan.c
> > +++ usb-3.16/drivers/scsi/scsi_scan.c
> > @@ -257,6 +257,7 @@ static struct scsi_device *scsi_alloc_sd
> >  
> >  	sdev->sdev_gendev.parent = get_device(&starget->dev);
> >  	sdev->sdev_target = starget;
> > +	sdev->scsi_level = starget->scsi_level;
> 
> Why is this necessary?  Isn't this set correctly in scsi_probe_lun?  The
> target level is actually set from the device level.

The reason was mentioned in the quoted text at the start of this email.

In greater detail: Yes, sdev->scsi_level is set correctly in
scsi_probe_lun, but only _after_ the INQUIRY data has been received
(because the level is part of the INQUIRY data).  So how can the
INQUIRY CDB be set up correctly before we know the device's level?

(When the current code sends an INQUIRY for LUN 1, it will _not_ put 
the LUN bits in CDB[1] -- even if LUN 0 reported SCSI-2.  That doesn't 
seem right.)

My answer is to use a default level copied from the target.  The
target's level in turn was set from the previously probed device...  
which means that the first device to be probed gets a default level of
SCSI-2.  That's okay as long as the first device probed is LUN 0.

Hmmm, but now I see the patch doesn't set a default value for the new
sdev->lun_in_cdb flag.  That needs to be fixed...

> Other than this, I'm fine with the code ... you can add the acked by
> from me when we resolve the above question.

Okay.  It's true that this issue is only tangentially related to the 
main point of the patch.  It could be removed and addressed later.

Alan Stern

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #18 from Alan Stern <stern@rowland.harvard.edu> ---
On Sun, 24 Aug 2014, Christoph Hellwig wrote:

> On Fri, Aug 22, 2014 at 01:29:32PM -0400, Alan Stern wrote:
> > > Other than this, I'm fine with the code ... you can add the acked by
> > > from me when we resolve the above question.
> > 
> > Okay.  It's true that this issue is only tangentially related to the 
> > main point of the patch.  It could be removed and addressed later.
> 
> Just make it a separate patch and send it along..

All right.  But I still want to know first whether the patch really
fixes the original problem.

Tiziano, do you intend to test this patch?

James, can you explain how the INQUIRY command in scsi_probe_lun()  
managed to work back in the days when multi-lun SCSI-2 devices were
common?  sdev->scsi_level doesn't get set when sdev is allocated, so it
initially contains 0, so the LUN bits won't get filled in when the
first INQUIRY command is sent.  Then how could the target know which
logical unit the INQUIRY was meant for?

Alan Stern

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #19 from Alan Stern <stern@rowland.harvard.edu> ---
On Mon, 25 Aug 2014, James Bottomley wrote:

> On Mon, 2014-08-25 at 10:44 -0400, Alan Stern wrote:
> 
> > James, can you explain how the INQUIRY command in scsi_probe_lun()  
> > managed to work back in the days when multi-lun SCSI-2 devices were
> > common?  sdev->scsi_level doesn't get set when sdev is allocated, so it
> > initially contains 0, so the LUN bits won't get filled in when the
> > first INQUIRY command is sent.  Then how could the target know which
> > logical unit the INQUIRY was meant for?
> 
> Best guess, some patches over the course of time altered the way we do
> this and no-one noticed.  I think it was probably the introduction of
> the unknown SCSI data level that caused the breakage.

Heh.  The change was made by commit 4d7db04a7a69 ([SCSI] add
SCSI_UNKNOWN and LUN transfer limit restrictions) back in 2006 -- the
2.6.17 kernel.  If nobody has complained in all this time then it's
probably not worth changing.

> Historically, the LUN in CMD bits is left over from SCSI-1; it was
> incorporated into SCSI-2 for backward compatibility (even though SCSI-2
> moved the LUN specification to the identify message).  In SCSI-3 and
> beyond, those bits were obsoleted and transports took sole
> responsibility for LUN handling.  I'm fairly certain all the SCSI-1
> devices relying on this behaviour have long ago migrated to the great
> data centre in the sky.
> 
> Alan's fix looks reasonable because we probe LUN 0 first (for SCSI-1 and
> 2 which has parallel scanning), which is why it doesn't matter (the bits
> are set to zero) and once we have LUN 0 we propagate the data to the
> target and make it the basis for future checks.  I'd like to see a
> comment explaining this in the code, though ...

If you think it would be a good idea, I could put it into a separate 
patch with an explanatory comment.  At the moment, I'm inclined to 
forget about it.

Alan Stern

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 80711] [PATCH]SG_FLAG_LUN_INHIBIT is no longer implemented and there's not way to prevent the kernel from using the 2nd cdb byte for the LUN

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=80711

--- Comment #20 from Alan Stern <stern@rowland.harvard.edu> ---
On Mon, 25 Aug 2014, Alan Stern wrote:

> On Mon, 25 Aug 2014, James Bottomley wrote:
> 
> > On Mon, 2014-08-25 at 10:44 -0400, Alan Stern wrote:
> > 
> > > James, can you explain how the INQUIRY command in scsi_probe_lun()  
> > > managed to work back in the days when multi-lun SCSI-2 devices were
> > > common?  sdev->scsi_level doesn't get set when sdev is allocated, so it
> > > initially contains 0, so the LUN bits won't get filled in when the
> > > first INQUIRY command is sent.  Then how could the target know which
> > > logical unit the INQUIRY was meant for?
> > 
> > Best guess, some patches over the course of time altered the way we do
> > this and no-one noticed.  I think it was probably the introduction of
> > the unknown SCSI data level that caused the breakage.
> 
> Heh.  The change was made by commit 4d7db04a7a69 ([SCSI] add
> SCSI_UNKNOWN and LUN transfer limit restrictions) back in 2006 -- the
> 2.6.17 kernel.  If nobody has complained in all this time then it's
> probably not worth changing.

It turns out the code is already there, and I didn't realize because I
was looking at the wrong source file.  scsi_sysfs_device_initialize()
already does:

    sdev->scsi_level = starget->scsi_level;

Here's the update to the patch, adding an appropriate comment and
setting the new sdev->lun_in_sdb flag properly:

Alan Stern


Index: usb-3.16/drivers/scsi/scsi_sysfs.c
===================================================================
--- usb-3.16.orig/drivers/scsi/scsi_sysfs.c
+++ usb-3.16/drivers/scsi/scsi_sysfs.c
@@ -1238,7 +1238,19 @@ void scsi_sysfs_device_initialize(struct
     sdev->sdev_dev.class = &sdev_class;
     dev_set_name(&sdev->sdev_dev, "%d:%d:%d:%d",
              sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
+    /*
+     * Get a default scsi_level from the target (derived from sibling
+     * devices).  This is the best we can do for guessing how to set
+     * sdev->lun_in_cdb for the initial INQUIRY command.  For LUN 0 the
+     * setting doesn't matter, because all the bits are zero anyway.
+     * But it does matter for higher LUNs.
+     */
     sdev->scsi_level = starget->scsi_level;
+    if (sdev->scsi_level <= SCSI_2 &&
+            sdev->scsi_level != SCSI_UNKNOWN &&
+            !shost->no_scsi2_lun_in_cdb)
+        sdev->lun_in_cdb = 1;
+
     transport_setup_device(&sdev->sdev_gendev);
     spin_lock_irqsave(shost->host_lock, flags);
     list_add_tail(&sdev->same_target_siblings, &starget->devices);

-- 
You are receiving this mail because:
You are the assignee for the bug.