On Fri, 2022-01-28 at 17:51 +0200, Kalle Valo wrote:
> Ping-Ke Shih <pkshih@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > ---
> > 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
