netdev - Re: RFC: wilc1000: refactor TX path to use sk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8bca161b9689bf02f0620e3b59f5040c7e8b2db1.camel@egauge.net>
Date:   Wed, 15 Dec 2021 17:54:27 -0700
From:   David Mosberger-Tang <davidm@...uge.net>
To:     Ajay.Kathat@...rochip.com, Claudiu.Beznea@...rochip.com,
        kvalo@...eaurora.org
Cc:     linux-wireless@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: RFC: wilc1000: refactor TX path to use sk_buff queue

On Wed, 2021-12-15 at 12:44 +0000, Ajay.Kathat@...rochip.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