[PATCH 4/4] aic7xxx: Enable 16-bit CDBs

[PATCH 4/4] aic7xxx: Enable 16-bit CDBs

[Hannes Reinecke]

The patch enables support for 16-bit CDBs in aic7xxx and aic79xx.
aic7xxx can actually support up to 32-bit CDBs, should they ever see
the light of day.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/aic7xxx/aic79xx.h     |    2 ++
 drivers/scsi/aic7xxx/aic79xx_osm.c |    1 +
 drivers/scsi/aic7xxx/aic7xxx_osm.c |    1 +
 3 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/aic7xxx/aic79xx.h b/drivers/scsi/aic7xxx/aic79xx.h
index ce638aa..883f26a 100644
--- a/drivers/scsi/aic7xxx/aic79xx.h
+++ b/drivers/scsi/aic7xxx/aic79xx.h
@@ -413,6 +413,8 @@ struct target_status {
  * the sense buffer address in the SCB.  This allows
  * us to retrieve sense information without interrupting
  * the host in packetized mode.
+ * The current firmware relies on a CDB len of 16, so
+ * don't change it unless you know what you're doing.
  */
 typedef uint32_t sense_addr_t;
 #define MAX_CDB_LEN 16
diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c b/drivers/scsi/aic7xxx/aic79xx_osm.c
index 2d02040..f4e12e1 100644
--- a/drivers/scsi/aic7xxx/aic79xx_osm.c
+++ b/drivers/scsi/aic7xxx/aic79xx_osm.c
@@ -1090,6 +1090,7 @@ ahd_linux_register_host(struct ahd_softc *ahd, struct scsi_host_template *templa
 	host->max_id = (ahd->features & AHD_WIDE) ? 16 : 8;
 	host->max_lun = AHD_NUM_LUNS;
 	host->max_channel = 0;
+	host->max_cmd_len = 16;
 	host->sg_tablesize = AHD_NSEG;
 	ahd_lock(ahd, &s);
 	ahd_set_unit(ahd, ahd_linux_unit++);
diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm.c b/drivers/scsi/aic7xxx/aic7xxx_osm.c
index 390b0fc..d488764 100644
--- a/drivers/scsi/aic7xxx/aic7xxx_osm.c
+++ b/drivers/scsi/aic7xxx/aic7xxx_osm.c
@@ -1048,6 +1048,7 @@ ahc_linux_register_host(struct ahc_softc *ahc, struct scsi_host_template *templa
 	host->max_id = (ahc->features & AHC_WIDE) ? 16 : 8;
 	host->max_lun = AHC_NUM_LUNS;
 	host->max_channel = (ahc->features & AHC_TWIN) ? 1 : 0;
+	host->max_cmd_len = 32;
 	host->sg_tablesize = AHC_NSEG;
 	ahc_lock(ahc, &s);
 	ahc_set_unit(ahc, ahc_linux_unit++);
-- 
1.5.2.4

Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs

[James Bottomley]

On Fri, 2007-10-19 at 10:32 +0200, Hannes Reinecke wrote:
> The patch enables support for 16-bit CDBs in aic7xxx and aic79xx.
> aic7xxx can actually support up to 32-bit CDBs, should they ever see
> the light of day.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/aic7xxx/aic79xx.h     |    2 ++
>  drivers/scsi/aic7xxx/aic79xx_osm.c |    1 +
>  drivers/scsi/aic7xxx/aic7xxx_osm.c |    1 +
>  3 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/aic7xxx/aic79xx.h b/drivers/scsi/aic7xxx/aic79xx.h
> index ce638aa..883f26a 100644
> --- a/drivers/scsi/aic7xxx/aic79xx.h
> +++ b/drivers/scsi/aic7xxx/aic79xx.h
> @@ -413,6 +413,8 @@ struct target_status {
>   * the sense buffer address in the SCB.  This allows
>   * us to retrieve sense information without interrupting
>   * the host in packetized mode.
> + * The current firmware relies on a CDB len of 16, so
> + * don't change it unless you know what you're doing.
>   */
>  typedef uint32_t sense_addr_t;
>  #define MAX_CDB_LEN 16
> diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c b/drivers/scsi/aic7xxx/aic79xx_osm.c
> index 2d02040..f4e12e1 100644
> --- a/drivers/scsi/aic7xxx/aic79xx_osm.c
> +++ b/drivers/scsi/aic7xxx/aic79xx_osm.c
> @@ -1090,6 +1090,7 @@ ahd_linux_register_host(struct ahd_softc *ahd, struct scsi_host_template *templa
>  	host->max_id = (ahd->features & AHD_WIDE) ? 16 : 8;
>  	host->max_lun = AHD_NUM_LUNS;
>  	host->max_channel = 0;
> +	host->max_cmd_len = 16;

Actually, the aic79xx doc we now have through the LF NDA programme says
that the 79xx can also go up to 32 byte CDBs.

>  	host->sg_tablesize = AHD_NSEG;
>  	ahd_lock(ahd, &s);
>  	ahd_set_unit(ahd, ahd_linux_unit++);
> diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm.c b/drivers/scsi/aic7xxx/aic7xxx_osm.c
> index 390b0fc..d488764 100644
> --- a/drivers/scsi/aic7xxx/aic7xxx_osm.c
> +++ b/drivers/scsi/aic7xxx/aic7xxx_osm.c
> @@ -1048,6 +1048,7 @@ ahc_linux_register_host(struct ahc_softc *ahc, struct scsi_host_template *templa
>  	host->max_id = (ahc->features & AHC_WIDE) ? 16 : 8;
>  	host->max_lun = AHC_NUM_LUNS;
>  	host->max_channel = (ahc->features & AHC_TWIN) ? 1 : 0;
> +	host->max_cmd_len = 32;
>  	host->sg_tablesize = AHC_NSEG;
>  	ahc_lock(ahc, &s);
>  	ahc_set_unit(ahc, ahc_linux_unit++);

But, I suppose the main point is: is this really necessary?  The
default, when unspecified, is 16 bytes and the mid-layer can't deliver
anything larger.  I would anticipate the large CDB infrastructure will
have some separate enabling, so why not simply do nothing until that
gets merged?

James

Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs

[Matthew Wilcox]

On Sun, Oct 21, 2007 at 11:19:22AM -0500, James Bottomley wrote:
> But, I suppose the main point is: is this really necessary?  The
> default, when unspecified, is 16 bytes and the mid-layer can't deliver
> anything larger.  I would anticipate the large CDB infrastructure will
> have some separate enabling, so why not simply do nothing until that
> gets merged?

You're sadly mistaken ... the default is 12 byte cdbs:

        shost->max_cmd_len = 12;

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs

[Hannes Reinecke]

Matthew Wilcox wrote:
> On Sun, Oct 21, 2007 at 11:19:22AM -0500, James Bottomley wrote:
>> But, I suppose the main point is: is this really necessary?  The
>> default, when unspecified, is 16 bytes and the mid-layer can't deliver
>> anything larger.  I would anticipate the large CDB infrastructure will
>> have some separate enabling, so why not simply do nothing until that
>> gets merged?
> 
> You're sadly mistaken ... the default is 12 byte cdbs:
> 
>         shost->max_cmd_len = 12;
> 
That's what I thought, too. Hence the patch.
But if we set the default to 16 byte cdbs you can of course ignore
the patch.

Cheers,

Hannes

-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs

[Matthew Wilcox]

On Mon, Oct 22, 2007 at 08:50:45AM +0200, Hannes Reinecke wrote:
> That's what I thought, too. Hence the patch.
> But if we set the default to 16 byte cdbs you can of course ignore
> the patch.

I think the comment in hosts.c explains why we shouldn't do that ...

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs

[Hannes Reinecke]

Matthew Wilcox wrote:
> On Mon, Oct 22, 2007 at 08:50:45AM +0200, Hannes Reinecke wrote:
>> That's what I thought, too. Hence the patch.
>> But if we set the default to 16 byte cdbs you can of course ignore
>> the patch.
> 
> I think the comment in hosts.c explains why we shouldn't do that ...
> 
_I_ certainly wouldn't. But if the maintainer says so :-)

Cheers,

Hannes

-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs

[Boaz Harrosh]

On Sun, Oct 21 2007 at 18:19 +0200, James Bottomley <James.Bottomley@SteelEye.com> wrote:
> On Fri, 2007-10-19 at 10:32 +0200, Hannes Reinecke wrote:
>> The patch enables support for 16-bit CDBs in aic7xxx and aic79xx.
>> aic7xxx can actually support up to 32-bit CDBs, should they ever see
>> the light of day.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/scsi/aic7xxx/aic79xx.h     |    2 ++
>>  drivers/scsi/aic7xxx/aic79xx_osm.c |    1 +
>>  drivers/scsi/aic7xxx/aic7xxx_osm.c |    1 +
>>  3 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/scsi/aic7xxx/aic79xx.h b/drivers/scsi/aic7xxx/aic79xx.h
>> index ce638aa..883f26a 100644
>> --- a/drivers/scsi/aic7xxx/aic79xx.h
>> +++ b/drivers/scsi/aic7xxx/aic79xx.h
>> @@ -413,6 +413,8 @@ struct target_status {
>>   * the sense buffer address in the SCB.  This allows
>>   * us to retrieve sense information without interrupting
>>   * the host in packetized mode.
>> + * The current firmware relies on a CDB len of 16, so
>> + * don't change it unless you know what you're doing.
>>   */
>>  typedef uint32_t sense_addr_t;
>>  #define MAX_CDB_LEN 16
>> diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c b/drivers/scsi/aic7xxx/aic79xx_osm.c
>> index 2d02040..f4e12e1 100644
>> --- a/drivers/scsi/aic7xxx/aic79xx_osm.c
>> +++ b/drivers/scsi/aic7xxx/aic79xx_osm.c
>> @@ -1090,6 +1090,7 @@ ahd_linux_register_host(struct ahd_softc *ahd, struct scsi_host_template *templa
>>  	host->max_id = (ahd->features & AHD_WIDE) ? 16 : 8;
>>  	host->max_lun = AHD_NUM_LUNS;
>>  	host->max_channel = 0;
>> +	host->max_cmd_len = 16;
> 
> Actually, the aic79xx doc we now have through the LF NDA programme says
> that the 79xx can also go up to 32 byte CDBs.
> 
>>  	host->sg_tablesize = AHD_NSEG;
>>  	ahd_lock(ahd, &s);
>>  	ahd_set_unit(ahd, ahd_linux_unit++);
>> diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm.c b/drivers/scsi/aic7xxx/aic7xxx_osm.c
>> index 390b0fc..d488764 100644
>> --- a/drivers/scsi/aic7xxx/aic7xxx_osm.c
>> +++ b/drivers/scsi/aic7xxx/aic7xxx_osm.c
>> @@ -1048,6 +1048,7 @@ ahc_linux_register_host(struct ahc_softc *ahc, struct scsi_host_template *templa
>>  	host->max_id = (ahc->features & AHC_WIDE) ? 16 : 8;
>>  	host->max_lun = AHC_NUM_LUNS;
>>  	host->max_channel = (ahc->features & AHC_TWIN) ? 1 : 0;
>> +	host->max_cmd_len = 32;
>>  	host->sg_tablesize = AHC_NSEG;
>>  	ahc_lock(ahc, &s);
>>  	ahc_set_unit(ahc, ahc_linux_unit++);
> 
> But, I suppose the main point is: is this really necessary?  The
> default, when unspecified, is 16 bytes and the mid-layer can't deliver
> anything larger.  I would anticipate the large CDB infrastructure will
> have some separate enabling, so why not simply do nothing until that
> gets merged?
> 
> James

I'm about to finish an RFC patchset for the extended commands.
I have implemented a more aggressive approach than the one
I've been sending for the last year.
(Matthew I have an extra 8-bytes save to scsi_cmnd on 
64bit and 12 bytes for 32bit. Guess how? ;))

>From what I have now, (And also the old patch sets) host->max_cmd_len serves the same
function, Limit the command size sent by the mid-layer. Even more so with the support
of extended commands.

So I recommend this patch.

If a driver does not have internal fixed-sized buffers to store the CDB than 
setting of host->max_cmd_len, and use of cmnd->cmd_len will be enough to support
extended CDB's.

Boaz

Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs

[Matthew Wilcox]

On Mon, Oct 22, 2007 at 09:10:49PM +0200, Boaz Harrosh wrote:
> I'm about to finish an RFC patchset for the extended commands.
> I have implemented a more aggressive approach than the one
> I've been sending for the last year.
> (Matthew I have an extra 8-bytes save to scsi_cmnd on 
> 64bit and 12 bytes for 32bit. Guess how? ;))

Well ... the command has to be stored somewhere.  If it's an additional
kmalloc, that's a loss.  If it's in the request, that's a loss too ...
let's see where you're keeping it ;-)

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs

[Benny Halevy]

On Oct. 22, 2007, 21:35 +0200, Matthew Wilcox <matthew@wil.cx> wrote:
> On Mon, Oct 22, 2007 at 09:10:49PM +0200, Boaz Harrosh wrote:
>> I'm about to finish an RFC patchset for the extended commands.
>> I have implemented a more aggressive approach than the one
>> I've been sending for the last year.
>> (Matthew I have an extra 8-bytes save to scsi_cmnd on 
>> 64bit and 12 bytes for 32bit. Guess how? ;))
> 
> Well ... the command has to be stored somewhere.  If it's an additional
> kmalloc, that's a loss.  If it's in the request, that's a loss too ...
> let's see where you're keeping it ;-)
> 
I love spoiling Boaz's surprise :)

The gist of it is scsi_cmnd->cmnd pointing at req->cmd
or at req->varlen_cmd as appropriate.

Most users can't tell the difference except if they
tried using sizeof(scsi_cmnd->cmnd) in which case they
can use MAX_COMMAND_SIZE instead.

Benny

Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs

[James Bottomley]

On Mon, 2007-10-22 at 13:57 +0200, Hannes Reinecke wrote:
> Matthew Wilcox wrote:
> > On Mon, Oct 22, 2007 at 08:50:45AM +0200, Hannes Reinecke wrote:
> >> That's what I thought, too. Hence the patch.
> >> But if we set the default to 16 byte cdbs you can of course ignore
> >> the patch.
> > 
> > I think the comment in hosts.c explains why we shouldn't do that ...
> > 
> _I_ certainly wouldn't. But if the maintainer says so :-)

Actually, I misremembered.  I thought I did a commit a few years ago
that automatically raised the cutoff to 16 byte cdbs and forced drivers
who couldn't do that to lower the limit, but apparently it was never
merged.

So, just pick either 16 or 32 for both drivers and add that as the max
cdb size.

James

Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs

[Hannes Reinecke]

James Bottomley wrote:
> On Mon, 2007-10-22 at 13:57 +0200, Hannes Reinecke wrote:
>> Matthew Wilcox wrote:
>>> On Mon, Oct 22, 2007 at 08:50:45AM +0200, Hannes Reinecke wrote:
>>>> That's what I thought, too. Hence the patch.
>>>> But if we set the default to 16 byte cdbs you can of course ignore
>>>> the patch.
>>> I think the comment in hosts.c explains why we shouldn't do that ...
>>>
>> _I_ certainly wouldn't. But if the maintainer says so :-)
> 
> Actually, I misremembered.  I thought I did a commit a few years ago
> that automatically raised the cutoff to 16 byte cdbs and forced drivers
> who couldn't do that to lower the limit, but apparently it was never
> merged.
> 
> So, just pick either 16 or 32 for both drivers and add that as the max
> cdb size.
> 
Hmm. But aic7xxx actually supports 32-byte cdbs (for whatever reason);
cf drivers/scsi/aic7xxx/aic7xxx.h:478.
And aic79xx only supports 16 byte cdbs; the clever 'use pointer for cdb
if it's longer than the 12 bytes' routine sadly isn't implemented
for aic79xx.

It somehow feels wrong to arbitrarily cut the limit to 16 bytes for
aic7xxx, hence the two different limits.

But again, you're the maintainer, you get to decide.
If you feel it's reasonable to limit both to 16 byte so be it.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html