Kalle Valo <kvalo@xxxxxxxxxx> writes: > (changing the subject and adding Arnd) > > Ping-Ke Shih <pkshih@xxxxxxxxxxx> 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@xxxxxxxxxxxxxx/ >>> >>> 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@xxxxxxxxxxx/ 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
