Message-ID: <8bca161b9689bf02f0620e3b59f5040c7e8b2db1.camel@egauge.net>
Subject: Re: RFC: wilc1000: refactor TX path to use sk_buff queue
From:   David Mosberger-Tang <davidm@egauge.net>
To:     Ajay.Kathat@microchip.com, Claudiu.Beznea@microchip.com,
        kvalo@codeaurora.org
Cc:     linux-wireless@vger.kernel.org, netdev@vger.kernel.org
Date:   Wed, 15 Dec 2021 17:54:27 -0700
In-Reply-To: <7a6956db-8710-4d6c-9bed-702ec1b7041e@microchip.com>
References: <e3502ecffe0c4c01b263ada8deed814d5135c24c.camel@egauge.net>
         <7a6956db-8710-4d6c-9bed-702ec1b7041e@microchip.com>
Organization: eGauge Systems LLC
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 2021-12-15 at 12:44 +0000, Ajay.Kathat@microchip.com wrote:
> On 12/12/21 02:02, David Mosberger-Tang wrote:
> > 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.

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