> -----Original Message----- > From: Sergei Shtylyov [mailto:sergei.shtyl...@cogentembedded.com] > Sent: Sunday, February 12, 2017 2:18 AM > To: Kirsher, Jeffrey T <jeffrey.t.kirs...@intel.com>; da...@davemloft.net > Cc: Keller, Jacob E <jacob.e.kel...@intel.com>; 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 <jacob.e.kel...@intel.com> > > > > 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 <jacob.e.kel...@intel.com> > > Tested-by: Andrew Bowers <andrewx.bow...@intel.com> > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirs...@intel.com> > > --- > > 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
