by Ryder Lee

[permalink] [raw]

Subject: Re: [RFC 12/17] mt7615: mcu: do not use function pointer whenever possible

On Wed, 2019-05-01 at 18:07 +0200, Lorenzo Bianconi wrote:
> Remove function pointer in mt7615_mcu_set_bss_info and run function
> directly. Moreover remove __mt7615_mcu_set_bss_info since it is run just
> by mt7615_mcu_set_bss_info and remove duplicated istructions
>
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
> .../net/wireless/mediatek/mt76/mt7615/mcu.c | 220 +++++++++---------
> 1 file changed, 105 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> index c90ae77db22b..b3fb3d731129 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> @@ -676,154 +676,107 @@ int mt7615_mcu_set_dev_info(struct mt7615_dev *dev,
> return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_DEV_INFO_UPDATE);
> }
>
> -static void bss_info_omac_handler (struct mt7615_dev *dev,
> - struct bss_info *bss_info,
> - struct sk_buff *skb)
> +static void
> +mt7615_mcu_bss_info_omac_header(struct mt7615_vif *mvif, u8 *data,
> + u32 conn_type)
> {
> - struct bss_info_omac tlv = {0};
> -
> - tlv.tag = cpu_to_le16(BSS_INFO_OMAC);
> - tlv.len = cpu_to_le16(sizeof(tlv));
> - tlv.hw_bss_idx = (bss_info->omac_idx > EXT_BSSID_START) ?
> - HW_BSSID_0 : bss_info->omac_idx;
> - tlv.omac_idx = bss_info->omac_idx;
> - tlv.band_idx = bss_info->band_idx;
> - tlv.conn_type = cpu_to_le32(bss_info->conn_type);
> -
> - memcpy(skb_put(skb, sizeof(tlv)), &tlv, sizeof(tlv));
> + struct bss_info_omac *hdr = (struct bss_info_omac *)data;
> + u8 idx;
> +
> + idx = mvif->omac_idx > EXT_BSSID_START ? HW_BSSID_0 : mvif->omac_idx;
> + hdr->tag = cpu_to_le16(BSS_INFO_OMAC);
> + hdr->len = cpu_to_le16(sizeof(struct bss_info_omac));
> + hdr->hw_bss_idx = idx;
> + hdr->omac_idx = mvif->omac_idx;
> + hdr->band_idx = mvif->band_idx;
> + hdr->conn_type = cpu_to_le32(conn_type);
> }
>
> -static void bss_info_basic_handler (struct mt7615_dev *dev,
> - struct bss_info *bss_info,
> - struct sk_buff *skb)
> +static void
> +mt7615_mcu_bss_info_basic_header(struct ieee80211_vif *vif, u8 *data,
> + u32 net_type, u8 tx_wlan_idx,
> + bool enable)
> {
> - struct bss_info_basic tlv = {0};
> -
> - tlv.tag = cpu_to_le16(BSS_INFO_BASIC);
> - tlv.len = cpu_to_le16(sizeof(tlv));
> - tlv.network_type = cpu_to_le32(bss_info->network_type);
> - tlv.active = bss_info->enable;
> - tlv.bcn_interval = cpu_to_le16(bss_info->bcn_interval);
> - memcpy(tlv.bssid, bss_info->bssid, ETH_ALEN);
> - tlv.wmm_idx = bss_info->wmm_idx;
> - tlv.dtim_period = bss_info->dtim_period;
> - tlv.bmc_tx_wlan_idx = bss_info->bmc_tx_wlan_idx;
> -
> - memcpy(skb_put(skb, sizeof(tlv)), &tlv, sizeof(tlv));
> + struct mt7615_vif *mvif = (struct mt7615_vif *)vif->drv_priv;
> + struct bss_info_basic *hdr = (struct bss_info_basic *)data;
> +
> + hdr->tag = cpu_to_le16(BSS_INFO_BASIC);
> + hdr->len = cpu_to_le16(sizeof(struct bss_info_basic));
> + hdr->network_type = cpu_to_le32(net_type);

If I read this patch correctly, it seems nobody sets the net_type?

> + hdr->active = enable;
> + hdr->bcn_interval = cpu_to_le16(vif->bss_conf.beacon_int);
> + memcpy(hdr->bssid, vif->bss_conf.bssid, ETH_ALEN);
> + hdr->wmm_idx = mvif->wmm_idx;
> + hdr->dtim_period = vif->bss_conf.dtim_period;
> + hdr->bmc_tx_wlan_idx = tx_wlan_idx;
> }
>
> -static void bss_info_ext_bss_handler (struct mt7615_dev *dev,
> - struct bss_info *bss_info,
> - struct sk_buff *skb)
> +static void
> +mt7615_mcu_bss_info_ext_header(struct mt7615_vif *mvif, u8 *data)
> {
> /* SIFS 20us + 512 byte beacon tranmitted by 1Mbps (3906us) */
> #define BCN_TX_ESTIMATE_TIME (4096 + 20)
> - struct bss_info_ext_bss tlv = {0};
> + struct bss_info_ext_bss *hdr = (struct bss_info_ext_bss *)data;
> int ext_bss_idx;
>
> - ext_bss_idx = bss_info->omac_idx - EXT_BSSID_START;
> -
> + ext_bss_idx = mvif->omac_idx - EXT_BSSID_START;
> if (ext_bss_idx < 0)
> return;
>
> - tlv.tag = cpu_to_le16(BSS_INFO_EXT_BSS);
> - tlv.len = cpu_to_le16(sizeof(tlv));
> - tlv.mbss_tsf_offset = ext_bss_idx * BCN_TX_ESTIMATE_TIME;
> -
> - memcpy(skb_put(skb, sizeof(tlv)), &tlv, sizeof(tlv));
> + hdr->tag = cpu_to_le16(BSS_INFO_EXT_BSS);
> + hdr->len = cpu_to_le16(sizeof(struct bss_info_ext_bss));
> + hdr->mbss_tsf_offset = ext_bss_idx * BCN_TX_ESTIMATE_TIME;
> }
>
> -static struct bss_info_tag_handler bss_info_tag_handler[] = {
> - {BSS_INFO_OMAC, sizeof(struct bss_info_omac), bss_info_omac_handler},
> - {BSS_INFO_BASIC, sizeof(struct bss_info_basic), bss_info_basic_handler},
> - {BSS_INFO_RF_CH, sizeof(struct bss_info_rf_ch), NULL},
> - {BSS_INFO_PM, 0, NULL},
> - {BSS_INFO_UAPSD, 0, NULL},
> - {BSS_INFO_ROAM_DETECTION, 0, NULL},
> - {BSS_INFO_LQ_RM, 0, NULL},
> - {BSS_INFO_EXT_BSS, sizeof(struct bss_info_ext_bss), bss_info_ext_bss_handler},
> - {BSS_INFO_BMC_INFO, 0, NULL},
> - {BSS_INFO_SYNC_MODE, 0, NULL},
> - {BSS_INFO_RA, 0, NULL},
> - {BSS_INFO_MAX_NUM, 0, NULL},
> -};
> -
> -static int __mt7615_mcu_set_bss_info(struct mt7615_dev *dev,
> - struct bss_info *bss_info)
> +int mt7615_mcu_set_bss_info(struct mt7615_dev *dev,
> + struct ieee80211_vif *vif, int en)
> {
> + struct mt7615_vif *mvif = (struct mt7615_vif *)vif->drv_priv;
> struct req_hdr {
> u8 bss_idx;
> u8 rsv0;
> __le16 tlv_num;
> u8 is_tlv_append;
> u8 rsv1[3];
> - } __packed req_hdr = {0};
> + } __packed;
> + int len = sizeof(struct req_hdr) + sizeof(struct bss_info_basic);
> + int ret, i, features = BIT(BSS_INFO_BASIC), ntlv = 1;
> + u32 conn_type = NETWORK_INFRA, net_type = 0;
> + u8 *buf, *data, tx_wlan_idx = 0;
> + struct req_hdr *hdr;
> struct sk_buff *skb;
> - u16 tlv_num = 0;
> - u32 size = 0;
> - int i;
>
> - for (i = 0; i < BSS_INFO_MAX_NUM; i++)
> - if ((BIT(bss_info_tag_handler[i].tag) & bss_info->feature) &&
> - bss_info_tag_handler[i].handler) {
> - tlv_num++;
> - size += bss_info_tag_handler[i].len;
> + if (en) {
> + len += sizeof(struct bss_info_omac);
> + features |= BIT(BSS_INFO_OMAC);
> + if (mvif->omac_idx > EXT_BSSID_START) {
> + len += sizeof(struct bss_info_ext_bss);
> + features |= BIT(BSS_INFO_EXT_BSS);
> + ntlv++;
> }
> -
> - skb = mt7615_mcu_msg_alloc(NULL, sizeof(req_hdr) + size);
> -
> - req_hdr.bss_idx = bss_info->bss_idx;
> - req_hdr.tlv_num = cpu_to_le16(tlv_num);
> - req_hdr.is_tlv_append = tlv_num ? 1 : 0;
> -
> - memcpy(skb_put(skb, sizeof(req_hdr)), &req_hdr, sizeof(req_hdr));
> -
> - for (i = 0; i < BSS_INFO_MAX_NUM; i++)
> - if ((BIT(bss_info_tag_handler[i].tag) & bss_info->feature) &&
> - bss_info_tag_handler[i].handler)
> - bss_info_tag_handler[i].handler(dev, bss_info, skb);
> -
> - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_BSS_INFO_UPDATE);
> -}
> -
> -int mt7615_mcu_set_bss_info(struct mt7615_dev *dev,
> - struct ieee80211_vif *vif, int en)
> -{
> - struct mt7615_vif *mvif = (struct mt7615_vif *)vif->drv_priv;
> - struct bss_info bss_info = {
> - .bss_idx = mvif->idx,
> - .omac_idx = mvif->omac_idx,
> - .band_idx = mvif->band_idx,
> - .bcn_interval = vif->bss_conf.beacon_int,
> - .dtim_period = vif->bss_conf.dtim_period,
> - .enable = en,
> - .feature = BIT(BSS_INFO_BASIC),
> - .wmm_idx = mvif->wmm_idx,
> - };
> + ntlv++;
> + }
>
> switch (vif->type) {
> case NL80211_IFTYPE_AP:
> - bss_info.bmc_tx_wlan_idx = mvif->sta.wcid.idx;
> - bss_info.network_type = NETWORK_INFRA;
> - bss_info.conn_type = CONNECTION_INFRA_AP;
> + tx_wlan_idx = mvif->sta.wcid.idx;
> + conn_type = CONNECTION_INFRA_AP;
> break;
> case NL80211_IFTYPE_STATION: {
> - /* find the unicast entry for sta mode bmc tx */
> - struct ieee80211_sta *ap_sta;
> + struct ieee80211_sta *sta;
> struct mt7615_sta *msta;
>
> rcu_read_lock();
>
> - ap_sta = ieee80211_find_sta(vif, vif->bss_conf.bssid);
> - if (!ap_sta) {
> + sta = ieee80211_find_sta(vif, vif->bss_conf.bssid);
> + if (!sta) {
> rcu_read_unlock();
> return -EINVAL;
> }
>
> - msta = (struct mt7615_sta *)ap_sta->drv_priv;
> - bss_info.bmc_tx_wlan_idx = msta->wcid.idx;
> - bss_info.network_type = NETWORK_INFRA;
> - bss_info.conn_type = CONNECTION_INFRA_STA;
> + msta = (struct mt7615_sta *)sta->drv_priv;
> + tx_wlan_idx = msta->wcid.idx;
> + conn_type = CONNECTION_INFRA_STA;
>
> rcu_read_unlock();
> break;
> @@ -832,15 +785,52 @@ int mt7615_mcu_set_bss_info(struct mt7615_dev *dev,
> WARN_ON(1);
> break;
> }
> - memcpy(bss_info.bssid, vif->bss_conf.bssid, ETH_ALEN);
>
> - if (en) {
> - bss_info.feature |= BIT(BSS_INFO_OMAC);
> - if (mvif->omac_idx > EXT_BSSID_START)
> - bss_info.feature |= BIT(BSS_INFO_EXT_BSS);
> + buf = kzalloc(len, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + hdr = (struct req_hdr *)buf;
> + hdr->bss_idx = mvif->idx;
> + hdr->tlv_num = cpu_to_le16(ntlv);
> + hdr->is_tlv_append = 1;
> +
> + data = buf + sizeof(*hdr);
> + for (i = 0; i < BSS_INFO_MAX_NUM; i++) {
> + int tag = ffs(features & BIT(i)) - 1;
> +
> + switch (tag) {
> + case BSS_INFO_OMAC:
> + mt7615_mcu_bss_info_omac_header(mvif, data,
> + conn_type);
> + data += sizeof(struct bss_info_omac);
> + break;
> + case BSS_INFO_BASIC:
> + mt7615_mcu_bss_info_basic_header(vif, data, net_type,
> + tx_wlan_idx, en);
> + data += sizeof(struct bss_info_basic);
> + break;
> + case BSS_INFO_EXT_BSS:
> + mt7615_mcu_bss_info_ext_header(mvif, data);
> + data += sizeof(struct bss_info_ext_bss);
> + break;
> + default:
> + break;
> + }
> + }
> +
> + skb = mt7615_mcu_msg_alloc(buf, len);
> + if (!skb) {
> + ret = -ENOMEM;
> + goto out;
> }
>
> - return __mt7615_mcu_set_bss_info(dev, &bss_info);
> + ret = mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_BSS_INFO_UPDATE);
> +
> +out:
> + kfree(buf);
> +
> + return ret;
> }
>
> static int

2019-05-02 08:28:18

> Hi Lorenzo,
>

Hi Roy,

> IMHO, the skb_ret parameter from mt7615_mcu_msg_send is not just used by
> patch semaphore control.
> The other cmds also got their own event response that can be interpret
> accordingly by their own caller, which can provide the status of the cmd
> (success/fail) and other information that might be helpful case by case.

Do you mean the fw does not put the status code in a 'event struct'
at the beginning of the skb data buffer? What are the cmds that do not use
that approach?

Regards,
Lorenzo

> For now, we just assume every cmd works well and do no error handling,
> which is risky.
> If we are to take FW response into consideration in the future, maybe we
> should keep this parameter.
>
>
> Regards,
> Cheng-Hao (Roy) Luo
>
>
> On Thu, May 2, 2019 at 12:08 AM Lorenzo Bianconi <[email protected]> wrote:
>
> > Remove skb_ret parameter from mt7615_mcu_msg_send signature since it is
> > actually used just by mt7615_mcu_patch_sem_ctrl. This is a prelimanry
> > patch to use mt76 common mcu API
> >
> > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > ---
> > .../net/wireless/mediatek/mt76/mt7615/mcu.c | 77 ++++++-------------
> > 1 file changed, 24 insertions(+), 53 deletions(-)
> >
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> > b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> > index b8d928e8949c..4d1d4c0bc2e2 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> > @@ -116,8 +116,7 @@ static int __mt7615_mcu_msg_send(struct mt7615_dev
> > *dev, struct sk_buff *skb,
> > }
> >
> > static int
> > -mt7615_mcu_msg_send(struct mt7615_dev *dev, struct sk_buff *skb,
> > - int cmd, struct sk_buff **skb_ret)
> > +mt7615_mcu_msg_send(struct mt7615_dev *dev, struct sk_buff *skb, int cmd)
> > {
> > unsigned long expires = jiffies + 10 * HZ;
> > struct mt7615_mcu_rxd *rxd;
> > @@ -142,18 +141,11 @@ mt7615_mcu_msg_send(struct mt7615_dev *dev, struct
> > sk_buff *skb,
> > if (seq != rxd->seq)
> > continue;
> >
> > - if (skb_ret) {
> > - int hdr_len = sizeof(*rxd);
> > -
> > - if (!test_bit(MT76_STATE_MCU_RUNNING,
> > - &dev->mt76.state))
> > - hdr_len -= 4;
> > - skb_pull(skb, hdr_len);
> > - *skb_ret = skb;
> > - } else {
> > - dev_kfree_skb(skb);
> > + if (cmd == -MCU_CMD_PATCH_SEM_CONTROL) {
> > + skb_pull(skb, sizeof(*rxd) - 4);
> > + ret = *skb->data;
> > }
> > -
> > + dev_kfree_skb(skb);
> > break;
> > }
> >
> > @@ -177,8 +169,7 @@ static int mt7615_mcu_init_download(struct mt7615_dev
> > *dev, u32 addr,
> > };
> > struct sk_buff *skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> >
> > - return mt7615_mcu_msg_send(dev, skb,
> > -MCU_CMD_TARGET_ADDRESS_LEN_REQ,
> > - NULL);
> > + return mt7615_mcu_msg_send(dev, skb,
> > -MCU_CMD_TARGET_ADDRESS_LEN_REQ);
> > }
> >
> > static int mt7615_mcu_send_firmware(struct mt7615_dev *dev, const void
> > *data,
> > @@ -219,43 +210,26 @@ static int mt7615_mcu_start_firmware(struct
> > mt7615_dev *dev, u32 addr,
> > };
> > struct sk_buff *skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> >
> > - return mt7615_mcu_msg_send(dev, skb, -MCU_CMD_FW_START_REQ, NULL);
> > + return mt7615_mcu_msg_send(dev, skb, -MCU_CMD_FW_START_REQ);
> > }
> >
> > static int mt7615_mcu_restart(struct mt7615_dev *dev)
> > {
> > struct sk_buff *skb = mt7615_mcu_msg_alloc(NULL, 0);
> >
> > - return mt7615_mcu_msg_send(dev, skb, -MCU_CMD_RESTART_DL_REQ,
> > NULL);
> > + return mt7615_mcu_msg_send(dev, skb, -MCU_CMD_RESTART_DL_REQ);
> > }
> >
> > static int mt7615_mcu_patch_sem_ctrl(struct mt7615_dev *dev, bool get)
> > {
> > struct {
> > - __le32 operation;
> > + __le32 op;
> > } req = {
> > - .operation = cpu_to_le32(get ? PATCH_SEM_GET :
> > - PATCH_SEM_RELEASE),
> > + .op = cpu_to_le32(get ? PATCH_SEM_GET : PATCH_SEM_RELEASE),
> > };
> > - struct event {
> > - u8 status;
> > - u8 reserved[3];
> > - } *resp;
> > struct sk_buff *skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> > - struct sk_buff *skb_ret;
> > - int ret;
> >
> > - ret = mt7615_mcu_msg_send(dev, skb, -MCU_CMD_PATCH_SEM_CONTROL,
> > - &skb_ret);
> > - if (ret)
> > - goto out;
> > -
> > - resp = (struct event *)(skb_ret->data);
> > - ret = resp->status;
> > - dev_kfree_skb(skb_ret);
> > -
> > -out:
> > - return ret;
> > + return mt7615_mcu_msg_send(dev, skb, -MCU_CMD_PATCH_SEM_CONTROL);
> > }
> >
> > static int mt7615_mcu_start_patch(struct mt7615_dev *dev)
> > @@ -268,7 +242,7 @@ static int mt7615_mcu_start_patch(struct mt7615_dev
> > *dev)
> > };
> > struct sk_buff *skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> >
> > - return mt7615_mcu_msg_send(dev, skb, -MCU_CMD_PATCH_FINISH_REQ,
> > NULL);
> > + return mt7615_mcu_msg_send(dev, skb, -MCU_CMD_PATCH_FINISH_REQ);
> > }
> >
> > static int mt7615_driver_own(struct mt7615_dev *dev)
> > @@ -554,8 +528,7 @@ int mt7615_mcu_set_eeprom(struct mt7615_dev *dev)
> > for (off = MT_EE_NIC_CONF_0; off < __MT_EE_MAX; off++)
> > data[off - MT_EE_NIC_CONF_0].val = eep[off];
> >
> > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_EFUSE_BUFFER_MODE,
> > - NULL);
> > + return mt7615_mcu_msg_send(dev, skb,
> > MCU_EXT_CMD_EFUSE_BUFFER_MODE);
> > }
> >
> > int mt7615_mcu_init_mac(struct mt7615_dev *dev)
> > @@ -570,7 +543,7 @@ int mt7615_mcu_init_mac(struct mt7615_dev *dev)
> > };
> > struct sk_buff *skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> >
> > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_MAC_INIT_CTRL,
> > NULL);
> > + return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_MAC_INIT_CTRL);
> > }
> >
> > int mt7615_mcu_set_rts_thresh(struct mt7615_dev *dev, u32 val)
> > @@ -589,7 +562,7 @@ int mt7615_mcu_set_rts_thresh(struct mt7615_dev *dev,
> > u32 val)
> > };
> > struct sk_buff *skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> >
> > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_PROTECT_CTRL,
> > NULL);
> > + return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_PROTECT_CTRL);
> > }
> >
> > int mt7615_mcu_set_wmm(struct mt7615_dev *dev, u8 queue,
> > @@ -627,7 +600,7 @@ int mt7615_mcu_set_wmm(struct mt7615_dev *dev, u8
> > queue,
> > }
> >
> > skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_EDCA_UPDATE,
> > NULL);
> > + return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_EDCA_UPDATE);
> > }
> >
> > int mt7615_mcu_ctrl_pm_state(struct mt7615_dev *dev, int enter)
> > @@ -657,7 +630,7 @@ int mt7615_mcu_ctrl_pm_state(struct mt7615_dev *dev,
> > int enter)
> > };
> > struct sk_buff *skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> >
> > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_PM_STATE_CTRL,
> > NULL);
> > + return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_PM_STATE_CTRL);
> > }
> >
> > static int __mt7615_mcu_set_dev_info(struct mt7615_dev *dev,
> > @@ -704,8 +677,7 @@ static int __mt7615_mcu_set_dev_info(struct mt7615_dev
> > *dev,
> >
> > memcpy(skb_push(skb, sizeof(req_hdr)), &req_hdr, sizeof(req_hdr));
> >
> > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_DEV_INFO_UPDATE,
> > - NULL);
> > + return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_DEV_INFO_UPDATE);
> > }
> >
> > int mt7615_mcu_set_dev_info(struct mt7615_dev *dev, struct ieee80211_vif
> > *vif,
> > @@ -830,8 +802,7 @@ static int __mt7615_mcu_set_bss_info(struct mt7615_dev
> > *dev,
> > bss_info_tag_handler[i].handler)
> > bss_info_tag_handler[i].handler(dev, bss_info,
> > skb);
> >
> > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_BSS_INFO_UPDATE,
> > - NULL);
> > + return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_BSS_INFO_UPDATE);
> > }
> >
> > int mt7615_mcu_set_bss_info(struct mt7615_dev *dev,
> > @@ -914,7 +885,7 @@ __mt7615_mcu_set_wtbl(struct mt7615_dev *dev, int
> > wlan_idx,
> > if (buf && buf_len)
> > memcpy(skb_put(skb, buf_len), buf, buf_len);
> >
> > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_WTBL_UPDATE,
> > NULL);
> > + return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_WTBL_UPDATE);
> > }
> >
> > static enum mt7615_cipher_type
> > @@ -1092,7 +1063,7 @@ __mt7615_mcu_set_sta_rec(struct mt7615_dev *dev, int
> > bss_idx,
> > if (buf && buf_len)
> > memcpy(skb_put(skb, buf_len), buf, buf_len);
> >
> > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_STA_REC_UPDATE,
> > NULL);
> > + return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_STA_REC_UPDATE);
> > }
> >
> > int mt7615_mcu_set_sta_rec_bmc(struct mt7615_dev *dev,
> > @@ -1220,7 +1191,7 @@ int mt7615_mcu_set_bcn(struct mt7615_dev *dev,
> > struct ieee80211_vif *vif,
> >
> > skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> >
> > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_BCN_OFFLOAD,
> > NULL);
> > + return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_BCN_OFFLOAD);
> > }
> >
> > int mt7615_mcu_set_channel(struct mt7615_dev *dev)
> > @@ -1285,12 +1256,12 @@ int mt7615_mcu_set_channel(struct mt7615_dev *dev)
> > memset(req.txpower_sku, 0x3f, 49);
> >
> > skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> > - ret = mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_CHANNEL_SWITCH,
> > NULL);
> > + ret = mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_CHANNEL_SWITCH);
> > if (ret)
> > return ret;
> >
> > skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_SET_RX_PATH,
> > NULL);
> > + return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_SET_RX_PATH);
> > }
> >
> > int mt7615_mcu_set_ht_cap(struct mt7615_dev *dev, struct ieee80211_vif
> > *vif,
> > --
> > 2.20.1
> >
> >

Attachments:

(No filename) (10.76 kB)
signature.asc (235.00 B)
Download all attachments

2019-05-03 09:33:44

by Lorenzo Bianconi

[permalink] [raw]

Subject: Re: [RFC 10/17] mt7615: mcu: remove skb_ret from mt7615_mcu_msg_send

> On Thu, May 2, 2019 at 11:31 PM Lorenzo Bianconi <[email protected]> wrote:
>
> > > Hi Lorenzo,
> > >
> >
> > Hi Roy,
> >
> > > IMHO, the skb_ret parameter from mt7615_mcu_msg_send is not just used by
> > > patch semaphore control.
> > > The other cmds also got their own event response that can be interpret
> > > accordingly by their own caller, which can provide the status of the cmd
> > > (success/fail) and other information that might be helpful case by case.
> >
> >
> > Do you mean the fw does not put the status code in a 'event struct'
> > at the beginning of the skb data buffer? What are the cmds that do not use
> > that approach?
> >
> > Regards,
> > Lorenzo
> >
> >
> What is the 'event struct' you're referring to?

I mean the one defined in mt7615_mcu_patch_sem_ctrl:

struct event {
u8 status;
u8 reserved[3];
};

> The common part in the beginning of the skb should be mt7615_mcu_rxd, the
> payload after that varies by cmd.
> Most cmds put status code in the beginning of the payload, then append some
> cmd specific content after it.
> There is also a little difference between ext cmd and non-ext cmd in terms
> of mcu rxd, which is why we do this tricky -4 to get status code out of
> patch sem event.
> skb_pull(skb, sizeof(*rxd) - 4);

I guess in the future we can manage the mcu reply in mt7615_mcu_msg_send() or
even add a new routine for it run by mt7615_mcu_msg_send.
I would avoid to modify __mt76_mcu_send_msg signature since it is used even by
mt76x02 and mt7603 and most of the times we will pass NULL to it.

Regards,
Lorenzo

>
>
> Regards,
> Cheng-Hao (Roy) Luo
>
>
>
> > > For now, we just assume every cmd works well and do no error handling,
> > > which is risky.
> > > If we are to take FW response into consideration in the future, maybe we
> > > should keep this parameter.
> > >
> > >
> > > Regards,
> > > Cheng-Hao (Roy) Luo
> > >
> > >
> > > On Thu, May 2, 2019 at 12:08 AM Lorenzo Bianconi <[email protected]>
> > wrote:
> > >
> > > > Remove skb_ret parameter from mt7615_mcu_msg_send signature since it is
> > > > actually used just by mt7615_mcu_patch_sem_ctrl. This is a prelimanry
> > > > patch to use mt76 common mcu API
> > > >
> > > > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > > > ---
> > > > .../net/wireless/mediatek/mt76/mt7615/mcu.c | 77 ++++++-------------
> > > > 1 file changed, 24 insertions(+), 53 deletions(-)
> > > >
> > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> > > > b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> > > > index b8d928e8949c..4d1d4c0bc2e2 100644
> > > > --- a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> > > > +++ b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
> > > > @@ -116,8 +116,7 @@ static int __mt7615_mcu_msg_send(struct mt7615_dev
> > > > *dev, struct sk_buff *skb,
> > > > }
> > > >
> > > > static int
> > > > -mt7615_mcu_msg_send(struct mt7615_dev *dev, struct sk_buff *skb,
> > > > - int cmd, struct sk_buff **skb_ret)
> > > > +mt7615_mcu_msg_send(struct mt7615_dev *dev, struct sk_buff *skb, int
> > cmd)
> > > > {
> > > > unsigned long expires = jiffies + 10 * HZ;
> > > > struct mt7615_mcu_rxd *rxd;
> > > > @@ -142,18 +141,11 @@ mt7615_mcu_msg_send(struct mt7615_dev *dev,
> > struct
> > > > sk_buff *skb,
> > > > if (seq != rxd->seq)
> > > > continue;
> > > >
> > > > - if (skb_ret) {
> > > > - int hdr_len = sizeof(*rxd);
> > > > -
> > > > - if (!test_bit(MT76_STATE_MCU_RUNNING,
> > > > - &dev->mt76.state))
> > > > - hdr_len -= 4;
> > > > - skb_pull(skb, hdr_len);
> > > > - *skb_ret = skb;
> > > > - } else {
> > > > - dev_kfree_skb(skb);
> > > > + if (cmd == -MCU_CMD_PATCH_SEM_CONTROL) {
> > > > + skb_pull(skb, sizeof(*rxd) - 4);
> > > > + ret = *skb->data;
> > > > }
> > > > -
> > > > + dev_kfree_skb(skb);
> > > > break;
> > > > }
> > > >
> > > > @@ -177,8 +169,7 @@ static int mt7615_mcu_init_download(struct
> > mt7615_dev
> > > > *dev, u32 addr,
> > > > };
> > > > struct sk_buff *skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> > > >
> > > > - return mt7615_mcu_msg_send(dev, skb,
> > > > -MCU_CMD_TARGET_ADDRESS_LEN_REQ,
> > > > - NULL);
> > > > + return mt7615_mcu_msg_send(dev, skb,
> > > > -MCU_CMD_TARGET_ADDRESS_LEN_REQ);
> > > > }
> > > >
> > > > static int mt7615_mcu_send_firmware(struct mt7615_dev *dev, const void
> > > > *data,
> > > > @@ -219,43 +210,26 @@ static int mt7615_mcu_start_firmware(struct
> > > > mt7615_dev *dev, u32 addr,
> > > > };
> > > > struct sk_buff *skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> > > >
> > > > - return mt7615_mcu_msg_send(dev, skb, -MCU_CMD_FW_START_REQ,
> > NULL);
> > > > + return mt7615_mcu_msg_send(dev, skb, -MCU_CMD_FW_START_REQ);
> > > > }
> > > >
> > > > static int mt7615_mcu_restart(struct mt7615_dev *dev)
> > > > {
> > > > struct sk_buff *skb = mt7615_mcu_msg_alloc(NULL, 0);
> > > >
> > > > - return mt7615_mcu_msg_send(dev, skb, -MCU_CMD_RESTART_DL_REQ,
> > > > NULL);
> > > > + return mt7615_mcu_msg_send(dev, skb, -MCU_CMD_RESTART_DL_REQ);
> > > > }
> > > >
> > > > static int mt7615_mcu_patch_sem_ctrl(struct mt7615_dev *dev, bool get)
> > > > {
> > > > struct {
> > > > - __le32 operation;
> > > > + __le32 op;
> > > > } req = {
> > > > - .operation = cpu_to_le32(get ? PATCH_SEM_GET :
> > > > - PATCH_SEM_RELEASE),
> > > > + .op = cpu_to_le32(get ? PATCH_SEM_GET :
> > PATCH_SEM_RELEASE),
> > > > };
> > > > - struct event {
> > > > - u8 status;
> > > > - u8 reserved[3];
> > > > - } *resp;
> > > > struct sk_buff *skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> > > > - struct sk_buff *skb_ret;
> > > > - int ret;
> > > >
> > > > - ret = mt7615_mcu_msg_send(dev, skb, -MCU_CMD_PATCH_SEM_CONTROL,
> > > > - &skb_ret);
> > > > - if (ret)
> > > > - goto out;
> > > > -
> > > > - resp = (struct event *)(skb_ret->data);
> > > > - ret = resp->status;
> > > > - dev_kfree_skb(skb_ret);
> > > > -
> > > > -out:
> > > > - return ret;
> > > > + return mt7615_mcu_msg_send(dev, skb,
> > -MCU_CMD_PATCH_SEM_CONTROL);
> > > > }
> > > >
> > > > static int mt7615_mcu_start_patch(struct mt7615_dev *dev)
> > > > @@ -268,7 +242,7 @@ static int mt7615_mcu_start_patch(struct mt7615_dev
> > > > *dev)
> > > > };
> > > > struct sk_buff *skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> > > >
> > > > - return mt7615_mcu_msg_send(dev, skb, -MCU_CMD_PATCH_FINISH_REQ,
> > > > NULL);
> > > > + return mt7615_mcu_msg_send(dev, skb,
> > -MCU_CMD_PATCH_FINISH_REQ);
> > > > }
> > > >
> > > > static int mt7615_driver_own(struct mt7615_dev *dev)
> > > > @@ -554,8 +528,7 @@ int mt7615_mcu_set_eeprom(struct mt7615_dev *dev)
> > > > for (off = MT_EE_NIC_CONF_0; off < __MT_EE_MAX; off++)
> > > > data[off - MT_EE_NIC_CONF_0].val = eep[off];
> > > >
> > > > - return mt7615_mcu_msg_send(dev, skb,
> > MCU_EXT_CMD_EFUSE_BUFFER_MODE,
> > > > - NULL);
> > > > + return mt7615_mcu_msg_send(dev, skb,
> > > > MCU_EXT_CMD_EFUSE_BUFFER_MODE);
> > > > }
> > > >
> > > > int mt7615_mcu_init_mac(struct mt7615_dev *dev)
> > > > @@ -570,7 +543,7 @@ int mt7615_mcu_init_mac(struct mt7615_dev *dev)
> > > > };
> > > > struct sk_buff *skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> > > >
> > > > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_MAC_INIT_CTRL,
> > > > NULL);
> > > > + return mt7615_mcu_msg_send(dev, skb,
> > MCU_EXT_CMD_MAC_INIT_CTRL);
> > > > }
> > > >
> > > > int mt7615_mcu_set_rts_thresh(struct mt7615_dev *dev, u32 val)
> > > > @@ -589,7 +562,7 @@ int mt7615_mcu_set_rts_thresh(struct mt7615_dev
> > *dev,
> > > > u32 val)
> > > > };
> > > > struct sk_buff *skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> > > >
> > > > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_PROTECT_CTRL,
> > > > NULL);
> > > > + return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_PROTECT_CTRL);
> > > > }
> > > >
> > > > int mt7615_mcu_set_wmm(struct mt7615_dev *dev, u8 queue,
> > > > @@ -627,7 +600,7 @@ int mt7615_mcu_set_wmm(struct mt7615_dev *dev, u8
> > > > queue,
> > > > }
> > > >
> > > > skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> > > > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_EDCA_UPDATE,
> > > > NULL);
> > > > + return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_EDCA_UPDATE);
> > > > }
> > > >
> > > > int mt7615_mcu_ctrl_pm_state(struct mt7615_dev *dev, int enter)
> > > > @@ -657,7 +630,7 @@ int mt7615_mcu_ctrl_pm_state(struct mt7615_dev
> > *dev,
> > > > int enter)
> > > > };
> > > > struct sk_buff *skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> > > >
> > > > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_PM_STATE_CTRL,
> > > > NULL);
> > > > + return mt7615_mcu_msg_send(dev, skb,
> > MCU_EXT_CMD_PM_STATE_CTRL);
> > > > }
> > > >
> > > > static int __mt7615_mcu_set_dev_info(struct mt7615_dev *dev,
> > > > @@ -704,8 +677,7 @@ static int __mt7615_mcu_set_dev_info(struct
> > mt7615_dev
> > > > *dev,
> > > >
> > > > memcpy(skb_push(skb, sizeof(req_hdr)), &req_hdr,
> > sizeof(req_hdr));
> > > >
> > > > - return mt7615_mcu_msg_send(dev, skb,
> > MCU_EXT_CMD_DEV_INFO_UPDATE,
> > > > - NULL);
> > > > + return mt7615_mcu_msg_send(dev, skb,
> > MCU_EXT_CMD_DEV_INFO_UPDATE);
> > > > }
> > > >
> > > > int mt7615_mcu_set_dev_info(struct mt7615_dev *dev, struct
> > ieee80211_vif
> > > > *vif,
> > > > @@ -830,8 +802,7 @@ static int __mt7615_mcu_set_bss_info(struct
> > mt7615_dev
> > > > *dev,
> > > > bss_info_tag_handler[i].handler)
> > > > bss_info_tag_handler[i].handler(dev, bss_info,
> > > > skb);
> > > >
> > > > - return mt7615_mcu_msg_send(dev, skb,
> > MCU_EXT_CMD_BSS_INFO_UPDATE,
> > > > - NULL);
> > > > + return mt7615_mcu_msg_send(dev, skb,
> > MCU_EXT_CMD_BSS_INFO_UPDATE);
> > > > }
> > > >
> > > > int mt7615_mcu_set_bss_info(struct mt7615_dev *dev,
> > > > @@ -914,7 +885,7 @@ __mt7615_mcu_set_wtbl(struct mt7615_dev *dev, int
> > > > wlan_idx,
> > > > if (buf && buf_len)
> > > > memcpy(skb_put(skb, buf_len), buf, buf_len);
> > > >
> > > > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_WTBL_UPDATE,
> > > > NULL);
> > > > + return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_WTBL_UPDATE);
> > > > }
> > > >
> > > > static enum mt7615_cipher_type
> > > > @@ -1092,7 +1063,7 @@ __mt7615_mcu_set_sta_rec(struct mt7615_dev *dev,
> > int
> > > > bss_idx,
> > > > if (buf && buf_len)
> > > > memcpy(skb_put(skb, buf_len), buf, buf_len);
> > > >
> > > > - return mt7615_mcu_msg_send(dev, skb,
> > MCU_EXT_CMD_STA_REC_UPDATE,
> > > > NULL);
> > > > + return mt7615_mcu_msg_send(dev, skb,
> > MCU_EXT_CMD_STA_REC_UPDATE);
> > > > }
> > > >
> > > > int mt7615_mcu_set_sta_rec_bmc(struct mt7615_dev *dev,
> > > > @@ -1220,7 +1191,7 @@ int mt7615_mcu_set_bcn(struct mt7615_dev *dev,
> > > > struct ieee80211_vif *vif,
> > > >
> > > > skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> > > >
> > > > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_BCN_OFFLOAD,
> > > > NULL);
> > > > + return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_BCN_OFFLOAD);
> > > > }
> > > >
> > > > int mt7615_mcu_set_channel(struct mt7615_dev *dev)
> > > > @@ -1285,12 +1256,12 @@ int mt7615_mcu_set_channel(struct mt7615_dev
> > *dev)
> > > > memset(req.txpower_sku, 0x3f, 49);
> > > >
> > > > skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> > > > - ret = mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_CHANNEL_SWITCH,
> > > > NULL);
> > > > + ret = mt7615_mcu_msg_send(dev, skb,
> > MCU_EXT_CMD_CHANNEL_SWITCH);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > skb = mt7615_mcu_msg_alloc(&req, sizeof(req));
> > > > - return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_SET_RX_PATH,
> > > > NULL);
> > > > + return mt7615_mcu_msg_send(dev, skb, MCU_EXT_CMD_SET_RX_PATH);
> > > > }
> > > >
> > > > int mt7615_mcu_set_ht_cap(struct mt7615_dev *dev, struct ieee80211_vif
> > > > *vif,
> > > > --
> > > > 2.20.1
> > > >
> > > >
> >

Attachments:

(No filename) (13.04 kB)
signature.asc (235.00 B)
Download all attachments