Re: [PATCH 3/7] mwl8k: allow for padding in AP transmit descriptor

[Lennert Buytenhek <buytenh@wantstofly.org>]

On Mon, Nov 01, 2010 at 05:55:47PM -0700, Brian Cavagnolo wrote:

> +/*
> + * The size of the tx descriptors used by the firmware varies depending on
> + * whether we use AP firmware or STA firmware.  However, the fields used by
> + * this driver are identical, so the difference in size is treated as padding
> + * at the end of the structure.
> + */
> +#define MWL8K_TX_DESC_SIZE_STA			32
> +#define MWL8K_TX_DESC_SIZE_AP			52

This is misleading, if not downright a lie.

Only the STA firmware branch bothers to mostly keep a degree of ABI
compatibility -- the TX descriptor for STA firmware images is 32 bytes,
no matter whether you're on 8687, 8363, or 8366.

(I like this very much about STA firmware -- it meant I could take the
8687 STA driver and run it on 8363 and 8366 STA with hardly any changes.
And there's reports that mwl8k runs on 8361P STA with almost no patching
needed either.)

But the AP firmware branch changes stuff around all the time, as the AP
people generally supply their driver along with the firmware, and can
make concurrent changes to those while not having to give a damn about
the other users of their firmware.

E.g., the format of the SET_MAC_ADDR command packet changing if you're
compiling the AP firmware for multi-BSS (a 16 bit field is added in the
_middle_ of the command packet if you do that, see mwl8k_cmd_set_mac_addr)
-- and changing the TX descriptor format is another one of those things.

The reason that the AP TX descriptor for 8366 is 52 bytes is not because
it has been defined to be such, but because the "official" AP driver for
8366 just so happens, at this moment, to have 20 bytes of driver-private
info for every TX packet, which it stashes into the TX descriptor (that
doesn't even make any sense either, but hey).  If next week they decide
they need 4 more bytes for driver-private info, they'll just bump it to
56, and we'll be left to pick up the pieces.

So this

> +#define MWL8K_TX_DESC_SIZE_AP                        52

is very misleading, as:

- It suggests that something is set in stone which isn't set in stone at
  all.
- It only applies to this particular 8366 AP image, and might not apply
  to other 8366 AP images or even AP images in general.

In other words, it's just a quick hack to get things to work for one
more HW/firmware/version combo, but it doesn't solve the problem in a
clean way or in general.  (Then again, you probably don't care about
that anyway.)

Since you're being paid by Marvell to do this, please tell them that
this nonsense needs to stop, and that they need to add something to the
firmware so that the driver can figure out what options the firmware
has been built with, or include some way that we can reliably figure
out which ABI variation to use to talk to the firmware.