Return-path: Received: from s3.sipsolutions.net ([5.9.151.49]:46495 "EHLO sipsolutions.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757883AbbFQH1n (ORCPT ); Wed, 17 Jun 2015 03:27:43 -0400 Message-ID: <1434526059.1884.7.camel@sipsolutions.net> (sfid-20150617_092753_985133_FB773336) Subject: Re: [PATCH v2] Add new mac80211 driver mwlwifi. From: Johannes Berg To: David Lin Cc: "linux-wireless@vger.kernel.org" , Pete Hsieh , Chor Teck Law Date: Wed, 17 Jun 2015 09:27:39 +0200 In-Reply-To: <2127ea28a2a74cd6a89d45b88caf06cb@SC-EXCH02.marvell.com> References: <2127ea28a2a74cd6a89d45b88caf06cb@SC-EXCH02.marvell.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Sender: linux-wireless-owner@vger.kernel.org List-ID: 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