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

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

[Ying Chu]

>From 2269b0ff3c2573f76cb0569eb5da99e9f12711d1 Mon Sep 17 00:00:00 2001
From: ayan <ayan@marvell.com>
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 <jasonchu@marvell.com>
Signed-off-by: Andy Yan <ayan@marvell.com>
Signed-off-by: Ke Wei <kewei@marvell.com>
---
 drivers/scsi/mvsas/mv_sas.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h
index 75b9748..04f27bf 100644
--- a/drivers/scsi/mvsas/mv_sas.h
+++ b/drivers/scsi/mvsas/mv_sas.h
@@ -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];
-- 
1.6.0.3

-- 
Regards,
Ying Chu

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

[James Bottomley]

On Fri, 2009-05-08 at 14:18 +0800, Ying Chu wrote:
> >From 2269b0ff3c2573f76cb0569eb5da99e9f12711d1 Mon Sep 17 00:00:00 2001
> From: ayan <ayan@marvell.com>
> 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 <jasonchu@marvell.com>
> Signed-off-by: Andy Yan <ayan@marvell.com>
> Signed-off-by: Ke Wei <kewei@marvell.com>
> ---
>  drivers/scsi/mvsas/mv_sas.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h
> index 75b9748..04f27bf 100644
> --- a/drivers/scsi/mvsas/mv_sas.h
> +++ b/drivers/scsi/mvsas/mv_sas.h
> @@ -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

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

[Jeff Garzik]

James Bottomley wrote:
> On Fri, 2009-05-08 at 14:18 +0800, Ying Chu wrote:
>> >From 2269b0ff3c2573f76cb0569eb5da99e9f12711d1 Mon Sep 17 00:00:00 2001
>> From: ayan <ayan@marvell.com>
>> 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 <jasonchu@marvell.com>
>> Signed-off-by: Andy Yan <ayan@marvell.com>
>> Signed-off-by: Ke Wei <kewei@marvell.com>
>> ---
>>  drivers/scsi/mvsas/mv_sas.h |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h
>> index 75b9748..04f27bf 100644
>> --- a/drivers/scsi/mvsas/mv_sas.h
>> +++ b/drivers/scsi/mvsas/mv_sas.h
>> @@ -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.

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

	Jeff

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

[Jeff Garzik]

Ying Chu wrote:
> 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

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

[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];

Thank you.

-----Original Message-----
From: Jeff Garzik [mailto:jeff@garzik.org] 
Sent: 2009年5月9日 0:23
To: James Bottomley
Cc: Ying Chu; linux-scsi@vger.kernel.org
Subject: Re: [PATCH 4/4] mvsas: Fine-tuned tags array for alignment

James Bottomley wrote:
> On Fri, 2009-05-08 at 14:18 +0800, Ying Chu wrote:
>> >From 2269b0ff3c2573f76cb0569eb5da99e9f12711d1 Mon Sep 17 00:00:00 
>> >2001
>> From: ayan <ayan@marvell.com>
>> 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 <jasonchu@marvell.com>
>> Signed-off-by: Andy Yan <ayan@marvell.com>
>> Signed-off-by: Ke Wei <kewei@marvell.com>
>> ---
>>  drivers/scsi/mvsas/mv_sas.h |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/scsi/mvsas/mv_sas.h 
>> b/drivers/scsi/mvsas/mv_sas.h index 75b9748..04f27bf 100644
>> --- a/drivers/scsi/mvsas/mv_sas.h
>> +++ b/drivers/scsi/mvsas/mv_sas.h
>> @@ -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.

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

	Jeff

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

[Ying Chu]

Well, thank you.

-----Original Message-----
From: Jeff Garzik [mailto:jeff@garzik.org] 
Sent: 2009年5月9日 14:52
To: Ying Chu
Cc: James Bottomley; linux-scsi@vger.kernel.org; Andy Yan; Ke Wei
Subject: Re: [PATCH 4/4] mvsas: Fine-tuned tags array for alignment

Ying Chu wrote:
> 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

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

[James Bottomley]

On Sat, 2009-05-09 at 02:52 -0400, Jeff Garzik wrote:
> Ying Chu wrote:
> > 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)]

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