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

On Wed, 2021-12-15 at 12:44 +0000, Ajay.Kathat@microchip.com wrote:

On 12/12/21 02:02, David Mosberger-Tang wrote:

quoted

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.

I'll start working on a patch series.  There are some before and some
after the sk_buff refactor, but those are all pretty straight-forward
in comparison.

quoted

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

Yeah, the WID refactoring and structure renaming can be split off
easily, good suggestion.  Let me start working on that.

<snip>

quoted

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

Indeed, thanks for catching that!

  --david