Re: [dpdk-dev] [PATCH] net/bnxt: Fix bug with duplicate filter pattern for flow director

[Somnath Kotur <somnath.kotur@broadcom.com>]

Hi Ferruh,


On Fri, Mar 23, 2018 at 12:16 AM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 2/22/2018 2:58 AM, Somnath Kotur wrote:
>
> Please start with lowercase after "net/bnxt: fix"
>
> > When user reissues same flow director cmd with a different queue
> > update the existing filter to redirect flow to the new desired
> > queue as destination just like the other filters like 5 tuple and
> > generic flow.
>
> Can you please add a fixes line?
>
Actually, I'm not sure if this qualifies as a 'fix for a regression', it
prescribes a new behavior for this scenario (of same flow-director cmd ,
different queue)  we never handled this before at all , Do you still think
it needs it ?

>
> Also ./devtools/check-git-log.sh complains about long title, what about
> something like (just a sample):
> "net/bnxt: fix flow director with same cmd different queue"
>
> >
> > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
>
> <...>
>
> > @@ -2436,11 +2440,32 @@ bnxt_fdir_filter(struct rte_eth_dev *dev,
> >                       goto free_filter;
> >               filter->filter_type = HWRM_CFA_NTUPLE_FILTER;
> >
> > -             match = bnxt_match_fdir(bp, filter);
> > +             if (fdir->action.behavior == RTE_ETH_FDIR_REJECT)
> > +                     vnic = STAILQ_FIRST(&bp->ff_pool[0]);
> > +             else
> > +                     vnic =
> > +                     STAILQ_FIRST(&bp->ff_pool[fdir->action.rx_queue]);
>
> Is this done because of column limit? If so I would prefer a few extra
> chars
> instead of this assignment.
>
Yes, please thank you , it was going over by 1 character :)

>
> btw, not related to this patch, but in this switch there are a few "/*
> FALLTHROUGH */" comments but they may not be required (or wrong), can you
> please
> check.
>
> Sure , will do.

> <...>
>

Thanks
Som