On 12/12/21 02:02, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> I'd like to propose to restructure the wilc1000 TX path to take
> advantage of the existing sk_buff queuing and buffer operations rather
> than using a driver-specific solution. To me, the resulting code looks
> simpler and the diffstat shows a fair amount of code-reduction:
>
> cfg80211.c | 35 ----
> mon.c | 36 ----
> netdev.c | 28 ---
> netdev.h | 10 -
> wlan.c | 499 ++++++++++++++++++++++++++-----------------------------------
> wlan.h | 51 ++----
> 6 files changed, 255 insertions(+), 404 deletions(-)
>
> Performance isn't significantly affected by this patch:
>
> Before this patch:
> TX [Mbps] RX [Mbps]
> PSM off: 15.4 19.7
> PSM on: 12.2 17.9
>
> With this patch:
> TX [Mbps] RX [Mbps]
> PSM off: 15.9 20.5
> PSM on: 12.3 18.8
>
> The question I have is whether something along these lines would be
> even considered for merging. The problem is that the patch is fairly
> large and I don't see any obvious way of making it smaller or splitting
> it into smaller pieces: once you switch the tx queue data structure,
> there is just a bunch of code that needs to get updated as well in
> order to get a working driver again.
>
> Notes:
>
> - Don't try to apply this patch as is. There are two other small
> but unrelated changes that this patch below depends on.
Btw what are the other two changes required to make it work. I believe
the working flow shouldn't get impacted after the restructuring
especially when code is refactor(replaced) using the existing solution.
> - This patch leaves txq_spinlock in place even though its only
> remaining function is to serialize access to wilc->tx_q_limit
> and vif->ack_filter. This obviously could be renamed in
> a separate patch. Actually, speaking of which, is there
> no common code in the kernel to handle duplicate-ack
> elimination?
>
> Thoughts and/or suggestions?
Using sk_buff queue is a good idea but the patch has other changes also.
This patch should be splited into a few different logical related
changes. Using a single patch for all these changes may not be easy to
review and handle if there are any regressions.
One way to split the changes could be like:
- sk_buff handling for mgmt/data and config frames
- WID(config packets) functions refactoring
- Functions refactoring
- Rename related changes
[snip]
> static inline void ac_update_fw_ac_pkt_info(struct wilc *wl, u32 reg)
> {
> - wl->txq[AC_BK_Q].fw.count = FIELD_GET(BK_AC_COUNT_FIELD, reg);
> - wl->txq[AC_BE_Q].fw.count = FIELD_GET(BE_AC_COUNT_FIELD, reg);
> - wl->txq[AC_VI_Q].fw.count = FIELD_GET(VI_AC_COUNT_FIELD, reg);
> - wl->txq[AC_VO_Q].fw.count = FIELD_GET(VO_AC_COUNT_FIELD, reg);
> -
> - wl->txq[AC_BK_Q].fw.acm = FIELD_GET(BK_AC_ACM_STAT_FIELD, reg);
> - wl->txq[AC_BE_Q].fw.acm = FIELD_GET(BE_AC_ACM_STAT_FIELD, reg);
> - wl->txq[AC_VI_Q].fw.acm = FIELD_GET(VI_AC_ACM_STAT_FIELD, reg);
> - wl->txq[AC_VO_Q].fw.acm = FIELD_GET(VO_AC_ACM_STAT_FIELD, reg);
> + wl->fw[AC_BK_Q].count = FIELD_GET(BK_AC_COUNT_FIELD, reg);
> + wl->fw[AC_BE_Q].count = FIELD_GET(BE_AC_COUNT_FIELD, reg);
> + wl->fw[AC_VI_Q].count = FIELD_GET(VI_AC_COUNT_FIELD, reg);
> + wl->fw[AC_VO_Q].count = FIELD_GET(VO_AC_COUNT_FIELD, reg);
> +
> + wl->fw[AC_BK_Q].acm = FIELD_GET(BK_AC_ACM_STAT_FIELD, reg);
> + wl->fw[AC_BE_Q].acm = FIELD_GET(BE_AC_ACM_STAT_FIELD, reg);
> + wl->fw[AC_VI_Q].acm = FIELD_GET(VI_AC_ACM_STAT_FIELD, reg);
> + wl->fw[AC_VO_Q].acm = FIELD_GET(VO_AC_ACM_STAT_FIELD, reg);
> }
name changes like above can be part of separate patch
<snip>
> (*ac)++;
>
> +static int wilc_wlan_cfg_apply_wid(struct wilc_vif *vif, int start, u16 wid,
> + u8 *buffer, u32 buffer_size, int commit,
> + u32 drv_handler, bool set)
> {
> - u32 offset;
> int ret_size;
> struct wilc *wilc = vif->wilc;
>
> mutex_lock(&wilc->cfg_cmd_lock);
>
> - if (start)
> - wilc->cfg_frame_offset = 0;
> -
> - offset = wilc->cfg_frame_offset;
> - ret_size = wilc_wlan_cfg_set_wid(wilc->cfg_frame.frame, offset,
> - wid, buffer, buffer_size);
> - offset += ret_size;
> - wilc->cfg_frame_offset = offset;
> -
> - if (!commit) {
> - mutex_unlock(&wilc->cfg_cmd_lock);
> - return ret_size;
> + if (start) {
> + WARN_ON(wilc->cfg_skb);
> + wilc->cfg_skb = alloc_cfg_skb(vif);
> + if (!wilc->cfg_skb) {
'cfg_cmd_lock' mutex unlock is missing.
> + netdev_dbg(vif->ndev, "Failed to alloc cfg_skb");
> + return 0;
> + }
> }
>
Regards,
Ajay
