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