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

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

[Robert P. J. Day]

Signed-off-by: Robert P. J. Day <rpjday@crashcourse.ca>

---

 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
========================================================================

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

[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."

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

[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
========================================================================

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

[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."

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

[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
========================================================================

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

[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.
>
> >  		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.

so, of the above, which can safely be rewritten using
"[!]is_power_of_2"?  and, for now, let's leave out the ones that would
semantically make more sense to be testing for single-bitness -- i'll
try to sneak in a macro like that in the next merge window.

rday

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

http://crashcourse.ca
========================================================================

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

[Mike Christie]

Robert P. J. Day wrote:
> Signed-off-by: Robert P. J. Day <rpjday@crashcourse.ca>
> 
> ---

Thanks for the iscsi bits. We have the libiscsi part from 
vignesh.babu@wipro.com 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.