Re: [PATCH wireless-next 02/35] wifi: mm81x: add command.c

[Johannes Berg]

Hi,

Hm. So I _was_ going to say this just _looked_ bad, but now I think it's
actually wrong:

On Fri, 2026-02-27 at 15:10 +1100, Lachlan Hodges wrote:
> +int mm81x_cmd_sta_state(struct mm81x *mm, struct mm81x_vif *mm_vif, u16 aid,
> +			struct ieee80211_sta *sta,
> +			enum ieee80211_sta_state state)
> +{
> +	struct host_cmd_req_set_sta_state req;
> +	struct host_cmd_resp_set_sta_state resp;
> +
> +	memset(&req, 0, sizeof(req));

This is fine of course.

> +	mm81x_cmd_init(mm, &req.hdr, HOST_CMD_ID_SET_STA_STATE, mm_vif->id,
> +		       sizeof(req));
> +
> +	memcpy(req.sta_addr, sta->addr, sizeof(req.sta_addr));
> +	req.aid = cpu_to_le16(aid);
> +	req.state = cpu_to_le16(state);
> +	req.uapsd_queues = sta->uapsd_queues;

(you write other fields here otherwise it'd be useless)


But then there are cases like this:

> +int mm81x_cmd_add_if(struct mm81x *mm, u16 *vif_id, const u8 *addr,
> +		     enum nl80211_iftype type)
> +{
> +	int ret;
> +	struct host_cmd_req_add_interface req;
> +	struct host_cmd_resp_add_interface resp;
> +
> +	mm81x_cmd_init(mm, &req.hdr, HOST_CMD_ID_ADD_INTERFACE, 0, sizeof(req));
> +
> +	switch (type) {
> +	case NL80211_IFTYPE_STATION:
> +		req.interface_type = cpu_to_le32(HOST_CMD_INTERFACE_TYPE_STA);
> +		break;
> +	case NL80211_IFTYPE_AP:
> +		req.interface_type = cpu_to_le32(HOST_CMD_INTERFACE_TYPE_AP);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	memcpy(req.addr.octet, addr, sizeof(req.addr.octet));
> +
> +	ret = mm81x_cmd_tx(mm, (struct host_cmd_resp *)&resp,
> +			   (struct host_cmd_req *)&req, sizeof(resp), 0);


Where you're sending uninitialised data to the firmware, regardless of
what the actual command does, mm81x_cmd_init() doesn't initialise the
'pad' field in the header, and there might be per-command fields too?


It'd also be far more obvious that it's not sending uninitialised data
if it was simply each command built as a C99 initialiser:

#define INIT_HDR(_var, _cmd, _vif_id)                           \
        {                                                       \
                .message_id = cpu_to_le16(_cmd),                \
                .vif_id = cpu_to_le16(_vif_id),                 \
                .len = sizeof(_var) - sizeof(_var).hdr,         \
        }

...

int mm81x_cmd_sta_state(struct mm81x *mm, struct mm81x_vif *mm_vif, u16 aid,
                        struct ieee80211_sta *sta,
                        enum ieee80211_sta_state state)
{
        struct host_cmd_req_set_sta_state req = {
                .hdr = INIT_HDR(req, HOST_CMD_ID_SET_STA_STATE, mm_vif->id),
                .aid = cpu_to_le16(aid),
                .state = cpu_to_le16(state),
                .uapsd_queues = sta->uapsd_queues,
        };
        struct host_cmd_resp_set_sta_state resp;

        memcpy(req.sta_addr, sta->addr, sizeof(req.sta_addr));

        return mm81x_cmd_tx(mm, (struct host_cmd_resp *)&resp,
                            (struct host_cmd_req *)&req, sizeof(resp), 0);
}


etc.

That assumes .hdr is always the header, but that didn't seem
unreasonable. It also doesn't work for a few cases that dynamically
allocate (filter), but that could just use kzalloc() instead.


It also seems (and that's really nitpicking now) that the response
argument to mm81x_cmd_tx() is actually optional, so a few (more) places
like this one could use NULL there (e.g. also key removal, beacon timer,
frag threshold)

johannes