Home » Mailing list archives » linux-kernel » 2008 » March » 13

Re: [PATCH] mass storage : emulation of sat scsi_pass_thru with ATACB

Previous thread: [PATCH] keyring: Incorrect permissions checking in __keyring_search_one() by Arun Raghavan on Saturday, March 8, 2008 - 10:22 am. (2 messages)

Next thread: ATM CARD by MR DAVID MARK on Friday, March 7, 2008 - 5:44 pm. (1 message)
[view original message]
From: matthieu castet
Subject: [PATCH] mass storage : emulation of sat scsi_pass_thru with ATACB
Date: Saturday, March 8, 2008 - 10:32 am

Hi,

I have got a cypress usb-ide bridge and I would like to tune or monitor
my disk with tools like hdparm, hddtemp or smartctl.

My controller support a way to send raw ATA command to the disk with
something call atacb (see
http://download.cypress.com.edgesuite.net/design_resources/datasheets/contents/cy7c683...).

Atacb support can be added for each application, but there is some 
disadvantages :
- all application need to be patched
- A race is possible if there other accesses, because the emulation can
be split in 2 atacb scsi transactions. One for sending the command, one
for reading the register (if ck_cond is set).

I have implemented the emulation in usb-storage with a special 
proto_handler,
and an unsual entry.

Signed-off-by: Matthieu CASTET <castet.matthieu@free.fr>

Index: linux-2.6.24.2/drivers/usb/storage/protocol.c
===================================================================
--- linux-2.6.24.2.orig/drivers/usb/storage/protocol.c	2008-02-29 
22:14:09.000000000 +0100
+++ linux-2.6.24.2/drivers/usb/storage/protocol.c	2008-03-08 
18:18:22.000000000 +0100
@@ -47,6 +47,8 @@
  #include <linux/highmem.h>
  #include <scsi/scsi.h>
  #include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_eh.h>
+#include <linux/ata.h>

  #include "usb.h"
  #include "protocol.h"
@@ -144,6 +146,179 @@
  	usb_stor_invoke_transport(srb, us);
  }

+/*
+ * ATACB is a protocol used on cypress usb<->ata bridge to
+ * send raw ATA command over mass storage
+ * There is a ATACB2 protocol that support LBA48 on newer chip.
+ * More info that be found on cy7c68310_8.pdf and cy7c68300c_8.pdf
+ * datasheet from cypress.com.
+ */
+static void emulate_pass_thru_with_atacb(struct scsi_cmnd *srb,
+				       struct us_data *us)
+{
+	unsigned char save_cmnd[MAX_COMMAND_SIZE];
+	memcpy(save_cmnd, srb->cmnd, sizeof(save_cmnd));
+	memset(srb->cmnd, 0, sizeof(srb->cmnd));
+
+	/* check if we support the command */
+	if (save_cmnd[1] >> 5) /* MULTIPLE_COUNT */
+		goto ...
[ message continues ]
[view original message]
From: Matthew Dharm
Subject: Re: [PATCH] mass storage : emulation of sat scsi_pass_thru with ATACB
Date: Saturday, March 8, 2008 - 11:31 am

Why are you using an initializer instead of a new protocol code?

Most of this should probably be moved into it's own file, just like all of
the other protocol handlers.

Actually, why do you even have a separate 'dispatcher' function?  Why not
just one protocol handler function which checks the command at the top and
calls invoke_transport there?

Also, unless ATACB is a new standard (and I don't think it is, as the
Cypress datasheet uses the term 'vendor specific'), then your functions
need renaming.  Instead of 'emulate_pass_thru_with_atacb', how about
something like 'cypress_atacb' --  since it's already a protocol handler,
everyone already knows it's for passing commands.

Matt

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=

--=20
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.=
net=20
Maintainer, Linux USB Mass Storage Driver

A:  The most ironic oxymoron wins ...
DP: "Microsoft Works"
A:  Uh, okay, you win.
					-- A.J. & Dust Puppy
User Friendly, 1/18/1998

[ message continues ]
[view original message]
From: matthieu castet
Subject: Re: [PATCH] mass storage : emulation of sat scsi_pass_thru with ATACB
Date: Saturday, March 8, 2008 - 1:08 pm

Hi Matthew,

thanks for your comments

Because using a new protocol code means I need to patch all the place 
where there is a comparison between us->subclass and US_SC_SCSI.
After all I am US_SC_SCSI with a special case for ATA12 & ATA16 commands.
What do you means by having a separate 'dispatcher' function?
You means why I have 2 functions emulate_pass_thru_with_atacb and 
usb_stor_transparent_scsi_command_atacb ?
I did 2 functions for having a code more clean.

You suggest something like
void usb_stor_transparent_scsi_command_atacb(struct scsi_cmnd *srb,
                        struct us_data *us)
{
     if (srb->cmnd[0] != ATA_16 && srb->cmnd[0] != ATA_12) {
         usb_stor_invoke_transport(srb, us);
         return;
     }
     copy emulate_pass_thru_with_atacb code here
But 'emulate_pass_thru_with_atacb' only handle ATA pass_thru scsi 
commands. It doesn't translate all scsi commands to atacb like 
'cypress_atacb' could suggest.
That's why I put 'usb_stor_transparent_scsi_command_atacb' saying it is 
transparent_scsi_command + atacb support.

Matthieu
--


[ message continues ]
[view original message]
From: Matthew Dharm
Subject: Re: [PATCH] mass storage : emulation of sat scsi_pass_thru with ATACB
Date: Saturday, March 8, 2008 - 2:21 pm

Yet, you call invoke_transport directly, just like any other protocol
handler.

The proper way to do this is as a separate protocol handler.  If you want
to make it clear that you are only intercepting a couple of command types,
then don't call invoke_transport() directly, call the transparent scsi
protocol handler (which, of course, does the same thing but provides
clearer layering).


Yes, modulo my comment above about calling the transparent scsi protocol

Yes, but your name suggests that ATACB is a new industry standard which is
implemented by more than a few chips from one specific vendor.  That's not
acceptable.

Try 'cypress_atacb_passthrough' instead?

Matt

--=20
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.=
net=20
Maintainer, Linux USB Mass Storage Driver

P:  Nine more messages in admin.policy.
M: I know, I'm typing as fast as I can!
					-- Pitr and Mike
User Friendly, 11/27/97

[ message continues ]
[view original message]
From: matthieu castet
Subject: Re: [PATCH] mass storage : emulation of sat scsi_pass_thru with ATACB
Date: Sunday, March 9, 2008 - 1:21 am

Hum, may be to avoid confusion with a new protocol handler, I can add my 
hook in usb_stor_control_thread with a new flag.

Something like :
[...]
  /* Handle those devices which need us to fake
          * their inquiry data */
         else if ((us->srb->cmnd[0] == INQUIRY) &&
                 (us->flags & US_FL_FIX_INQUIRY)) {
[...]
	else if (( (us->srb->cmnd[0] == ATA_12) || (us->srb->cmnd[0] == ATA_16)) &&
                 (us->flags & US_FL_CYPRESS_ATACB)) {
         US_DEBUGP("emulating ATA pass thru\n");
		call to emulate_pass_thru_with_atacb code
         }

         /* we've got a command, let's do it! */
         else {
             US_DEBUG(usb_stor_show_command(us->srb));
             us->proto_handler(us->srb, us);
         }

Does it sound better ?

Matthieu
--


[ message continues ]
[view original message]
From: Matthew Dharm
Subject: Re: [PATCH] mass storage : emulation of sat scsi_pass_thru with ATACB
Date: Sunday, March 9, 2008 - 11:45 am

Somewhat better.  I still like a separate protocol handler better.

I guess I see this as being more like the ISD200 case.  In that code, it
attempts to identify the device as ATA or ATAPI.  If it's ATAPI, it
switches back to the transparent scsi protocol handler.  Here, we're just
talking about doing it on a command-by-command basis, instead of a one-time
selection.  But there is no confusion that the ISD200 code handles all
commands; but it does implement a new 'protocol' (a way to transport
commands to/from the device), which is exactly what you're doing.

Matt

--=20
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.=
net=20
Maintainer, Linux USB Mass Storage Driver

I'm seen in many forms.  Now open your mouth.  It's caffeine time.
					-- Cola Man to Greg
User Friendly, 10/28/1998

[ message continues ]
[view original message]
From: matthieu castet
Subject: Re: [PATCH] mass storage : emulation of sat scsi_pass_thru with ATACB
Date: Sunday, March 9, 2008 - 2:42 pm

Hi

here an update version of the patch


I have got a cypress usb-ide bridge and I would like to tune or monitor
my disk with tools like hdparm, hddtemp or smartctl.

My controller support a way to send raw ATA command to the disk with
something call atacb (see
http://download.cypress.com.edgesuite.net/design_resources/datasheets/contents/cy7c683...).

Atacb support can be added for each application, but there is some 
disadvantages :
- all application need to be patched
- A race is possible if there other accesses, because the emulation can
be split in 2 atacb scsi transactions. One for sending the command, one
for reading the register (if ck_cond is set).

I have implemented the emulation in usb-storage with a special 
proto_handler, and an unsual entry.

Signed-off-by: Matthieu CASTET <castet.matthieu@free.fr>

Index: linux-2.6.24.2/drivers/usb/storage/unusual_devs.h
===================================================================
--- linux-2.6.24.2.orig/drivers/usb/storage/unusual_devs.h	2008-02-29 
22:14:09.000000000 +0100
+++ linux-2.6.24.2/drivers/usb/storage/unusual_devs.h	2008-03-09 
22:10:59.000000000 +0100
@@ -1588,6 +1588,14 @@
  		US_SC_DEVICE, US_PR_DEVICE, NULL,
  		US_FL_CAPACITY_HEURISTICS),

+#ifdef CONFIG_USB_STORAGE_CYPRESS_ATACB
+UNUSUAL_DEV(  0x04b4, 0x6830, 0x0000, 0x9999,
+		"Cypress",
+		"Cypress AT2LP",
+		US_SC_CYP_ATACB, US_PR_BULK, NULL,
+		0),
+#endif
+
  /* Control/Bulk transport for all SubClass values */
  USUAL_DEV(US_SC_RBC, US_PR_CB, USB_US_TYPE_STOR),
  USUAL_DEV(US_SC_8020, US_PR_CB, USB_US_TYPE_STOR),
Index: linux-2.6.24.2/drivers/usb/storage/cypress_atacb.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.24.2/drivers/usb/storage/cypress_atacb.c	2008-03-09 
22:04:18.000000000 +0100
@@ -0,0 +1,199 @@
+/*
+ * Support for emulating SAT (ata pass through) on devices based
+ *       on the Cypress USB/ATA bridge supporting ...
[ message continues ]
[view original message]
From: Matthew Dharm
Subject: Re: [PATCH] mass storage : emulation of sat scsi_pass_thru with ATACB
Date: Sunday, March 9, 2008 - 6:20 pm

Line wrap problem.  This happens a few times in your patch, but this was

What are these magic constants?  Symbolic names or comments would be

What does this block do?  In general, you could use some more comments

This is an interesting section of code.  Do you have reason to believe that
some of the units you've claimed via unusual_devs.h do not support this
protocol, or are you just being forward-looking?


I thought we changed this so that the non-spec protocols started from 0xfe
and worked their way down?

Matt

--=20
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.=
net=20
Maintainer, Linux USB Mass Storage Driver

C:  They kicked your ass, didn't they?
S:  They were cheating!
					-- The Chief and Stef
User Friendly, 11/19/1997

[ message continues ]
[view original message]
From: matthieu castet
Subject: Re: [PATCH] mass storage : emulation of sat scsi_pass_thru with ATACB
Date: Thursday, March 13, 2008 - 2:22 pm

Ha, may be my mailer is broken with inline patch. I will attach the next 
It's a bit that should be set for these commands. I will add a comment
when I did this code, I was wondering where to plug the hook and was 
thinking about auto-discovering of atacb device.
I think I can remove it now and replace it with a message.
But I should find a way to discover other devices supporting atacb. May 
Oh, I suppose I need to port this to lastest git.

- I updated the patch to git version (I have to change some stuff for 
scsi) and I follow the above comments.


Matthieu

PS : I don't have a running kernel git version. So I only build the mass 
storage driver with this patch, I didn't run it.
----

[ message continues ]
[view original message]
From: Matthew Dharm
Subject: Re: [PATCH] mass storage : emulation of sat scsi_pass_thru with ATACB
Date: Thursday, March 13, 2008 - 5:51 pm

I may have missed this in an earlier patch, but why to you declare and use
these 3 variables like this?  Seems odd and confusing to me....

Matt

--=20
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.=
net=20
Maintainer, Linux USB Mass Storage Driver

It was a new hope.
					-- Dust Puppy
User Friendly, 12/25/1998

[ message continues ]
[view original message]
From: Alan Cox
Subject: Re: [PATCH] mass storage : emulation of sat scsi_pass_thru with ATACB
Date: Sunday, March 9, 2008 - 3:13 am

On Sat, 8 Mar 2008 10:31:25 -0800

Would it perhaps be simpler to just attach the device to the ATA driver
and use the ATACB interface for everything ?

Alan
--


[ message continues ]
Previous thread: [PATCH] keyring: Incorrect permissions checking in __keyring_search_one() by Arun Raghavan on Saturday, March 8, 2008 - 10:22 am. (2 messages)

Next thread: ATM CARD by MR DAVID MARK on Friday, March 7, 2008 - 5:44 pm. (1 message)