Re: [PATCH] - export scsilun_to_int
Eric Moore wrote: On Wednesday, January 31, 2007 6:52 PM, James Bottomley wrote: what's wrong with memcmp(lun1->scsi_lun, lun2->scsi_lun, 8) rather than introducing a wrapper? The compiler can even optimise memcmp for a fixed size nicely. James Changed to using memcmp. This replaces the prevous patch. Signed-off-by: Eric Moore <[EMAIL PROTECTED]> IMO a wrapper is better. memcmp() is not type-safe nor type-aware, and we have already created a type for SCSI LUNs. Jeff - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] - export scsilun_to_int
On Wednesday, January 31, 2007 6:52 PM, James Bottomley wrote: > > what's wrong with > > memcmp(lun1->scsi_lun, lun2->scsi_lun, 8) > > rather than introducing a wrapper? The compiler can even optimise > memcmp for a fixed size nicely. > > James > Changed to using memcmp. This replaces the prevous patch. Signed-off-by: Eric Moore <[EMAIL PROTECTED]> diff -uarpN b/drivers/message/fusion/mptscsih.c a/drivers/message/fusion/mptscsih.c --- b/drivers/message/fusion/mptscsih.c 2007-01-27 19:09:00.0 -0700 +++ a/drivers/message/fusion/mptscsih.c 2007-02-01 10:09:24.0 -0700 @@ -1016,7 +1016,7 @@ mptscsih_search_running_cmds(MPT_SCSI_HO int ii; int max = hd->ioc->req_depth; struct scsi_cmnd *sc; - int lun; + struct scsi_lun lun; dsprintk((KERN_INFO MYNAM ": search_running channel %d id %d lun %d max %d\n", vdevice->vtarget->channel, vdevice->vtarget->id, vdevice->lun, max)); @@ -1027,13 +1027,14 @@ mptscsih_search_running_cmds(MPT_SCSI_HO mf = (SCSIIORequest_t *)MPT_INDEX_2_MFPTR(hd->ioc, ii); if (mf == NULL) continue; - lun = scsilun_to_int((struct scsi_lun *)mf->LUN); - dsprintk(( "search_running: found (sc=%p, mf = %p) chanel %d id %d, lun %d \n", - hd->ScsiLookup[ii], mf, mf->Bus, mf->TargetID, lun)); + int_to_scsilun(vdevice->lun, &lun); if ((mf->Bus != vdevice->vtarget->channel) || (mf->TargetID != vdevice->vtarget->id) || - (lun != vdevice->lun)) + memcmp(lun.scsi_lun, mf->LUN, 8)) continue; + dsprintk(( "search_running: found (sc=%p, mf = %p) " + "channel %d id %d, lun %d \n", hd->ScsiLookup[ii], + mf, mf->Bus, mf->TargetID, vdevice->lun)); /* Cleanup */ - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] - export scsilun_to_int
James Bottomley wrote: On Wed, 2007-01-31 at 15:54 -0700, Eric Moore wrote: static int +mptscsih_cmp_scsilun(struct scsi_lun * lun1, struct scsi_lun * lun2) +{ + int i; + + for (i = 0; i < 8 ; i++) + if (lun1->scsi_lun[i] != lun2->scsi_lun[i]) + return 1; + + return 0; +} what's wrong with memcmp(lun1->scsi_lun, lun2->scsi_lun, 8) rather than introducing a wrapper? The compiler can even optimise memcmp for a fixed size nicely. I would rather introduce a wrapper that calls memcmp() That's why I have done in my scsilun tree (jgarzik/scsilun-2.6.git, branches submit1, submit1 and hacking) Jeff - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] - export scsilun_to_int
On Wed, 2007-01-31 at 15:54 -0700, Eric Moore wrote: > static int > +mptscsih_cmp_scsilun(struct scsi_lun * lun1, struct scsi_lun * lun2) > +{ > + int i; > + > + for (i = 0; i < 8 ; i++) > + if (lun1->scsi_lun[i] != lun2->scsi_lun[i]) > + return 1; > + > + return 0; > +} what's wrong with memcmp(lun1->scsi_lun, lun2->scsi_lun, 8) rather than introducing a wrapper? The compiler can even optimise memcmp for a fixed size nicely. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] - export scsilun_to_int
On Wednesday, January 31, 2007 1:49 PM, James Bottomley wrote: > Yes, I missed that. However, the mf (SCSIIORequest_t) comes back with > the 8 byte luns, couldn't you just run vdevice->lun through > int_to_scsilun and compare on that? > > I'm really reluctant to export the lun to int lossy transform > because it > will encourage bad uses. I've removed usage of scsilun_to_int. Now I use int_to_scsilun to convert vdevice->lun, then added a new function mptscsih_cmp_scsilun which compares the two luns. This applies over all the previous patchs I posted this week. Signed-off-by: Eric Moore <[EMAIL PROTECTED]> diff -uarpN b/drivers/message/fusion/mptscsih.c a/drivers/message/fusion/mptscsih.c --- b/drivers/message/fusion/mptscsih.c 2007-01-27 19:09:00.0 -0700 +++ a/drivers/message/fusion/mptscsih.c 2007-01-31 15:22:34.0 -0700 @@ -996,6 +996,26 @@ mptscsih_flush_running_cmds(MPT_SCSI_HOS } /* + * mptscsih_cmp_scsilun - compare two luns, lun1 and lun2 + * @lun1: 1st lun + * @lun2: 2nd lun + * + * Returns: zero, if they compare, or 1 when it doesn't + * + */ +static int +mptscsih_cmp_scsilun(struct scsi_lun * lun1, struct scsi_lun * lun2) +{ + int i; + + for (i = 0; i < 8 ; i++) + if (lun1->scsi_lun[i] != lun2->scsi_lun[i]) + return 1; + + return 0; +} + +/* * mptscsih_search_running_cmds - Delete any commands associated * with the specified target and lun. Function called only * when a lun is disable by mid-layer. @@ -1016,7 +1036,7 @@ mptscsih_search_running_cmds(MPT_SCSI_HO int ii; int max = hd->ioc->req_depth; struct scsi_cmnd *sc; - int lun; + struct scsi_lun lun; dsprintk((KERN_INFO MYNAM ": search_running channel %d id %d lun %d max %d\n", vdevice->vtarget->channel, vdevice->vtarget->id, vdevice->lun, max)); @@ -1027,13 +1047,15 @@ mptscsih_search_running_cmds(MPT_SCSI_HO mf = (SCSIIORequest_t *)MPT_INDEX_2_MFPTR(hd->ioc, ii); if (mf == NULL) continue; - lun = scsilun_to_int((struct scsi_lun *)mf->LUN); - dsprintk(( "search_running: found (sc=%p, mf = %p) chanel %d id %d, lun %d \n", - hd->ScsiLookup[ii], mf, mf->Bus, mf->TargetID, lun)); + int_to_scsilun(vdevice->lun, &lun); if ((mf->Bus != vdevice->vtarget->channel) || (mf->TargetID != vdevice->vtarget->id) || - (lun != vdevice->lun)) + mptscsih_cmp_scsilun(&lun, + (struct scsi_lun *)mf->LUN)) continue; + dsprintk(( "search_running: found (sc=%p, mf = %p) " + "channel %d id %d, lun %d \n", hd->ScsiLookup[ii], + mf, mf->Bus, mf->TargetID, vdevice->lun)); /* Cleanup */ - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] - export scsilun_to_int
On Wed, 2007-01-31 at 12:44 -0700, Moore, Eric wrote: > On Wednesday, January 31, 2007 10:02 AM, James Bottomley wrote: > > This is wrong. the "int" represents our internal coding of the 8 byte > > extended LUN (currently it's a lossy transform that only allows up to > > two level LUNs, so one day this will definitely change). No driver > > should be using this internally. From the patches it merely > > looks like > > you want to print out the value of a struct scsilun in debugging code, > > so perhaps a simple print_scsilun or something would be better? > > > > James > > > No, this section of code is needed for more than printing the lun value. > Here is a snip from that patch you may of missed: > > - if ((mf->TargetID != ((u8)vdevice->vtarget->target_id)) || (mf->LUN[1] > != ((u8) vdevice->lun))) > +if ((mf->Bus != vdevice->vtarget->channel) || > +(mf->TargetID != vdevice->vtarget->id) || > +(lun != vdevice->lun)) Yes, I missed that. However, the mf (SCSIIORequest_t) comes back with the 8 byte luns, couldn't you just run vdevice->lun through int_to_scsilun and compare on that? I'm really reluctant to export the lun to int lossy transform because it will encourage bad uses. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] - export scsilun_to_int
On Wednesday, January 31, 2007 10:02 AM, James Bottomley wrote: > This is wrong. the "int" represents our internal coding of the 8 byte > extended LUN (currently it's a lossy transform that only allows up to > two level LUNs, so one day this will definitely change). No driver > should be using this internally. From the patches it merely > looks like > you want to print out the value of a struct scsilun in debugging code, > so perhaps a simple print_scsilun or something would be better? > > James No, this section of code is needed for more than printing the lun value. Here is a snip from that patch you may of missed: - if ((mf->TargetID != ((u8)vdevice->vtarget->target_id)) || (mf->LUN[1] != ((u8) vdevice->lun))) +if ((mf->Bus != vdevice->vtarget->channel) || +(mf->TargetID != vdevice->vtarget->id) || +(lun != vdevice->lun)) We need to convert the SAM3 LUN format, back to the scsi-mid layer version of the lun value, which currently is defined as an integer in linux.We save that scsi midlayer verison of lun, inside vdevice->lun at the start of day. The mf->LUN field will be in SAM3 LUN format. This sanity if/then check is required as we are trying to complete outstanding request for a given scsi_device, eg. lun. Do you want me to create my own function for converting the lun value, or can I use this one already defined in the linux kernel? Eric - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] - export scsilun_to_int
On Mon, 2007-01-29 at 09:40 -0700, Eric Moore wrote: > export symbol to be used in 1st fusion patch > > Signed-off-by: Eric Moore <[EMAIL PROTECTED]> This is wrong. the "int" represents our internal coding of the 8 byte extended LUN (currently it's a lossy transform that only allows up to two level LUNs, so one day this will definitely change). No driver should be using this internally. From the patches it merely looks like you want to print out the value of a struct scsilun in debugging code, so perhaps a simple print_scsilun or something would be better? James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html