Re: [PATCH] SCSI: Use is_power_of_2() macro for simplicity.

2007-11-07 Thread Mike Christie 

Robert P. J. Day wrote:

Signed-off-by: Robert P. J. Day [EMAIL PROTECTED]

---


Thanks for the iscsi bits. We have the libiscsi part from 
[EMAIL PROTECTED] queued for 2.6.25. And the iscsi_tcp parts you 
are patching over are broken and I am going to fix that for 2.6.25. We 
only support 1 r2t right now so we cannot hit the bug where if userspace 
negotiated X r2ts we have to use X r2ts we cannot round up.

-
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

[PATCH] SCSI: Use is_power_of_2() macro for simplicity.

2007-11-06 Thread Robert P. J. Day 

Signed-off-by: Robert P. J. Day [EMAIL PROTECTED]

---

 drivers/scsi/NCR53C9x.c  |3 ++-
 drivers/scsi/esp_scsi.c  |3 ++-
 drivers/scsi/iscsi_tcp.c |3 ++-
 drivers/scsi/libiscsi.c  |3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/NCR53C9x.c b/drivers/scsi/NCR53C9x.c
index 5b0efc9..2823f7d 100644
--- a/drivers/scsi/NCR53C9x.c
+++ b/drivers/scsi/NCR53C9x.c
@@ -33,6 +33,7 @@
 #include linux/proc_fs.h
 #include linux/stat.h
 #include linux/init.h
+#include linux/log2.h

 #include scsi.h
 #include scsi/scsi_host.h
@@ -1656,7 +1657,7 @@ static inline int reconnect_target(struct NCR_ESP *esp, 
struct ESP_regs *eregs)
if(!(it  me))
return -1;
it = ~me;
-   if(it  (it - 1))
+   if(!is_power_of_2(it))
return -1;
while(!(it  1))
targ++, it = 1;
diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 4ed3a52..83c454d 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -14,6 +14,7 @@
 #include linux/moduleparam.h
 #include linux/init.h
 #include linux/irqreturn.h
+#include linux/log2.h

 #include asm/irq.h
 #include asm/io.h
@@ -1126,7 +1127,7 @@ static int esp_reconnect(struct esp *esp)
if (!(bits  esp-scsi_id_mask))
goto do_reset;
bits = ~esp-scsi_id_mask;
-   if (!bits || (bits  (bits - 1)))
+   if (!is_power_of_2(bits))
goto do_reset;

target = ffs(bits) - 1;
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 4bcf916..40a7fcc 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -35,6 +35,7 @@
 #include linux/delay.h
 #include linux/kfifo.h
 #include linux/scatterlist.h
+#include linux/log2.h
 #include net/tcp.h
 #include scsi/scsi_cmnd.h
 #include scsi/scsi_device.h
@@ -2076,7 +2077,7 @@ iscsi_conn_set_param(struct iscsi_cls_conn *cls_conn, 
enum iscsi_param param,
break;
iscsi_r2tpool_free(session);
iscsi_set_param(cls_conn, param, buf, buflen);
-   if (session-max_r2t  (session-max_r2t - 1))
+   if (!is_power_of_2(session-max_r2t))
session-max_r2t = roundup_pow_of_two(session-max_r2t);
if (iscsi_r2tpool_alloc(session))
return -ENOMEM;
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index efceed4..daf8a4b 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -24,6 +24,7 @@
 #include linux/types.h
 #include linux/kfifo.h
 #include linux/delay.h
+#include linux/log2.h
 #include asm/unaligned.h
 #include net/tcp.h
 #include scsi/scsi_cmnd.h
@@ -1390,7 +1391,7 @@ iscsi_session_setup(struct iscsi_transport *iscsit,
qdepth = ISCSI_DEF_CMD_PER_LUN;
}

-   if (cmds_max  2 || (cmds_max  (cmds_max - 1)) ||
+   if (cmds_max  2 || !is_power_of_2(cmds_max) ||
cmds_max = ISCSI_MGMT_ITT_OFFSET) {
if (cmds_max != 0)
printk(KERN_ERR iscsi: invalid can_queue of %d. 
-- 

Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://crashcourse.ca

-
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] SCSI: Use is_power_of_2() macro for simplicity.

2007-11-06 Thread Matthew Wilcox 
On Tue, Nov 06, 2007 at 10:20:46AM -0500, Robert P. J. Day wrote:
 @@ -1656,7 +1657,7 @@ static inline int reconnect_target(struct NCR_ESP *esp, 
 struct ESP_regs *eregs)
   if(!(it  me))
   return -1;
   it = ~me;
 - if(it  (it - 1))
 + if(!is_power_of_2(it))
   return -1;

Non-equivalent transform.  Probably a bug.

   bits = ~esp-scsi_id_mask;
 - if (!bits || (bits  (bits - 1)))
 + if (!is_power_of_2(bits))
   goto do_reset;

Non-equivalent transform.  Definitely a bug.

   iscsi_set_param(cls_conn, param, buf, buflen);
 - if (session-max_r2t  (session-max_r2t - 1))
 + if (!is_power_of_2(session-max_r2t))
   session-max_r2t = roundup_pow_of_two(session-max_r2t);

Non-equivalent transform.  Not sure if it's a bug or not.

 
 - if (cmds_max  2 || (cmds_max  (cmds_max - 1)) ||
 + if (cmds_max  2 || !is_power_of_2(cmds_max) ||
   cmds_max = ISCSI_MGMT_ITT_OFFSET) {

This one's OK.

-- 
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.
-
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] SCSI: Use is_power_of_2() macro for simplicity.

2007-11-06 Thread Matthew Wilcox 
On Tue, Nov 06, 2007 at 11:09:51AM -0500, Robert P. J. Day wrote:
 bits = ~esp-scsi_id_mask;
   - if (!bits || (bits  (bits - 1)))
   + if (!is_power_of_2(bits))
 goto do_reset;
 
  Non-equivalent transform.  Definitely a bug.
 
 ok, that one i'm curious about.  how is that not an equivalent
 transform?  am i missing something painfully obvious?

Apologies, I got my boolean algebra wrong.  It is equivalent:

bool is_power_of_2(unsigned long n)
return (n != 0  ((n  (n - 1)) == 0));

substitute it in:

if (!is_power_of_2(bits))

if (!(bits != 0  ((bits  (bits - 1)) == 0)))

push the ! inside brackets:

if (bits == 0 || ((bits  (bits - 1)) != 0))

   - if (!bits || (bits  (bits - 1)))

Clearly the same thing.  Still ... I don't like it because we're not
really looking for 'is this a power of two', we want to know 'is there
exactly one bit set'.  Which, after a bit of thinking, is the same
thing, but it's a bad name for this usage.  Perhaps we could add

#define exactly_one_bit_set is_power_of_2

to the header file.

-- 
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.
-
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] SCSI: Use is_power_of_2() macro for simplicity.

2007-11-06 Thread Robert P. J. Day 
On Tue, 6 Nov 2007, Matthew Wilcox wrote:

 On Tue, Nov 06, 2007 at 10:20:46AM -0500, Robert P. J. Day wrote:
  @@ -1656,7 +1657,7 @@ static inline int reconnect_target(struct NCR_ESP 
  *esp, struct ESP_regs *eregs)
  if(!(it  me))
  return -1;
  it = ~me;
  -   if(it  (it - 1))
  +   if(!is_power_of_2(it))
  return -1;

 Non-equivalent transform.  Probably a bug.

  bits = ~esp-scsi_id_mask;
  -   if (!bits || (bits  (bits - 1)))
  +   if (!is_power_of_2(bits))
  goto do_reset;

 Non-equivalent transform.  Definitely a bug.

ok, that one i'm curious about.  how is that not an equivalent
transform?  am i missing something painfully obvious?

rday
-- 

Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://crashcourse.ca

-
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] SCSI: Use is_power_of_2() macro for simplicity.

2007-11-06 Thread Robert P. J. Day 
On Tue, 6 Nov 2007, Matthew Wilcox wrote:

 On Tue, Nov 06, 2007 at 11:09:51AM -0500, Robert P. J. Day wrote:
bits = ~esp-scsi_id_mask;
-   if (!bits || (bits  (bits - 1)))
+   if (!is_power_of_2(bits))
goto do_reset;
  
   Non-equivalent transform.  Definitely a bug.
 
  ok, that one i'm curious about.  how is that not an equivalent
  transform?  am i missing something painfully obvious?

 Apologies, I got my boolean algebra wrong.  It is equivalent:

 bool is_power_of_2(unsigned long n)
 return (n != 0  ((n  (n - 1)) == 0));

 substitute it in:

   if (!is_power_of_2(bits))

   if (!(bits != 0  ((bits  (bits - 1)) == 0)))

 push the ! inside brackets:

   if (bits == 0 || ((bits  (bits - 1)) != 0))

-   if (!bits || (bits  (bits - 1)))

 Clearly the same thing.  Still ... I don't like it because we're not
 really looking for 'is this a power of two', we want to know 'is there
 exactly one bit set'.  Which, after a bit of thinking, is the same
 thing, but it's a bad name for this usage.  Perhaps we could add

 #define exactly_one_bit_set is_power_of_2

 to the header file.

as i recall, that was a point of early discussion and i don't see
anything terribly wrong with that.  i'll submit a patch for that
shortly and see what folks think.

rday
-- 

Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://crashcourse.ca

-
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