diff mbox series

[1/7] mt76: mt7615: simplify mcu_set_bmc flow

Message ID ae72dd289f8a26a2c0f42de1f940bb8b6d1f2c29.1579237414.git.ryder.lee@mediatek.com (mailing list archive)
State Superseded
Delegated to: Felix Fietkau
Headers show
Series [1/7] mt76: mt7615: simplify mcu_set_bmc flow | expand

Commit Message

Ryder Lee Jan. 21, 2020, 2:13 p.m. UTC 
Move mcu_wtbl_bmc into mcu_set_sta_rec_bmc to simplify flow.

Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
---
 .../net/wireless/mediatek/mt76/mt7615/main.c  |  3 +-
 .../net/wireless/mediatek/mt76/mt7615/mcu.c   | 97 ++++++++-----------
 .../wireless/mediatek/mt76/mt7615/mt7615.h    |  6 +-
 3 files changed, 45 insertions(+), 61 deletions(-)

Comments

Lorenzo Bianconi Jan. 21, 2020, 3:11 p.m. UTC | #1 
> Move mcu_wtbl_bmc into mcu_set_sta_rec_bmc to simplify flow.
> 
> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  .../net/wireless/mediatek/mt76/mt7615/main.c  |  3 +-
>  .../net/wireless/mediatek/mt76/mt7615/mcu.c   | 97 ++++++++-----------
>  .../wireless/mediatek/mt76/mt7615/mt7615.h    |  6 +-
>  3 files changed, 45 insertions(+), 61 deletions(-)
> 

[...]

> -int mt7615_mcu_set_sta_rec_bmc(struct mt7615_dev *dev,
> -			       struct ieee80211_vif *vif, bool en)
> +int mt7615_mcu_set_bmc(struct mt7615_dev *dev,
> +		       struct ieee80211_vif *vif, bool en)
>  {
>  	struct mt7615_vif *mvif = (struct mt7615_vif *)vif->drv_priv;
>  	struct {
>  		struct sta_req_hdr hdr;
>  		struct sta_rec_basic basic;
> -	} req = {
> +		u8 buf[MT7615_WTBL_UPDATE_MAX_SIZE];
> +	} __packed req = {
>  		.hdr = {
>  			.bss_idx = mvif->idx,
>  			.wlan_idx = mvif->sta.wcid.idx,
> @@ -1109,8 +1059,18 @@ int mt7615_mcu_set_sta_rec_bmc(struct mt7615_dev *dev,
>  			.conn_type = cpu_to_le32(CONNECTION_INFRA_BC),
>  		},
>  	};
> +	struct wtbl_req_hdr *wtbl_hdr;
> +	struct wtbl_generic *wtbl_g;
> +	struct wtbl_rx *wtbl_rx;
> +	u8 *buf = req.buf;
> +
>  	eth_broadcast_addr(req.basic.peer_addr);
>  
> +	wtbl_hdr = (struct wtbl_req_hdr *)buf;
> +	buf += sizeof(*wtbl_hdr);
> +	wtbl_hdr->wlan_idx = mvif->sta.wcid.idx;
> +	wtbl_hdr->operation = WTBL_RESET_AND_SET;
> +
>  	if (en) {
>  		req.basic.conn_state = CONN_STATE_PORT_SECURE;
>  		req.basic.extra_info = cpu_to_le16(EXTRA_INFO_VER |
> @@ -1118,10 +1078,37 @@ int mt7615_mcu_set_sta_rec_bmc(struct mt7615_dev *dev,
>  	} else {
>  		req.basic.conn_state = CONN_STATE_DISCONNECT;
>  		req.basic.extra_info = cpu_to_le16(EXTRA_INFO_VER);
> +
> +		__mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD_STA_REC_UPDATE,
> +				    &req, (u8 *)wtbl_hdr - (u8 *)&req, true);

we need to check the return value from __mt76_mcu_send_msg here.
Moreover, here (u8 *)wtbl_hdr - (u8 *)&req is
sizeof(struct sta_req_hdr) + sizeof(struct sta_rec_basic), right?
I guess it would be easier to understand if we explicit the length, what do you think?

> +
> +		return __mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD_WTBL_UPDATE,
> +					   (u8 *)wtbl_hdr, buf - (u8 *)wtbl_hdr,
> +					   true);
>  	}
>  
> +	wtbl_g = (struct wtbl_generic *)buf;
> +	buf += sizeof(*wtbl_g);
> +	wtbl_g->tag = cpu_to_le16(WTBL_GENERIC);
> +	wtbl_g->len = cpu_to_le16(sizeof(*wtbl_g));
> +	wtbl_g->muar_idx = 0xe;
> +	eth_broadcast_addr(wtbl_g->peer_addr);
> +
> +	wtbl_rx = (struct wtbl_rx *)buf;
> +	buf += sizeof(*wtbl_rx);
> +	wtbl_rx->tag = cpu_to_le16(WTBL_RX);
> +	wtbl_rx->len = cpu_to_le16(sizeof(*wtbl_rx));
> +	wtbl_rx->rv = 1;
> +	wtbl_rx->rca1 = 1;
> +	wtbl_rx->rca2 = 1;
> +
> +	wtbl_hdr->tlv_num = cpu_to_le16(2);
> +
> +	__mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD_WTBL_UPDATE,
> +			    (u8 *)wtbl_hdr, buf - (u8 *)wtbl_hdr, true);

we need to check the return value from __mt76_mcu_send_msg here
> +
>  	return __mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD_STA_REC_UPDATE,
> -				   &req, sizeof(req), true);
> +				   &req, (u8 *)wtbl_hdr - (u8 *)&req, true);

same here about the length.

Regards,
Lorenzo

>  }
>  
>  int mt7615_mcu_set_sta_rec(struct mt7615_dev *dev, struct ieee80211_vif *vif,
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h b/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h
> index eaafae9cc279..84949256601f 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h
> @@ -241,14 +241,12 @@ int mt7615_mcu_set_bss_info(struct mt7615_dev *dev, struct ieee80211_vif *vif,
>  void mt7615_mac_set_rates(struct mt7615_phy *phy, struct mt7615_sta *sta,
>  			  struct ieee80211_tx_rate *probe_rate,
>  			  struct ieee80211_tx_rate *rates);
> -int mt7615_mcu_wtbl_bmc(struct mt7615_dev *dev, struct ieee80211_vif *vif,
> -			bool enable);
>  int mt7615_mcu_add_wtbl(struct mt7615_dev *dev, struct ieee80211_vif *vif,
>  			struct ieee80211_sta *sta);
>  int mt7615_mcu_del_wtbl(struct mt7615_dev *dev, struct ieee80211_sta *sta);
>  int mt7615_mcu_del_wtbl_all(struct mt7615_dev *dev);
> -int mt7615_mcu_set_sta_rec_bmc(struct mt7615_dev *dev,
> -			       struct ieee80211_vif *vif, bool en);
> +int mt7615_mcu_set_bmc(struct mt7615_dev *dev, struct ieee80211_vif *vif,
> +		       bool en);
>  int mt7615_mcu_set_sta_rec(struct mt7615_dev *dev, struct ieee80211_vif *vif,
>  			   struct ieee80211_sta *sta, bool en);
>  int mt7615_mcu_set_bcn(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> -- 
> 2.18.0
Ryder Lee Jan. 21, 2020, 4 p.m. UTC | #2 
On Tue, 2020-01-21 at 16:11 +0100, Lorenzo Bianconi wrote:
> > Move mcu_wtbl_bmc into mcu_set_sta_rec_bmc to simplify flow.
> > 
> > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  .../net/wireless/mediatek/mt76/mt7615/main.c  |  3 +-
> >  .../net/wireless/mediatek/mt76/mt7615/mcu.c   | 97 ++++++++-----------
> >  .../wireless/mediatek/mt76/mt7615/mt7615.h    |  6 +-
> >  3 files changed, 45 insertions(+), 61 deletions(-)
> > 
> 
> [...]
> 
> > -int mt7615_mcu_set_sta_rec_bmc(struct mt7615_dev *dev,
> > -			       struct ieee80211_vif *vif, bool en)
> > +int mt7615_mcu_set_bmc(struct mt7615_dev *dev,
> > +		       struct ieee80211_vif *vif, bool en)
> >  {
> >  	struct mt7615_vif *mvif = (struct mt7615_vif *)vif->drv_priv;
> >  	struct {
> >  		struct sta_req_hdr hdr;
> >  		struct sta_rec_basic basic;
> > -	} req = {
> > +		u8 buf[MT7615_WTBL_UPDATE_MAX_SIZE];
> > +	} __packed req = {
> >  		.hdr = {
> >  			.bss_idx = mvif->idx,
> >  			.wlan_idx = mvif->sta.wcid.idx,
> > @@ -1109,8 +1059,18 @@ int mt7615_mcu_set_sta_rec_bmc(struct mt7615_dev *dev,
> >  			.conn_type = cpu_to_le32(CONNECTION_INFRA_BC),
> >  		},
> >  	};
> > +	struct wtbl_req_hdr *wtbl_hdr;
> > +	struct wtbl_generic *wtbl_g;
> > +	struct wtbl_rx *wtbl_rx;
> > +	u8 *buf = req.buf;
> > +
> >  	eth_broadcast_addr(req.basic.peer_addr);
> >  
> > +	wtbl_hdr = (struct wtbl_req_hdr *)buf;
> > +	buf += sizeof(*wtbl_hdr);
> > +	wtbl_hdr->wlan_idx = mvif->sta.wcid.idx;
> > +	wtbl_hdr->operation = WTBL_RESET_AND_SET;
> > +
> >  	if (en) {
> >  		req.basic.conn_state = CONN_STATE_PORT_SECURE;
> >  		req.basic.extra_info = cpu_to_le16(EXTRA_INFO_VER |
> > @@ -1118,10 +1078,37 @@ int mt7615_mcu_set_sta_rec_bmc(struct mt7615_dev *dev,
> >  	} else {
> >  		req.basic.conn_state = CONN_STATE_DISCONNECT;
> >  		req.basic.extra_info = cpu_to_le16(EXTRA_INFO_VER);
> > +
> > +		__mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD_STA_REC_UPDATE,
> > +				    &req, (u8 *)wtbl_hdr - (u8 *)&req, true);
> 
> we need to check the return value from __mt76_mcu_send_msg here.

Okay, but it seems we lack of some error handling for mcu in main.c.

> Moreover, here (u8 *)wtbl_hdr - (u8 *)&req is
> sizeof(struct sta_req_hdr) + sizeof(struct sta_rec_basic), right?
> I guess it would be easier to understand if we explicit the length, what do you think?

I'd love to explicit the length, but the length of these variable tlv
rely on sta's ht/vht_cap. Especially we have to take backward
compatibility (firmware v1) into account, and this actually makes code a
bit messy. 

> > +
> > +		return __mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD_WTBL_UPDATE,
> > +					   (u8 *)wtbl_hdr, buf - (u8 *)wtbl_hdr,
> > +					   true);
> >  	}
> >  
> > +	wtbl_g = (struct wtbl_generic *)buf;
> > +	buf += sizeof(*wtbl_g);
> > +	wtbl_g->tag = cpu_to_le16(WTBL_GENERIC);
> > +	wtbl_g->len = cpu_to_le16(sizeof(*wtbl_g));
> > +	wtbl_g->muar_idx = 0xe;
> > +	eth_broadcast_addr(wtbl_g->peer_addr);
> > +
> > +	wtbl_rx = (struct wtbl_rx *)buf;
> > +	buf += sizeof(*wtbl_rx);
> > +	wtbl_rx->tag = cpu_to_le16(WTBL_RX);
> > +	wtbl_rx->len = cpu_to_le16(sizeof(*wtbl_rx));
> > +	wtbl_rx->rv = 1;
> > +	wtbl_rx->rca1 = 1;
> > +	wtbl_rx->rca2 = 1;
> > +
> > +	wtbl_hdr->tlv_num = cpu_to_le16(2);
> > +
> > +	__mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD_WTBL_UPDATE,
> > +			    (u8 *)wtbl_hdr, buf - (u8 *)wtbl_hdr, true);
> 
> we need to check the return value from __mt76_mcu_send_msg here
> > +
> >  	return __mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD_STA_REC_UPDATE,
> > -				   &req, sizeof(req), true);
> > +				   &req, (u8 *)wtbl_hdr - (u8 *)&req, true);
> 
> same here about the length.
> 
> Regards,
> Lorenzo
> 
Ryder
diff mbox series

Patch

diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/main.c b/drivers/net/wireless/mediatek/mt76/mt7615/main.c
index 2a85859da754..dbf6200525c4 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/main.c
@@ -425,8 +425,7 @@  static void mt7615_bss_info_changed(struct ieee80211_hw *hw,
 
 	if (changed & BSS_CHANGED_BEACON_ENABLED) {
 		mt7615_mcu_set_bss_info(dev, vif, info->enable_beacon);
-		mt7615_mcu_wtbl_bmc(dev, vif, info->enable_beacon);
-		mt7615_mcu_set_sta_rec_bmc(dev, vif, info->enable_beacon);
+		mt7615_mcu_set_bmc(dev, vif, info->enable_beacon);
 	}
 
 	if (changed & (BSS_CHANGED_BEACON |
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
index c8d6a36f5d0a..73a5bf11e902 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
@@ -978,57 +978,6 @@  int mt7615_mcu_set_bss_info(struct mt7615_dev *dev,
 	return ret;
 }
 
-static int
-mt7615_mcu_add_wtbl_bmc(struct mt7615_dev *dev,
-			struct mt7615_vif *mvif)
-{
-	struct {
-		struct wtbl_req_hdr hdr;
-		struct wtbl_generic g_wtbl;
-		struct wtbl_rx rx_wtbl;
-	} req = {
-		.hdr = {
-			.wlan_idx = mvif->sta.wcid.idx,
-			.operation = WTBL_RESET_AND_SET,
-			.tlv_num = cpu_to_le16(2),
-		},
-		.g_wtbl = {
-			.tag = cpu_to_le16(WTBL_GENERIC),
-			.len = cpu_to_le16(sizeof(struct wtbl_generic)),
-			.muar_idx = 0xe,
-		},
-		.rx_wtbl = {
-			.tag = cpu_to_le16(WTBL_RX),
-			.len = cpu_to_le16(sizeof(struct wtbl_rx)),
-			.rca1 = 1,
-			.rca2 = 1,
-			.rv = 1,
-		},
-	};
-	eth_broadcast_addr(req.g_wtbl.peer_addr);
-
-	return __mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD_WTBL_UPDATE,
-				   &req, sizeof(req), true);
-}
-
-int mt7615_mcu_wtbl_bmc(struct mt7615_dev *dev,
-			struct ieee80211_vif *vif, bool enable)
-{
-	struct mt7615_vif *mvif = (struct mt7615_vif *)vif->drv_priv;
-
-	if (!enable) {
-		struct wtbl_req_hdr req = {
-			.wlan_idx = mvif->sta.wcid.idx,
-			.operation = WTBL_RESET_AND_SET,
-		};
-
-		return __mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD_WTBL_UPDATE,
-					   &req, sizeof(req), true);
-	}
-
-	return mt7615_mcu_add_wtbl_bmc(dev, mvif);
-}
-
 int mt7615_mcu_add_wtbl(struct mt7615_dev *dev, struct ieee80211_vif *vif,
 			struct ieee80211_sta *sta)
 {
@@ -1088,14 +1037,15 @@  int mt7615_mcu_del_wtbl_all(struct mt7615_dev *dev)
 				   &req, sizeof(req), true);
 }
 
-int mt7615_mcu_set_sta_rec_bmc(struct mt7615_dev *dev,
-			       struct ieee80211_vif *vif, bool en)
+int mt7615_mcu_set_bmc(struct mt7615_dev *dev,
+		       struct ieee80211_vif *vif, bool en)
 {
 	struct mt7615_vif *mvif = (struct mt7615_vif *)vif->drv_priv;
 	struct {
 		struct sta_req_hdr hdr;
 		struct sta_rec_basic basic;
-	} req = {
+		u8 buf[MT7615_WTBL_UPDATE_MAX_SIZE];
+	} __packed req = {
 		.hdr = {
 			.bss_idx = mvif->idx,
 			.wlan_idx = mvif->sta.wcid.idx,
@@ -1109,8 +1059,18 @@  int mt7615_mcu_set_sta_rec_bmc(struct mt7615_dev *dev,
 			.conn_type = cpu_to_le32(CONNECTION_INFRA_BC),
 		},
 	};
+	struct wtbl_req_hdr *wtbl_hdr;
+	struct wtbl_generic *wtbl_g;
+	struct wtbl_rx *wtbl_rx;
+	u8 *buf = req.buf;
+
 	eth_broadcast_addr(req.basic.peer_addr);
 
+	wtbl_hdr = (struct wtbl_req_hdr *)buf;
+	buf += sizeof(*wtbl_hdr);
+	wtbl_hdr->wlan_idx = mvif->sta.wcid.idx;
+	wtbl_hdr->operation = WTBL_RESET_AND_SET;
+
 	if (en) {
 		req.basic.conn_state = CONN_STATE_PORT_SECURE;
 		req.basic.extra_info = cpu_to_le16(EXTRA_INFO_VER |
@@ -1118,10 +1078,37 @@  int mt7615_mcu_set_sta_rec_bmc(struct mt7615_dev *dev,
 	} else {
 		req.basic.conn_state = CONN_STATE_DISCONNECT;
 		req.basic.extra_info = cpu_to_le16(EXTRA_INFO_VER);
+
+		__mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD_STA_REC_UPDATE,
+				    &req, (u8 *)wtbl_hdr - (u8 *)&req, true);
+
+		return __mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD_WTBL_UPDATE,
+					   (u8 *)wtbl_hdr, buf - (u8 *)wtbl_hdr,
+					   true);
 	}
 
+	wtbl_g = (struct wtbl_generic *)buf;
+	buf += sizeof(*wtbl_g);
+	wtbl_g->tag = cpu_to_le16(WTBL_GENERIC);
+	wtbl_g->len = cpu_to_le16(sizeof(*wtbl_g));
+	wtbl_g->muar_idx = 0xe;
+	eth_broadcast_addr(wtbl_g->peer_addr);
+
+	wtbl_rx = (struct wtbl_rx *)buf;
+	buf += sizeof(*wtbl_rx);
+	wtbl_rx->tag = cpu_to_le16(WTBL_RX);
+	wtbl_rx->len = cpu_to_le16(sizeof(*wtbl_rx));
+	wtbl_rx->rv = 1;
+	wtbl_rx->rca1 = 1;
+	wtbl_rx->rca2 = 1;
+
+	wtbl_hdr->tlv_num = cpu_to_le16(2);
+
+	__mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD_WTBL_UPDATE,
+			    (u8 *)wtbl_hdr, buf - (u8 *)wtbl_hdr, true);
+
 	return __mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD_STA_REC_UPDATE,
-				   &req, sizeof(req), true);
+				   &req, (u8 *)wtbl_hdr - (u8 *)&req, true);
 }
 
 int mt7615_mcu_set_sta_rec(struct mt7615_dev *dev, struct ieee80211_vif *vif,
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h b/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h
index eaafae9cc279..84949256601f 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h
@@ -241,14 +241,12 @@  int mt7615_mcu_set_bss_info(struct mt7615_dev *dev, struct ieee80211_vif *vif,
 void mt7615_mac_set_rates(struct mt7615_phy *phy, struct mt7615_sta *sta,
 			  struct ieee80211_tx_rate *probe_rate,
 			  struct ieee80211_tx_rate *rates);
-int mt7615_mcu_wtbl_bmc(struct mt7615_dev *dev, struct ieee80211_vif *vif,
-			bool enable);
 int mt7615_mcu_add_wtbl(struct mt7615_dev *dev, struct ieee80211_vif *vif,
 			struct ieee80211_sta *sta);
 int mt7615_mcu_del_wtbl(struct mt7615_dev *dev, struct ieee80211_sta *sta);
 int mt7615_mcu_del_wtbl_all(struct mt7615_dev *dev);
-int mt7615_mcu_set_sta_rec_bmc(struct mt7615_dev *dev,
-			       struct ieee80211_vif *vif, bool en);
+int mt7615_mcu_set_bmc(struct mt7615_dev *dev, struct ieee80211_vif *vif,
+		       bool en);
 int mt7615_mcu_set_sta_rec(struct mt7615_dev *dev, struct ieee80211_vif *vif,
 			   struct ieee80211_sta *sta, bool en);
 int mt7615_mcu_set_bcn(struct ieee80211_hw *hw, struct ieee80211_vif *vif,