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

[Ryder Lee <ryder.lee@mediatek.com>]

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