Re: [PATCH RFT] b43: Add support for new firmware

[Michael Buesch <mb@bu3sch.de>]

On Friday 11 January 2008 17:40:04 Larry Finger wrote:
> Michael Buesch wrote:
> > This patch adds support for new firmware.
> > Please test this on old and new firmware.
> 
> I have tested this patch with old firmware. It seems to work; however my testing is not complete as
> my computer has started hanging with the "Caps Lock" light flashing. The crash is not caused by this
> patch as it happened with 2.6.24-rc5, which has run for many days. I do have a suggestion for
> changing the patch (see below).
> 
> > +static inline
> > +size_t b43_txhdr_size(struct b43_wldev *dev)
> > +{
> > +	if (b43_is_old_txhdr_format(dev))
> > +		return 100 + sizeof(struct b43_plcp_hdr6);
> > +	return 104 + sizeof(struct b43_plcp_hdr6);
> > +}
> 
> Why not eliminate most of the magic numbers in this part with
> 
> size_t b43_txhdr_size(struct b43_wldev *dev)
> {
> 	if (b43_is_old_txhdr_format(dev))
> 		return sizeof(struct b43_txhdr) - 4;
> 	return sizeof(struct b43_txhdr);
> }

Because this is IMO as magic as the above.
The struct b43_txhdr is _not_ meant to be used as an object anymore,
as it now contains this union magic stuff. So we must only use it
as a pointer type. The sizeof, however, uses it as an object.
I'm perfectly fine with the hardcoded constants. And they really
are constants, as they are defined by the hard(firm)ware.
I think this all leads to the same issue as "Should we use
#defines for the PCI IDs in PCI ID tables?".
However, this code will go away in summer anyway. So it doesn't
really matter. It really is just a hack.

-- 
Greetings Michael.