[PATCH] a100u2w: fix bitmap lookup routine

[PATCH] a100u2w: fix bitmap lookup routine

[Akinobu Mita]

This patch is only compile tested.

It seems that bitmap lookup routine for allocation_map in
a100u2w driver is simply wrong.

It cannot lookup more than first 32 bits. If all first 32 bits
are set, it just returns 33-th orc_scb even though the 33-th bit
is not set.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Alan Cox <alan@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Doug Ledford <dledford@redhat.com>
Cc: James E.J. Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/a100u2w.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: 2.6-rc/drivers/scsi/a100u2w.c
===================================================================
--- 2.6-rc.orig/drivers/scsi/a100u2w.c
+++ 2.6-rc/drivers/scsi/a100u2w.c
@@ -674,12 +674,13 @@ static struct orc_scb *__orc_alloc_scb(s
 		for (index = 0; index < 32; index++) {
 			if ((host->allocation_map[channel][i] >> index) & 0x01) {
 				host->allocation_map[channel][i] &= ~(1 << index);
-				break;
+				idx = index + 32 * i;
+				/*
+				 * Translate the index to a structure instance
+				 */
+				return host->scb_virt + idx;
 			}
 		}
-		idx = index + 32 * i;
-		/* Translate the index to a structure instance */
-		return (struct orc_scb *) ((unsigned long) host->scb_virt + (idx * sizeof(struct orc_scb)));
 	}
 	return NULL;
 }

Re: [PATCH] a100u2w: fix bitmap lookup routine

[Akinobu Mita]

2008/3/17, Akinobu Mita <akinobu.mita@gmail.com>:
> This patch is only compile tested.
>
>  It seems that bitmap lookup routine for allocation_map in
>  a100u2w driver is simply wrong.
>
>  It cannot lookup more than first 32 bits. If all first 32 bits
>  are set, it just returns 33-th orc_scb even though the 33-th bit

   are "not" set, ...

>  is not set.
>

Re: [PATCH] a100u2w: fix bitmap lookup routine

[James Bottomley]

On Mon, 2008-03-17 at 21:32 +0900, Akinobu Mita wrote:
> This patch is only compile tested.

It looks fine to me ... I don't suppose we can find someone with an
actual card to test it out, could we?

Another observation is that this code is really trying to reinvent
bitmaps, so if someone with a card cares they could convert it over to
the linux bitmap infrastructure.

James

Re: [PATCH] a100u2w: fix bitmap lookup routine

[Alan Cox]

On Mon, Mar 17, 2008 at 09:46:44AM -0500, James Bottomley wrote:
> It looks fine to me ... I don't suppose we can find someone with an
> actual card to test it out, could we?

Would that be a subtle hint  - I can test patches at some point but can't
be sure when it'll happen.

Re: [PATCH] a100u2w: fix bitmap lookup routine

[Alan Cox]

On Mon, Mar 17, 2008 at 09:46:44AM -0500, James Bottomley wrote:
> On Mon, 2008-03-17 at 21:32 +0900, Akinobu Mita wrote:
> > This patch is only compile tested.
> 
> It looks fine to me ... I don't suppose we can find someone with an
> actual card to test it out, could we?
> 
> Another observation is that this code is really trying to reinvent
> bitmaps, so if someone with a card cares they could convert it over to
> the linux bitmap infrastructure.

If you want it tested then send me the patch.

Re: [PATCH] a100u2w: fix bitmap lookup routine

[FUJITA Tomonori]

On Wed, 19 Mar 2008 10:24:19 -0400
Alan Cox <alan@redhat.com> wrote:

> On Mon, Mar 17, 2008 at 09:46:44AM -0500, James Bottomley wrote:
> > On Mon, 2008-03-17 at 21:32 +0900, Akinobu Mita wrote:
> > > This patch is only compile tested.
> > 
> > It looks fine to me ... I don't suppose we can find someone with an
> > actual card to test it out, could we?
> > 
> > Another observation is that this code is really trying to reinvent
> > bitmaps, so if someone with a card cares they could convert it over to
> > the linux bitmap infrastructure.
> 
> If you want it tested then send me the patch.

Can you try this?

Thanks,

diff --git a/drivers/scsi/a100u2w.c b/drivers/scsi/a100u2w.c
index f608d4a..a183dab 100644
--- a/drivers/scsi/a100u2w.c
+++ b/drivers/scsi/a100u2w.c
@@ -664,22 +664,16 @@ static int orc_device_reset(struct orc_host * host, struct scsi_cmnd *cmd, unsig
 
 static struct orc_scb *__orc_alloc_scb(struct orc_host * host)
 {
-	u8 channel;
-	unsigned long idx;
-	u8 index;
-	u8 i;
+	long idx;
+	unsigned size = sizeof(host->allocation_map[host->index]) * 8;
+	unsigned long *map = (unsigned long *)host->allocation_map[host->index];
 
-	channel = host->index;
-	for (i = 0; i < 8; i++) {
-		for (index = 0; index < 32; index++) {
-			if ((host->allocation_map[channel][i] >> index) & 0x01) {
-				host->allocation_map[channel][i] &= ~(1 << index);
-				break;
-			}
-		}
-		idx = index + 32 * i;
+	idx = find_first_bit(map, size);
+	if (idx < size) {
+		clear_bit(idx, map);
 		/* Translate the index to a structure instance */
-		return (struct orc_scb *) ((unsigned long) host->scb_virt + (idx * sizeof(struct orc_scb)));
+		return (struct orc_scb *)((unsigned long) host->scb_virt
+					  + (idx * sizeof(struct orc_scb)));
 	}
 	return NULL;
 }
@@ -715,14 +709,9 @@ static struct orc_scb *orc_alloc_scb(struct orc_host * host)
 static void orc_release_scb(struct orc_host *host, struct orc_scb *scb)
 {
 	unsigned long flags;
-	u8 index, i, channel;
 
 	spin_lock_irqsave(&(host->allocation_lock), flags);
-	channel = host->index;	/* Channel */
-	index = scb->scbidx;
-	i = index / 32;
-	index %= 32;
-	host->allocation_map[channel][i] |= (1 << index);
+	set_bit(scb->scbidx, host->allocation_map[host->index]);
 	spin_unlock_irqrestore(&(host->allocation_lock), flags);
 }
 

Re: [PATCH] a100u2w: fix bitmap lookup routine

[Akinobu Mita]

On Thu, Mar 20, 2008 at 01:56:28PM +0900, FUJITA Tomonori wrote:

> @@ -715,14 +709,9 @@ static struct orc_scb *orc_alloc_scb(struct orc_host * host)
>  static void orc_release_scb(struct orc_host *host, struct orc_scb *scb)
>  {
>  	unsigned long flags;
> -	u8 index, i, channel;
>  
>  	spin_lock_irqsave(&(host->allocation_lock), flags);
> -	channel = host->index;	/* Channel */
> -	index = scb->scbidx;
> -	i = index / 32;
> -	index %= 32;
> -	host->allocation_map[channel][i] |= (1 << index);
> +	set_bit(scb->scbidx, host->allocation_map[host->index]);
>  	spin_unlock_irqrestore(&(host->allocation_lock), flags);
>  }
>  

set_bit() and clear_bit() take unsigned long pointer as a bitmap in
the second argument on some architectures. I'd rather change the type of
allocation_map than proliferating the casts.

This is incremental patch to the original bugfix patch.

Subject: [patch 2/2] a100u2w: convert to use bitmap library for allocation_map

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/scsi/a100u2w.c |   38 ++++++++++++++------------------------
 drivers/scsi/a100u2w.h |    3 ++-
 2 files changed, 16 insertions(+), 25 deletions(-)

Index: 2.6-rc/drivers/scsi/a100u2w.c
===================================================================
--- 2.6-rc.orig/drivers/scsi/a100u2w.c
+++ 2.6-rc/drivers/scsi/a100u2w.c
@@ -71,6 +71,7 @@
 #include <linux/ioport.h>
 #include <linux/slab.h>
 #include <linux/dma-mapping.h>
+#include <linux/bitmap.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -478,13 +479,10 @@ static void setup_SCBs(struct orc_host *
 
 static void init_alloc_map(struct orc_host * host)
 {
-	u8 i, j;
+	int i;
 
-	for (i = 0; i < MAX_CHANNELS; i++) {
-		for (j = 0; j < 8; j++) {
-			host->allocation_map[i][j] = 0xffffffff;
-		}
-	}
+	for (i = 0; i < MAX_CHANNELS; i++)
+		bitmap_fill(host->allocation_map[i], 256);
 }
 
 /**
@@ -666,21 +664,16 @@ static struct orc_scb *__orc_alloc_scb(s
 {
 	u8 channel;
 	unsigned long idx;
-	u8 index;
-	u8 i;
 
 	channel = host->index;
-	for (i = 0; i < 8; i++) {
-		for (index = 0; index < 32; index++) {
-			if ((host->allocation_map[channel][i] >> index) & 0x01) {
-				host->allocation_map[channel][i] &= ~(1 << index);
-				idx = index + 32 * i;
-				/*
-				 * Translate the index to a structure instance
-				 */
-				return host->scb_virt + idx;
-			}
-		}
+
+	idx = find_first_bit(host->allocation_map[channel], 256);
+	if (idx < 256) {
+		clear_bit(idx, host->allocation_map[channel]);
+		/*
+		 * Translate the index to a structure instance
+		 */
+		return host->scb_virt + idx;
 	}
 	return NULL;
 }
@@ -716,14 +709,11 @@ static struct orc_scb *orc_alloc_scb(str
 static void orc_release_scb(struct orc_host *host, struct orc_scb *scb)
 {
 	unsigned long flags;
-	u8 index, i, channel;
+	u8 channel;
 
 	spin_lock_irqsave(&(host->allocation_lock), flags);
 	channel = host->index;	/* Channel */
-	index = scb->scbidx;
-	i = index / 32;
-	index %= 32;
-	host->allocation_map[channel][i] |= (1 << index);
+	set_bit(scb->scbidx, host->allocation_map[channel]);
 	spin_unlock_irqrestore(&(host->allocation_lock), flags);
 }
 
Index: 2.6-rc/drivers/scsi/a100u2w.h
===================================================================
--- 2.6-rc.orig/drivers/scsi/a100u2w.h
+++ 2.6-rc/drivers/scsi/a100u2w.h
@@ -239,7 +239,8 @@ struct orc_host {
 	dma_addr_t escb_phys;	/* scatter list Physical address */
 	u8 target_flag[16];	/* target configuration, TCF_EN_TAG */
 	u8 max_tags[16];	/* ORC_MAX_SCBS */
-	u32 allocation_map[MAX_CHANNELS][8];	/* Max STB is 256, So 256/32 */
+	/* Max STB is 256 */
+	unsigned long allocation_map[MAX_CHANNELS][BITS_TO_LONGS(256)];
 	spinlock_t allocation_lock;
 	struct pci_dev *pdev;
 };

Re: [PATCH] a100u2w: fix bitmap lookup routine

[Akinobu Mita]

On Thu, Mar 20, 2008 at 06:34:48PM +0900, Akinobu Mita wrote:
> This is incremental patch to the original bugfix patch.

This is the original bugfix for the reference

Subject: [patch 1/2] a100u2w: fix bitmap lookup routine

This patch is only compile tested.

It seems that bitmap lookup routine for allocation_map in
a100u2w driver is simply wrong.

It cannot lookup more than first 32 bits. If all first 32 bits
are not set, it always returns 33-th orc_scb even though the 33-th bit
is not set.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Alan Cox <alan@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Doug Ledford <dledford@redhat.com>
Cc: James E.J. Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/a100u2w.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: 2.6-rc/drivers/scsi/a100u2w.c
===================================================================
--- 2.6-rc.orig/drivers/scsi/a100u2w.c
+++ 2.6-rc/drivers/scsi/a100u2w.c
@@ -674,12 +674,13 @@ static struct orc_scb *__orc_alloc_scb(s
 		for (index = 0; index < 32; index++) {
 			if ((host->allocation_map[channel][i] >> index) & 0x01) {
 				host->allocation_map[channel][i] &= ~(1 << index);
-				break;
+				idx = index + 32 * i;
+				/*
+				 * Translate the index to a structure instance
+				 */
+				return host->scb_virt + idx;
 			}
 		}
-		idx = index + 32 * i;
-		/* Translate the index to a structure instance */
-		return (struct orc_scb *) ((unsigned long) host->scb_virt + (idx * sizeof(struct orc_scb)));
 	}
 	return NULL;
 }

Re: [PATCH] a100u2w: fix bitmap lookup routine

[Alan Cox]

> set_bit() and clear_bit() take unsigned long pointer as a bitmap in
> the second argument on some architectures. I'd rather change the type of
> allocation_map than proliferating the casts.
> 
> This is incremental patch to the original bugfix patch.

This doesn't apply over the original patch I was sent. Can someone send me
a single complete patch everyone is happy with.

Re: [PATCH] a100u2w: fix bitmap lookup routine

[James Bottomley]

On Thu, 2008-03-20 at 10:12 -0400, Alan Cox wrote:
> > set_bit() and clear_bit() take unsigned long pointer as a bitmap in
> > the second argument on some architectures. I'd rather change the type of
> > allocation_map than proliferating the casts.
> > 
> > This is incremental patch to the original bugfix patch.
> 
> This doesn't apply over the original patch I was sent. Can someone send me
> a single complete patch everyone is happy with.

Actually, could you just verify the original patch from Akinobu Mita?
That's the -rc-fixes candidate.  We can worry about doing bitmaps better
later since it will be for the merge window.  I'll regenerate it against
the tree for you when I get the first one merged.

Thanks,

James

Re: [PATCH] a100u2w: fix bitmap lookup routine

[Alan Cox]

On Thu, Mar 20, 2008 at 09:17:22AM -0500, James Bottomley wrote:
> Actually, could you just verify the original patch from Akinobu Mita?
> That's the -rc-fixes candidate.  We can worry about doing bitmaps better
> later since it will be for the merge window.  I'll regenerate it against
> the tree for you when I get the first one merged.

Seems to work with the basic testing I can do