Re: Implement mode_select for atapi tape drives - round 4

[David Laight <david%l8s.co.uk@localhost>]

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.

Using structures to map hardware structures implicitly assumes knowledge
of the way compilers lay out structures - which may, or may not, be
guaranteed by the C standard.
However doing anything else is much more bug-prone.

        David

-- 
David Laight: david%l8s.co.uk@localhost