[PATCH] - export scsilun_to_int

[PATCH] - export scsilun_to_int

[Eric Moore]

export symbol to be used in 1st fusion patch

Signed-off-by: Eric Moore <Eric.Moore@lsi.com>

diff -uarpN b/drivers/scsi/scsi_scan.c a/drivers/scsi/scsi_scan.c
--- b/drivers/scsi/scsi_scan.c	2007-01-24 19:19:28.000000000 -0700
+++ a/drivers/scsi/scsi_scan.c	2007-01-28 12:42:12.000000000 -0700
@@ -1192,7 +1192,7 @@ static void scsi_sequential_lun_scan(str
  *     Given a struct scsi_lun of: 0a 04 0b 03 00 00 00 00, this function returns
  *     the integer: 0x0b030a04
  **/
-static int scsilun_to_int(struct scsi_lun *scsilun)
+int scsilun_to_int(struct scsi_lun *scsilun)
 {
 	int i;
 	unsigned int lun;
@@ -1203,6 +1203,7 @@ static int scsilun_to_int(struct scsi_lu
 			      scsilun->scsi_lun[i + 1]) << (i * 8));
 	return lun;
 }
+EXPORT_SYMBOL(scsilun_to_int);
 
 /**
  * int_to_scsilun: reverts an int into a scsi_lun
diff -uarpN b/include/scsi/scsi_device.h a/include/scsi/scsi_device.h
--- b/include/scsi/scsi_device.h	2007-01-24 19:19:28.000000000 -0700
+++ a/include/scsi/scsi_device.h	2007-01-28 12:42:06.000000000 -0700
@@ -281,6 +281,7 @@ extern void scsi_target_block(struct dev
 extern void scsi_target_unblock(struct device *);
 extern void scsi_remove_target(struct device *);
 extern void int_to_scsilun(unsigned int, struct scsi_lun *);
+extern int scsilun_to_int(struct scsi_lun *scsilun);
 extern const char *scsi_device_state_name(enum scsi_device_state);
 extern int scsi_is_sdev_device(const struct device *);
 extern int scsi_is_target_device(const struct device *);

Re: [PATCH] - export scsilun_to_int

[James Bottomley]

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 <Eric.Moore@lsi.com>

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

Re: [PATCH] - export scsilun_to_int

[Jeff Garzik]

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 <Eric.Moore@lsi.com>

IMO a wrapper is better.

memcmp() is not type-safe nor type-aware, and we have already created a 
type for SCSI LUNs.

	Jeff

RE: [PATCH] - export scsilun_to_int

[Eric Moore]

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 <Eric.Moore@lsi.com>


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.000000000 -0700
+++ a/drivers/message/fusion/mptscsih.c	2007-02-01 10:09:24.000000000 -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
 			 */

Re: [PATCH] - export scsilun_to_int

[Jeff Garzik]

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

RE: [PATCH] - export scsilun_to_int

[James Bottomley]

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

RE: [PATCH] - export scsilun_to_int

[Eric Moore]

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 <Eric.Moore@lsi.com>


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.000000000 -0700
+++ a/drivers/message/fusion/mptscsih.c	2007-01-31 15:22:34.000000000 -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
 			 */

RE: [PATCH] - export scsilun_to_int

[James Bottomley]

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

RE: [PATCH] - export scsilun_to_int

[Moore, Eric]

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