Re: Implement mode_select for atapi tape drives - round 4

[David Young <dyoung%pobox.com@localhost>]

On Sun, Aug 09, 2009 at 08:37:29PM +0100, David Laight wrote:
> On Sun, Aug 09, 2009 at 01:36:02PM +0200, Manuel Bouyer wrote:
> > On Sun, Aug 09, 2009 at 09:29:05AM +0100, Iain Hibbert wrote:
> > > On Sat, 8 Aug 2009, Paul Goyette wrote:
> > > 
> > > > Further comment/criticism is certainly welcomed.  In the absence of any
> > > > objection, I'd like to commit this in the next 7 to 10 days.
> > > 
> > > +/*
> > > + * Send a filled out parameter structure to the drive to
> > > + * set it into the desire modes etc.
> > > + */
> > > +int
> > > +st_mode_select(struct st_softc *st, int flags)
> > > +{
> > > + u_int select_len;
> > > + struct select {
> > > +         struct scsi_mode_parameter_header_6 header;
> > > +         struct scsi_general_block_descriptor blk_desc;
> > > +         u_char sense_data[MAX_PAGE_0_SIZE];
> > > + } select;
> > > + struct scsipi_periph *periph = st->sc_periph;
> > > +
> > > + select_len = sizeof(select.header) + sizeof(select.blk_desc) +
> > > +              st->page_0_size;
> > > 
> > > should select be __packed?
> > 
> > Yes, it should probably be. I know most hardware structures are not
> > declared __packed in the scsipi code, we've been luky this didn't cause
> > issues for now ...
> 
> Actually it probably shouldn't!
> You may want to define the alignment restriction of some of the members,
> and possibly of the structure itself.
> (ie if there are any 32bit items that are on odd 16bit boundaries, or
> where the entire structure isn't a multiple of 4 bytes - for systems
> where the ABI forces all structures to have 32bit alignment.)
> 
> Forcing 'packed' is overkill and will lead to very slow code in
> unnecessary places on systems that cannot do misaligned transfers in
> hardware.

I have found that if the programmer has padded the struct, if the widest
field in the struct is N bytes wide, if every field aligns with natural
boundaries, and if the host aligns the structs with the right boundary,
then __packed __aligned(N) packs the struct while letting the compiler
avoid the performance penalty of misaligned transfers.  That's a lot of
ifs, but they seem to be true an awful lot of the time.

Dave

-- 
David Young             OJC Technologies
dyoung%ojctech.com@localhost      Urbana, IL * (217) 278-3933