* [PATCH 4/4] aic7xxx: Enable 16-bit CDBs @ 2007-10-19 8:32 Hannes Reinecke 2007-10-21 16:19 ` James Bottomley 0 siblings, 1 reply; 11+ messages in thread From: Hannes Reinecke @ 2007-10-19 8:32 UTC (permalink / raw) To: James Bottomley; +Cc: linux-scsi 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs 2007-10-19 8:32 [PATCH 4/4] aic7xxx: Enable 16-bit CDBs Hannes Reinecke @ 2007-10-21 16:19 ` James Bottomley 2007-10-22 3:46 ` Matthew Wilcox 2007-10-22 19:10 ` Boaz Harrosh 0 siblings, 2 replies; 11+ messages in thread From: James Bottomley @ 2007-10-21 16:19 UTC (permalink / raw) To: Hannes Reinecke; +Cc: linux-scsi 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs 2007-10-21 16:19 ` James Bottomley @ 2007-10-22 3:46 ` Matthew Wilcox 2007-10-22 6:50 ` Hannes Reinecke 2007-10-22 19:10 ` Boaz Harrosh 1 sibling, 1 reply; 11+ messages in thread From: Matthew Wilcox @ 2007-10-22 3:46 UTC (permalink / raw) To: James Bottomley; +Cc: Hannes Reinecke, linux-scsi 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." ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs 2007-10-22 3:46 ` Matthew Wilcox @ 2007-10-22 6:50 ` Hannes Reinecke 2007-10-22 11:23 ` Matthew Wilcox 0 siblings, 1 reply; 11+ messages in thread From: Hannes Reinecke @ 2007-10-22 6:50 UTC (permalink / raw) To: Matthew Wilcox; +Cc: James Bottomley, linux-scsi 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs 2007-10-22 6:50 ` Hannes Reinecke @ 2007-10-22 11:23 ` Matthew Wilcox 2007-10-22 11:57 ` Hannes Reinecke 0 siblings, 1 reply; 11+ messages in thread From: Matthew Wilcox @ 2007-10-22 11:23 UTC (permalink / raw) To: Hannes Reinecke; +Cc: James Bottomley, linux-scsi 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." ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs 2007-10-22 11:23 ` Matthew Wilcox @ 2007-10-22 11:57 ` Hannes Reinecke 2007-10-23 16:56 ` James Bottomley 0 siblings, 1 reply; 11+ messages in thread From: Hannes Reinecke @ 2007-10-22 11:57 UTC (permalink / raw) To: Matthew Wilcox; +Cc: James Bottomley, linux-scsi 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs 2007-10-22 11:57 ` Hannes Reinecke @ 2007-10-23 16:56 ` James Bottomley 2007-10-25 6:31 ` Hannes Reinecke 0 siblings, 1 reply; 11+ messages in thread From: James Bottomley @ 2007-10-23 16:56 UTC (permalink / raw) To: Hannes Reinecke; +Cc: Matthew Wilcox, linux-scsi 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs 2007-10-23 16:56 ` James Bottomley @ 2007-10-25 6:31 ` Hannes Reinecke 0 siblings, 0 replies; 11+ messages in thread From: Hannes Reinecke @ 2007-10-25 6:31 UTC (permalink / raw) To: James Bottomley; +Cc: Matthew Wilcox, linux-scsi 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs 2007-10-21 16:19 ` James Bottomley 2007-10-22 3:46 ` Matthew Wilcox @ 2007-10-22 19:10 ` Boaz Harrosh 2007-10-22 19:35 ` Matthew Wilcox 1 sibling, 1 reply; 11+ messages in thread From: Boaz Harrosh @ 2007-10-22 19:10 UTC (permalink / raw) To: James Bottomley, Hannes Reinecke, Matthew Wilcox; +Cc: linux-scsi 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs 2007-10-22 19:10 ` Boaz Harrosh @ 2007-10-22 19:35 ` Matthew Wilcox 2007-10-22 21:24 ` Benny Halevy 0 siblings, 1 reply; 11+ messages in thread From: Matthew Wilcox @ 2007-10-22 19:35 UTC (permalink / raw) To: Boaz Harrosh; +Cc: James Bottomley, Hannes Reinecke, linux-scsi 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." ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] aic7xxx: Enable 16-bit CDBs 2007-10-22 19:35 ` Matthew Wilcox @ 2007-10-22 21:24 ` Benny Halevy 0 siblings, 0 replies; 11+ messages in thread From: Benny Halevy @ 2007-10-22 21:24 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Boaz Harrosh, James Bottomley, Hannes Reinecke, linux-scsi 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2007-10-25 6:31 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-10-19 8:32 [PATCH 4/4] aic7xxx: Enable 16-bit CDBs Hannes Reinecke 2007-10-21 16:19 ` James Bottomley 2007-10-22 3:46 ` Matthew Wilcox 2007-10-22 6:50 ` Hannes Reinecke 2007-10-22 11:23 ` Matthew Wilcox 2007-10-22 11:57 ` Hannes Reinecke 2007-10-23 16:56 ` James Bottomley 2007-10-25 6:31 ` Hannes Reinecke 2007-10-22 19:10 ` Boaz Harrosh 2007-10-22 19:35 ` Matthew Wilcox 2007-10-22 21:24 ` Benny Halevy
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).