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

[Dan Williams <dcbw@redhat.com>]

On Wed, 2015-06-17 at 09:27 +0200, Johannes Berg wrote:
> 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.

I can't see anywhere that mwl_tx_desc->sta_info actually gets read by
the driver; it only appears to be written by mwl_tx_skb().  So (even if
it was intended to be) it's clearly not a stashed pointer for later.

Like you say it's size also needs to be specified, since won't it be 64
bits or 32 depending on the architecture?  I can't imagine the firmware
would like that much.

At the very least, this member and its usage in mwl_tx_skb() deserves a
code comment about what's going on and why a host pointer is getting
stuffed into a field that the firmware is reading...

Dan

> > +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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html