Re: [PATCH 01/12] rtwlan: main files

[Stanislaw Gruszka <sgruszka@redhat.com>]

Hi,

Below is some more detailed review of first patch (with mostly
nitpicks).

On Fri, Sep 21, 2018 at 02:03:56PM +0800, yhchuang@realtek.com wrote:
> +static void rtw_ops_tx(struct ieee80211_hw *hw,
> +		       struct ieee80211_tx_control *control,
> +		       struct sk_buff *skb)
> +{
> +	struct rtw_dev *rtwdev = hw->priv;
> +	struct rtw_tx_pkt_info pkt_info;
> +
> +	if (!rtw_flag_check(rtwdev, RTW_FLAG_RUNNING))
> +		goto out;
> +
> +	memset(&pkt_info, 0, sizeof(pkt_info));
> +	rtw_tx_pkt_info_update(rtwdev, &pkt_info, control, skb);
> +	if (rtw_hci_tx(rtwdev, &pkt_info, skb))
> +		goto out;
> +
> +	return;
> +
> +out:
> +	dev_kfree_skb_any(skb);
This can be simplified by just do kfree after if ().
   
> +static int rtw_ops_add_interface(struct ieee80211_hw *hw,
> +				 struct ieee80211_vif *vif)
> +{
> +	struct rtw_dev *rtwdev = hw->priv;
> +	struct rtw_vif *rtwvif = (struct rtw_vif *)vif->drv_priv;
> +	enum rtw_net_type net_type;
> +	u32 config = 0;
> +	u8 port = 0;
<snip>
> +	list_add(&rtwvif->list, &rtwdev->vif_list);
How rtwdev->vif_list is protected agains concurent access ?

> +	INIT_LIST_HEAD(&rtwvif->sta_list);
> +
> +	rtwvif->conf = &rtw_vif_port[port];
port is always 0, this either should be fixes or max interfaces for
STA should be set to 1 (temporally).
 
> +static void rtw_ops_bss_info_changed(struct ieee80211_hw *hw,
> +				     struct ieee80211_vif *vif,
> +				     struct ieee80211_bss_conf *conf,
> +				     u32 changed)
> +{
> +	struct rtw_dev *rtwdev = hw->priv;
> +	struct rtw_vif *rtwvif = (struct rtw_vif *)vif->drv_priv;
> +	u32 config = 0;
> +
> +	if (changed & BSS_CHANGED_ASSOC) {
> +		struct rtw_sta_info *ap;
> +		struct rtw_chip_info *chip = rtwdev->chip;
> +		enum rtw_net_type net_type;
> +
> +		ap = list_first_entry(&rtwvif->sta_list,
> +				      struct rtw_sta_info, list);
How rtlwvif->sta_list is protected against concurrent access ? 

> +static u8 rtw_acquire_macid(struct rtw_dev *rtwdev)
> +{
> +	u8 i;
> +
> +	for (i = 0; i < RTW_MAX_MAC_ID_NUM; i++) {
> +		if (!rtwdev->macid_used[i]) {
> +			rtwdev->macid_used[i] = true;
> +			return i;
> +		}
> +	}
> +
> +	return i;
> +}
> +
> +static void rtw_release_macid(struct rtw_dev *rtwdev, u8 mac_id)
> +{
> +	rtwdev->macid_used[mac_id] = false;
> +}
> +
> +static int rtw_ops_sta_add(struct ieee80211_hw *hw,
> +			   struct ieee80211_vif *vif,
> +			   struct ieee80211_sta *sta)
> +{
> +	struct rtw_dev *rtwdev = hw->priv;
> +	struct rtw_sta_info *si = (struct rtw_sta_info *)sta->drv_priv;
> +	struct rtw_vif *rtwvif = (struct rtw_vif *)vif->drv_priv;
> +
> +	si->mac_id = rtw_acquire_macid(rtwdev);
<snip>
> +
> +static int rtw_ops_sta_remove(struct ieee80211_hw *hw,
> +			      struct ieee80211_vif *vif,
> +			      struct ieee80211_sta *sta)
> +{
> +	struct rtw_dev *rtwdev = hw->priv;
> +	struct rtw_sta_info *si = (struct rtw_sta_info *)sta->drv_priv;
> +
> +	rtw_release_macid(rtwdev, si->mac_id);
I'm not sure if mac80211 can not add one STA and remove another one
STA at the same time. This need to be verified.

> +static struct ieee80211_iface_limit rtw_5port_limits[] = {
> +	{ .max = 1, .types = BIT(NL80211_IFTYPE_AP) |
> +			     BIT(NL80211_IFTYPE_ADHOC) |
> +			     BIT(NL80211_IFTYPE_MESH_POINT), },
> +	{ .max = 5, .types = BIT(NL80211_IFTYPE_STATION), },
<snip>
> +static struct ieee80211_iface_combination rtw_5port_if_combs[] = {
> +	{
> +		.limits = rtw_5port_limits,
> +		.n_limits = ARRAY_SIZE(rtw_5port_limits),
> +		.max_interfaces = 5,
> +		.num_different_channels = 1,
> +	},
Here: change to max interfaces to 1 or fix port in add/remove interface.

> +static void rtw_watch_dog_work(struct work_struct *work)
> +{
> +	struct rtw_dev *rtwdev = container_of(work, struct rtw_dev,
> +					      watch_dog_work.work);
> +	struct rtw_vif *rtwvif;
> +
> +	if (!rtw_flag_check(rtwdev, RTW_FLAG_RUNNING))
> +		return;
> +
> +	ieee80211_queue_delayed_work(rtwdev->hw, &rtwdev->watch_dog_work,
> +				     RTW_WATCH_DOG_DELAY_TIME);
> +
> +	/* check if we can enter lps */
> +	rtw_lps_enter_check(rtwdev);
> +
> +	/* reset tx/rx statictics */
> +	rtwdev->stats.tx_unicast = 0;
> +	rtwdev->stats.rx_unicast = 0;
> +	rtwdev->stats.tx_cnt = 0;
> +	rtwdev->stats.rx_cnt = 0;
> +	list_for_each_entry(rtwvif, &rtwdev->vif_list, list) {
> +		rtwvif->stats.tx_unicast = 0;
> +		rtwvif->stats.rx_unicast = 0;
> +		rtwvif->stats.tx_cnt = 0;
> +		rtwvif->stats.rx_cnt = 0;
If we just nulify statistics here, what is the point to provide them ?
I can not see code where we read those stats. Perhaps code
that use them will be added, so this possibly is fine.

> +	hal->current_channel = center_chan;
Not used anywhere.

> +static int rtw_load_firmware(struct rtw_dev *rtwdev, const char *fw_name)
> +{
> +	struct rtw_fw_state *fw = &rtwdev->fw;
> +	const struct firmware *firmware;
> +	int ret;
> +
> +	ret = request_firmware(&firmware, fw_name, rtwdev->dev);
> +	if (ret) {
> +		rtw_err(rtwdev, "failed to request firmware\n");
> +		return ret;
> +	}
> +
> +	fw->firmware = firmware;
> +	fw->data = firmware->data;
> +	fw->size = firmware->size;
Seems not needed, we can use fw->firmware->{data,size}
since fw->firwware is not released till chip de-init.

> +#define BIT_LEN_MASK_32(__bitlen) (0xFFFFFFFF >> (32 - (__bitlen)))
> +#define BIT_OFFSET_LEN_MASK_32(__bitoffset, __bitlen)                          \
> +	(BIT_LEN_MASK_32(__bitlen) << (__bitoffset))
> +#define LE_P4BYTE_TO_HOST_4BYTE(__start) (le32_to_cpu(*((__le32 *)(__start))))
> +#define LE_BITS_CLEARED_TO_4BYTE(__start, __bitoffset, __bitlen)               \
> +	(LE_P4BYTE_TO_HOST_4BYTE(__start) &                                    \
> +	 (~BIT_OFFSET_LEN_MASK_32(__bitoffset, __bitlen)))
> +#define LE_BITS_TO_4BYTE(__start, __bitoffset, __bitlen)                       \
> +	((LE_P4BYTE_TO_HOST_4BYTE(__start) >> (__bitoffset)) &                 \
> +	 BIT_LEN_MASK_32(__bitlen))
> +#define SET_BITS_TO_LE_4BYTE(__start, __bitoffset, __bitlen, __value)          \
> +	do {                                                                   \
> +		*((__le32 *)(__start)) = \
> +		cpu_to_le32( \
> +		LE_BITS_CLEARED_TO_4BYTE(__start, __bitoffset, __bitlen) |     \
> +		((((u32)__value) & BIT_LEN_MASK_32(__bitlen)) << (__bitoffset))\
> +		);                                                             \
> +	} while (0)
I think this should be somehow cleaned up or replaced by something sane.

> +struct rtw_sta_info {
> +	struct list_head list;
> +
> +	struct ieee80211_sta *sta;
> +	struct ieee80211_vif *vif;
> +
> +	struct ewma_rssi avg_rssi;
> +	u8 rssi_level;
> +
> +	u8 mac_id;
> +	u8 rate_id;
> +	enum rtw_bandwidth bw_mode;

Can we talk with different STA using different bandwidh ? I think this
is configures channel property and bw_mode is the same for all
connected stations.
 
> +struct rtw_table {
> +	const void *data;
> +	const u32 size;
> +	void (*parse)(struct rtw_dev *rtwdev, const struct rtw_table *tbl);
> +	void (*do_cfg)(struct rtw_dev *rtwdev, const struct rtw_table *tbl,
> +		       u32 addr, u32 data);
> +	enum rtw_rf_path rf_path;
> +};
> +
> +static inline void rtw_load_table(struct rtw_dev *rtwdev,
> +				  const struct rtw_table *tbl)
> +{
> +	(*tbl->parse)(rtwdev, tbl);
> +}

This interface of loading/processing tables of data looks very strange.
I don't think is incorrect, but seems to be somewhat not necessary
complicated. I'll try provide more detailed comment about that when
review other files.

> +static inline u8 *get_hdr_bssid(struct ieee80211_hdr *hdr)
> +{
> +	__le16 fc = hdr->frame_control;
> +	u8 *bssid;
> +
> +	if (ieee80211_has_tods(fc))
> +		bssid = hdr->addr1;
> +	else if (ieee80211_has_fromds(fc))
> +		bssid = hdr->addr2;
> +	else
> +		bssid = hdr->addr3;
> +
> +	return bssid;

This seems to not to be that simple and depend on frame type and
interface type. See ieee80211_get_bssid().

> +#define BIT_SET_RXGCK_VHT_FIFOTHR(x, v)                                        \
> +	(BIT_CLEAR_RXGCK_VHT_FIFOTHR(x) | BIT_RXGCK_VHT_FIFOTHR(v))
> +
> +#define BIT_SHIFT_RXGCK_HT_FIFOTHR 24
> +#define BIT_MASK_RXGCK_HT_FIFOTHR 0x3
> +#define BIT_RXGCK_HT_FIFOTHR(x)                                                \
> +	(((x) & BIT_MASK_RXGCK_HT_FIFOTHR) << BIT_SHIFT_RXGCK_HT_FIFOTHR)
> +#define BITS_RXGCK_HT_FIFOTHR                                                  \
> +	(BIT_MASK_RXGCK_HT_FIFOTHR << BIT_SHIFT_RXGCK_HT_FIFOTHR)
> +#define BIT_CLEAR_RXGCK_HT_FIFOTHR(x) ((x) & (~BITS_RXGCK_HT_FIFOTHR))
> +#define BIT_GET_RXGCK_HT_FIFOTHR(x)                                            \
> +	(((x) >> BIT_SHIFT_RXGCK_HT_FIFOTHR) & BIT_MASK_RXGCK_HT_FIFOTHR)
> +#define BIT_SET_RXGCK_HT_FIFOTHR(x, v)                                         \
> +	(BIT_CLEAR_RXGCK_HT_FIFOTHR(x) | BIT_RXGCK_HT_FIFOTHR(v))
> +
> +#define BIT_SHIFT_RXGCK_OFDM_FIFOTHR 22
> +#define BIT_MASK_RXGCK_OFDM_FIFOTHR 0x3
> +#define BIT_RXGCK_OFDM_FIFOTHR(x)                                              \
> +	(((x) & BIT_MASK_RXGCK_OFDM_FIFOTHR) << BIT_SHIFT_RXGCK_OFDM_FIFOTHR)
> +#define BITS_RXGCK_OFDM_FIFOTHR                                                \
> +	(BIT_MASK_RXGCK_OFDM_FIFOTHR << BIT_SHIFT_RXGCK_OFDM_FIFOTHR)
> +#define BIT_CLEAR_RXGCK_OFDM_FIFOTHR(x) ((x) & (~BITS_RXGCK_OFDM_FIFOTHR))
> +#define BIT_GET_RXGCK_OFDM_FIFOTHR(x)                                          \
> +	(((x) >> BIT_SHIFT_RXGCK_OFDM_FIFOTHR) & BIT_MASK_RXGCK_OFDM_FIFOTHR)
> +#define BIT_SET_RXGCK_OFDM_FIFOTHR(x, v)                                       \
> +	(BIT_CLEAR_RXGCK_OFDM_FIFOTHR(x) | BIT_RXGCK_OFDM_FIFOTHR(v))

Lot of not used defines.

Regards
Stanislaw