Re: rtw88/rtw89: command/event structure handling

[Ping-Ke Shih <pkshih@realtek.com>]

On Mon, 2023-04-03 at 16:23 +0300, Kalle Valo wrote:
> 
> 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)"

Super appreciate to propose pseudo code and these clear rules to us. :-)
I will record these rules in our internal wiki page.

> > 
> > 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.

Since they will be a lot of works and I have a lot of local patches on
hand, can I apply these rules to the patches and submit them ahead? 
Until all things or a bunch of conversion are completed (maybe weeks or
one or two months later), I can submit patches that only convert
these H2C/C2H with new rules.

Does it work to you?

> 
> 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!

Sorry, I forget to mention this in original discussion thread.
As I mentioned in v4, I will open mind to convert them again
if any suggestion to improve these things in the future. 

Appreciate your suggestion again. 

Ping-Ke