diff mbox series

[13/19] rtw89: extend role_maintain to support AP mode

Message ID 20220107034239.22002-14-pkshih@realtek.com
State Superseded
Headers show
Series rtw89: support AP mode | expand

Commit Message

Ping-Ke Shih Jan. 7, 2022, 3:42 a.m. UTC
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(-)

Comments

Kalle Valo Jan. 28, 2022, 3:51 p.m. UTC | #1
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?
Kalle Valo Jan. 28, 2022, 3:53 p.m. UTC | #2
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
Ping-Ke Shih Jan. 29, 2022, 3:36 a.m. UTC | #3
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
Kalle Valo Feb. 3, 2022, 8:42 a.m. UTC | #4
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.
Ping-Ke Shih Feb. 3, 2022, 9:41 a.m. UTC | #5
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 mbox series

Patch

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;