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

[James Bottomley <James.Bottomley@SteelEye.com>]

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


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/