[PATCH 8/8] firmware: mark some structs as packed

[Harald Welte]

Hi Alexander,

On Thu, Nov 24, 2011 at 10:36:27AM +0100, Alexander Huemer wrote:
> On Thu, Nov 24, 2011 at 07:58:33AM +0100, Sylvain Munaut wrote:
> > Hi,
> > 
> > First, thanks for this series of patch, I'll be sure to review and test them.
> > 
> Well, no problem, I had some time on hand I couldn't use otherwise.
> > > this eliminates the occurrance of gcc warning
> > > warning: cast increases required alignment of target type
> > > ---
> > >  src/target/firmware/include/comm/timer.h           |    4 +++-
> > >  src/target/firmware/include/layer1/sched_gsmtime.h |    5 ++++-
> > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > > -};
> > > +} __packed;
> > 
> > I'm not conviced by this one : Why should we use packed for structures
> > not used as 'communication packets' ?
> > 
> What are the drawbacks ?

normally, the compiler will lay out the structure in a way that
optimizes accesses to members.  So e.g. on an ARM, a 

struct {
	uint8_t byte1;
	uint8_t byte2;
};

will very likely contain 3 bytes padding between the two uint8 values in
order to make sure no unaligned loads/stores will be required.

However, if you change that to '__packed', the padding will not be
generated and any access to struct members will need to deal with
unaligned accesses, which can be inflating the code size considerably.

> Steve and Holger raised some reasonable concerns on some of the other
> patches, it seems I was quite unconcentrated and they are mostly crap.

don't say that.  We love to receive cleanup patches like yours, and
even if 2-3 have some issues, at a series of 8 it is still considerable
gain for the project.

> The patches do not contain any "magic" anyway. Just straight-forward
> reaction on the different kind of warnings. Most of them can be
> eliminated easily.

yes, in most cases.  However, sometimes we keep the warning around as a
reminder that something still needs to be done, such as e.g. handling an
undhandled enum value in switch() or the like.  In such a case of course
we shuold implement the missing member, and not introduce a default
case.

Regards,
	Harald

-- 
- Harald Welte <laforge at gnumonks.org>           http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
                                                  (ETSI EN 300 175-7 Ch. A6)