linux-ide.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-02 20:33 Timothy Thelin
  2005-11-02 21:55 ` thomas schorpp
  0 siblings, 1 reply; 54+ messages in thread
From: Timothy Thelin @ 2005-11-02 20:33 UTC (permalink / raw)
  To: Alan Stern, thomas schorpp; +Cc: usb-storage, linux-ide


> They aren't ATA read/write commands; they are SCSI commands.  
> If there is 
> a version of SMART that runs over SCSI, you might be able to use it.
> 
> > and sata is short of smart support. both use scsi-emu(?).
> > 
> > i dont think theres a hardware problem.
> 
> Yes there is.  The USB interface on the drive won't recognize 
> an ATA SMART
> command and won't pass it on to the drive controller.
> 

To expand on this, its really the USB device firmware which needs
the passthru mechanism implmented, since (as stated) it expects
to be getting scsi commands.  Now there are many vendor specific
ways to embed ATA commands inside of scsi cdbs, and then there are
two public methods that I'm aware of: SAT and the At2lp (cypress
bridge board, docs on their website).

And to send these commands, user-space already has SG_IO.

So I'm not sure usb-storage can do much, since it can't force a usb
device to accept passthru cdbs, and SG_IO already allows cdbs
to be sent to usb devices.

Regards,
Tim Thelin

^ permalink raw reply	[flat|nested] 54+ 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; 54+ 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] 54+ 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; 54+ 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] 54+ 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; 54+ 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] 54+ messages in thread
* 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; 54+ 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] 54+ messages in thread
* RE: [usb-storage] [Merging ATA passthru] on integrating SMART/ATA-Security in usb-storage driver
@ 2005-11-02 22:18 Timothy Thelin
  2005-11-02 23:22 ` thomas schorpp
  2005-11-02 23:45 ` Matthew Dharm
  0 siblings, 2 replies; 54+ messages in thread
From: Timothy Thelin @ 2005-11-02 22:18 UTC (permalink / raw)
  To: t.schorpp; +Cc: Alan Stern, usb-storage, linux-ide



> >>They aren't ATA read/write commands; they are SCSI commands.  
> >>If there is 
> >>a version of SMART that runs over SCSI, you might be able to use it.
> >>
> >>
> >>>and sata is short of smart support. both use scsi-emu(?).
> >>>
> >>>i dont think theres a hardware problem.
> >>
> >>Yes there is.  The USB interface on the drive won't recognize 
> >>an ATA SMART
> >>command and won't pass it on to the drive controller.
> >>
> > 
> > 
> > To expand on this, its really the USB device firmware which needs
> > the passthru mechanism implmented, since (as stated) it expects
> > to be getting scsi commands.  Now there are many vendor specific
> > ways to embed ATA commands inside of scsi cdbs, and then there are
> > two public methods that I'm aware of: SAT and the At2lp (cypress
> > bridge board, docs on their website).
> > 
> > And to send these commands, user-space already has SG_IO.
> > 
> 
> so sg3_utils and sdparm could be used for commands(?)
>  

If those utilites understood the ATACB and SAT cdb specifications,
they could be made to do security / smart commands by using SG_IO.
My current understanding is that those utilities only think in
terms of scsi, and not in terms of ATA passthru.

I have hands on experience using SG_IO to send ATACB cdbs to usb
devices to do various things.  ATACB allows you to do most ATA
commands, and it works pretty well.

The biggest issue is that because USB device are exported to
SCSI core as scsi2 devices, SCSI core adds LUN info into the
cdb which messes up the ATACB signature and causes the bridge
to reject the command (this will be a SAT passthru issue
as well). When wanting to do ATACB I had to patch the kernel
to stop this behavior so that the cdb would pass through the
system unmangled.

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)


Regards,
Tim Thelin


^ permalink raw reply	[flat|nested] 54+ messages in thread
* [Merging ATA passthru] on integrating SMART/ATA-Security in usb-storage driver
@ 2005-11-02 14:08 thomas schorpp
  2005-11-02 16:05 ` [usb-storage] " Alan Stern
  0 siblings, 1 reply; 54+ messages in thread
From: thomas schorpp @ 2005-11-02 14:08 UTC (permalink / raw)
  To: usb-storage; +Cc: linux-ide

hi,

i'll try integrating SMART/ATA-Security in usb-storage driver for usb2
ide disks.

other people working on it yet? ive read something on SMART efforts for
SATA.

i think of using the passthrough code (libata?) or module in
usb-storage.ko like the SATA driver for ATA-Security.

comments?

thx
y
tom


-------- Original Message --------
Subject: Merging ATA passthru
Date: Fri, 21 Oct 2005 15:14:18 -0400
From: Jeff Garzik <jgarzik@pobox.com>
To: linux-ide@vger.kernel.org <linux-ide@vger.kernel.org>
CC: Linux Kernel <linux-kernel@vger.kernel.org>


Folks,

Taking Mark Lord's (and others) criticism to heart, I'm going to merge
the ATA passthru work upstream, once 2.6.14 is released.

Since there are still some reported problems that I haven't had time to
track down, I'm going to -- like ATAPI -- introduce a module option that
enables passthru.  It will default to off.

Other features that follow a similar pattern -- 98% there but needs a
few final tweaks -- will be treated in the same way.

This gets lesser-used features upstream where they can get the most
testing, while defaulting them to off ensures that we won't perturb the
known-working code.

This also will help me, in that I won't have to maintain a bunch of
parallel codebases.

	Jeff



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


^ permalink raw reply	[flat|nested] 54+ messages in thread
end of thread, other threads:[~2005-11-09 15:35 UTC | newest]

Thread overview: 54+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-11-02 20:33 [usb-storage] [Merging ATA passthru] on integrating SMART/ATA-Security in usb-storage driver Timothy Thelin
2005-11-02 21:55 ` thomas schorpp
  -- strict thread matches above, loose matches on Subject: below --
2005-11-08 19:50 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
2005-11-04 18:30 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-11-06 23:15             ` 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-02 22:18 Timothy Thelin
2005-11-02 23:22 ` thomas schorpp
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
2005-11-02 14:08 thomas schorpp
2005-11-02 16:05 ` [usb-storage] " Alan Stern
2005-11-02 19:12   ` thomas schorpp
2005-11-02 19:26     ` Alan Stern
2005-11-02 20:41       ` thomas schorpp

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).