[RFC] endianness issues in drivers/net/ethernet/qlogic/qed

[RFC] endianness issues in drivers/net/ethernet/qlogic/qed

[Al Viro]

	"qed: Utilize FW 8.10.3.0" has attempted some endianness
annotations in that driver; unfortunately, either annotations are
BS or the driver is genuinely broken on big-endian hosts.

	For example, struct init_qm_vport_params is claimed to have
->vport_wfq little-endian 16bit.  However, *all* uses are host-endian -
the things like
                vport_params[i].vport_wfq = (wfq_speed * QED_WFQ_UNIT) /
                                                min_pf_rate;
and it's not even "... and at some point we convert it to little-endian
in place, or when copying to another instance".  It is consistently
host-endian.  If that's how it should be, that __le16 is BS; otherwise,
the driver's broken on big-endian.

	Another example:
struct qm_rf_pq_map {
        __le32 reg;
#define QM_RF_PQ_MAP_PQ_VALID_MASK              0x1
#define QM_RF_PQ_MAP_PQ_VALID_SHIFT             0
#define QM_RF_PQ_MAP_RL_ID_MASK                 0xFF
#define QM_RF_PQ_MAP_RL_ID_SHIFT                1
#define QM_RF_PQ_MAP_VP_PQ_ID_MASK              0x1FF
#define QM_RF_PQ_MAP_VP_PQ_ID_SHIFT             9
#define QM_RF_PQ_MAP_VOQ_MASK                   0x1F
#define QM_RF_PQ_MAP_VOQ_SHIFT                  18
#define QM_RF_PQ_MAP_WRR_WEIGHT_GROUP_MASK      0x3
#define QM_RF_PQ_MAP_WRR_WEIGHT_GROUP_SHIFT     23
#define QM_RF_PQ_MAP_RL_VALID_MASK              0x1
#define QM_RF_PQ_MAP_RL_VALID_SHIFT             25
#define QM_RF_PQ_MAP_RESERVED_MASK              0x3F
#define QM_RF_PQ_MAP_RESERVED_SHIFT             26
};
with instances of that manipulated with
                memset(&tx_pq_map, 0, sizeof(tx_pq_map));
                SET_FIELD(tx_pq_map.reg, QM_RF_PQ_MAP_PQ_VALID, 1);
                SET_FIELD(tx_pq_map.reg,
                          QM_RF_PQ_MAP_RL_VALID, rl_valid ? 1 : 0);
                SET_FIELD(tx_pq_map.reg, QM_RF_PQ_MAP_VP_PQ_ID, first_tx_pq_id);
                SET_FIELD(tx_pq_map.reg, QM_RF_PQ_MAP_RL_ID,
                          rl_valid ?
                          p_params->pq_params[i].vport_id : 0);
                SET_FIELD(tx_pq_map.reg, QM_RF_PQ_MAP_VOQ, voq);
                SET_FIELD(tx_pq_map.reg, QM_RF_PQ_MAP_WRR_WEIGHT_GROUP,
                          p_params->pq_params[i].wrr_group);
                /* Write PQ map entry to CAM */
                STORE_RT_REG(p_hwfn, QM_REG_TXPQMAP_RT_OFFSET + pq_id,
                             *((u32 *)&tx_pq_map));
SET_FIELD manipulates the damn thing as *host-endian* - e.g.
                SET_FIELD(v, QM_RF_PQ_MAP_RL_ID, n)
would expand to v = (v & ~0x1fe) | ((n & 0xff) << 1).  Then we proceed to
pass tx_pq_map.reg (fetched in a bloody convoluted way) to
qed_init_store_rt_reg(), which stores it into p_hwfn->rt_data.init_val[...],
which is apparently host-endian.

So what is this __le32 about?

The really worrying case is this:

/* Binary buffer header */
struct bin_buffer_hdr {
        __le32 offset;
        __le32 length;
};

int qed_init_fw_data(struct qed_dev *cdev, const u8 *data)
{
        struct qed_fw_data *fw = cdev->fw_data;
        struct bin_buffer_hdr *buf_hdr;
        u32 offset, len;

        if (!data) {
                DP_NOTICE(cdev, "Invalid fw data\n");
                return -EINVAL;
        }

        /* First Dword contains metadata and should be skipped */
        buf_hdr = (struct bin_buffer_hdr *)data;

        offset = buf_hdr[BIN_BUF_INIT_FW_VER_INFO].offset;
        fw->fw_ver_info = (struct fw_ver_info *)(data + offset);

        offset = buf_hdr[BIN_BUF_INIT_CMD].offset;
        fw->init_ops = (union init_op *)(data + offset);

We are clearly using those as host-endian here.  Yet in _that_
case fixed-endian would appear to make sense, unless we want
separate firmware images for e.g. amd64 and sparc64 hosts...

Another fun one is struct regpair:
        p_ramrod->qp_handle_for_cqe.hi = cpu_to_le32(qp->qp_handle.hi);
        p_ramrod->qp_handle_for_cqe.lo = cpu_to_le32(qp->qp_handle.lo);
Both sides are struct regpair and by the look of helper macros it *is*
meant to be little-endian.  Here, however, one of those (and not
necessary p_ramrod->qp_handle_for_cqe) is host-endian.

Is that driver intended to be used on big-endian hosts at all?

RE: [RFC] endianness issues in drivers/net/ethernet/qlogic/qed

[Tayar, Tomer]

> 	"qed: Utilize FW 8.10.3.0" has attempted some endianness annotations
> in that driver; unfortunately, either annotations are BS or the driver is genuinely
> broken on big-endian hosts.
[...]
> Is that driver intended to be used on big-endian hosts at all?

Thanks for taking the time to review our driver and pointing out these mistakes.
Support for BE machines is planned to be added but currently it is not available.
However, the structures which are used to abstract the HW carry endianity annotations.
Obviously, there are some misses and some annotations were added when not required.
We will prepare a patch that fixes the issues you pointed out and similar ones.

Re: [RFC] endianness issues in drivers/net/ethernet/qlogic/qed

[Al Viro]

On Sun, Sep 24, 2017 at 02:34:19PM +0000, Tayar, Tomer wrote:
> 
> > 	"qed: Utilize FW 8.10.3.0" has attempted some endianness annotations
> > in that driver; unfortunately, either annotations are BS or the driver is genuinely
> > broken on big-endian hosts.
> [...]
> > Is that driver intended to be used on big-endian hosts at all?
> 
> Thanks for taking the time to review our driver and pointing out these mistakes.
> Support for BE machines is planned to be added but currently it is not available.
> However, the structures which are used to abstract the HW carry endianity annotations.
> Obviously, there are some misses and some annotations were added when not required.
> We will prepare a patch that fixes the issues you pointed out and similar ones.

OK...  sparse is pretty good at spotting the problems; if you have any questions - just
ask.  A bit of random braindump concerning that kind of work:

	* bitfields and fixed-endian data do not mix.  It's much better to have just
__le32 (or __le64, etc.) in the structure and use GET_FIELD/SET_FIELD or similar for
accesses.  Another safe technics is something like
	if ((foo->bar & cpu_to_le32(BAR_MASK)) == cpu_to_le32(BAR_THIS << BAR_SHIFT))
instead of
	if (get_bar(foo) == BAR_THIS))
since that keeps shift and endianness conversion on the constant size.  The same goes
for
	if ((foo->bar ^ baz->bar) & cpu_to_le32(BAR_MASK))
instead of
	if (get_bar(foo) != get_bar(baz))
If would be nice if compiler had recognized that kind of stuff and transformed the
latter into the former on its own, but...

	* swab... is a Bloody Bad Idea(tm) in almost all situations.  Keeping track of
whether given data is little-endian or host-endian is much easier than keeping track of
how many times have we flipped it.

	* don't mix little-endian and host-endian in the same variable.  See the previous
point for the reasons - static typing is much safer and easier to reason about.  Code
doing
	n = cpu_to_le32(n);
is asking for trouble.  For local variables it's not even an optimization - compiler
is generally pretty good at spotting two local variables that are never live at the
same point and reusing memory.  And for anything non-local you are introducing a hidden
piece of state - "is that field in this structure little-endian or host-endian at the
moment?", making it very easy to screw up a few months down the road.  Brittle and
hell to debug...

	* one very common source of noise is cpu_to_le32() when le32_to_cpu() was
intended.  Sure, they do the same transformation on anything even remotely plausible
(something like V0 V2 V3 V1 is not a byte order likely to happen on any hardware),
but the choice documents what kind of values do you have before and after the
conversion.  Both the human readers and automated typechecking (sparse) have much
easier life if those are accurate.  Again, see the point re keeping track of the
number of flips vs. keeping track of what's host-endian and what's little-endian.
The latter is local, the former takes reasoning about control flow.

	* for situations like "use this le32 value as search key in binary tree",
where you are really OK with having differently-shaped trees on l-e and b-e hosts,
use something like
	if ((__force __u32) key > node->key)
preferably with a comment explaining why treating this value that way is OK.