Re: [patch net-next 16/26] bonding: implement lower state change propagation

Nikolay Aleksandrov
2015-12-02T11:40:53Z

On 12/01/2015 02:48 PM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Let netdev notifier listeners know about link and slave state change.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
> drivers/net/bonding/bond_main.c | 10 ++++++++++
> include/net/bonding.h | 7 +++++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index c9943fc..e153a87 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1315,6 +1315,16 @@ void bond_queue_slave_event(struct slave *slave)
> queue_delayed_work(slave->bond->wq, &nnw->work, 0);
> }
>
> +void bond_lower_state_changed(struct slave *slave)
> +{
> + struct netdev_lag_lower_state_info info;
> +
> + info.link_up = slave->link == BOND_LINK_UP ||
> + slave->link == BOND_LINK_FAIL;
> + info.tx_enabled = bond_is_active_slave(slave);
> + netdev_lower_state_changed(slave->dev, &info);
> +}
> +

Hmm, but does this tell the listeners what changed ? I think it just
sends the current slave state and the listener has to decide what has
changed.
For example, right now it's possible for multiple identical events to
be sent (e.g. set_inactive_flags called two times on release, once because
of curr_active_slave change and second because of your set), the listeners
should be able to cope with that. For this same example I see that the mlxsw
will call mlxsw_sp_port_lag_changed() two times with the same values in that
case. I'm not saying this is necessarily bad, just noting it. :-)