Pkshih <pkshih@xxxxxxxxxxx> writes:
> 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.
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
