a100u2w: fix bitmap lookup routine

PATCH In review Akinobu Mita 4 participants 11 messages Mar 17, 2008 – Mar 20, 2008 SCSI

lore

Share & tools

lore

Akinobu Mita

author

2008-03-17 12:40 UTC #0 lore raw

PATCH 1/1 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 set, it just returns 33-th orc_scb even though the 33-th bit is not set.

Signed-off-by Akinobu Mita <[email protected]>

Cc Alan Cox <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Doug Ledford <[email protected]>

Cc James E.J. Bottomley <[email protected]>

---
 drivers/scsi/a100u2w.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/scsi/a100u2w.c +5 -4

Index: 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;
 }

Akinobu Mita

author

2008-03-17 13:45 UTC #1 lore raw

2008/3/17, Akinobu Mita <[email protected]>:

Quoted 7 lines

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

Quoted 2 lines

is not set.

James Bottomley

maintainer

2008-03-17 14:46 UTC #2 lore raw

In reply to Akinobu Mita

Quoted 1 line · Akinobu Mita

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

Alan Cox

2008-03-17 16:04 UTC #3 lore raw

Quoted 2 lines · James Bottomley

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.

Alan Cox

2008-03-19 19:49 UTC #4 lore raw

In reply to James Bottomley

Quoted 9 lines · James Bottomley

Quoted 1 line · Akinobu Mita
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.

FUJITA Tomonori

2008-03-20 04:57 UTC #5 lore raw

Quoted 4 lines · On Wed, 19 Mar 2008 10:24:19 -0400 Alan Cox <[email protected]> wrote:

Quoted 3 lines · James Bottomley
Quoted 1 line · Akinobu Mita
This patch is only compile tested.

Quoted 8 lines

Quoted 6 lines
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,

drivers/scsi/a100u2w.c +9 ~~-20~~

index f608d4a..a183dab 100644
@@ -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);
 }

Akinobu Mita

author

2008-03-20 09:43 UTC #6 lore raw

Quoted 6 lines · FUJITA Tomonori

(quoted diff) +0 -1

@@ -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;

Review static struct orc_scb *orc_alloc_scb(struct orc_host * host) · line 717

Quoted 10 lines

(quoted diff) +1 -5

 	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 <[email protected]>

---
 drivers/scsi/a100u2w.c |   38 ++++++++++++++------------------------
 drivers/scsi/a100u2w.h |    3 ++-
 2 files changed, 16 insertions(+), 25 deletions(-)

drivers/scsi/a100u2w.c +14 ~~-24~~

Index: 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);
 }

drivers/scsi/a100u2w.h +2 -1

Index: 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;
 };

Akinobu Mita

author

2008-03-20 09:44 UTC #7 lore raw

Quoted 1 line · Akinobu Mita

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 <[email protected]>

Cc Alan Cox <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Doug Ledford <[email protected]>

Cc James E.J. Bottomley <[email protected]>

---
 drivers/scsi/a100u2w.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/scsi/a100u2w.c +5 -4

Index: 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;
 }

Alan Cox

2008-03-20 14:13 UTC #8 lore raw

In reply to Akinobu Mita

Quoted 5 lines

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.

James Bottomley

maintainer

2008-03-20 14:17 UTC #9 lore raw

Quoted 8 lines · Alan Cox

Quoted 5 lines
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

Alan Cox

2008-03-20 16:34 UTC #10 lore raw

Quoted 4 lines · James Bottomley

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