Re: [RFC PATCH-mm] usb: file_storage use unaligned endian helpersrather than private versions

[Harvey Harrison]

On Wed, 2008-12-03 at 16:29 -0500, Alan Stern wrote:
> On Tue, 2 Dec 2008, Harvey Harrison wrote:
> 
> > Use the new helpers for unaligned endian access.  Make some small changes
> > to reading 24-bit lba values, read the full 32 bit value and mask.  Produces
> > smaller and faster code on x86 and on powerpc.
> > 
> > Coalesce some byte-writes in 32bit writes to allow byteswapping to happen
> > at compile time and become a 32-bit write without swapping if possible (x86
> > especially)
> > 
> > This shrinks the size of file_storage.o by ~64 bytes on x86_32.
> > 
> > Signed-off-by: Harvey Harrison <harvey.harrison@xxxxxxxxx>
> > ---
> > Alan, what do you think?
> 
> Well, it looks correct... but it's awfully ugly.  Can't we keep the 
> benefits of the new helpers while not messing the code up too much?

<snip>

> Suppose instead we do this:
> 
> #define get_be16(buf)	load_be16_noalign((be16 *) (buf))
> #define get_be24(buf)	(load_be32_noalign((be32 *) (buf)) >> 8)
> #define get_be32(buf)	load_be32_noalign((be32 *) (buf))
> 
> #define put_be16(buf, val)	store_be16_noalign((be16 *) (buf), val)
> #define put_be32(buf, val)	store_be32_noalign((be32 *) (buf), val)
> 
> Then almost no more changes would be needed, only the 24-bit
> consolidation stuff.
> 

I'd rather the common functions just get used directly and cast as
necessary...but it's your code.  Or define a few generic structs
and get a pointer to those and get typechecking for free.

> > @@ -1583,9 +1552,9 @@ static int do_read(struct fsg_dev *fsg)
> >  	/* Get the starting Logical Block Address and check that it's
> >  	 * not too big */
> >  	if (fsg->cmnd[0] == SC_READ_6)
> > -		lba = (fsg->cmnd[1] << 16) | get_be16(&fsg->cmnd[2]);
> > +		lba = load_be32_noalign((__be32 *)&fsg->cmnd[0]) & 0xffffff;
> 
> Like this, which would become
> 
> 		lba = get_be24(&fsg->cmnd[1]);

Again, I could live with it, but would suggest just coding it directly and
maybe just add a comment that this really only wants 24 bits (I thought it was 21 bits,
but then again, I know next to squat about scsi).  <shrug>

> 
> > @@ -2126,9 +2095,9 @@ static int do_request_sense(struct fsg_dev *fsg, struct fsg_buffhd *bh)
> >  static int do_read_capacity(struct fsg_dev *fsg, struct fsg_buffhd *bh)
> >  {
> >  	struct lun	*curlun = fsg->curlun;
> > -	u32		lba = get_be32(&fsg->cmnd[2]);
> > +	u32		lba = load_be32_noalign((__be32 *)&fsg->cmnd[2]);
> >  	int		pmi = fsg->cmnd[8];
> > -	u8		*buf = (u8 *) bh->buf;
> > +	__be32		*buf = (__be32 *)bh->buf;
> >  
> >  	/* Check the PMI and LBA fields */
> >  	if (pmi > 1 || (pmi == 0 && lba != 0)) {
> > @@ -2136,8 +2105,8 @@ static int do_read_capacity(struct fsg_dev *fsg, struct fsg_buffhd *bh)
> >  		return -EINVAL;
> >  	}
> >  
> > -	put_be32(&buf[0], curlun->num_sectors - 1);	// Max logical block
> > -	put_be32(&buf[4], 512);				// Block length
> > +	store_be32_noalign(&buf[0], curlun->num_sectors - 1);	// Max logical block
> > +	store_be32_noalign(&buf[1], 512);			// Block length
> >  	return 8;
> >  }
> >  
> 
> I don't like these changes.  You've gone from an array of bytes to an
> array of 32-bit words, which doesn't agree with the data structures
> defined in the SCSI specification.  Besides, with my new macros this
> isn't needed.

OK, if you'd rather keep the array-of-bytes and use the offsets method,
that's fine too.

Something like (is this really so bad?):

	store_be32_noalign((__be32 *)&buf[0], curlun->num_sectors - 1);
	store_be32_noalign((__be32 *)&buf[4], 512);

> What do you think?

Personally I don't think it's that ugly just using the common functions
directly. BTW, note that if you know the alignment, there are aligned versions
coming as well.  YMMV.

Harvey

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