Re: [PATCH] - export scsilun_to_int

2007-02-01 Thread 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


-
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

2007-02-01 Thread 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 [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

2007-02-01 Thread 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 [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

2007-01-31 Thread 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 [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

RE: [PATCH] - export scsilun_to_int

2007-01-31 Thread 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
-
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

2007-01-31 Thread 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


-
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

2007-01-31 Thread 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 [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

2007-01-31 Thread 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


-
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