Re: RFC: wilc1000: refactor TX path to use sk_buff queue

[<Ajay.Kathat@microchip.com>]

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