Re: [PATCH v2] Add new mac80211 driver mwlwifi.

[Johannes Berg <johannes@sipsolutions.net>]

On Wed, 2015-06-17 at 06:06 +0000, David Lin wrote:

(not really reading all of this right now, just two small comments)

> +struct mwl_tx_desc {
> +	u8 data_rate;
> +	u8 tx_priority;
> +	__le16 qos_ctrl;
> +	__le32 pkt_ptr;
> +	__le16 pkt_len;
> +	u8 dest_addr[ETH_ALEN];
> +	__le32 pphys_next;
> +	__le32 sap_pkt_info;
> +	__le32 rate_info;
> +	u8 type;
> +	u8 xmit_control;     /* bit 0: use rateinfo, bit 1: disable ampdu */
> +	__le16 reserved;
> +	__le32 tcpack_sn;
> +	__le32 tcpack_src_dst;
> +	struct sk_buff *psk_buff;
> +	struct mwl_tx_desc *pnext;
> +	u8 reserved1[2];
> +	u8 packet_info;
> +	u8 packet_id;
> +	__le16 packet_len_and_retry;
> +	__le16 packet_rate_info;
> +	u8 *sta_info;
> +	__le32 status;
> +} __packed;

This ... cannot be right. You can't have a pointer in a structure that's
used by firmware, and the __leXX and __packed indicates that it's used
by firmware. But pointer size is variable, so *clearly* this cannot be
correct.

> +struct mwl_rx_desc {
> +	__le16 pkt_len;              /* total length of received data      */
> +	__le16 rate;                 /* receive rate information           */
> +	__le32 pphys_buff_data;      /* physical address of payload data   */
> +	__le32 pphys_next;           /* physical address of next RX desc   */
> +	__le16 qos_ctrl;             /* received QosCtrl field variable    */
> +	__le16 ht_sig2;              /* like name states                   */
> +	__le32 hw_rssi_info;
> +	__le32 hw_noise_floor_info;
> +	u8 noise_floor;
> +	u8 reserved[3];
> +	u8 rssi;                     /* received signal strengt indication */
> +	u8 status;                   /* status field containing USED bit   */
> +	u8 channel;                  /* channel this pkt was received on   */
> +	u8 rx_control;               /* the control element of the desc    */
> +	/* above are 32bits aligned and is same as FW, RxControl put at end
> +	 * for sync
> +	 */
> +	struct sk_buff *psk_buff;    /* associated sk_buff for Linux       */
> +	void *pbuff_data;            /* virtual address of payload data    */
> +	struct mwl_rx_desc *pnext;   /* virtual address of next RX desc    */
> +} __packed;

Same here. If you really need to have a firmware and driver struct as
indicated by the comment, you should probably have

struct mwl_rx_info {
	/* firmware info */
} __packed;

struct mwl_rx_desc {
	struct mwl_rx_info info;
	/* driver info */
};

(note the lack of __packed also, __packed often makes accesses far less
efficient)

> +struct mwl_desc_data {
> +	dma_addr_t pphys_tx_ring;          /* ptr to first TX desc (phys.)    */
> +	struct mwl_tx_desc *ptx_ring;      /* ptr to first TX desc (virt.)    */
> +	struct mwl_tx_desc *pnext_tx_desc; /* next TX desc that can be used   */
> +	struct mwl_tx_desc *pstale_tx_desc;/* the staled TX descriptor        */
> +	dma_addr_t pphys_rx_ring;          /* ptr to first RX desc (phys.)    */
> +	struct mwl_rx_desc *prx_ring;      /* ptr to first RX desc (virt.)    */
> +	struct mwl_rx_desc *pnext_rx_desc; /* next RX desc that can be used   */
> +	unsigned int wcb_base;             /* FW base offset for registers    */
> +	unsigned int rx_desc_write;        /* FW descriptor write position    */
> +	unsigned int rx_desc_read;         /* FW descriptor read position     */
> +	unsigned int rx_buf_size;          /* length of the RX buffers        */
> +} __packed;

ditto - don't pack driver structs

Also, you probably really should have two header files - one for the
firmware structs and one for the driver structs - especially since you
seem to be confusing the two!

> +++ b/drivers/net/wireless/mwlwifi/fwcmd.c

There are a lot of struct definitions here that you could move to the
same header file.

> +#if SYSADPT_NUM_OF_DESC_DATA > 3

how does that get set?

> +/* Description:  This file implements main functions of this module.
> + */

You still have a pretty strange comment style.

> +	hw->flags |= IEEE80211_HW_AMPDU_AGGREGATION;

This was really what I was looking for :)

Note that I changed this entirely in the mac80211-next tree. I don't
know how Kalle wants to handle this, but given that your driver isn't
going to make the cut for 4.2 anyway, you should probably rebase it on
mac80211-next, and then Kalle can pick it up after he merges that back
from net-next, or perhaps after the merge window closes.

[snip rest]

johannes