Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Kalle Valo <kvalo@kernel.org>
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     Bernie Huang <phhuang@realtek.com>,
        "linux-wireless\@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        Arnd Bergmann <arnd@arndb.de>
Subject: Re: rtw88/rtw89: command/event structure handling
References: <20230310034631.45299-1-pkshih@realtek.com>
        <20230310034631.45299-2-pkshih@realtek.com>
        <87zg8emn4i.fsf@kernel.org>
        <e3670d1075f54c69ba3971067b3d06b7@realtek.com>
        <87a5zpb71j.fsf_-_@kernel.org>
Date:   Mon, 03 Apr 2023 16:23:10 +0300
In-Reply-To: <87a5zpb71j.fsf_-_@kernel.org> (Kalle Valo's message of "Mon, 03
        Apr 2023 13:21:12 +0300")
Message-ID: <871ql1aym9.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Kalle Valo <kvalo@kernel.org> writes:

> (changing the subject and adding Arnd)
>
> Ping-Ke Shih <pkshih@realtek.com> writes:
>
>>> > @@ -3181,6 +3204,15 @@ static inline struct rtw89_fw_c2h_attr *RTW89_SKB_C2H_CB(struct sk_buff *skb)
>>> >  #define RTW89_GET_MAC_C2H_REV_ACK_H2C_SEQ(c2h) \
>>> >       le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(23, 16))
>>> >
>>> > +#define RTW89_GET_MAC_BCNFLTR_RPT_MACID(c2h) \
>>> > +     le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(7, 0))
>>> > +#define RTW89_GET_MAC_BCNFLTR_RPT_TYPE(c2h) \
>>> > +     le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(9, 8))
>>> > +#define RTW89_GET_MAC_BCNFLTR_RPT_EVENT(c2h) \
>>> > +     le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(11, 10))
>>> > +#define RTW89_GET_MAC_BCNFLTR_RPT_MA(c2h) \
>>> > +     le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(23, 16))
>>> 
>>> I have to admit that I every time I see this code pattern it makes me
>>> regret it. Something like what Arnd proposed back in the day would look
>>> so much cleaner:
>>> 
>>> https://lore.kernel.org/all/CAK8P3a1rsKZZKMKFTDWgE3usX9gYKJqUvTMxSdEuZrp8BaKdaA@mail.gmail.com/
>>> 
>>> Of course this is just a generic comment about rtw89, and has nothing to
>>> do with this patchset, but it would be great if someone could take a
>>> look and try out Arnd's proposal. It would be good to start with just
>>> one or two commands and send that as an RFC to see how it looks like.
>>> 
>>
>> I write a draft RFC here. Please see if it's in expectation. If so, I can
>> change all of them by another patch or RFC.
>>
>> In header file:
>>
>> #define RTW89_C2H_MAC_BCNFLTR_RPT_W2_MACID_MASK GENMASK(7, 0)
>> #define RTW89_C2H_MAC_BCNFLTR_RPT_W2_TYPE_MASK GENMASK(9, 8)
>> #define RTW89_C2H_MAC_BCNFLTR_RPT_W2_EVENT_MASK GENMASK(11, 10)
>> #define RTW89_C2H_MAC_BCNFLTR_RPT_W2_MA_MASK GENMASK(23, 16)
>>
>>
>> Access the values via le32_get_bits() in functions somewhere:
>>
>> 	const __le32 *c2h = skb->data;
>>
>> 	type =   le32_get_bits(c2h[2], RTW89_C2H_MAC_BCNFLTR_RPT_W2_MACID_MASK);
>> 	sig = le32_get_bits(c2h[2],
>> RTW89_C2H_MAC_BCNFLTR_RPT_W2_MA_MASK) - MAX_RSSI;
>> 	event =  le32_get_bits(c2h[2], RTW89_C2H_MAC_BCNFLTR_RPT_W2_EVENT_MASK);
>> 	mac_id = le32_get_bits(c2h[2], RTW89_C2H_MAC_BCNFLTR_RPT_W2_MACID_MASK);
>
> I was thinking more something towards Arnd's idea he suggests in [1].
> Here's my proposal for the beacon filter command as pseudo code (so not
> compiled and very much buggy!) from the patch[2] which started this
> recent discussion.
>
> So in the header file we would have something like this:
>
> #define RTW89_C2H_BEACON_FILTER_WORD0_MACID_MASK GENMASK(7, 0)
> #define RTW89_C2H_BEACON_FILTER_WORD0_TYPE_MASK GENMASK(9, 8)
> #define RTW89_C2H_BEACON_FILTER_WORD0_EVENT_MASK GENMASK(11, 10)
> #define RTW89_C2H_BEACON_FILTER_WORD0_MA_MASK GENMASK(23, 16)
>
> struct rtw89_h2c_cfg_beacon_filter {
>      __le32 word0;
> }
>
> static inline void rtw89_h2c_cfg_beacon_filter_set_word0(struct rtw89_h2c_cfg_beacon_filter *cmd,
>         u32 macid, u32 type, u32 event_mask, u32 ma)
>
> {
>         le32_replace_bits(cmd->word0, macid, RTW89_C2H_BEACON_FILTER_WORD0_MACID_MASK);
>         le32_replace_bits(cmd->word0, type, RTW89_C2H_BEACON_FILTER_WORD0_TYPE_MASK);
>         le32_replace_bits(cmd->word0, event, RTW89_C2H_BEACON_FILTER_WORD0_EVENT_MASK);
>         le32_replace_bits(cmd->word0, ma, RTW89_C2H_BEACON_FILTER_WORD0_MA_MASK);
> }
>
> static inline u32 rtw89_h2c_cfg_beacon_filter_get_mac_id(const struct rtw89_h2c_cfg_beacon_filter *cmd)
> {
>         return le32_get_bits(cmd->word0, RTW89_C2H_BEACON_FILTER_WORD0_MACID_MASK);
> }
>
> And an example how to use these:
>
> struct rtw89_h2c_cfg_beacon_filter *cmd;
>
> skb = rtw89_fw_h2c_alloc_skb_with_hdr(rtwdev, sizeof(*cmd));
> cmd = (struct rtw89_h2c_cfg_beacon_filter *)skb->data;
> rtw89_h2c_cfg_beacon_filter_set_word0(cmd, 1, 2, 0, 0);
>
> I'm sure this is very buggy and I'm missing a lot but I hope you get the
> idea anyway. My keypoints here are:
>
> * there's a clear struct for the command (an "object" from OOP point of
>   view), something like "__le32 *c2h" is very confusing
> * no casting
> * no pointer arithmetic
> * you get length with a simple "sizeof(*cmd)"
>
> Downside of course is that there's quite a lot of boilerplate code but I
> still consider that positives outweight the negatives. Thoughts?
>
> And I'll emphasise that this is not a blocker for anything but it would
> be nice to clean this up both in rtw88 and rtw89 at some point, if we
> can.

Heh, I didn't notice that Ping had done almost the same in v4:

https://patchwork.kernel.org/project/linux-wireless/patch/20230320124125.15873-2-pkshih@realtek.com/

The only difference I notice that you didn't use special functions for
setting or getting the fields:

	h2c->w0 = le32_encode_bits(connect, RTW89_H2C_BCNFLTR_W0_MON_RSSI) |
		  le32_encode_bits(connect, RTW89_H2C_BCNFLTR_W0_MON_BCN) |
		  le32_encode_bits(connect, RTW89_H2C_BCNFLTR_W0_MON_EN) |
		  le32_encode_bits(RTW89_BCN_FLTR_OFFLOAD_MODE_DEFAULT,
				   RTW89_H2C_BCNFLTR_W0_MODE) |
		  le32_encode_bits(RTW89_BCN_LOSS_CNT, RTW89_H2C_BCNFLTR_W0_BCN_LOSS_CNT) |
		  le32_encode_bits(bss_conf->cqm_rssi_hyst, RTW89_H2C_BCNFLTR_W0_RSSI_HYST) |
		  le32_encode_bits(bss_conf->cqm_rssi_thold + MAX_RSSI,
				   RTW89_H2C_BCNFLTR_W0_RSSI_THRESHOLD) |
		  le32_encode_bits(rtwvif->mac_id, RTW89_H2C_BCNFLTR_W0_MAC_ID);

And I understand why you did it like this, less boilerplate code. So
looks good to me, thanks!

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

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