[PATCH 1/2] scsi: Add scsi_device max_cmd_len (resend)

[PATCH 1/2] scsi: Add scsi_device max_cmd_len (resend)

[Brian King]

Add a max_cmd_len field to the scsi_device struct
to allow for per device limits of allowable command
lengths. This patch was submitted earlier and resulted
in a bit of discussion regarding whether or not
CDB length is a limitation of the host or the device.
For ATA, both the host and the device can limit the
CDB length. Currently libata reads the IDENTIFY
PACKET DEVICE data for an ATAPI device and sets
the max_cmd_len in the scsi host for the maximum
supported CDB length of all ATA/ATAPI devices attached
to the same scsi host. This patch allows libata to
set the max CDB length on a per device basis and
allows the SAS/SATA HBA to set its own max command
length in the scsi host template.

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-04-17 17:17:50.000000000 -0500
+++ libata-dev-bjking1/include/scsi/scsi_device.h	2006-04-17 17:17:50.000000000 -0500
@@ -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-04-17 17:17:50.000000000 -0500
+++ libata-dev-bjking1/drivers/scsi/scsi_scan.c	2006-04-17 17:17:50.000000000 -0500
@@ -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-04-17 17:17:50.000000000 -0500
+++ libata-dev-bjking1/drivers/scsi/scsi.c	2006-04-17 17:17:50.000000000 -0500
@@ -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);
_

[PATCH 2/2] libata: Use scsi_device max_cmd_len (resend)

[Brian King]

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



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: libata_sd_cdb_len.patch --]
[-- Type: text/x-patch; name="libata_sd_cdb_len.patch", Size: 2305 bytes --]


Use the newly added max_cmd_len in the scsi_device
struct instead of the field by the same name in
the scsi_host struct. This allows for better
granularity of control of the max allowed command
length and allows for SAS/SATA hosts to impose their
own host restrictions on max command length, if needed.
It also simplifies using libata for SATA devices under
SAS HBAs.

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

 libata-dev-bjking1/drivers/scsi/libata-core.c |   10 ++--------
 libata-dev-bjking1/drivers/scsi/libata-scsi.c |    1 +
 2 files changed, 3 insertions(+), 8 deletions(-)

diff -puN drivers/scsi/libata-core.c~libata_sd_cdb_len drivers/scsi/libata-core.c
--- libata-dev/drivers/scsi/libata-core.c~libata_sd_cdb_len	2006-04-17 17:30:20.000000000 -0500
+++ libata-dev-bjking1/drivers/scsi/libata-core.c	2006-04-17 17:30:20.000000000 -0500
@@ -1225,7 +1225,7 @@ static int ata_dev_configure(struct ata_
 {
 	const u16 *id = dev->id;
 	unsigned int xfer_mask;
-	int i, rc;
+	int rc;
 
 	if (!ata_dev_enabled(dev)) {
 		DPRINTK("ENTER/EXIT (host %u, dev %u) -- nodev\n",
@@ -1328,12 +1328,6 @@ static int ata_dev_configure(struct ata_
 			       ap->id, dev->devno, ata_mode_string(xfer_mask));
 	}
 
-	ap->host->max_cmd_len = 0;
-	for (i = 0; i < ATA_MAX_DEVICES; i++)
-		ap->host->max_cmd_len = max_t(unsigned int,
-					      ap->host->max_cmd_len,
-					      ap->device[i].cdb_len);
-
 	/* limit bridge transfers to udma5, 200 sectors */
 	if (ata_dev_knobble(ap, dev)) {
 		if (print_info)
@@ -4573,7 +4567,7 @@ static void ata_host_init(struct ata_por
 	host->max_lun = 1;
 	host->max_channel = 1;
 	host->unique_id = ata_unique_id++;
-	host->max_cmd_len = 12;
+	host->max_cmd_len = ATAPI_CDB_LEN;
 
 	ap->flags = ATA_FLAG_DISABLED;
 	ap->id = host->unique_id;
diff -puN drivers/scsi/libata-scsi.c~libata_sd_cdb_len drivers/scsi/libata-scsi.c
--- libata-dev/drivers/scsi/libata-scsi.c~libata_sd_cdb_len	2006-04-17 17:30:20.000000000 -0500
+++ libata-dev-bjking1/drivers/scsi/libata-scsi.c	2006-04-17 17:30:20.000000000 -0500
@@ -670,6 +670,7 @@ static void ata_scsi_dev_config(struct s
 		max_sectors = dev->max_sectors;
 
 	blk_queue_max_sectors(sdev->request_queue, max_sectors);
+	sdev->max_cmd_len = dev->cdb_len;
 
 	/*
 	 * SATA DMA transfers must be multiples of 4 byte, so
_

Re: [PATCH 2/2] libata: Use scsi_device max_cmd_len (resend)

[Jeff Garzik]

Brian King wrote:
> @@ -4573,7 +4567,7 @@ static void ata_host_init(struct ata_por
>  	host->max_lun = 1;
>  	host->max_channel = 1;
>  	host->unique_id = ata_unique_id++;
> -	host->max_cmd_len = 12;
> +	host->max_cmd_len = ATAPI_CDB_LEN;
>  


Have you audited the code paths to ensure that a CDB of length 15 is 
_NEVER_ sent, before ata_scsi_dev_config() is called?

The current code intentionally uses the minimum -- 12 -- and then raises 
it if both device and host are capable of more.

	Jeff

Re: [PATCH 2/2] libata: Use scsi_device max_cmd_len (resend)

[Brian King]

Jeff Garzik wrote:
> Brian King wrote:
>> @@ -4573,7 +4567,7 @@ static void ata_host_init(struct ata_por
>>  	host->max_lun = 1;
>>  	host->max_channel = 1;
>>  	host->unique_id = ata_unique_id++;
>> -	host->max_cmd_len = 12;
>> +	host->max_cmd_len = ATAPI_CDB_LEN;
>>  
> 
> 
> Have you audited the code paths to ensure that a CDB of length 15 is 
> _NEVER_ sent, before ata_scsi_dev_config() is called?

Yes. I don't see any problems. slave_configure is called before any
upper layer drivers are attached and before anything shows up in sysfs,
so that limits us to libata initiated commands issued as part of the
port probe process and scsi core initiated commands issued as part of
device scanning. All the commands libata issues as part of the port
probe process are issued through ata_exec_internal, which does not go
through scsi core, so there is no protection there today. scsi core
only issues inquiry and report luns commands as part of scanning, both
of which are not an issue.

> The current code intentionally uses the minimum -- 12 -- and then raises 
> it if both device and host are capable of more.

Actually, the current code does not allow for an ata host to limit the
max CDB length. ata_host_init initializes host->max_cmd_len to 12, then 
ata_dev_configure zeroes out ap->host->max_cmd_len, then sets it to the
maximum CDB length supported by any of the devices on that host. The obvious
followup patch to this one would be to change all the users of libata
to have them initialize scsi_host_template->max_cmd_len and remove that code
from ata_host_init.

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

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

[Brian King]

James,

Any comments on the patch below?

Thanks

Brian

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 patch was submitted earlier and resulted
> in a bit of discussion regarding whether or not
> CDB length is a limitation of the host or the device.
> For ATA, both the host and the device can limit the
> CDB length. Currently libata reads the IDENTIFY
> PACKET DEVICE data for an ATAPI device and sets
> the max_cmd_len in the scsi host for the maximum
> supported CDB length of all ATA/ATAPI devices attached
> to the same scsi host. This patch allows libata to
> set the max CDB length on a per device basis and
> allows the SAS/SATA HBA to set its own max command
> length in the scsi host template.
> 
> 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-04-17 17:17:50.000000000 -0500
> +++ libata-dev-bjking1/include/scsi/scsi_device.h	2006-04-17 17:17:50.000000000 -0500
> @@ -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-04-17 17:17:50.000000000 -0500
> +++ libata-dev-bjking1/drivers/scsi/scsi_scan.c	2006-04-17 17:17:50.000000000 -0500
> @@ -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-04-17 17:17:50.000000000 -0500
> +++ libata-dev-bjking1/drivers/scsi/scsi.c	2006-04-17 17:17:50.000000000 -0500
> @@ -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);
> _


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

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

[James Bottomley]

On Fri, 2006-04-28 at 09:28 -0500, Brian King wrote:
> Any comments on the patch below?

The mechanistic comment is that the max_cmd_len parameter should be per
target not per device.

However, since this is SATA only, and SATA is supposed to be moving out
of the SCSI subsystem, I'm a bit loath to add things we'll have to
attempt to extract later.  What would be the consequence of simply
lowering the host max_cmd_len by the result returned from IDENTIFY?

James

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

[Luben Tuikov]

--- Brian King <brking@us.ibm.com> wrote:

> James,
> 
> Any comments on the patch below?

Hi Brian,  I know I'm not "James" and you didn't direct you email
to me, but I've a question for you:

Why do you want this to live in SCSI Core?  I mean, do you find
it objectionable for a proper SATL (no, not libata), to return
proper SK/ASC/ASCQ when a CDB not supported by the device is sent
to the device?

    Luben

> 
> Thanks
> 
> Brian
> 
> 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 patch was submitted earlier and resulted
> > in a bit of discussion regarding whether or not
> > CDB length is a limitation of the host or the device.
> > For ATA, both the host and the device can limit the
> > CDB length. Currently libata reads the IDENTIFY
> > PACKET DEVICE data for an ATAPI device and sets
> > the max_cmd_len in the scsi host for the maximum
> > supported CDB length of all ATA/ATAPI devices attached
> > to the same scsi host. This patch allows libata to
> > set the max CDB length on a per device basis and
> > allows the SAS/SATA HBA to set its own max command
> > length in the scsi host template.
> > 
> > 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-04-17 17:17:50.000000000
> -0500
> > +++ libata-dev-bjking1/include/scsi/scsi_device.h	2006-04-17 17:17:50.000000000 -0500
> > @@ -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-04-17 17:17:50.000000000
> -0500
> > +++ libata-dev-bjking1/drivers/scsi/scsi_scan.c	2006-04-17 17:17:50.000000000 -0500
> > @@ -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-04-17 17:17:50.000000000 -0500
> > +++ libata-dev-bjking1/drivers/scsi/scsi.c	2006-04-17 17:17:50.000000000 -0500
> > @@ -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);
> > _
> 
> 
> -- 
> Brian King
> eServer Storage I/O
> IBM Linux Technology Center
> -
> 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
> 

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

[Luben Tuikov]

--- James Bottomley <James.Bottomley@SteelEye.com> wrote:
> On Fri, 2006-04-28 at 09:28 -0500, Brian King wrote:
> > Any comments on the patch below?
> 
> The mechanistic comment is that the max_cmd_len parameter should be per
> target not per device.
> 
> However, since this is SATA only, and SATA is supposed to be moving out
> of the SCSI subsystem,

Can you justify this "decision" with a technological argument.*  A lot of
technologists following this mailing list would love to learn.

Thank you,
    Luben
* I.e. a technological argument aside from Linux and the state of SCSI Core.

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

[Brian King]

Luben Tuikov wrote:
> Hi Brian,  I know I'm not "James" and you didn't direct you email
> to me, but I've a question for you:
> 
> Why do you want this to live in SCSI Core?  I mean, do you find
> it objectionable for a proper SATL (no, not libata), to return
> proper SK/ASC/ASCQ when a CDB not supported by the device is sent
> to the device?

Actually, I don't have a preference either way. In fact, I have
patches for both solutions. The problem I encountered with containing
it entirely within libata was that it was seen as a "regression"
by Jeff since I changed the behavior for existing libata users:

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

I could respin the libata patch above so that it only affects SAS
users of libata. 

Jeff - do you have any problem if we contain this function in libata?
I can either resubmit this patch:

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

or rediff it such that it does not affect current users. I prefer
to avoid special casing SAS users, but I'm open to any solution
that works.

Brian


> 
>     Luben
> 
>> Thanks
>>
>> Brian
>>
>> 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 patch was submitted earlier and resulted
>>> in a bit of discussion regarding whether or not
>>> CDB length is a limitation of the host or the device.
>>> For ATA, both the host and the device can limit the
>>> CDB length. Currently libata reads the IDENTIFY
>>> PACKET DEVICE data for an ATAPI device and sets
>>> the max_cmd_len in the scsi host for the maximum
>>> supported CDB length of all ATA/ATAPI devices attached
>>> to the same scsi host. This patch allows libata to
>>> set the max CDB length on a per device basis and
>>> allows the SAS/SATA HBA to set its own max command
>>> length in the scsi host template.
>>>
>>> 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-04-17 17:17:50.000000000
>> -0500
>>> +++ libata-dev-bjking1/include/scsi/scsi_device.h	2006-04-17 17:17:50.000000000 -0500
>>> @@ -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-04-17 17:17:50.000000000
>> -0500
>>> +++ libata-dev-bjking1/drivers/scsi/scsi_scan.c	2006-04-17 17:17:50.000000000 -0500
>>> @@ -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-04-17 17:17:50.000000000 -0500
>>> +++ libata-dev-bjking1/drivers/scsi/scsi.c	2006-04-17 17:17:50.000000000 -0500
>>> @@ -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);
>>> _
>>
>> -- 
>> Brian King
>> eServer Storage I/O
>> IBM Linux Technology Center
>> -
>> 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
>>
> 
> -
> 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


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

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

[Brian King]

James Bottomley wrote:
> On Fri, 2006-04-28 at 09:28 -0500, Brian King wrote:
>> Any comments on the patch below?
> 
> The mechanistic comment is that the max_cmd_len parameter should be per
> target not per device.

I don't have a problem with that. It could go in the target as well.

> However, since this is SATA only, and SATA is supposed to be moving out
> of the SCSI subsystem, I'm a bit loath to add things we'll have to
> attempt to extract later.  What would be the consequence of simply
> lowering the host max_cmd_len by the result returned from IDENTIFY?

That is what is done today, which works fine if you only have one
device per host, but when you have multiple devices per host, there is
no *good* value to put in the host max_cmd_len, hence the patch. This
could also be completely contained in libata as my previous post suggests,
if Jeff is OK with that.


Brian


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

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

[James Bottomley]

On Fri, 2006-04-28 at 13:19 -0500, Brian King wrote:
> That is what is done today, which works fine if you only have one
> device per host, but when you have multiple devices per host, there is
> no *good* value to put in the host max_cmd_len, hence the patch. This
> could also be completely contained in libata as my previous post suggests,
> if Jeff is OK with that.

The value should be the minimum of the currently supported lengths.  I
presume we have 10, 12 and 16 for SATA?  In which case the only problem
is not doing 16 and then only if you want to go beyond 2TB ... 

Perhaps you could tell me what the actual failure case is?  my
recollection is that sd does either 6, 10 or 16, but that it only ever
tries 16 if it has to access a sector beyond 2TB, so as a matter of
practice, you should be safe even for a 10 only device on a host that
supports 16.

James

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

[Brian King]

James Bottomley wrote:
> On Fri, 2006-04-28 at 13:19 -0500, Brian King wrote:
>> That is what is done today, which works fine if you only have one
>> device per host, but when you have multiple devices per host, there is
>> no *good* value to put in the host max_cmd_len, hence the patch. This
>> could also be completely contained in libata as my previous post suggests,
>> if Jeff is OK with that.
> 
> The value should be the minimum of the currently supported lengths.  I
> presume we have 10, 12 and 16 for SATA?  In which case the only problem
> is not doing 16 and then only if you want to go beyond 2TB ... 

Which is what libata is doing today. This doesn't work as well for SAS
adapters which support SATA drives and also support > 2TB disk arrays.

> Perhaps you could tell me what the actual failure case is?  

I don't have any data on how ATA/ATAPI devices respond if they receive
too large of a CDB, but my guess is they probably don't react nicely.
Today libata uses the hosts's max_cmd_len for some protection against
this, I was merely trying to continue with a similar level of
protection in my new usage of libata.

The reason for this patch is so that libata no longer has to overload
scsi_host->max_cmd_len for what is really a device attribute for SATA.
I can go back down the path of containing this in libata, if we don't
see any use for such an attribute outside of SATA.

Thanks

Brian


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

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

[James Bottomley]

On Fri, 2006-04-28 at 14:03 -0500, Brian King wrote:
> > Perhaps you could tell me what the actual failure case is?  
> 
> I don't have any data on how ATA/ATAPI devices respond if they receive
> too large of a CDB, but my guess is they probably don't react nicely.
> Today libata uses the hosts's max_cmd_len for some protection against
> this, I was merely trying to continue with a similar level of
> protection in my new usage of libata.

But how would they receive too large a CDB?  This is identical to the
situation today with a legacy SPI drive and a huge array on the same
HBA.  The sd driver is designed to cope with this by not issuing 16 byte
commands until you go over the 32 byte block limit.  This should work
identically for SATA devices, shouldn't it?

James

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

[Brian King]

James Bottomley wrote:
> On Fri, 2006-04-28 at 14:03 -0500, Brian King wrote:
>>> Perhaps you could tell me what the actual failure case is?  
>> I don't have any data on how ATA/ATAPI devices respond if they receive
>> too large of a CDB, but my guess is they probably don't react nicely.
>> Today libata uses the hosts's max_cmd_len for some protection against
>> this, I was merely trying to continue with a similar level of
>> protection in my new usage of libata.
> 
> But how would they receive too large a CDB?  

I suppose the only real exposure then would be someone doing passthru
commands in which we could argue that userspace should be smart enough
not to do things like this.

Going along with that argument, there's really no good reason for
libata to be playing games with scsi_host->max_cmd_len based on
the attached devices.

Brian

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

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

[James Bottomley]

On Fri, 2006-04-28 at 16:11 -0500, Brian King wrote:
> I suppose the only real exposure then would be someone doing passthru
> commands in which we could argue that userspace should be smart enough
> not to do things like this.
> 
> Going along with that argument, there's really no good reason for
> libata to be playing games with scsi_host->max_cmd_len based on
> the attached devices.

Exactly!  The parameter documents the HBA limit, which is something we
wouldn't be able to work out heuristically.  The SCSI operating model
has good heuristics for getting the right command size to devices
(although we can always add something if more there's a demonstrated
problems).

James

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

[Jeff Garzik]

Brian King wrote:
> James Bottomley wrote:
>> On Fri, 2006-04-28 at 14:03 -0500, Brian King wrote:
>>>> Perhaps you could tell me what the actual failure case is?  
>>> I don't have any data on how ATA/ATAPI devices respond if they receive
>>> too large of a CDB, but my guess is they probably don't react nicely.
>>> Today libata uses the hosts's max_cmd_len for some protection against
>>> this, I was merely trying to continue with a similar level of
>>> protection in my new usage of libata.
>> But how would they receive too large a CDB?  
> 
> I suppose the only real exposure then would be someone doing passthru
> commands in which we could argue that userspace should be smart enough
> not to do things like this.
> 
> Going along with that argument, there's really no good reason for
> libata to be playing games with scsi_host->max_cmd_len based on
> the attached devices.

If both host controller and device support 16-byte CDB, it should not be 
limited to 12 bytes.

libata is not "playing games", just exporting what the hardware supports 
as best the API allows.  It needs to support 16-byte CDBs for the same 
reason any SCSI LLDD supports 16-byte CDBs.

Remember ATAPI<->SCSI bridges exist, thus any SCSI disk can appear 
attached to ATAPI.

	Jeff

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

[Brian King]

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

Jeff Garzik wrote:
> Brian King wrote:
>> Going along with that argument, there's really no good reason for
>> libata to be playing games with scsi_host->max_cmd_len based on
>> the attached devices.
> 
> If both host controller and device support 16-byte CDB, it should not be 
> limited to 12 bytes.

Agreed.

> libata is not "playing games", just exporting what the hardware supports 
> as best the API allows.  It needs to support 16-byte CDBs for the same 
> reason any SCSI LLDD supports 16-byte CDBs.
> 
> Remember ATAPI<->SCSI bridges exist, thus any SCSI disk can appear 
> attached to ATAPI.

The libata code in question is the code that changes
scsi_host->max_cmd_len based on what the attached devices support.
I propose the following patch. This patch is untested.


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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: libata_max_cmd_len.patch --]
[-- Type: text/x-patch; name="libata_max_cmd_len.patch", Size: 1131 bytes --]



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

 libata-dev-bjking1/drivers/scsi/libata-core.c |    8 +-------
 1 files changed, 1 insertion(+), 7 deletions(-)

diff -puN drivers/scsi/libata-core.c~libata_max_cmd_len drivers/scsi/libata-core.c
--- libata-dev/drivers/scsi/libata-core.c~libata_max_cmd_len	2006-04-28 16:34:16.000000000 -0500
+++ libata-dev-bjking1/drivers/scsi/libata-core.c	2006-04-28 16:34:40.000000000 -0500
@@ -1328,12 +1328,6 @@ static int ata_dev_configure(struct ata_
 			       ap->id, dev->devno, ata_mode_string(xfer_mask));
 	}
 
-	ap->host->max_cmd_len = 0;
-	for (i = 0; i < ATA_MAX_DEVICES; i++)
-		ap->host->max_cmd_len = max_t(unsigned int,
-					      ap->host->max_cmd_len,
-					      ap->device[i].cdb_len);
-
 	/* limit bridge transfers to udma5, 200 sectors */
 	if (ata_dev_knobble(ap, dev)) {
 		if (print_info)
@@ -4573,7 +4567,7 @@ static void ata_host_init(struct ata_por
 	host->max_lun = 1;
 	host->max_channel = 1;
 	host->unique_id = ata_unique_id++;
-	host->max_cmd_len = 12;
+	host->max_cmd_len = ATAPI_CDB_LEN;
 
 	ap->flags = ATA_FLAG_DISABLED;
 	ap->id = host->unique_id;
_

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

[James Bottomley]

On Fri, 2006-04-28 at 17:32 -0400, Jeff Garzik wrote:
> If both host controller and device support 16-byte CDB, it should not be 
> limited to 12 bytes.

Well, the limit should really only reflect what the controller supports.
The SCSI heuristics, or slave configure flags should take care of the
devices.

> libata is not "playing games", just exporting what the hardware supports 
> as best the API allows.  It needs to support 16-byte CDBs for the same 
> reason any SCSI LLDD supports 16-byte CDBs.

I think he means this piece of code in libata-core.c:ata_dev_configure()

	ap->host->max_cmd_len = 0;
	for (i = 0; i < ATA_MAX_DEVICES; i++)
		ap->host->max_cmd_len = max_t(unsigned int,
					      ap->host->max_cmd_len,
					      ap->device[i].cdb_len);

You're setting the max_cmd_len based on the largest one supported by the
device.  What happens if that's larger than the HBA can support?

> Remember ATAPI<->SCSI bridges exist, thus any SCSI disk can appear 
> attached to ATAPI.

Well, that's like attaching a 4TB SPI array to a qla1280 (which has a
firmware 12 byte command limit).  You don't do it unless you want a
horrible mess.

James