> -Original Message-
> From: Sergei Shtylyov [mailto:sergei.shtyl...@cogentembedded.com]
> Sent: Sunday, February 12, 2017 2:18 AM
> To: Kirsher, Jeffrey T ; da...@davemloft.net
> Cc: Keller, Jacob E ; netdev@vger.kernel.org;
> nhor...@redhat.com; sassm...@redhat.com; jogre...@redhat.com
> Subject: Re: [net-next 12/14] i40e: allow i40e_update_filter_state to skip
> broadcast filters
>
> Hello!
>
> On 2/12/2017 8:30 AM, Jeff Kirsher wrote:
>
> > From: Jacob Keller
> >
> > Fix a bug where we modified the mac_filter_hash while outside a lock,
> > when handling addition of broadcast filters.
> >
> > Normally, we add filters to firmware by batching the additions into
> > lists and issuing 1 update for every few filters. Broadcast filters are
> > handled differently, by instead setting the broadcast promiscuous mode
> > flags. In order to make sure the 1<->1 mapping of filters in our
> > addition array lined up with filters in the hlist tmp_add_list, we had
> > to remove the filter and move it back to the main hash. However, we
> > didn't do this under lock, which could cause consistency problems for
> > the list.
> >
> > Fix this by updating i40e_update_filter_state logic so that it knows to
> > avoid broadcast filters. This ensures that we don't have to remove the
> > filter separately, and can put it back using the normal flow.
> >
> > Change-ID: Id288fade80b3e3a9a54b68cc249188cb95147518
> > Signed-off-by: Jacob Keller
> > Tested-by: Andrew Bowers
> > Signed-off-by: Jeff Kirsher
> > ---
> > drivers/net/ethernet/intel/i40e/i40e_main.c | 37
> ++---
> > 1 file changed, 29 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > index fa4a04d..06c80d4 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > @@ -1843,6 +1843,31 @@ static void i40e_undo_filter_entries(struct i40e_vsi
> *vsi,
> > }
> >
> > /**
> > + * i40e_next_entry - Get the next non-broadcast filter from a list
> > + * @f: pointer to filter in list
> > + *
> > + * Returns the next non-broadcast filter in the list. Required so that we
> > + * ignore broadcast filters within the list, since these are not handled
> > via
> > + * the normal firmware update path.
> > + */
> > +static struct i40e_mac_filter *i40e_next_filter(struct i40e_mac_filter *f)
> > +{
> > + while (f) {
> > + f = hlist_entry(f->hlist.next,
> > + typeof(struct i40e_mac_filter),
> > + hlist);
> > +
> > + /* keep going if we found a broadcast filter */
> > + if (f && is_broadcast_ether_addr(f->macaddr))
> > + continue;
> > +
> > + break;
>
> Isn't it simpler to *break* on an inverted condition above?
> This way, *continue* isn;'t needed...
>
When I wrote the code originally it seemed better, but now that I think about
it, the inverted conditional isn't that much more complicated, so I'll change
it.
Thanks,
Jake
> [...]
>
> MBR, Sergei