RE: [PATCH] cciss: Fix warnings during compilation under 32bit environment

RE: [PATCH] cciss: Fix warnings during compilation under 32bit environment

[Miller, Mike (OS Dev)]

 

> -----Original Message-----
> From: Hisashi Hifumi [mailto:hifumi.hisashi@oss.ntt.co.jp] 
> Sent: Wednesday, April 18, 2007 2:03 AM
> To: Miller, Mike (OS Dev); akpm@linux-foundation.org
> Subject: [PATCH] cciss: Fix warnings during compilation under 
> 32bit environment
> 
> 
> Hi.
> 
> I fixed following warnings.
> 
> drivers/block/cciss.c: In function `do_cciss_request':
> drivers/block/cciss.c:2555: warning: right shift count >= 
> width of type
> drivers/block/cciss.c:2556: warning: right shift count >= 
> width of type
> drivers/block/cciss.c:2557: warning: right shift count >= 
> width of type
> drivers/block/cciss.c:2558: warning: right shift count >= 
> width of type
> 
> 
> Signed-off-by :Hisashi Hifumi <hifumi.hisashi@oss.ntt.co.jp>

Nak. You still haven't told where you saw these warnings. What compiler
are you using? I do not see these in my 32-bit environment.

mikem

> 
> 
> --- linux-2.6.21-rc7.org/drivers/block/cciss.c	2007-04-17 
> 16:36:02.000000000 +0900
> +++ linux-2.6.21-rc7/drivers/block/cciss.c	2007-04-17 
> 16:25:53.000000000 +0900
> @@ -2552,10 +2552,10 @@ static void do_cciss_request(request_que
>   	} else {
>   		c->Request.CDBLen = 16;
>   		c->Request.CDB[1]= 0;
> -		c->Request.CDB[2]= (start_blk >> 56) & 0xff;	//MSB
> -		c->Request.CDB[3]= (start_blk >> 48) & 0xff;
> -		c->Request.CDB[4]= (start_blk >> 40) & 0xff;
> -		c->Request.CDB[5]= (start_blk >> 32) & 0xff;
> +		c->Request.CDB[2]= sizeof(start_blk) > 4 ? 
> (start_blk >> 56) & 0xff : 0;	//MSB
> +		c->Request.CDB[3]= sizeof(start_blk) > 4 ? 
> (start_blk >> 48) & 0xff : 0;
> +		c->Request.CDB[4]= sizeof(start_blk) > 4 ? 
> (start_blk >> 40) & 0xff : 0;
> +		c->Request.CDB[5]= sizeof(start_blk) > 4 ? 
> (start_blk >> 32) & 0xff : 
> +0;
>   		c->Request.CDB[6]= (start_blk >> 24) & 0xff;
>   		c->Request.CDB[7]= (start_blk >> 16) & 0xff;
>   		c->Request.CDB[8]= (start_blk >>  8) & 0xff;
> 
> 
> 
> Thanks,
> 

RE: [PATCH] cciss: Fix warnings during compilation under 32bit environment

[Miller, Mike (OS Dev)]

 

> -----Original Message-----
> From: Hisashi Hifumi [mailto:hifumi.hisashi@oss.ntt.co.jp] 
> Sent: Wednesday, April 18, 2007 2:03 AM
> To: Miller, Mike (OS Dev); akpm@linux-foundation.org
> Subject: [PATCH] cciss: Fix warnings during compilation under 
> 32bit environment
> 
> 
> Hi.
> 
> I fixed following warnings.
> 
> drivers/block/cciss.c: In function `do_cciss_request':
> drivers/block/cciss.c:2555: warning: right shift count >= 
> width of type
> drivers/block/cciss.c:2556: warning: right shift count >= 
> width of type
> drivers/block/cciss.c:2557: warning: right shift count >= 
> width of type
> drivers/block/cciss.c:2558: warning: right shift count >= 
> width of type
> 
> 
> Signed-off-by :Hisashi Hifumi <hifumi.hisashi@oss.ntt.co.jp>

Nak. You still haven't told where you saw these warnings. What compiler
are you using? I do not see these in my 32-bit environment.

mikem

> 
> 
> --- linux-2.6.21-rc7.org/drivers/block/cciss.c	2007-04-17 
> 16:36:02.000000000 +0900
> +++ linux-2.6.21-rc7/drivers/block/cciss.c	2007-04-17 
> 16:25:53.000000000 +0900
> @@ -2552,10 +2552,10 @@ static void do_cciss_request(request_que
>   	} else {
>   		c->Request.CDBLen = 16;
>   		c->Request.CDB[1]= 0;
> -		c->Request.CDB[2]= (start_blk >> 56) & 0xff;	//MSB
> -		c->Request.CDB[3]= (start_blk >> 48) & 0xff;
> -		c->Request.CDB[4]= (start_blk >> 40) & 0xff;
> -		c->Request.CDB[5]= (start_blk >> 32) & 0xff;
> +		c->Request.CDB[2]= sizeof(start_blk) > 4 ? 
> (start_blk >> 56) & 0xff : 0;	//MSB
> +		c->Request.CDB[3]= sizeof(start_blk) > 4 ? 
> (start_blk >> 48) & 0xff : 0;
> +		c->Request.CDB[4]= sizeof(start_blk) > 4 ? 
> (start_blk >> 40) & 0xff : 0;
> +		c->Request.CDB[5]= sizeof(start_blk) > 4 ? 
> (start_blk >> 32) & 0xff : 
> +0;
>   		c->Request.CDB[6]= (start_blk >> 24) & 0xff;
>   		c->Request.CDB[7]= (start_blk >> 16) & 0xff;
>   		c->Request.CDB[8]= (start_blk >>  8) & 0xff;
> 
> 
> 
> Thanks,
> 

RE: [PATCH] cciss: Fix warnings during compilation under 32bit environment

[James Bottomley]

On Thu, 2007-04-19 at 15:09 +0000, Miller, Mike (OS Dev) wrote:
> Nak. You still haven't told where you saw these warnings. What compiler
> are you using? I do not see these in my 32-bit environment.

I think it's seen with CONFIG_LBD=n on 32 bits

In that configuration, sector_t is a u32 (it's u64 even on 32 bits with
CONFIG_LBD=y).  The proposed code change is a simple cut and paste from
the sd driver.

James

RE: [PATCH] cciss: Fix warnings during compilation under 32bit environment

[James Bottomley]

On Thu, 2007-04-19 at 15:09 +0000, Miller, Mike (OS Dev) wrote:
> Nak. You still haven't told where you saw these warnings. What compiler
> are you using? I do not see these in my 32-bit environment.

I think it's seen with CONFIG_LBD=n on 32 bits

In that configuration, sector_t is a u32 (it's u64 even on 32 bits with
CONFIG_LBD=y).  The proposed code change is a simple cut and paste from
the sd driver.

James

RE: [PATCH] cciss: Fix warnings during compilation under 32bit environment

[Miller, Mike (OS Dev)]

 

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@SteelEye.com] 
> Sent: Thursday, April 19, 2007 10:19 AM
> To: Miller, Mike (OS Dev)
> Cc: Hisashi Hifumi; akpm@linux-foundation.org; 
> jens.axboe@oracle.com; linux-kernel@vger.kernel.org; 
> linux-scsi@vger.kernel.org; Cameron, Steve
> Subject: RE: [PATCH] cciss: Fix warnings during compilation 
> under 32bit environment
> 
> On Thu, 2007-04-19 at 15:09 +0000, Miller, Mike (OS Dev) wrote:
> > Nak. You still haven't told where you saw these warnings. What 
> > compiler are you using? I do not see these in my 32-bit environment.
> 
> I think it's seen with CONFIG_LBD=n on 32 bits
> 
> In that configuration, sector_t is a u32 (it's u64 even on 32 
> bits with CONFIG_LBD=y).  The proposed code change is a 
> simple cut and paste from the sd driver.

Isn't there a better way than testing each one?

mikem

> 
> James
> 
> 
> 

RE: [PATCH] cciss: Fix warnings during compilation under 32bit environment

[Miller, Mike (OS Dev)]

 

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@SteelEye.com] 
> Sent: Thursday, April 19, 2007 10:19 AM
> To: Miller, Mike (OS Dev)
> Cc: Hisashi Hifumi; akpm@linux-foundation.org; 
> jens.axboe@oracle.com; linux-kernel@vger.kernel.org; 
> linux-scsi@vger.kernel.org; Cameron, Steve
> Subject: RE: [PATCH] cciss: Fix warnings during compilation 
> under 32bit environment
> 
> On Thu, 2007-04-19 at 15:09 +0000, Miller, Mike (OS Dev) wrote:
> > Nak. You still haven't told where you saw these warnings. What 
> > compiler are you using? I do not see these in my 32-bit environment.
> 
> I think it's seen with CONFIG_LBD=n on 32 bits
> 
> In that configuration, sector_t is a u32 (it's u64 even on 32 
> bits with CONFIG_LBD=y).  The proposed code change is a 
> simple cut and paste from the sd driver.

Isn't there a better way than testing each one?

mikem

> 
> James
> 
> 
> 

RE: [PATCH] cciss: Fix warnings during compilation under 32bit environment

[James Bottomley]

On Thu, 2007-04-19 at 16:12 +0000, Miller, Mike (OS Dev) wrote:
> > > Nak. You still haven't told where you saw these warnings. What 
> > > compiler are you using? I do not see these in my 32-bit environment.
> > 
> > I think it's seen with CONFIG_LBD=n on 32 bits
> > 
> > In that configuration, sector_t is a u32 (it's u64 even on 32 
> > bits with CONFIG_LBD=y).  The proposed code change is a 
> > simple cut and paste from the sd driver.
> 
> Isn't there a better way than testing each one?

It's not such a bad option.  The sizeof() test is compile time
determinable, so the compiler simply zeros the fields in the
CONFIG_LBD=n case and does the shift for CONFIG_LBD=y.  It certainly
never compiles to four inline condition checks.

James

RE: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment

[Miller, Mike (OS Dev)]

 

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@SteelEye.com] 
> Sent: Thursday, April 19, 2007 11:22 AM
> To: Miller, Mike (OS Dev)
> Cc: Hisashi Hifumi; akpm@linux-foundation.org; 
> jens.axboe@oracle.com; linux-kernel@vger.kernel.org; 
> linux-scsi@vger.kernel.org; Cameron, Steve
> Subject: RE: [PATCH] cciss: Fix warnings during compilation 
> under 32bitenvironment
> 
> On Thu, 2007-04-19 at 16:12 +0000, Miller, Mike (OS Dev) wrote:
> > > > Nak. You still haven't told where you saw these warnings. What 
> > > > compiler are you using? I do not see these in my 32-bit 
> environment.
> > > 
> > > I think it's seen with CONFIG_LBD=n on 32 bits
> > > 
> > > In that configuration, sector_t is a u32 (it's u64 even 
> on 32 bits 
> > > with CONFIG_LBD=y).  The proposed code change is a simple cut and 
> > > paste from the sd driver.
> > 
> > Isn't there a better way than testing each one?
> 
> It's not such a bad option.  The sizeof() test is compile 
> time determinable, so the compiler simply zeros the fields in 
> the CONFIG_LBD=n case and does the shift for CONFIG_LBD=y.  
> It certainly never compiles to four inline condition checks.
> 
OKIE-DOKIE then, add the change.

Acked-by: Mike Miller <mike.miller@hp.com>

RE: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment

[Miller, Mike (OS Dev)]

 

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@SteelEye.com] 
> Sent: Thursday, April 19, 2007 11:22 AM
> To: Miller, Mike (OS Dev)
> Cc: Hisashi Hifumi; akpm@linux-foundation.org; 
> jens.axboe@oracle.com; linux-kernel@vger.kernel.org; 
> linux-scsi@vger.kernel.org; Cameron, Steve
> Subject: RE: [PATCH] cciss: Fix warnings during compilation 
> under 32bitenvironment
> 
> On Thu, 2007-04-19 at 16:12 +0000, Miller, Mike (OS Dev) wrote:
> > > > Nak. You still haven't told where you saw these warnings. What 
> > > > compiler are you using? I do not see these in my 32-bit 
> environment.
> > > 
> > > I think it's seen with CONFIG_LBD=n on 32 bits
> > > 
> > > In that configuration, sector_t is a u32 (it's u64 even 
> on 32 bits 
> > > with CONFIG_LBD=y).  The proposed code change is a simple cut and 
> > > paste from the sd driver.
> > 
> > Isn't there a better way than testing each one?
> 
> It's not such a bad option.  The sizeof() test is compile 
> time determinable, so the compiler simply zeros the fields in 
> the CONFIG_LBD=n case and does the shift for CONFIG_LBD=y.  
> It certainly never compiles to four inline condition checks.
> 
OKIE-DOKIE then, add the change.

Acked-by: Mike Miller <mike.miller@hp.com>

RE: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment

[Cameron, Steve]

Something like 

if (sizeof(blah) > 4) {
   do all the assignments with shifts
}

might be slighly better since the CDB is already zeroed
by cmd_alloc() and doesn't need to be zeroed a 2nd time.

-- steve

-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@SteelEye.com]
Sent: Thu 4/19/2007 11:22 AM
To: Miller, Mike (OS Dev)
Cc: Hisashi Hifumi; akpm@linux-foundation.org; jens.axboe@oracle.com; linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Cameron, Steve
Subject: RE: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment
 
On Thu, 2007-04-19 at 16:12 +0000, Miller, Mike (OS Dev) wrote:
> > > Nak. You still haven't told where you saw these warnings. What 
> > > compiler are you using? I do not see these in my 32-bit environment.
> > 
> > I think it's seen with CONFIG_LBD=n on 32 bits
> > 
> > In that configuration, sector_t is a u32 (it's u64 even on 32 
> > bits with CONFIG_LBD=y).  The proposed code change is a 
> > simple cut and paste from the sd driver.
> 
> Isn't there a better way than testing each one?

It's not such a bad option.  The sizeof() test is compile time
determinable, so the compiler simply zeros the fields in the
CONFIG_LBD=n case and does the shift for CONFIG_LBD=y.  It certainly
never compiles to four inline condition checks.

James

RE: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment

[Cameron, Steve]

Something like 

if (sizeof(blah) > 4) {
   do all the assignments with shifts
}

might be slighly better since the CDB is already zeroed
by cmd_alloc() and doesn't need to be zeroed a 2nd time.

-- steve

-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@SteelEye.com]
Sent: Thu 4/19/2007 11:22 AM
To: Miller, Mike (OS Dev)
Cc: Hisashi Hifumi; akpm@linux-foundation.org; jens.axboe@oracle.com; linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Cameron, Steve
Subject: RE: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment
 
On Thu, 2007-04-19 at 16:12 +0000, Miller, Mike (OS Dev) wrote:
> > > Nak. You still haven't told where you saw these warnings. What 
> > > compiler are you using? I do not see these in my 32-bit environment.
> > 
> > I think it's seen with CONFIG_LBD=n on 32 bits
> > 
> > In that configuration, sector_t is a u32 (it's u64 even on 32 
> > bits with CONFIG_LBD=y).  The proposed code change is a 
> > simple cut and paste from the sd driver.
> 
> Isn't there a better way than testing each one?

It's not such a bad option.  The sizeof() test is compile time
determinable, so the compiler simply zeros the fields in the
CONFIG_LBD=n case and does the shift for CONFIG_LBD=y.  It certainly
never compiles to four inline condition checks.

James

Re: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment

[Andrew Morton]

On Thu, 19 Apr 2007 16:27:26 -0000 "Cameron, Steve" <Steve.Cameron@hp.com> wrote:

> 
> Something like 
> 
> if (sizeof(blah) > 4) {
>    do all the assignments with shifts
> }
> 
> might be slighly better since the CDB is already zeroed
> by cmd_alloc() and doesn't need to be zeroed a 2nd time.
> 
> -- steve
> 
> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@SteelEye.com]
> Sent: Thu 4/19/2007 11:22 AM
> To: Miller, Mike (OS Dev)
> Cc: Hisashi Hifumi; akpm@linux-foundation.org; jens.axboe@oracle.com; linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Cameron, Steve
> Subject: RE: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment
>  
> On Thu, 2007-04-19 at 16:12 +0000, Miller, Mike (OS Dev) wrote:
> > > > Nak. You still haven't told where you saw these warnings. What 
> > > > compiler are you using? I do not see these in my 32-bit environment.
> > > 
> > > I think it's seen with CONFIG_LBD=n on 32 bits
> > > 
> > > In that configuration, sector_t is a u32 (it's u64 even on 32 
> > > bits with CONFIG_LBD=y).  The proposed code change is a 
> > > simple cut and paste from the sd driver.
> > 
> > Isn't there a better way than testing each one?
> 
> It's not such a bad option.  The sizeof() test is compile time
> determinable, so the compiler simply zeros the fields in the
> CONFIG_LBD=n case and does the shift for CONFIG_LBD=y.  It certainly
> never compiles to four inline condition checks.
> 

Boy you guys make a mess of a nice email trail :(


--- linux-2.6.21-rc7.org/drivers/block/cciss.c	2007-04-17 16:36:02.000000000 +0900
+++ linux-2.6.21-rc7/drivers/block/cciss.c	2007-04-17 16:25:53.000000000 +0900
@@ -2552,10 +2552,10 @@ static void do_cciss_request(request_que
 	} else {
 		c->Request.CDBLen = 16;
 		c->Request.CDB[1]= 0;
-		c->Request.CDB[2]= (start_blk >> 56) & 0xff;	//MSB
-		c->Request.CDB[3]= (start_blk >> 48) & 0xff;
-		c->Request.CDB[4]= (start_blk >> 40) & 0xff;
-		c->Request.CDB[5]= (start_blk >> 32) & 0xff;
+		c->Request.CDB[2]= sizeof(start_blk) > 4 ? (start_blk >> 56) & 0xff : 0;	//MSB
+		c->Request.CDB[3]= sizeof(start_blk) > 4 ? (start_blk >> 48) & 0xff : 0;
+		c->Request.CDB[4]= sizeof(start_blk) > 4 ? (start_blk >> 40) & 0xff : 0;
+		c->Request.CDB[5]= sizeof(start_blk) > 4 ? (start_blk >> 32) & 0xff : 0;
 		c->Request.CDB[6]= (start_blk >> 24) & 0xff;
 		c->Request.CDB[7]= (start_blk >> 16) & 0xff;
 		c->Request.CDB[8]= (start_blk >>  8) & 0xff;

This is not the first time we've hit this problem and presumably it won't
be the last time.

Could we do something like

#if (BITS_PER_LONG > 32) || defined(CONFIG_LBD)
#define sector_upper_32(sector) ((sector) >> 32)
#else
#define sector_upper_32(sector) (0)
#endif

and then cciss can do

-	c->Request.CDB[2]= start_blk >> 56;
+	c->Request.CDB[2]= sector_upper_32(start_blk) >> 24;

which will do the right thing.


- I think it's safer as a macro - if we make it an inline then the
  compiler might still try to evaluate the argument and will still warn

- we could do something like

  static inline sector_t sector_shifted_right_by(sector_t s, int distance)
  {
	<fancy code goes here>
  }

  But I think that won't be as generally useful as the very basic
  sector_upper_32().

- sector_upper_32() isn't a vey nice name, but it has clarity-of-purpose..

Re: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment

[Andrew Morton]

On Thu, 19 Apr 2007 16:27:26 -0000 "Cameron, Steve" <Steve.Cameron@hp.com> wrote:

> 
> Something like 
> 
> if (sizeof(blah) > 4) {
>    do all the assignments with shifts
> }
> 
> might be slighly better since the CDB is already zeroed
> by cmd_alloc() and doesn't need to be zeroed a 2nd time.
> 
> -- steve
> 
> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@SteelEye.com]
> Sent: Thu 4/19/2007 11:22 AM
> To: Miller, Mike (OS Dev)
> Cc: Hisashi Hifumi; akpm@linux-foundation.org; jens.axboe@oracle.com; linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Cameron, Steve
> Subject: RE: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment
>  
> On Thu, 2007-04-19 at 16:12 +0000, Miller, Mike (OS Dev) wrote:
> > > > Nak. You still haven't told where you saw these warnings. What 
> > > > compiler are you using? I do not see these in my 32-bit environment.
> > > 
> > > I think it's seen with CONFIG_LBD=n on 32 bits
> > > 
> > > In that configuration, sector_t is a u32 (it's u64 even on 32 
> > > bits with CONFIG_LBD=y).  The proposed code change is a 
> > > simple cut and paste from the sd driver.
> > 
> > Isn't there a better way than testing each one?
> 
> It's not such a bad option.  The sizeof() test is compile time
> determinable, so the compiler simply zeros the fields in the
> CONFIG_LBD=n case and does the shift for CONFIG_LBD=y.  It certainly
> never compiles to four inline condition checks.
> 

Boy you guys make a mess of a nice email trail :(


--- linux-2.6.21-rc7.org/drivers/block/cciss.c	2007-04-17 16:36:02.000000000 +0900
+++ linux-2.6.21-rc7/drivers/block/cciss.c	2007-04-17 16:25:53.000000000 +0900
@@ -2552,10 +2552,10 @@ static void do_cciss_request(request_que
 	} else {
 		c->Request.CDBLen = 16;
 		c->Request.CDB[1]= 0;
-		c->Request.CDB[2]= (start_blk >> 56) & 0xff;	//MSB
-		c->Request.CDB[3]= (start_blk >> 48) & 0xff;
-		c->Request.CDB[4]= (start_blk >> 40) & 0xff;
-		c->Request.CDB[5]= (start_blk >> 32) & 0xff;
+		c->Request.CDB[2]= sizeof(start_blk) > 4 ? (start_blk >> 56) & 0xff : 0;	//MSB
+		c->Request.CDB[3]= sizeof(start_blk) > 4 ? (start_blk >> 48) & 0xff : 0;
+		c->Request.CDB[4]= sizeof(start_blk) > 4 ? (start_blk >> 40) & 0xff : 0;
+		c->Request.CDB[5]= sizeof(start_blk) > 4 ? (start_blk >> 32) & 0xff : 0;
 		c->Request.CDB[6]= (start_blk >> 24) & 0xff;
 		c->Request.CDB[7]= (start_blk >> 16) & 0xff;
 		c->Request.CDB[8]= (start_blk >>  8) & 0xff;

This is not the first time we've hit this problem and presumably it won't
be the last time.

Could we do something like

#if (BITS_PER_LONG > 32) || defined(CONFIG_LBD)
#define sector_upper_32(sector) ((sector) >> 32)
#else
#define sector_upper_32(sector) (0)
#endif

and then cciss can do

-	c->Request.CDB[2]= start_blk >> 56;
+	c->Request.CDB[2]= sector_upper_32(start_blk) >> 24;

which will do the right thing.


- I think it's safer as a macro - if we make it an inline then the
  compiler might still try to evaluate the argument and will still warn

- we could do something like

  static inline sector_t sector_shifted_right_by(sector_t s, int distance)
  {
	<fancy code goes here>
  }

  But I think that won't be as generally useful as the very basic
  sector_upper_32().

- sector_upper_32() isn't a vey nice name, but it has clarity-of-purpose..

Re: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment

[James Bottomley]

On Thu, 2007-04-19 at 22:20 -0700, Andrew Morton wrote:
> On Thu, 19 Apr 2007 16:27:26 -0000 "Cameron, Steve" <Steve.Cameron@hp.com> wrote:
> > 
> > Something like 
> > 
> > if (sizeof(blah) > 4) {
> >    do all the assignments with shifts
> > }
> > 
> > might be slighly better since the CDB is already zeroed
> > by cmd_alloc() and doesn't need to be zeroed a 2nd time.
> > 
> > -- steve
> > 
> > -----Original Message-----
> > From: James Bottomley [mailto:James.Bottomley@SteelEye.com]
> > Sent: Thu 4/19/2007 11:22 AM
> > To: Miller, Mike (OS Dev)
> > Cc: Hisashi Hifumi; akpm@linux-foundation.org; jens.axboe@oracle.com; linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Cameron, Steve
> > Subject: RE: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment
> >  
> > On Thu, 2007-04-19 at 16:12 +0000, Miller, Mike (OS Dev) wrote:
> > > > > Nak. You still haven't told where you saw these warnings. What 
> > > > > compiler are you using? I do not see these in my 32-bit environment.
> > > > 
> > > > I think it's seen with CONFIG_LBD=n on 32 bits
> > > > 
> > > > In that configuration, sector_t is a u32 (it's u64 even on 32 
> > > > bits with CONFIG_LBD=y).  The proposed code change is a 
> > > > simple cut and paste from the sd driver.
> > > 
> > > Isn't there a better way than testing each one?
> > 
> > It's not such a bad option.  The sizeof() test is compile time
> > determinable, so the compiler simply zeros the fields in the
> > CONFIG_LBD=n case and does the shift for CONFIG_LBD=y.  It certainly
> > never compiles to four inline condition checks.
> > 
> 
> Boy you guys make a mess of a nice email trail :(
> 
> 
> --- linux-2.6.21-rc7.org/drivers/block/cciss.c	2007-04-17 16:36:02.000000000 +0900
> +++ linux-2.6.21-rc7/drivers/block/cciss.c	2007-04-17 16:25:53.000000000 +0900
> @@ -2552,10 +2552,10 @@ static void do_cciss_request(request_que
>  	} else {
>  		c->Request.CDBLen = 16;
>  		c->Request.CDB[1]= 0;
> -		c->Request.CDB[2]= (start_blk >> 56) & 0xff;	//MSB
> -		c->Request.CDB[3]= (start_blk >> 48) & 0xff;
> -		c->Request.CDB[4]= (start_blk >> 40) & 0xff;
> -		c->Request.CDB[5]= (start_blk >> 32) & 0xff;
> +		c->Request.CDB[2]= sizeof(start_blk) > 4 ? (start_blk >> 56) & 0xff : 0;	//MSB
> +		c->Request.CDB[3]= sizeof(start_blk) > 4 ? (start_blk >> 48) & 0xff : 0;
> +		c->Request.CDB[4]= sizeof(start_blk) > 4 ? (start_blk >> 40) & 0xff : 0;
> +		c->Request.CDB[5]= sizeof(start_blk) > 4 ? (start_blk >> 32) & 0xff : 0;
>  		c->Request.CDB[6]= (start_blk >> 24) & 0xff;
>  		c->Request.CDB[7]= (start_blk >> 16) & 0xff;
>  		c->Request.CDB[8]= (start_blk >>  8) & 0xff;
> 
> This is not the first time we've hit this problem and presumably it won't
> be the last time.
> 
> Could we do something like
> 
> #if (BITS_PER_LONG > 32) || defined(CONFIG_LBD)
> #define sector_upper_32(sector) ((sector) >> 32)
> #else
> #define sector_upper_32(sector) (0)
> #endif
> 
> and then cciss can do
> 
> -	c->Request.CDB[2]= start_blk >> 56;
> +	c->Request.CDB[2]= sector_upper_32(start_blk) >> 24;
> 
> which will do the right thing.

Sure, we could do that.  The slight problem is that we don't have
general agreement in the kernel how to handle sector_t.  So, the only
consumer in scsi, sd, does the sizeof(block) ? thing.  If you look in
libata you'll see that it unconditionally uses a u64 for picking up the
value of sector_t so the shift is never invalid ... if we're going to
start making macros for handling this, we probably need to ask the
janitors to fix all of our existing code to use them ... or we could
just let sleeping dogs lie ..

Is there even a valid use for CONFIG_LBD=n anymore, anyway?

James

Re: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment

[Alan Cox]

> > #if (BITS_PER_LONG > 32) || defined(CONFIG_LBD)
> > #define sector_upper_32(sector) ((sector) >> 32)
> > #else
> > #define sector_upper_32(sector) (0)
> > #endif

Gak

Just do

	sector_upper_32(sector)   (((sector) >> 31) >> 1)

and lose all the ifdefs,

Alan

Re: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment

[Andrew Morton]

On Fri, 20 Apr 2007 09:10:41 -0400
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> On Thu, 2007-04-19 at 22:20 -0700, Andrew Morton wrote:
> > On Thu, 19 Apr 2007 16:27:26 -0000 "Cameron, Steve" <Steve.Cameron@hp.com> wrote:
> > > 
> > > Something like 
> > > 
> > > if (sizeof(blah) > 4) {
> > >    do all the assignments with shifts
> > > }
> > > 
> > > might be slighly better since the CDB is already zeroed
> > > by cmd_alloc() and doesn't need to be zeroed a 2nd time.
> > > 
> > > -- steve
> > > 
> > > -----Original Message-----
> > > From: James Bottomley [mailto:James.Bottomley@SteelEye.com]
> > > Sent: Thu 4/19/2007 11:22 AM
> > > To: Miller, Mike (OS Dev)
> > > Cc: Hisashi Hifumi; akpm@linux-foundation.org; jens.axboe@oracle.com; linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Cameron, Steve
> > > Subject: RE: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment
> > >  
> > > On Thu, 2007-04-19 at 16:12 +0000, Miller, Mike (OS Dev) wrote:
> > > > > > Nak. You still haven't told where you saw these warnings. What 
> > > > > > compiler are you using? I do not see these in my 32-bit environment.
> > > > > 
> > > > > I think it's seen with CONFIG_LBD=n on 32 bits
> > > > > 
> > > > > In that configuration, sector_t is a u32 (it's u64 even on 32 
> > > > > bits with CONFIG_LBD=y).  The proposed code change is a 
> > > > > simple cut and paste from the sd driver.
> > > > 
> > > > Isn't there a better way than testing each one?
> > > 
> > > It's not such a bad option.  The sizeof() test is compile time
> > > determinable, so the compiler simply zeros the fields in the
> > > CONFIG_LBD=n case and does the shift for CONFIG_LBD=y.  It certainly
> > > never compiles to four inline condition checks.
> > > 
> > 
> > Boy you guys make a mess of a nice email trail :(
> > 
> > 
> > --- linux-2.6.21-rc7.org/drivers/block/cciss.c	2007-04-17 16:36:02.000000000 +0900
> > +++ linux-2.6.21-rc7/drivers/block/cciss.c	2007-04-17 16:25:53.000000000 +0900
> > @@ -2552,10 +2552,10 @@ static void do_cciss_request(request_que
> >  	} else {
> >  		c->Request.CDBLen = 16;
> >  		c->Request.CDB[1]= 0;
> > -		c->Request.CDB[2]= (start_blk >> 56) & 0xff;	//MSB
> > -		c->Request.CDB[3]= (start_blk >> 48) & 0xff;
> > -		c->Request.CDB[4]= (start_blk >> 40) & 0xff;
> > -		c->Request.CDB[5]= (start_blk >> 32) & 0xff;
> > +		c->Request.CDB[2]= sizeof(start_blk) > 4 ? (start_blk >> 56) & 0xff : 0;	//MSB
> > +		c->Request.CDB[3]= sizeof(start_blk) > 4 ? (start_blk >> 48) & 0xff : 0;
> > +		c->Request.CDB[4]= sizeof(start_blk) > 4 ? (start_blk >> 40) & 0xff : 0;
> > +		c->Request.CDB[5]= sizeof(start_blk) > 4 ? (start_blk >> 32) & 0xff : 0;
> >  		c->Request.CDB[6]= (start_blk >> 24) & 0xff;
> >  		c->Request.CDB[7]= (start_blk >> 16) & 0xff;
> >  		c->Request.CDB[8]= (start_blk >>  8) & 0xff;
> > 
> > This is not the first time we've hit this problem and presumably it won't
> > be the last time.
> > 
> > Could we do something like
> > 
> > #if (BITS_PER_LONG > 32) || defined(CONFIG_LBD)
> > #define sector_upper_32(sector) ((sector) >> 32)
> > #else
> > #define sector_upper_32(sector) (0)
> > #endif
> > 
> > and then cciss can do
> > 
> > -	c->Request.CDB[2]= start_blk >> 56;
> > +	c->Request.CDB[2]= sector_upper_32(start_blk) >> 24;
> > 
> > which will do the right thing.
> 
> Sure, we could do that.  The slight problem is that we don't have
> general agreement in the kernel how to handle sector_t.  So, the only
> consumer in scsi, sd, does the sizeof(block) ? thing.  If you look in
> libata you'll see that it unconditionally uses a u64 for picking up the
> value of sector_t so the shift is never invalid ... if we're going to
> start making macros for handling this, we probably need to ask the
> janitors to fix all of our existing code to use them ... or we could
> just let sleeping dogs lie ..

Let's decide how we _should_ do it, implement that, then teach cciss about
it, then try to ensure that new code uses the approved interfaces.  Over
time, people may (or may not) migrate existing code over to the new
interfaces.

They may also merge new code which uses open-coded hand-rolled stuff, too :(

> Is there even a valid use for CONFIG_LBD=n anymore, anyway?

Lots and lots and lots of systems don't have any disks larger than 2TB.

CONFIG_LBD=y gives us an additional 3kb of instructions on i386
allnoconfig.  Other architectures might do less well.  It's not a huge
difference, but that's the way in which creeping bloatiness happens.

Re: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment

[James Bottomley]

On Fri, 2007-04-20 at 11:43 -0700, Andrew Morton wrote:
> On Fri, 20 Apr 2007 09:10:41 -0400
> James Bottomley <James.Bottomley@SteelEye.com> wrote:
> 
> > On Thu, 2007-04-19 at 22:20 -0700, Andrew Morton wrote:
> > > On Thu, 19 Apr 2007 16:27:26 -0000 "Cameron, Steve" <Steve.Cameron@hp.com> wrote:
> > > > 
> > > > Something like 
> > > > 
> > > > if (sizeof(blah) > 4) {
> > > >    do all the assignments with shifts
> > > > }
> > > > 
> > > > might be slighly better since the CDB is already zeroed
> > > > by cmd_alloc() and doesn't need to be zeroed a 2nd time.
> > > > 
> > > > -- steve
> > > > 
> > > > -----Original Message-----
> > > > From: James Bottomley [mailto:James.Bottomley@SteelEye.com]
> > > > Sent: Thu 4/19/2007 11:22 AM
> > > > To: Miller, Mike (OS Dev)
> > > > Cc: Hisashi Hifumi; akpm@linux-foundation.org; jens.axboe@oracle.com; linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Cameron, Steve
> > > > Subject: RE: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment
> > > >  
> > > > On Thu, 2007-04-19 at 16:12 +0000, Miller, Mike (OS Dev) wrote:
> > > > > > > Nak. You still haven't told where you saw these warnings. What 
> > > > > > > compiler are you using? I do not see these in my 32-bit environment.
> > > > > > 
> > > > > > I think it's seen with CONFIG_LBD=n on 32 bits
> > > > > > 
> > > > > > In that configuration, sector_t is a u32 (it's u64 even on 32 
> > > > > > bits with CONFIG_LBD=y).  The proposed code change is a 
> > > > > > simple cut and paste from the sd driver.
> > > > > 
> > > > > Isn't there a better way than testing each one?
> > > > 
> > > > It's not such a bad option.  The sizeof() test is compile time
> > > > determinable, so the compiler simply zeros the fields in the
> > > > CONFIG_LBD=n case and does the shift for CONFIG_LBD=y.  It certainly
> > > > never compiles to four inline condition checks.
> > > > 
> > > 
> > > Boy you guys make a mess of a nice email trail :(
> > > 
> > > 
> > > --- linux-2.6.21-rc7.org/drivers/block/cciss.c	2007-04-17 16:36:02.000000000 +0900
> > > +++ linux-2.6.21-rc7/drivers/block/cciss.c	2007-04-17 16:25:53.000000000 +0900
> > > @@ -2552,10 +2552,10 @@ static void do_cciss_request(request_que
> > >  	} else {
> > >  		c->Request.CDBLen = 16;
> > >  		c->Request.CDB[1]= 0;
> > > -		c->Request.CDB[2]= (start_blk >> 56) & 0xff;	//MSB
> > > -		c->Request.CDB[3]= (start_blk >> 48) & 0xff;
> > > -		c->Request.CDB[4]= (start_blk >> 40) & 0xff;
> > > -		c->Request.CDB[5]= (start_blk >> 32) & 0xff;
> > > +		c->Request.CDB[2]= sizeof(start_blk) > 4 ? (start_blk >> 56) & 0xff : 0;	//MSB
> > > +		c->Request.CDB[3]= sizeof(start_blk) > 4 ? (start_blk >> 48) & 0xff : 0;
> > > +		c->Request.CDB[4]= sizeof(start_blk) > 4 ? (start_blk >> 40) & 0xff : 0;
> > > +		c->Request.CDB[5]= sizeof(start_blk) > 4 ? (start_blk >> 32) & 0xff : 0;
> > >  		c->Request.CDB[6]= (start_blk >> 24) & 0xff;
> > >  		c->Request.CDB[7]= (start_blk >> 16) & 0xff;
> > >  		c->Request.CDB[8]= (start_blk >>  8) & 0xff;
> > > 
> > > This is not the first time we've hit this problem and presumably it won't
> > > be the last time.
> > > 
> > > Could we do something like
> > > 
> > > #if (BITS_PER_LONG > 32) || defined(CONFIG_LBD)
> > > #define sector_upper_32(sector) ((sector) >> 32)
> > > #else
> > > #define sector_upper_32(sector) (0)
> > > #endif
> > > 
> > > and then cciss can do
> > > 
> > > -	c->Request.CDB[2]= start_blk >> 56;
> > > +	c->Request.CDB[2]= sector_upper_32(start_blk) >> 24;
> > > 
> > > which will do the right thing.
> > 
> > Sure, we could do that.  The slight problem is that we don't have
> > general agreement in the kernel how to handle sector_t.  So, the only
> > consumer in scsi, sd, does the sizeof(block) ? thing.  If you look in
> > libata you'll see that it unconditionally uses a u64 for picking up the
> > value of sector_t so the shift is never invalid ... if we're going to
> > start making macros for handling this, we probably need to ask the
> > janitors to fix all of our existing code to use them ... or we could
> > just let sleeping dogs lie ..
> 
> Let's decide how we _should_ do it, implement that, then teach cciss about
> it, then try to ensure that new code uses the approved interfaces.  Over
> time, people may (or may not) migrate existing code over to the new
> interfaces.
> 
> They may also merge new code which uses open-coded hand-rolled stuff, too :(
> 
> > Is there even a valid use for CONFIG_LBD=n anymore, anyway?
> 
> Lots and lots and lots of systems don't have any disks larger than 2TB.
> 
> CONFIG_LBD=y gives us an additional 3kb of instructions on i386
> allnoconfig.  Other architectures might do less well.  It's not a huge
> difference, but that's the way in which creeping bloatiness happens.

OK, sure, but if we really care about this saving, then unconditionally
casting to u64 is therefore wrong as well ... this is starting to open
quite a large can of worms ...

For the record, if we have to do this, I fancy sector_upper_32() ... we
should already have some similar accessor for dma_addr_t as well.

James

Re: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment

[Andrew Morton]

On Fri, 20 Apr 2007 14:50:06 -0400
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> > CONFIG_LBD=y gives us an additional 3kb of instructions on i386
> > allnoconfig.  Other architectures might do less well.  It's not a huge
> > difference, but that's the way in which creeping bloatiness happens.
> 
> OK, sure, but if we really care about this saving, then unconditionally
> casting to u64 is therefore wrong as well ... this is starting to open
> quite a large can of worms ...
> 
> For the record, if we have to do this, I fancy sector_upper_32() ... we
> should already have some similar accessor for dma_addr_t as well.

hm.  How about this?

--- a/include/linux/kernel.h~upper-32-bits
+++ a/include/linux/kernel.h
@@ -40,6 +40,17 @@ extern const char linux_proc_banner[];
 #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
 #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
 
+/**
+ * upper_32_bits - return bits 32-63 of a number
+ * @n: the number we're accessing
+ *
+ * A basic shift-right of a 64- or 32-bit quantity.  Use this to suppress
+ * the "right shift count >= width of type" warning when that quantity is
+ * 32-bits.
+ */
+#define upper_32_bits(n) (((u64)(n)) >> 32)
+
+
 #define	KERN_EMERG	"<0>"	/* system is unusable			*/
 #define	KERN_ALERT	"<1>"	/* action must be taken immediately	*/
 #define	KERN_CRIT	"<2>"	/* critical conditions			*/
_

It seems to generate the desired code.  I avoided Alan's ((n >> 31) >> 1)
trick because it'll generate peculiar results with signed 64-bit
quantities.

Re: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment

[James Bottomley]

On Fri, 2007-04-20 at 12:30 -0700, Andrew Morton wrote:
> On Fri, 20 Apr 2007 14:50:06 -0400
> James Bottomley <James.Bottomley@SteelEye.com> wrote:
> 
> > > CONFIG_LBD=y gives us an additional 3kb of instructions on i386
> > > allnoconfig.  Other architectures might do less well.  It's not a huge
> > > difference, but that's the way in which creeping bloatiness happens.
> > 
> > OK, sure, but if we really care about this saving, then unconditionally
> > casting to u64 is therefore wrong as well ... this is starting to open
> > quite a large can of worms ...
> > 
> > For the record, if we have to do this, I fancy sector_upper_32() ... we
> > should already have some similar accessor for dma_addr_t as well.
> 
> hm.  How about this?
> 
> --- a/include/linux/kernel.h~upper-32-bits
> +++ a/include/linux/kernel.h
> @@ -40,6 +40,17 @@ extern const char linux_proc_banner[];
>  #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
>  #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
>  
> +/**
> + * upper_32_bits - return bits 32-63 of a number
> + * @n: the number we're accessing
> + *
> + * A basic shift-right of a 64- or 32-bit quantity.  Use this to suppress
> + * the "right shift count >= width of type" warning when that quantity is
> + * 32-bits.
> + */
> +#define upper_32_bits(n) (((u64)(n)) >> 32)

Won't this have the unwanted side effect of promoting everything in a
calculation to long long on 32 bit platforms, even if n was only 32
bits?

> +
> +
>  #define	KERN_EMERG	"<0>"	/* system is unusable			*/
>  #define	KERN_ALERT	"<1>"	/* action must be taken immediately	*/
>  #define	KERN_CRIT	"<2>"	/* critical conditions			*/
> _
> 
> It seems to generate the desired code.  I avoided Alan's ((n >> 31) >> 1)
> trick because it'll generate peculiar results with signed 64-bit
> quantities.

I've seen the trick done similarly with ((n >> 16) >> 16) which
shouldn't have the issue.

James

Re: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment

[Andrew Morton]

On Fri, 20 Apr 2007 16:20:59 -0400
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> On Fri, 2007-04-20 at 12:30 -0700, Andrew Morton wrote:
> > On Fri, 20 Apr 2007 14:50:06 -0400
> > James Bottomley <James.Bottomley@SteelEye.com> wrote:
> > 
> > > > CONFIG_LBD=y gives us an additional 3kb of instructions on i386
> > > > allnoconfig.  Other architectures might do less well.  It's not a huge
> > > > difference, but that's the way in which creeping bloatiness happens.
> > > 
> > > OK, sure, but if we really care about this saving, then unconditionally
> > > casting to u64 is therefore wrong as well ... this is starting to open
> > > quite a large can of worms ...
> > > 
> > > For the record, if we have to do this, I fancy sector_upper_32() ... we
> > > should already have some similar accessor for dma_addr_t as well.
> > 
> > hm.  How about this?
> > 
> > --- a/include/linux/kernel.h~upper-32-bits
> > +++ a/include/linux/kernel.h
> > @@ -40,6 +40,17 @@ extern const char linux_proc_banner[];
> >  #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
> >  #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
> >  
> > +/**
> > + * upper_32_bits - return bits 32-63 of a number
> > + * @n: the number we're accessing
> > + *
> > + * A basic shift-right of a 64- or 32-bit quantity.  Use this to suppress
> > + * the "right shift count >= width of type" warning when that quantity is
> > + * 32-bits.
> > + */
> > +#define upper_32_bits(n) (((u64)(n)) >> 32)
> 
> Won't this have the unwanted side effect of promoting everything in a
> calculation to long long on 32 bit platforms, even if n was only 32
> bits?

bummer.

> > +
> > +
> >  #define	KERN_EMERG	"<0>"	/* system is unusable			*/
> >  #define	KERN_ALERT	"<1>"	/* action must be taken immediately	*/
> >  #define	KERN_CRIT	"<2>"	/* critical conditions			*/
> > _
> > 
> > It seems to generate the desired code.  I avoided Alan's ((n >> 31) >> 1)
> > trick because it'll generate peculiar results with signed 64-bit
> > quantities.
> 
> I've seen the trick done similarly with ((n >> 16) >> 16) which
> shouldn't have the issue.

That works if we know the caller is treating the return value as 32 bits,
but we don't know that.

If we have

#define upper_32_bits(x)  ((x >> 16) >> 16)

then

	upper_32_bits(0x8888777766665555)

will return 0x88887777 if it's treated as 32-bits, but it'll return
0xffffffff88887777 if the caller is using 64-bits.

I spose

#define upper_32_bits(x)  ((u32)((x >> 16) >> 16))

will do the trick.

Re: [PATCH] cciss: Fix warnings during compilation under 32bitenvironment

[John Anthony Kazos Jr.]

On Fri, 20 Apr 2007, Andrew Morton wrote:

> On Fri, 20 Apr 2007 16:20:59 -0400
> James Bottomley <James.Bottomley@SteelEye.com> wrote:
> 
> > On Fri, 2007-04-20 at 12:30 -0700, Andrew Morton wrote:
> > > On Fri, 20 Apr 2007 14:50:06 -0400
> > > James Bottomley <James.Bottomley@SteelEye.com> wrote:
> > > 
> > > > > CONFIG_LBD=y gives us an additional 3kb of instructions on i386
> > > > > allnoconfig.  Other architectures might do less well.  It's not a huge
> > > > > difference, but that's the way in which creeping bloatiness happens.
> > > > 
> > > > OK, sure, but if we really care about this saving, then unconditionally
> > > > casting to u64 is therefore wrong as well ... this is starting to open
> > > > quite a large can of worms ...
> > > > 
> > > > For the record, if we have to do this, I fancy sector_upper_32() ... we
> > > > should already have some similar accessor for dma_addr_t as well.
> > > 
> > > hm.  How about this?
> > > 
> > > --- a/include/linux/kernel.h~upper-32-bits
> > > +++ a/include/linux/kernel.h
> > > @@ -40,6 +40,17 @@ extern const char linux_proc_banner[];
> > >  #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
> > >  #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
> > >  
> > > +/**
> > > + * upper_32_bits - return bits 32-63 of a number
> > > + * @n: the number we're accessing
> > > + *
> > > + * A basic shift-right of a 64- or 32-bit quantity.  Use this to suppress
> > > + * the "right shift count >= width of type" warning when that quantity is
> > > + * 32-bits.
> > > + */
> > > +#define upper_32_bits(n) (((u64)(n)) >> 32)
> > 
> > Won't this have the unwanted side effect of promoting everything in a
> > calculation to long long on 32 bit platforms, even if n was only 32
> > bits?
> 
> bummer.
> 
> > > +
> > > +
> > >  #define	KERN_EMERG	"<0>"	/* system is unusable			*/
> > >  #define	KERN_ALERT	"<1>"	/* action must be taken immediately	*/
> > >  #define	KERN_CRIT	"<2>"	/* critical conditions			*/
> > > _
> > > 
> > > It seems to generate the desired code.  I avoided Alan's ((n >> 31) >> 1)
> > > trick because it'll generate peculiar results with signed 64-bit
> > > quantities.
> > 
> > I've seen the trick done similarly with ((n >> 16) >> 16) which
> > shouldn't have the issue.
> 
> That works if we know the caller is treating the return value as 32 bits,
> but we don't know that.
> 
> If we have
> 
> #define upper_32_bits(x)  ((x >> 16) >> 16)
> 
> then
> 
> 	upper_32_bits(0x8888777766665555)
> 
> will return 0x88887777 if it's treated as 32-bits, but it'll return
> 0xffffffff88887777 if the caller is using 64-bits.
> 
> I spose
> 
> #define upper_32_bits(x)  ((u32)((x >> 16) >> 16))
> 
> will do the trick.

What about this?

#define upper_32_bits(x) (sizeof(x) == 8 ? (u64)(x) >> 32 : 0)

The u64 cast prevents the sign bit from being carried over and therefore 
eliminates the need for a subsequent cast to u32 since the upper 32 of the 
result will be 0. Shouldn't be any case where an integer gets promoted if 
64 bits is the largest possible promotion.

Assuming, of course, I'm not an idiot. Which I somewhat frequently am.

[PATCH] utilities: add helper functions for safe 64-bit integer operations as 32-bit halves

[John Anthony Kazos Jr.]

From: John Anthony Kazos Jr. <jakj@j-a-k-j.com>

Add helper functions "upper_32_bits" and "lower_32_bits" to 
<include/linux/kernel.h> to allow 64-bit integers to be separated into 
their 32-bit upper and lower halves without promoting integers, without 
stretching sign bits, and without generating compiler warnings when used 
with any integer not greater than 64 bits wide. High-order bits are 
assumed to be zero for integers with fewer than 64 of them.

Signed-off-by: John Anthony Kazos Jr. <jakj@j-a-k-j.com>

---

Using these functions with signed quantities is an error, especially if 
you read a 32-bit quantity from disk that happens to have the high bit set 
into an int on a 32-bit machine, then use it with a function taking a u64 
which screws your data. When switching to using these functions, it's a 
good opportunity to check for these signedness errors. (Haven't we learned 
anything over the past decades of computing about assuming that one little 
bit doesn't matter?)

Not sure exactly whom the maintainer is for this, so I added 
trivial@kernel.org. It's certainly not limited to one subsystem anymore, 
and converting the whole kernel to this could be a good step for 
readability and correctness across architectures of any word size.

--- linux-2.6.21-rc7-git4.orig/include/linux/kernel.h	2007-04-20 20:22:13.000000000 -0400
+++ linux-2.6.21-rc7-git4.mod/include/linux/kernel.h	2007-04-20 20:37:41.000000000 -0400
@@ -40,6 +40,23 @@ extern const char linux_proc_banner[];
 #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
 #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
 
+/**
+ * lower_32_bits, upper_32_bits - separate the halves of a 64-bit integer
+ * @n: the integer to separate
+ *
+ * Separate a 64-bit integer into its upper and lower 32-bit halves.
+ * Designed to avoid integer promotions and compiler warnings when used
+ * with smaller integers, in which case the missing bits are assumed to
+ * be zero. Designed to treat integers as unsigned whether or not they
+ * really are. (If you are using these with signed integers, your code
+ * is almost certainly wrong. The cast is good for people too lazy to
+ * type "unsigned" in their code, since breaking things is bad.)
+ *
+ * These assume the integer used is NOT greater than 64 bits wide.
+ */
+#define upper_32_bits(n) (sizeof(n) == 8 ? (u64)(n) >> 32 : 0)
+#define lower_32_bits(n) (sizeof(n) == 8 ? (u32)(n) : (n))
+
 #define	KERN_EMERG	"<0>"	/* system is unusable			*/
 #define	KERN_ALERT	"<1>"	/* action must be taken immediately	*/
 #define	KERN_CRIT	"<2>"	/* critical conditions			*/

Re: [PATCH] utilities: add helper functions for safe 64-bit integer operations as 32-bit halves

[Andrew Morton]

On Fri, 20 Apr 2007 20:55:49 -0400 (EDT) "John Anthony Kazos Jr." <jakj@j-a-k-j.com> wrote:

> +#define upper_32_bits(n) (sizeof(n) == 8 ? (u64)(n) >> 32 : 0)

It's very unclear what type this returns, in terms of both size and
signedness.  Perhaps it always returns a u64, dunno.  If it does, that will
cause the arithmetic which uses this macro to go 64-bit too.  Casting the
whole return value to u32 would fix all those doubts up.

> +#define lower_32_bits(n) (sizeof(n) == 8 ? (u32)(n) : (n))

n&0xffffffff would be simpler.

Do we actually have any call for this?

Re: [PATCH] utilities: add helper functions for safe 64-bit integer operations as 32-bit halves

[John Anthony Kazos Jr.]

From: John Anthony Kazos Jr. <jakj@j-a-k-j.com>

Add helper functions "upper_32_bits" and "lower_32_bits" to 
<include/linux/kernel.h> to allow 64-bit integers to be separated into 
their 32-bit upper and lower halves without promoting integers, without 
stretching sign bits, and without generating compiler warnings when used 
with any integer not greater than 64 bits wide. High-order bits are 
assumed to be zero for integers with fewer than 64 of them. Result values 
are always 32-bit unsigned.

Signed-off-by: John Anthony Kazos Jr. <jakj@j-a-k-j.com>

---

> > +#define upper_32_bits(n) (sizeof(n) == 8 ? (u64)(n) >> 32 : 0)
> 
> It's very unclear what type this returns, in terms of both size and
> signedness.  Perhaps it always returns a u64, dunno.  If it does, that will
> cause the arithmetic which uses this macro to go 64-bit too.  Casting the
> whole return value to u32 would fix all those doubts up.

If the argument is 64-bit, the return value is 64-bit; otherwise, the 
return value is the same as the size of the argument. This prevents 
integer promotion. I was trying to also not promote,, say, a short int, if 
for some stupid reason somebody was using one. But you're right, a cast is 
clearer, and the functions are clearly intended for 32-bit arithmetic.

> > +#define lower_32_bits(n) (sizeof(n) == 8 ? (u32)(n) : (n))
> 
> n&0xffffffff would be simpler.

I suppose. I'm trying to be careful and account for stupid edge cases, but 
then again, such code is horribly broken so we can instead ignore them? We 
shall assume the mask gets promoted as necessary.

> Do we actually have any call for this?

It enhances readability and prevents compiler warnings without increasing 
code-local complexity. Besides, if we have upper_32_bits, we should have 
lower_32_bits. Two similarly-named function calls is better than a 
function call and a mask.

And it's really nice to be able to write code that is word-size 
independent and also not cluttered. And if any code declares macros like 
this now, they can be eliminated and make a little more utility code 
central.

--- linux-2.6.21-rc7-git4.orig/include/linux/kernel.h	2007-04-20 20:22:13.000000000 -0400
+++ linux-2.6.21-rc7-git4.mod/include/linux/kernel.h	2007-04-21 08:59:01.000000000 -0400
@@ -40,6 +40,24 @@ extern const char linux_proc_banner[];
 #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
 #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
 
+/**
+ * lower_32_bits, upper_32_bits - separate the halves of a 64-bit integer
+ * @n: the integer to separate
+ *
+ * Separate a 64-bit integer into its upper and lower 32-bit halves.
+ * Designed to avoid integer promotions and compiler warnings when used
+ * with smaller integers, in which case the missing bits are assumed to
+ * be zero. Designed to treat integers as unsigned whether or not they
+ * really are. (If you are using these with signed integers, your code
+ * is almost certainly wrong. The cast is good for people too lazy to
+ * type "unsigned" in their code, since breaking things is bad.)
+ *
+ * These assume the integer used is NOT greater than 64 bits wide.
+ * Return values are always 32-bit unsigned integers.
+ */
+#define upper_32_bits(n) ((u32)(sizeof(n) == 8 ? (u64)(n) >> 32 : 0))
+#define lower_32_bits(n) ((u32)(n & ~(u32)0))
+
 #define	KERN_EMERG	"<0>"	/* system is unusable			*/
 #define	KERN_ALERT	"<1>"	/* action must be taken immediately	*/
 #define	KERN_CRIT	"<2>"	/* critical conditions			*/

Re: [PATCH] utilities: add helper functions for safe 64-bit integer operations as 32-bit halves

[Alan Cox]

> > +#define lower_32_bits(n) (sizeof(n) == 8 ? (u32)(n) : (n))
> 
> n&0xffffffff would be simpler.
> 
> Do we actually have any call for this?

The only case for all of this we care about is sector_t, which is one
type, with specific properties (eg always being positive). The rest is
over-engineering. Call it sector_upper32() do it the simple way and stop
trying to solve a problem we don't have

Re: [PATCH] utilities: add helper functions for safe 64-bit integer operations as 32-bit halves

[Andrew Morton]

On Sat, 21 Apr 2007 22:01:47 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > > +#define lower_32_bits(n) (sizeof(n) == 8 ? (u32)(n) : (n))
> > 
> > n&0xffffffff would be simpler.
> > 
> > Do we actually have any call for this?
> 
> The only case for all of this we care about is sector_t, which is one
> type, with specific properties (eg always being positive). The rest is
> over-engineering. Call it sector_upper32() do it the simple way and stop
> trying to solve a problem we don't have

James said we have the same problem with dma_addr_t.

Re: [PATCH] utilities: add helper functions for safe 64-bit integer operations as 32-bit halves

[Christoph Hellwig]

On Sat, Apr 21, 2007 at 02:06:22PM -0700, Andrew Morton wrote:
> On Sat, 21 Apr 2007 22:01:47 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> 
> > > > +#define lower_32_bits(n) (sizeof(n) == 8 ? (u32)(n) : (n))
> > > 
> > > n&0xffffffff would be simpler.
> > > 
> > > Do we actually have any call for this?
> > 
> > The only case for all of this we care about is sector_t, which is one
> > type, with specific properties (eg always being positive). The rest is
> > over-engineering. Call it sector_upper32() do it the simple way and stop
> > trying to solve a problem we don't have
> 
> James said we have the same problem with dma_addr_t.

Yes.  It's in fact the far more common case and we have a bread of
macros dealing with the issue in various drivers.

Re: [PATCH] utilities: add helper functions for safe 64-bit integer operations as 32-bit halves

[Christoph Hellwig]

On Sun, Apr 22, 2007 at 01:38:23PM +0100, Alan Cox wrote:
> > > > over-engineering. Call it sector_upper32() do it the simple way and stop
> > > > trying to solve a problem we don't have
> > > 
> > > James said we have the same problem with dma_addr_t.
> > 
> > Yes.  It's in fact the far more common case and we have a bread of
> > macros dealing with the issue in various drivers.
> 
> So we still only need it for unsigned 32/64bit values ?

Yes.

Re: [PATCH] utilities: add helper functions for safe 64-bit integer operations as 32-bit halves

[Alan Cox]

> > > over-engineering. Call it sector_upper32() do it the simple way and stop
> > > trying to solve a problem we don't have
> > 
> > James said we have the same problem with dma_addr_t.
> 
> Yes.  It's in fact the far more common case and we have a bread of
> macros dealing with the issue in various drivers.

So we still only need it for unsigned 32/64bit values ?



Alan