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

[Kalle Valo <kvalo@kernel.org>]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches