linux-scsi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* RE: [usb-storage] [Merging ATA passthru] on integrating SMART/ATA-Security in usb-storage driver
@ 2005-11-04 18:30 Timothy Thelin
  2005-11-04 18:58 ` James Bottomley
  2005-11-04 23:46 ` Pete Zaitcev
  0 siblings, 2 replies; 49+ messages in thread
From: Timothy Thelin @ 2005-11-04 18:30 UTC (permalink / raw)
  To: Matthew Dharm, James Bottomley
  Cc: t.schorpp, usb-storage, linux-ide, Linux SCSI list

> 
> On Thu, Nov 03, 2005 at 11:08:07PM -0500, James Bottomley wrote:
> > On Wed, 2005-11-02 at 15:45 -0800, Matthew Dharm wrote:
> > > On Wed, Nov 02, 2005 at 02:18:52PM -0800, Timothy Thelin wrote:
> > > > 
> > > > If you had time to spare, instead of touching usb-storage,
> > > > it might be better spent resurecting SG_FLAG_LUN_INHIBIT to
> > > > stop the above behavior so that SG_IO cdbs can be passed
> > > > through untouched.
> > > > (SG_FLAG_FUN_INHIBIT was a flag SG_IO used to support a long
> > > > time ago, and I have no idea why it was dropped, but it was)
> > > 
> > > I didn't realize that had been removed.  Anyone that sends a
> > > vendor-specific command to a device needs this flag to 
> make sure it goes
> > > through unmangled.
> > > 
> > > Perhaps someone on linux-scsi can comment on why this was 
> removed and how
> > > we might get it back?
> > 
> > I've no distinct recollection of someone removing this, but if I
> > remember correctly what it used to do, it was a hack to stop us from
> > mangling SCSI-3 CDB's.  We fixed the mid-layer not to 
> require the hack
> > by only setting the CDB[1] lun field for SCSI-1 and SCSI-2 
> devices (as
> > the standards mandate).  What's the actual problem?  No 
> SCSI-1 or SCSI-2
> > device should have any vendor specific CDBs that uses these bits in
> > CDB[1].
> 
> Unfortunately, reality appears to disagree with the last 
> "should".  I've
> personally seen devices with vendor-specific commands that 
> want to control
> CDB[1] in SCSI-2.
> 
> I didn't know it was removed; I only know what Timothy Thelin 
> told me.  Can
> we get the feature back?
> 
> Matt

And for an even more concrete example:
The CY7C68300B cypress bridge board (has various siblings as well
on their site that act very similar) implements SCSI spec 0 (ie it
doesn't claim to support any scsi spec).  Now usb-storage sees
this in inquiry, and decides to export the device as a scsi2 device
since based on the usb-storage devs' experience most usb devices
really want scsi2 cdbs.  So SCSI core thinks this is a scsi2 device
and procedes to mangle the cdbs as they're going through.

The problem then is with vendor specific commands as SCSI core
mangles those as well.  The vendor specific command I had issues
with is the "ATACB" ATA passthru cdb that cypress boards understand.
CDB[1] is a signature byte that must be 0x24.  Needless to say,
that leading 2 gets masked off to a 0, and the drive aborts the
ATACB.  Now cypress could have been smarter about designing their
cdb, but they wern't, and so there really needs to be a way to tell
SCSI core "hands off the cdb".

This is also going to be an issue with USB devices that implement
SAT passthru but state they implement SCSI spec 0, as SAT passthru
CDB[1] can't afford to be mangled either.

I started this conversation a little over a month ago, but it died.
http://marc.theaimsgroup.com/?l=linux-scsi&m=112714917116713&w=2

Regards,
Tim Thelin






^ permalink raw reply	[flat|nested] 49+ messages in thread
* RE: [usb-storage] [Merging ATA passthru] on integrating SMART/ATA-Security in usb-storage driver
@ 2005-11-08 19:50 Timothy Thelin
  2005-11-09  9:04 ` thomas schorpp
  0 siblings, 1 reply; 49+ messages in thread
From: Timothy Thelin @ 2005-11-08 19:50 UTC (permalink / raw)
  To: Alan Stern, James Bottomley
  Cc: Patrick Mansfield, Matthew Dharm, thomas schorpp,
	USB Storage list, Linux SCSI list, linux-ide

[-- Attachment #1: Type: text/plain, Size: 2441 bytes --]


> On Mon, 7 Nov 2005, James Bottomley wrote:
> 
> > > It's quite possible that usb-storage no longer needs to force the 
> > > scsi-level to 2.  No one has recently tested what would 
> happen without 
> > > it.  Matt probably has the best selection of devices for 
> testing...
> > > 
> > > There is one problem we have with devices that report 
> themselves as SCSI-3 
> > > or SCSI-4 but hang when they receive a REPORT LUNS 
> command.  That's easily 
> > > handled by making usb-storage set the NOREPORTLUN flag.  
> Maybe that's all 
> > > we need to do.
> > 
> > If you could try this out, I'd be grateful.
> 
> I tried the patch below on several combinations of SCSI 
> levels and device 
> types.  It seems to do exactly what we want; for reads I always saw 
> READ(10) and for MODE SENSE I saw either the 6- or 10-byte 
> form, depending 
> on the device type and USB subclass value.
> 
> The question remains whether pass-thru will now work and 
> whether the patch
> will mess up some existing non-compliant device that 
> currently works okay.  
> The only way to know is by trying it out.  Matt and Timothy, 
> that's up to
> you.
> 
> Alan Stern
> 

I did some initial testing on Alan's patch with a sandisk cruzer
mini thumb drive (level 2), a memorex thumb drive (level 0),
and that cypress part (level 0), and I haven't seen any IO
problems.

After that I made another patch (below and attached because
of wrapping) you can apply after Alan's that disables cdb mangling
on level 0 devices. It was generated against Linus' git repo after
applying Alan's patch.

With both patches applied I can now do passthru cdbs on the
cypress part without issue, and I still don't see any IO issues.

Anyone out there have a better selection of hardware to test with?


Signed-off-by: Timothy Thelin <timothy.thelin@wdc.com>

---

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -569,9 +569,10 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
 	}
 
 	/* 
-	 * If SCSI-2 or lower, store the LUN value in cmnd.
+	 * If SCSI-2 or lower (except SCSI-0), store the LUN value in cmnd.
 	 */
-	if (cmd->device->scsi_level <= SCSI_2) {
+	if (cmd->device->scsi_level != SCSI_UNKNOWN &&
+		cmd->device->scsi_level <= SCSI_2) {
 		cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
 			       (cmd->device->lun << 5 & 0xe0);
 	}

[-- Attachment #2: scsi-level-0-aware.patch --]
[-- Type: application/octet-stream, Size: 532 bytes --]

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -569,9 +569,10 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
 	}
 
 	/* 
-	 * If SCSI-2 or lower, store the LUN value in cmnd.
+	 * If SCSI-2 or lower (except SCSI-0), store the LUN value in cmnd.
 	 */
-	if (cmd->device->scsi_level <= SCSI_2) {
+	if (cmd->device->scsi_level != SCSI_UNKNOWN &&
+		cmd->device->scsi_level <= SCSI_2) {
 		cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
 			       (cmd->device->lun << 5 & 0xe0);
 	}

^ permalink raw reply	[flat|nested] 49+ messages in thread
* RE: [usb-storage] [Merging ATA passthru] on integrating SMART/ATA-Security in usb-storage driver
@ 2005-11-07 20:18 Timothy Thelin
  0 siblings, 0 replies; 49+ messages in thread
From: Timothy Thelin @ 2005-11-07 20:18 UTC (permalink / raw)
  To: Alan Stern, Patrick Mansfield
  Cc: thomas schorpp, James Bottomley, usb-storage, Linux SCSI list,
	linux-ide


> > 
> > Can we trigger black listing based on the above 
> vendor/product values?
> > i.e. can you check for these values in usb-storage 
> slave_configure code?
> 
> Yes.  The patch below illustrates how.  You'll have to change 
> the name of 
> the sdev flag to something sensible...
> 
> I firmly believe this is the wrong approach, however.  It's a 
> specific 
> solution to a general problem.  I would much prefer to add a 
> new flag to 
> struct request.

I totally agree.

> 
> > Yeh, some other hardware might want lun inihibit in the 
> future, and that
> > could still be added back, but IMHO black list is better.
> 
> I'd say it's inevitable.  And it may even end up being 
> standard-based, not 
> vendor/product based.

Within a year from now, there are going to be USB devices
natively supporting the SAT passthru CDB.  Once this happens,
its going to be CDB based and not vendor/product based on
when to not mangle the CDB; chips from different vendors
will support the CDB.  The blacklist approach really isn't
going to scale, where the flag approch will.  Plus with
a blacklist there would always be a lead time until a
device with passthru supported works correctly under the
kernel (blacklist update), where with a flag approach,
support is automatic.

Regards,
Tim Thelin


^ permalink raw reply	[flat|nested] 49+ messages in thread
* RE: [usb-storage] [Merging ATA passthru] on integrating SMART/ATA-Security in usb-storage driver
@ 2005-11-07 17:51 Timothy Thelin
  0 siblings, 0 replies; 49+ messages in thread
From: Timothy Thelin @ 2005-11-07 17:51 UTC (permalink / raw)
  To: Patrick Mansfield, thomas schorpp, James Bottomley, usb-storage,
	linux-ide, Linux SCSI list



> > Okay, what am I missing?
> > 
> > Looking at that patch, it looks to me like if sg.c set a 
> flag in the SCSI
> > command block then scsi_core.c could use that flag in the 
> test to determine
> > if the LUN should be masked-in.
> > 
> > HOWEVER, I keep hearing that the changes will be extensive. 
>  What am I
> > missing?
> 
> Not extensive AFAICT ... 
> 
> The scmd/cdb is not available until we call scsi_get_command() in the
> request function. So you would have to add a field into 
> scsi_request, set
> it in sg (in both sg.c block/scsi_ioctl.c, test via sd and sg 
> when you are
> done), and add the field in scsi_cmnd, and set scmd one in
> scsi_init_cmd_from_req().
> 

I'l sign up as a tester =D

> But as far as black listing, it does seem like a better 
> solution in that
> user apps do not need special code.
> 
> Do the devices that require it (well so far) have useable 
> vendor + model
> strings or usb id's?
> 

Actually, from my point of view its better to do the flag than
use a blacklist.  Blacklists require active maintenence, so
everytime WD releases a new product based on this cypress chip,
the kernel needs yet another entry (and there are a couple more
on our roadmap).  I actually see the flag solution as a more
generic forward looking solution; it's one patch verses one
patch per device. And from the point of view of someone who
creates utilities that need raw access to hard drives, this
flag is trivial to support in userspace and makes logical sense.

The two userspace cases I can think of:
1) Tool is aware of what device its trying to access.  Well since
it knows the device it wants to get access to, it will also
be aware of if it needs that flag or not.
2) Tool is generic (sdparm, hdparm) and tries to access whatever
device it's pointed at.  We'll based off of the cdb it tries to
construct, its going to know if it needs that flag or not.  Both
the ATACB and the SAT passthru CDB would need that flag since
they both have meaningful information in the LUN bits.  For cdbs
that don't have meaningful bits there, they don't have to use the
flag.

So by going the flag route, you need one kernel change, and one
userspace change per tool to be flag aware.  Then future products
that use the same passthru cdbs will get supported for free, unlike
the blacklist approach.


Regards,
Tim Thelin
Software Engineer
Western Digital








^ permalink raw reply	[flat|nested] 49+ messages in thread
[parent not found: <CA45571DE57E1C45BF3552118BA92C9D69BF3A@WDSCEXBECL03.sc.wdc.com>]

end of thread, other threads:[~2005-12-01 11:34 UTC | newest]

Thread overview: 49+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-11-04 18:30 [usb-storage] [Merging ATA passthru] on integrating SMART/ATA-Security in usb-storage driver Timothy Thelin
2005-11-04 18:58 ` James Bottomley
2005-11-04 20:30   ` Matthew Dharm
2005-11-04 20:49     ` James Bottomley
2005-11-05 23:55       ` Matthew Dharm
2005-11-06  0:49         ` James Bottomley
2005-11-06  3:38           ` thomas schorpp
2005-11-06 21:58           ` Matthew Dharm
2005-11-06 22:28             ` thomas schorpp
2005-11-06 23:52               ` Patrick Mansfield
2005-11-07 16:59                 ` Matthew Dharm
2005-11-07 17:05                 ` Matthew Dharm
2005-11-07 17:24                   ` Patrick Mansfield
2005-11-07 17:46                     ` Alan Stern
2005-11-07 18:06                       ` thomas schorpp
2005-11-07 19:33                         ` Alan Stern
2005-11-07 20:07                           ` thomas schorpp
2005-11-07 17:53                     ` Christoph Hellwig
2005-11-07 17:54                     ` thomas schorpp
2005-11-07 18:57                       ` Patrick Mansfield
2005-11-07 19:53                         ` Alan Stern
2005-11-07 20:47                           ` Patrick Mansfield
2005-11-07 20:59                             ` Alan Stern
2005-11-07 22:05                               ` James Bottomley
2005-11-08 17:09                                 ` Alan Stern
2005-11-08 23:05                                   ` Mike Anderson
2005-11-09 15:35                                     ` Alan Stern
2005-12-01 11:35                                 ` [PATCH] writable scsi_level [was: [Merging ATA passthru] on integrating SMART/ATA-Security in usb-storage driver] Douglas Gilbert
2005-11-08 13:51                             ` [usb-storage] [Merging ATA passthru] on integratingSMART/ATA-Security in usb-storage driver Pat LaVarre
2005-11-06 23:15             ` [usb-storage] [Merging ATA passthru] on integrating SMART/ATA-Security " James Bottomley
2005-11-07 18:14         ` Patrick Mansfield
2005-11-04 23:56     ` Andries Brouwer
2005-11-04 23:46 ` Pete Zaitcev
2005-11-05 16:20   ` thomas schorpp
2005-11-05 18:01     ` [usb-storage] [Merging ATA passthru] on integratingSMART/ATA-Security " Pat LaVarre
  -- strict thread matches above, loose matches on Subject: below --
2005-11-08 19:50 [usb-storage] [Merging ATA passthru] on integrating SMART/ATA-Security " Timothy Thelin
2005-11-09  9:04 ` thomas schorpp
2005-11-09  9:45   ` thomas schorpp
2005-11-09 10:05     ` thomas schorpp
2005-11-09 13:21     ` Mark Lord
2005-11-09 14:05       ` thomas schorpp
2005-11-07 20:18 Timothy Thelin
2005-11-07 17:51 Timothy Thelin
     [not found] <CA45571DE57E1C45BF3552118BA92C9D69BF3A@WDSCEXBECL03.sc.wdc.com>
2005-11-02 23:45 ` Matthew Dharm
2005-11-04  4:08   ` James Bottomley
2005-11-04 17:28     ` Matthew Dharm
2005-11-04 18:33       ` James Bottomley
2005-11-04 20:30         ` Matthew Dharm
2005-11-04 20:53           ` James Bottomley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).