mvsas: Fine-tuned tags array for alignment

PATCH In review Ying Chu 3 participants 7 messages May 8, 2009 – May 9, 2009 SCSI

lore

Share & tools

lore

Ying Chu

author

2009-05-08 06:19 UTC #0 lore raw

PATCH 4/4 mvsas: Fine-tuned tags array for alignment

Quoted 1 line

From 2269b0ff3c2573f76cb0569eb5da99e9f12711d1 Mon Sep 17 00:00:00 2001

From: ayan <[email protected]>
Date: Fri, 8 May 2009 19:59:39 +0800
Subject: [PATCH 4/4] bug fix: alignment

Fine-tuned tags array with u32 for alignment.

Signed-off-by Ying Chu <[email protected]>

Signed-off-by Andy Yan <[email protected]>

Signed-off-by Ke Wei <[email protected]>

---
 drivers/scsi/mvsas/mv_sas.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/scsi/mvsas/mv_sas.h +1 -1

index 75b9748..04f27bf 100644
@@ -313,7 +313,7 @@ struct mvs_info {
 	const struct mvs_chip_info *chip;
 
 	int tags_num;
-	u8 tags[MVS_SLOTS >> 3];
+	u32 tags[MVS_SLOTS >> 5];
 
 	/* further per-slot information */
 	struct mvs_phy phy[MVS_MAX_PHYS];

Signature · 5 lines

1.6.0.3

-- 
Regards,
Ying Chu

James Bottomley

maintainer

2009-05-08 15:20 UTC #1 lore raw

Quoted 19 lines · Ying Chu

Quoted 1 line
From 2269b0ff3c2573f76cb0569eb5da99e9f12711d1 Mon Sep 17 00:00:00 2001
From: ayan <[email protected]>
Date: Fri, 8 May 2009 19:59:39 +0800
Subject: [PATCH 4/4] bug fix: alignment
Fine-tuned tags array with u32 for alignment.
Signed-off-by Ying Chu <[email protected]>
Signed-off-by Andy Yan <[email protected]>
Signed-off-by Ke Wei <[email protected]>
---
 drivers/scsi/mvsas/mv_sas.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/mvsas/mv_sas.h +0 -0
index 75b9748..04f27bf 100644

drivers/scsi/mvsas/mv_sas.h struct mvs_info { · line 316

Quoted 5 lines

drivers/scsi/mvsas/mv_sas.h +1 -1

@@ -313,7 +313,7 @@ struct mvs_info {
 	const struct mvs_chip_info *chip;
 
 	int tags_num;
-	u8 tags[MVS_SLOTS >> 3];
+	u32 tags[MVS_SLOTS >> 5];

Why not just do

DECLARE_BITMAP(tags, MVS_SLOTS);

here? ... that way you're using the correctly defined type for the bitmap operations and you can lose the spurious (void *) cast in your bitmap wrapper operations.

James

Jeff Garzik

2009-05-08 16:23 UTC #2 lore raw

Quoted 28 lines · James Bottomley

Quoted 19 lines · Ying Chu
Quoted 1 line
From 2269b0ff3c2573f76cb0569eb5da99e9f12711d1 Mon Sep 17 00:00:00 2001
From: ayan <[email protected]>
Date: Fri, 8 May 2009 19:59:39 +0800
Subject: [PATCH 4/4] bug fix: alignment
Fine-tuned tags array with u32 for alignment.
Signed-off-by Ying Chu <[email protected]>
Signed-off-by Andy Yan <[email protected]>
Signed-off-by Ke Wei <[email protected]>
---
 drivers/scsi/mvsas/mv_sas.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/mvsas/mv_sas.h +0 -0
index 75b9748..04f27bf 100644
drivers/scsi/mvsas/mv_sas.h struct mvs_info { · line 316
Quoted 5 lines
drivers/scsi/mvsas/mv_sas.h +1 -1
@@ -313,7 +313,7 @@ struct mvs_info {
 	const struct mvs_chip_info *chip;
 
 	int tags_num;
-	u8 tags[MVS_SLOTS >> 3];
+	u32 tags[MVS_SLOTS >> 5];
Why not just do

Quoted 5 lines

DECLARE_BITMAP(tags, MVS_SLOTS);
here? ... that way you're using the correctly defined type for the bitmap operations and you can lose the spurious (void *) cast in your bitmap wrapper operations.

Agreed. Type 'u32' is incorrect for bitmap operations... it needs to be 'unsigned long', which is what DECLARE_BITMAP gives you.

Jeff

Ying Chu

author

2009-05-09 06:42 UTC #3 lore raw

Hi, James

DECLARE_BITMAP isn't a good option for mvsas, since the tags array couldn't be global(it's chip-related) and even if we define the static tag array both in mv64xx.c and mv94xx.c, it still couldn't honor 88SE9480 as there are 2 separate cores. So if we use DECLARE_BITMAP, it's hard to maintain the corresponding tag for different core.

Can we use the following replacement:
- u8 tags[MVS_SLOTS >> 3];
+ unsigned long tags[MVS_SLOTS / BITS_PER_LONG];

Thank you.

-----Original Message-----
From: Jeff Garzik [mailto:[email protected]]
Sent: 2009年5月9日 0:23
To: James Bottomley
Cc: Ying Chu; [email protected]
Subject: Re: [PATCH 4/4] mvsas: Fine-tuned tags array for alignment

Quoted 29 lines · James Bottomley

Quoted 20 lines · Ying Chu
Quoted 2 lines
From 2269b0ff3c2573f76cb0569eb5da99e9f12711d1 Mon Sep 17 00:00:00 2001
From: ayan <[email protected]>
Date: Fri, 8 May 2009 19:59:39 +0800
Subject: [PATCH 4/4] bug fix: alignment
Fine-tuned tags array with u32 for alignment.
Signed-off-by Ying Chu <[email protected]>
Signed-off-by Andy Yan <[email protected]>
Signed-off-by Ke Wei <[email protected]>
---
 drivers/scsi/mvsas/mv_sas.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
(quoted diff) +0 -0
b/drivers/scsi/mvsas/mv_sas.h index 75b9748..04f27bf 100644
drivers/scsi/mvsas/mv_sas.h struct mvs_info { · line 316
Quoted 5 lines
drivers/scsi/mvsas/mv_sas.h +1 -1
@@ -313,7 +313,7 @@ struct mvs_info {
 	const struct mvs_chip_info *chip;
 
 	int tags_num;
-	u8 tags[MVS_SLOTS >> 3];
+	u32 tags[MVS_SLOTS >> 5];
Why not just do

Quoted 5 lines

DECLARE_BITMAP(tags, MVS_SLOTS);
here? ... that way you're using the correctly defined type for the bitmap operations and you can lose the spurious (void *) cast in your bitmap wrapper operations.

Agreed. Type 'u32' is incorrect for bitmap operations... it needs to be 'unsigned long', which is what DECLARE_BITMAP gives you.

Jeff

Jeff Garzik

2009-05-09 06:52 UTC #4 lore raw

Quoted 7 lines · Ying Chu

Hi, James
DECLARE_BITMAP isn't a good option for mvsas, since the tags array couldn't be global(it's chip-related) and even if we define the static tag array both in mv64xx.c and mv94xx.c, it still couldn't honor 88SE9480 as there are 2 separate cores. So if we use DECLARE_BITMAP, it's hard to maintain the corresponding tag for different core.
Can we use the following replacement:
- u8 tags[MVS_SLOTS >> 3];
+ unsigned long tags[MVS_SLOTS / BITS_PER_LONG];

You are doing exactly the same thing as DECLARE_BITMAP here, so just use DECLARE_BITMAP :)

include/linux/types.h:

	#define DECLARE_BITMAP(name,bits) \
	        unsigned long name[BITS_TO_LONGS(bits)]

Regards,

Jeff

Ying Chu

author

2009-05-09 06:59 UTC #5 lore raw

Well, thank you.

-----Original Message-----
From: Jeff Garzik [mailto:[email protected]]
Sent: 2009年5月9日 14:52
To: Ying Chu
Cc: James Bottomley; [email protected]; Andy Yan; Ke Wei
Subject: Re: [PATCH 4/4] mvsas: Fine-tuned tags array for alignment

Quoted 7 lines · Ying Chu

Hi, James
DECLARE_BITMAP isn't a good option for mvsas, since the tags array couldn't be global(it's chip-related) and even if we define the static tag array both in mv64xx.c and mv94xx.c, it still couldn't honor 88SE9480 as there are 2 separate cores. So if we use DECLARE_BITMAP, it's hard to maintain the corresponding tag for different core.
Can we use the following replacement:
- u8 tags[MVS_SLOTS >> 3];
+ unsigned long tags[MVS_SLOTS / BITS_PER_LONG];

You are doing exactly the same thing as DECLARE_BITMAP here, so just use DECLARE_BITMAP :)

include/linux/types.h:

	#define DECLARE_BITMAP(name,bits) \
	        unsigned long name[BITS_TO_LONGS(bits)]

Regards,

Jeff

James Bottomley

maintainer

2009-05-09 14:38 UTC #6 lore raw

In reply to Jeff Garzik

Quoted 9 lines · Jeff Garzik

Quoted 7 lines · Ying Chu
Hi, James
DECLARE_BITMAP isn't a good option for mvsas, since the tags array couldn't be global(it's chip-related) and even if we define the static tag array both in mv64xx.c and mv94xx.c, it still couldn't honor 88SE9480 as there are 2 separate cores. So if we use DECLARE_BITMAP, it's hard to maintain the corresponding tag for different core.
Can we use the following replacement:
- u8 tags[MVS_SLOTS >> 3];
+ unsigned long tags[MVS_SLOTS / BITS_PER_LONG];

Quoted 6 lines

You are doing exactly the same thing as DECLARE_BITMAP here, so just use DECLARE_BITMAP :)
include/linux/types.h:
	#define DECLARE_BITMAP(name,bits) \
	        unsigned long name[BITS_TO_LONGS(bits)]

Right, it's one of those annoying definitions that's designed to be compound, so things like

static DECLARE_BITMAP(x, y);

is legal as well as

struct {
	DECLARE_BITMAP(x,y);
};

in either case, the scope is precisely what an ordinary variable would have had at that point.

Most of our DECLARE_XX are designed like this.

James