[PATCH 1/2] scsi: Add scsi_device max_cmd_len

[PATCH 1/2] scsi: Add scsi_device max_cmd_len

[Brian King]

Add a max_cmd_len field to the scsi_device struct
to allow for per device limits of allowable command
lengths. This will be used by libata, which is currently
using the max_cmd_len field in the scsi_host struct,
which doesn't work for attaching libata controlled
SATA devices to SAS HBAs.

Signed-off-by: Brian King <brking@us.ibm.com>
---

 libata-dev-bjking1/drivers/scsi/scsi.c        |    3 ++-
 libata-dev-bjking1/drivers/scsi/scsi_scan.c   |    1 +
 libata-dev-bjking1/include/scsi/scsi_device.h |    1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

diff -puN include/scsi/scsi_device.h~scsi_device_cdb_len include/scsi/scsi_device.h
--- libata-dev/include/scsi/scsi_device.h~scsi_device_cdb_len	2006-03-27 13:24:04.000000000 -0600
+++ libata-dev-bjking1/include/scsi/scsi_device.h	2006-03-27 13:24:48.000000000 -0600
@@ -72,6 +72,7 @@ struct scsi_device {
 	unsigned int manufacturer;	/* Manufacturer of device, for using 
 					 * vendor-specific cmd's */
 	unsigned sector_size;	/* size in bytes */
+	unsigned char max_cmd_len;
 
 	void *hostdata;		/* available to low-level driver */
 	char type;
diff -puN drivers/scsi/scsi_scan.c~scsi_device_cdb_len drivers/scsi/scsi_scan.c
--- libata-dev/drivers/scsi/scsi_scan.c~scsi_device_cdb_len	2006-03-27 13:25:39.000000000 -0600
+++ libata-dev-bjking1/drivers/scsi/scsi_scan.c	2006-03-27 13:28:24.000000000 -0600
@@ -218,6 +218,7 @@ static struct scsi_device *scsi_alloc_sd
 	sdev->lun = lun;
 	sdev->channel = starget->channel;
 	sdev->sdev_state = SDEV_CREATED;
+	sdev->max_cmd_len = MAX_COMMAND_SIZE;
 	INIT_LIST_HEAD(&sdev->siblings);
 	INIT_LIST_HEAD(&sdev->same_target_siblings);
 	INIT_LIST_HEAD(&sdev->cmd_list);
diff -puN drivers/scsi/scsi.c~scsi_device_cdb_len drivers/scsi/scsi.c
--- libata-dev/drivers/scsi/scsi.c~scsi_device_cdb_len	2006-03-27 13:29:23.000000000 -0600
+++ libata-dev-bjking1/drivers/scsi/scsi.c	2006-03-27 13:30:03.000000000 -0600
@@ -610,7 +610,8 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
 	 * Before we queue this command, check if the command
 	 * length exceeds what the host adapter can handle.
 	 */
-	if (CDB_SIZE(cmd) > cmd->device->host->max_cmd_len) {
+	if (CDB_SIZE(cmd) > cmd->device->host->max_cmd_len ||
+	    CDB_SIZE(cmd) > cmd->device->max_cmd_len) {
 		SCSI_LOG_MLQUEUE(3,
 				printk("queuecommand : command too long.\n"));
 		cmd->result = (DID_ABORT << 16);
_

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[James Bottomley]

On Tue, 2006-03-28 at 16:17 -0600, Brian King wrote:
> Add a max_cmd_len field to the scsi_device struct
> to allow for per device limits of allowable command
> lengths. This will be used by libata, which is currently
> using the max_cmd_len field in the scsi_host struct,
> which doesn't work for attaching libata controlled
> SATA devices to SAS HBAs.

This really doesn't look correct.  What you want is a sata transport
class with a max command length in the host device.

James

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[Brian King]

James Bottomley wrote:
> On Tue, 2006-03-28 at 16:17 -0600, Brian King wrote:
>> Add a max_cmd_len field to the scsi_device struct
>> to allow for per device limits of allowable command
>> lengths. This will be used by libata, which is currently
>> using the max_cmd_len field in the scsi_host struct,
>> which doesn't work for attaching libata controlled
>> SATA devices to SAS HBAs.
> 
> This really doesn't look correct.  What you want is a sata transport
> class with a max command length in the host device.

My direction at this point has been to move away from
the virtual scsi host device for SATA devices attached
to SAS HBAs. This seemed to me to be the clearest implementation.
Then you have 1 SAS HBA = 1 struct scsi_host, no matter
how many SATA devices are attached underneath it either
via direct attach or via an expander.


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[James Bottomley]

On Tue, 2006-03-28 at 16:38 -0600, Brian King wrote:
> My direction at this point has been to move away from
> the virtual scsi host device for SATA devices attached
> to SAS HBAs. This seemed to me to be the clearest implementation.
> Then you have 1 SAS HBA = 1 struct scsi_host, no matter
> how many SATA devices are attached underneath it either
> via direct attach or via an expander.

That's what a transport class implementation would allow you to do.  As
long as the representation of the host controller device (be it PCI,
SCSI IDE or whatever) has an embedded generic device, the class can
attach and carry attributes.

James

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[Brian King]

James Bottomley wrote:
> On Tue, 2006-03-28 at 16:38 -0600, Brian King wrote:
>> My direction at this point has been to move away from
>> the virtual scsi host device for SATA devices attached
>> to SAS HBAs. This seemed to me to be the clearest implementation.
>> Then you have 1 SAS HBA = 1 struct scsi_host, no matter
>> how many SATA devices are attached underneath it either
>> via direct attach or via an expander.
> 
> That's what a transport class implementation would allow you to do.  As
> long as the representation of the host controller device (be it PCI,
> SCSI IDE or whatever) has an embedded generic device, the class can
> attach and carry attributes.

I'm still struggling a little with where you want to head here. Are you
proposing a scsi_transport_sata in addition to the existing scsi transports,
or are you proposing adding to the existing scsi_transport_sas code?
I assume it is the latter, since a single SAS HBA will be supporting
both SAS and SATA devices at the same time and currently scsi core only
handles a single transport per scsi_host.


Thanks,

Brian

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[James Bottomley]

On Tue, 2006-03-28 at 18:03 -0600, Brian King wrote:
> I'm still struggling a little with where you want to head here. Are you
> proposing a scsi_transport_sata in addition to the existing scsi transports,
> or are you proposing adding to the existing scsi_transport_sas code?
> I assume it is the latter, since a single SAS HBA will be supporting
> both SAS and SATA devices at the same time and currently scsi core only
> handles a single transport per scsi_host.

Well, no, not scsi transport; I think just a general transport ... see
the raid_class for an example of one of these (albeit not very related
to what you want to do, I'm afraid).

technically, you can have multiple transports per device, even with SCSI
ones.  It's just convenient to simplify the single attachement case
since that's the predominant one.

The key is in the match function.  Given a generic device, you have to
know how to verify its a device of the type you want.

James

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[Brian King]

James Bottomley wrote:
> Well, no, not scsi transport; I think just a general transport ... see
> the raid_class for an example of one of these (albeit not very related
> to what you want to do, I'm afraid).
> 
> technically, you can have multiple transports per device, even with SCSI
> ones.  It's just convenient to simplify the single attachement case
> since that's the predominant one.
> 
> The key is in the match function.  Given a generic device, you have to
> know how to verify its a device of the type you want.

 From what I understand, it looks like I should be creating a sata_class
transport of sorts, which would attach to scsi_hosts that support SATA
and would contain SATA host related attributes. It would also provide
APIs such as sata_dev_alloc, sata_dev_add, similar to sas_phy_alloc,
and sas_phy_add, which would be called by external users to attach
SATA devices to the upper layers. sata_dev_add would result in
the device getting hooked into libata and getting added to scsi core,
etc. There could then also be per-device hooks/attributes in the
sata_class transport as appropriate.

However, since scsi core is the one that checks max_cmd_len,
scsi core would somehow need to be able to poke into the sata host
transport attribute to decide whether or not to accept the command.
Probably an addition to struct scsi_transport_template?

This is quite a change from the patchset I have been carrying
for the past 5 months or so to add SATA support to ipr. The
current ipr patch to add this support can be found here:

http://marc.theaimsgroup.com/?l=linux-scsi&m=114263741229458&w=2

It requires no change to scsi core other than the patch we are
discussing (which could technically also be done in libata),
and requires minimal libata changes. I'm happy to start coding
up a sata_class, but is there any chance of this being a phase
2 solution, and for the interim we proceed with something closer
to what I have been submitting to the list for the past 5 months?
I'm happy to drop the patch that started this thread if that helps.
Would you be OK with the ipr patch in the URL above as an interim
solution?

Thanks,

Brian

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[Jeff Garzik]

James Bottomley wrote:
> On Tue, 2006-03-28 at 16:17 -0600, Brian King wrote:
>> Add a max_cmd_len field to the scsi_device struct
>> to allow for per device limits of allowable command
>> lengths. This will be used by libata, which is currently
>> using the max_cmd_len field in the scsi_host struct,
>> which doesn't work for attaching libata controlled
>> SATA devices to SAS HBAs.
> 
> This really doesn't look correct.  What you want is a sata transport
> class with a max command length in the host device.

Note that Tejun added an easy-to-miss ATA transport starter class in the
eh_timed_out patches.

I've always supported the additional of (and fleshing out of) an ATA 
transport class.

	Jeff

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[Christoph Hellwig]

On Tue, Mar 28, 2006 at 04:17:18PM -0600, Brian King wrote:
> 
> Add a max_cmd_len field to the scsi_device struct
> to allow for per device limits of allowable command
> lengths. This will be used by libata, which is currently
> using the max_cmd_len field in the scsi_host struct,
> which doesn't work for attaching libata controlled
> SATA devices to SAS HBAs.

this sounds wrong to me.  cdb length is a limitation of the host (driver).
A target will reject unknown commands, no matter what length they have.

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[Brian King]

Christoph Hellwig wrote:
> On Tue, Mar 28, 2006 at 04:17:18PM -0600, Brian King wrote:
>> Add a max_cmd_len field to the scsi_device struct
>> to allow for per device limits of allowable command
>> lengths. This will be used by libata, which is currently
>> using the max_cmd_len field in the scsi_host struct,
>> which doesn't work for attaching libata controlled
>> SATA devices to SAS HBAs.
> 
> this sounds wrong to me.  cdb length is a limitation of the host (driver).
> A target will reject unknown commands, no matter what length they have.

In that case, this patch can probably be dropped. Currently libata has
some code that looks at all the devices attached to a port and sets
the scsi_host's max_cmd_len to the max of these values. I was assuming
this was due to some misbehaving ATA/ATAPI devices out there.

Jeff, care to comment on this one?


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[Christoph Hellwig]

On Wed, Mar 29, 2006 at 08:15:24AM -0600, Brian King wrote:
> In that case, this patch can probably be dropped. Currently libata has
> some code that looks at all the devices attached to a port and sets
> the scsi_host's max_cmd_len to the max of these values. I was assuming
> this was due to some misbehaving ATA/ATAPI devices out there.

For ata devices the scsi cbds are translated to taskfiles anyway so it
doesn't matter.  For ATAPI there might be buggy devices but they should
be handled with the normal blacklist entry meachnisms if they really exist.

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[Jeff Garzik]

James Bottomley wrote:
> This really doesn't look correct.  What you want is a sata transport
> class with a max command length in the host device.

Christoph Hellwig wrote:
> this sounds wrong to me.  cdb length is a limitation of the host (driver).
> A target will reject unknown commands, no matter what length they have.


In practice, CDB length may be limited by both the host and the device. 
  This applies to ATAPI, and some USB storage too IIRC.  For ATAPI, you 
read the CDB length from the device's IDENTIFY PACKET DEVICE info page.

	Jeff

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[Brian King]

Jeff Garzik wrote:
> James Bottomley wrote:
>> This really doesn't look correct.  What you want is a sata transport
>> class with a max command length in the host device.
> 
> Christoph Hellwig wrote:
>> this sounds wrong to me.  cdb length is a limitation of the host (driver).
>> A target will reject unknown commands, no matter what length they have.
> 
> 
> In practice, CDB length may be limited by both the host and the device. 
>   This applies to ATAPI, and some USB storage too IIRC.  For ATAPI, you 
> read the CDB length from the device's IDENTIFY PACKET DEVICE info page.

So the question remains, do we need to police the CDB length on a per device
basis, or is a per host basis ok? Will we have ATAPI devices falling on the
floor if they get sent too large of a cdb?

Brian

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[Brian King]

Brian King wrote:
> Jeff Garzik wrote:
>> James Bottomley wrote:
>>> This really doesn't look correct.  What you want is a sata transport
>>> class with a max command length in the host device.
>> Christoph Hellwig wrote:
>>> this sounds wrong to me.  cdb length is a limitation of the host (driver).
>>> A target will reject unknown commands, no matter what length they have.
>>
>> In practice, CDB length may be limited by both the host and the device. 
>>   This applies to ATAPI, and some USB storage too IIRC.  For ATAPI, you 
>> read the CDB length from the device's IDENTIFY PACKET DEVICE info page.
> 
> So the question remains, do we need to police the CDB length on a per device
> basis, or is a per host basis ok? Will we have ATAPI devices falling on the
> floor if they get sent too large of a cdb?

Should have read Christoph's post... I guess we will handle these
devices with blacklists..

Brian

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[Jeff Garzik]

Brian King wrote:
> Jeff Garzik wrote:
>> James Bottomley wrote:
>>> This really doesn't look correct.  What you want is a sata transport
>>> class with a max command length in the host device.
>> Christoph Hellwig wrote:
>>> this sounds wrong to me.  cdb length is a limitation of the host (driver).
>>> A target will reject unknown commands, no matter what length they have.
>>
>> In practice, CDB length may be limited by both the host and the device. 
>>   This applies to ATAPI, and some USB storage too IIRC.  For ATAPI, you 
>> read the CDB length from the device's IDENTIFY PACKET DEVICE info page.
> 
> So the question remains, do we need to police the CDB length on a per device
> basis, or is a per host basis ok? Will we have ATAPI devices falling on the
> floor if they get sent too large of a cdb?

It _must_ be limited by both device and host.  If you have a device that 
supports 16-byte CDBs and a host controller which only supports 12-byte 
CDBs, clearly the limit is 12, due to host.  However, a reversed 
situation (limited by device) is equally plausible/possible.

	Jeff

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[Brian King]

Jeff Garzik wrote:
> Brian King wrote:
>> Jeff Garzik wrote:
>>> James Bottomley wrote:
>>>> This really doesn't look correct.  What you want is a sata transport
>>>> class with a max command length in the host device.
>>> Christoph Hellwig wrote:
>>>> this sounds wrong to me.  cdb length is a limitation of the host (driver).
>>>> A target will reject unknown commands, no matter what length they have.
>>> In practice, CDB length may be limited by both the host and the device. 
>>>   This applies to ATAPI, and some USB storage too IIRC.  For ATAPI, you 
>>> read the CDB length from the device's IDENTIFY PACKET DEVICE info page.
>> So the question remains, do we need to police the CDB length on a per device
>> basis, or is a per host basis ok? Will we have ATAPI devices falling on the
>> floor if they get sent too large of a cdb?
> 
> It _must_ be limited by both device and host.  If you have a device that 
> supports 16-byte CDBs and a host controller which only supports 12-byte 
> CDBs, clearly the limit is 12, due to host.  However, a reversed 
> situation (limited by device) is equally plausible/possible.

Ok. That is what I was assuming, which was the reason I submitted this
patch in the first place.

James, given this information, are you ok with the patch, or do you
still have an objection?

Thanks,

Brian

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH 1/2] scsi: Add scsi_device max_cmd_len

[Stefan Richter]

Jeff Garzik wrote on 2006-03-30:
> James Bottomley wrote:
>> This really doesn't look correct.  What you want is a sata transport
>> class with a max command length in the host device.
> 
> Christoph Hellwig wrote:
>> this sounds wrong to me.  cdb length is a limitation of the host 
>> (driver).
>> A target will reject unknown commands, no matter what length they have.
> 
> In practice, CDB length may be limited by both the host and the device. 
>  This applies to ATAPI, and some USB storage too IIRC.  For ATAPI, you 
> read the CDB length from the device's IDENTIFY PACKET DEVICE info page.

BTW, in case of SBP-2 devices, the CDB length is a property of the 
logical unit and is read from the IEEE 1394 node's configuration ROM. 
Linux' sbp2 driver implements only a CDB length of 12 bytes though.
-- 
Stefan Richter
-=====-=-==- -=-- ----=
http://arcgraph.de/sr/