Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes

[Luis R. Rodriguez]

On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
> <mcgrof@xxxxxxxxxxxxxxxx> wrote:
> >                         spin_lock_bh(&p->br->lock);
> >                         err = br_setport(p, tb);
> > +                       changed = br_stp_recalculate_bridge_id(p->br);
> 
> Looks like you only want to check if the mac addr gets changed here,

Nope, the reason why we want a full thorough check is that br_setport()
may change currently any of these:
                                                                                
  * IFLA_BRPORT_MODE
  * IFLA_BRPORT_GUARD
  * IFLA_BRPORT_FAST_LEAVE
  * IFLA_BRPORT_PROTECT
  * IFLA_BRPORT_LEARNING,
  * IFLA_BRPORT_UNICAST_FLOOD
  * IFLA_BRPORT_COST
  * IFLA_BRPORT_PRIORITY
  * IFLA_BRPORT_STATE

That's good reason to trigger a good inspection. Having the MAC address
change would be simply collateral and its just something we need to do
some additional work for outside of the locking context.

> but br_stp_recalculate_bridge_id() does more than just checking it,
> are you sure the side-effects are all what you want here?

Yeap.

> >                         spin_unlock_bh(&p->br->lock);
> > +                       if (changed)
> > +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
> > +                                                        p->br->dev);
> > +                       netdev_update_features(p->br->dev);
> 
> I think this is supposed to be in netdev event handler of br->dev
> instead of here.

Do you mean netdev_update_features() ? I mimic'd what was being done on
br_del_if() given that root blocking is doing something similar. If
we need to change something for the above then I suppose it means we need
to change br_del_if() too. Let me know if you see any reason for something
else.

  Luis

Attachment: pgpZA0RVmuBQL.pgp
Description: PGP signature