| Message ID | 20220107034239.22002-14-pkshih@realtek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtw89: support AP mode | expand |
Ping-Ke Shih <pkshih@realtek.com> writes: > Fill mac_id and self_role depends on the operation mode. > > In AP mode, echo connected station has an unique mac_id, and each vif also > has one mac_id to represent itself. > > The self_role is assigned to vif if the operation mode is decided, and > RTW89_SELF_ROLE_AP_CLIENT is assigned to the connected STA in AP mode, > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> > --- > drivers/net/wireless/realtek/rtw89/fw.c | 8 ++++++-- > drivers/net/wireless/realtek/rtw89/fw.h | 1 + > drivers/net/wireless/realtek/rtw89/mac.c | 4 ++-- > 3 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtw89/fw.c b/drivers/net/wireless/realtek/rtw89/fw.c > index 5209813275676..4641aadea0386 100644 > --- a/drivers/net/wireless/realtek/rtw89/fw.c > +++ b/drivers/net/wireless/realtek/rtw89/fw.c > @@ -993,9 +993,13 @@ int rtw89_fw_h2c_update_beacon(struct rtw89_dev *rtwdev, > #define H2C_ROLE_MAINTAIN_LEN 4 > int rtw89_fw_h2c_role_maintain(struct rtw89_dev *rtwdev, > struct rtw89_vif *rtwvif, > + struct rtw89_sta *rtwsta, > enum rtw89_upd_mode upd_mode) > { > struct sk_buff *skb; > + u8 mac_id = rtwsta ? rtwsta->mac_id : rtwvif->mac_id; > + u8 self_role = rtwvif->net_type == RTW89_NET_TYPE_AP_MODE && rtwsta ? > + RTW89_SELF_ROLE_AP_CLIENT : rtwvif->self_role; It seems you like '?' operator more than I do, and it's ok to use in simple cases. But the latter statement is not really readable, something like this is so much easier to read: if (rtwvif->net_type == RTW89_NET_TYPE_AP_MODE && rtwsta) self_role = RTW89_SELF_ROLE_AP_CLIENT else self_role = rtwvif->self_role; But should there a parenthesis around the == operator? I cannot now recall what's the preference in the kernel, can someone help on that? Maybe I also move check for rtwsta first?
Ping-Ke Shih <pkshih@realtek.com> wrote: > Fill mac_id and self_role depends on the operation mode. > > In AP mode, echo connected station has an unique mac_id, and each vif also > has one mac_id to represent itself. > > The self_role is assigned to vif if the operation mode is decided, and > RTW89_SELF_ROLE_AP_CLIENT is assigned to the connected STA in AP mode, > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> Dropping rest of the patches due to my comment. 7 patches set to Changes Requested. 12706170 [13/19] rtw89: extend role_maintain to support AP mode 12706171 [14/19] rtw89: add addr_cam field to sta to support AP mode 12706172 [15/19] rtw89: only STA mode change vif_type mapping dynamically 12706173 [16/19] rtw89: maintain assoc/disassoc STA states of firmware and hardware 12706174 [17/19] rtw89: implement ieee80211_ops::start_ap and stop_ap 12706175 [18/19] rtw89: debug: add stations entry to show ID assignment 12706176 [19/19] rtw89: declare AP mode support
On Fri, 2022-01-28 at 17:51 +0200, Kalle Valo wrote: > Ping-Ke Shih <pkshih@realtek.com> writes: > > > Fill mac_id and self_role depends on the operation mode. > > > > In AP mode, echo connected station has an unique mac_id, and each vif also > > has one mac_id to represent itself. > > > > The self_role is assigned to vif if the operation mode is decided, and > > RTW89_SELF_ROLE_AP_CLIENT is assigned to the connected STA in AP mode, > > > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> > > --- > > drivers/net/wireless/realtek/rtw89/fw.c | 8 ++++++-- > > drivers/net/wireless/realtek/rtw89/fw.h | 1 + > > drivers/net/wireless/realtek/rtw89/mac.c | 4 ++-- > > 3 files changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/wireless/realtek/rtw89/fw.c b/drivers/net/wireless/realtek/rtw89/fw.c > > index 5209813275676..4641aadea0386 100644 > > --- a/drivers/net/wireless/realtek/rtw89/fw.c > > +++ b/drivers/net/wireless/realtek/rtw89/fw.c > > @@ -993,9 +993,13 @@ int rtw89_fw_h2c_update_beacon(struct rtw89_dev *rtwdev, > > #define H2C_ROLE_MAINTAIN_LEN 4 > > int rtw89_fw_h2c_role_maintain(struct rtw89_dev *rtwdev, > > struct rtw89_vif *rtwvif, > > + struct rtw89_sta *rtwsta, > > enum rtw89_upd_mode upd_mode) > > { > > struct sk_buff *skb; > > + u8 mac_id = rtwsta ? rtwsta->mac_id : rtwvif->mac_id; > > + u8 self_role = rtwvif->net_type == RTW89_NET_TYPE_AP_MODE && rtwsta ? > > + RTW89_SELF_ROLE_AP_CLIENT : rtwvif->self_role; > > It seems you like '?' operator more than I do, and it's ok to use in > simple cases. But the latter statement is not really readable, something > like this is so much easier to read: > > if (rtwvif->net_type == RTW89_NET_TYPE_AP_MODE && rtwsta) > self_role = RTW89_SELF_ROLE_AP_CLIENT > else > self_role = rtwvif->self_role; > I use '?' to make code shorter, but your sugeestion would be eaiser to reviewer. I will send v2 after the Lunar New Year. > But should there a parenthesis around the == operator? I cannot now > recall what's the preference in the kernel, can someone help on that? The checkpatch will warn this if we add parenthesis, so preence is not to use parenthesis. CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'rtwvif->net_type == RTW89_NET_TYPE_AP_MODE' #9: FILE: fw.c:1004: + if ((rtwvif->net_type == RTW89_NET_TYPE_AP_MODE) && rtwsta) > > Maybe I also move check for rtwsta first? > The full logic is if (rtwvif->net_type == RTW89_NET_TYPE_AP_MODE) { if (rtwsta) self_role = RTW89_SELF_ROLE_AP_CLIENT else self_role = rtwvif->self_role; } else { self_role = rtwvif->self_role; } And, the meaning of 'rtwsta' here is to indicate we are going to setup a connected station that connects to this AP, but not check if the pointer is NULL. To emphasis the case is only existing in AP_MODE, I prefer to check net_type ahead. Or, this full logic is preferred? Ping-Ke
Pkshih <pkshih@realtek.com> writes: > On Fri, 2022-01-28 at 17:51 +0200, Kalle Valo wrote: >> Ping-Ke Shih <pkshih@realtek.com> writes: >> >> > Fill mac_id and self_role depends on the operation mode. >> > >> > In AP mode, echo connected station has an unique mac_id, and each vif also >> > has one mac_id to represent itself. >> > >> > The self_role is assigned to vif if the operation mode is decided, and >> > RTW89_SELF_ROLE_AP_CLIENT is assigned to the connected STA in AP mode, >> > >> > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> >> > --- >> > drivers/net/wireless/realtek/rtw89/fw.c | 8 ++++++-- >> > drivers/net/wireless/realtek/rtw89/fw.h | 1 + >> > drivers/net/wireless/realtek/rtw89/mac.c | 4 ++-- >> > 3 files changed, 9 insertions(+), 4 deletions(-) >> > >> > diff --git a/drivers/net/wireless/realtek/rtw89/fw.c b/drivers/net/wireless/realtek/rtw89/fw.c >> > index 5209813275676..4641aadea0386 100644 >> > --- a/drivers/net/wireless/realtek/rtw89/fw.c >> > +++ b/drivers/net/wireless/realtek/rtw89/fw.c >> > @@ -993,9 +993,13 @@ int rtw89_fw_h2c_update_beacon(struct rtw89_dev *rtwdev, >> > #define H2C_ROLE_MAINTAIN_LEN 4 >> > int rtw89_fw_h2c_role_maintain(struct rtw89_dev *rtwdev, >> > struct rtw89_vif *rtwvif, >> > + struct rtw89_sta *rtwsta, >> > enum rtw89_upd_mode upd_mode) >> > { >> > struct sk_buff *skb; >> > + u8 mac_id = rtwsta ? rtwsta->mac_id : rtwvif->mac_id; >> > + u8 self_role = rtwvif->net_type == RTW89_NET_TYPE_AP_MODE && rtwsta ? >> > + RTW89_SELF_ROLE_AP_CLIENT : rtwvif->self_role; >> >> It seems you like '?' operator more than I do, and it's ok to use in >> simple cases. But the latter statement is not really readable, something >> like this is so much easier to read: >> >> if (rtwvif->net_type == RTW89_NET_TYPE_AP_MODE && rtwsta) >> self_role = RTW89_SELF_ROLE_AP_CLIENT >> else >> self_role = rtwvif->self_role; >> > > I use '?' to make code shorter, but your sugeestion would be eaiser to reviewer. > I will send v2 after the Lunar New Year. Happy New Year :) >> But should there a parenthesis around the == operator? I cannot now >> recall what's the preference in the kernel, can someone help on that? > > The checkpatch will warn this if we add parenthesis, so preence is not to > use parenthesis. > > CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'rtwvif->net_type == > RTW89_NET_TYPE_AP_MODE' > #9: FILE: fw.c:1004: > + if ((rtwvif->net_type == RTW89_NET_TYPE_AP_MODE) && rtwsta) Ok, I need to remember that :) >> Maybe I also move check for rtwsta first? >> > > The full logic is > > if (rtwvif->net_type == RTW89_NET_TYPE_AP_MODE) { > if (rtwsta) > self_role = RTW89_SELF_ROLE_AP_CLIENT > else > self_role = rtwvif->self_role; > } else { > self_role = rtwvif->self_role; > } > > And, the meaning of 'rtwsta' here is to indicate we are going to setup > a connected station that connects to this AP, but not check if the > pointer is NULL. To emphasis the case is only existing in AP_MODE, > I prefer to check net_type ahead. Or, this full logic is preferred? I don't know what others think, but I find this full logic style most readable.
On Thu, 2022-02-03 at 10:42 +0200, Kalle Valo wrote: > Pkshih <pkshih@realtek.com> writes: > > > On Fri, 2022-01-28 at 17:51 +0200, Kalle Valo wrote: > > > > > > Maybe I also move check for rtwsta first? > > > > > > > The full logic is > > > > if (rtwvif->net_type == RTW89_NET_TYPE_AP_MODE) { > > if (rtwsta) > > self_role = RTW89_SELF_ROLE_AP_CLIENT > > else > > self_role = rtwvif->self_role; > > } else { > > self_role = rtwvif->self_role; > > } > > > > And, the meaning of 'rtwsta' here is to indicate we are going to setup > > a connected station that connects to this AP, but not check if the > > pointer is NULL. To emphasis the case is only existing in AP_MODE, > > I prefer to check net_type ahead. Or, this full logic is preferred? > > I don't know what others think, but I find this full logic style most > readable. > Maybe, a static analysis tool warns two else branches are the same. If so, we can make a decision at that time. I will send v2 next week, and wait for people who have other ideas. Happy New Year. -- Ping-Ke
diff --git a/drivers/net/wireless/realtek/rtw89/fw.c b/drivers/net/wireless/realtek/rtw89/fw.c index 5209813275676..4641aadea0386 100644 --- a/drivers/net/wireless/realtek/rtw89/fw.c +++ b/drivers/net/wireless/realtek/rtw89/fw.c @@ -993,9 +993,13 @@ int rtw89_fw_h2c_update_beacon(struct rtw89_dev *rtwdev, #define H2C_ROLE_MAINTAIN_LEN 4 int rtw89_fw_h2c_role_maintain(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif, + struct rtw89_sta *rtwsta, enum rtw89_upd_mode upd_mode) { struct sk_buff *skb; + u8 mac_id = rtwsta ? rtwsta->mac_id : rtwvif->mac_id; + u8 self_role = rtwvif->net_type == RTW89_NET_TYPE_AP_MODE && rtwsta ? + RTW89_SELF_ROLE_AP_CLIENT : rtwvif->self_role; skb = rtw89_fw_h2c_alloc_skb_with_hdr(H2C_ROLE_MAINTAIN_LEN); if (!skb) { @@ -1003,8 +1007,8 @@ int rtw89_fw_h2c_role_maintain(struct rtw89_dev *rtwdev, return -ENOMEM; } skb_put(skb, H2C_ROLE_MAINTAIN_LEN); - SET_FWROLE_MAINTAIN_MACID(skb->data, rtwvif->mac_id); - SET_FWROLE_MAINTAIN_SELF_ROLE(skb->data, rtwvif->self_role); + SET_FWROLE_MAINTAIN_MACID(skb->data, mac_id); + SET_FWROLE_MAINTAIN_SELF_ROLE(skb->data, self_role); SET_FWROLE_MAINTAIN_UPD_MODE(skb->data, upd_mode); SET_FWROLE_MAINTAIN_WIFI_ROLE(skb->data, rtwvif->wifi_role); diff --git a/drivers/net/wireless/realtek/rtw89/fw.h b/drivers/net/wireless/realtek/rtw89/fw.h index b30cf0a2cc1e0..83f4eaaf90f3b 100644 --- a/drivers/net/wireless/realtek/rtw89/fw.h +++ b/drivers/net/wireless/realtek/rtw89/fw.h @@ -1886,6 +1886,7 @@ void rtw89_fw_c2h_irqsafe(struct rtw89_dev *rtwdev, struct sk_buff *c2h); void rtw89_fw_c2h_work(struct work_struct *work); int rtw89_fw_h2c_role_maintain(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif, + struct rtw89_sta *rtwsta, enum rtw89_upd_mode upd_mode); int rtw89_fw_h2c_join_info(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif, struct rtw89_sta *rtwsta, bool dis_conn); diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c index 58aa24e71637d..19eb8ea1ba915 100644 --- a/drivers/net/wireless/realtek/rtw89/mac.c +++ b/drivers/net/wireless/realtek/rtw89/mac.c @@ -3012,7 +3012,7 @@ int rtw89_mac_vif_init(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif) if (ret) return ret; - ret = rtw89_fw_h2c_role_maintain(rtwdev, rtwvif, RTW89_ROLE_CREATE); + ret = rtw89_fw_h2c_role_maintain(rtwdev, rtwvif, NULL, RTW89_ROLE_CREATE); if (ret) return ret; @@ -3035,7 +3035,7 @@ int rtw89_mac_vif_deinit(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif) { int ret; - ret = rtw89_fw_h2c_role_maintain(rtwdev, rtwvif, RTW89_ROLE_REMOVE); + ret = rtw89_fw_h2c_role_maintain(rtwdev, rtwvif, NULL, RTW89_ROLE_REMOVE); if (ret) return ret;
Fill mac_id and self_role depends on the operation mode. In AP mode, echo connected station has an unique mac_id, and each vif also has one mac_id to represent itself. The self_role is assigned to vif if the operation mode is decided, and RTW89_SELF_ROLE_AP_CLIENT is assigned to the connected STA in AP mode, Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> --- drivers/net/wireless/realtek/rtw89/fw.c | 8 ++++++-- drivers/net/wireless/realtek/rtw89/fw.h | 1 + drivers/net/wireless/realtek/rtw89/mac.c | 4 ++-- 3 files changed, 9 insertions(+), 4 deletions(-)