| Message ID | 20230314111426.1254998-2-razor@blackwall.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bonding: properly restore flags when bond changes ether type | expand |
On Tue, Mar 14, 2023 at 01:14:23PM +0200, Nikolay Aleksandrov wrote: > Add bond_ether_setup helper which will be used in the following patches > to fix all ether_setup() calls in the bonding driver. It takes care of both > IFF_MASTER and IFF_SLAVE flags, the former is always restored and the > latter only if it was set. > > Fixes: e36b9d16c6a6d ("bonding: clean muticast addresses when device changes type") > Fixes: 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure") > Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> > --- > drivers/net/bonding/bond_main.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 00646aa315c3..d41024ad2c18 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -1775,6 +1775,18 @@ void bond_lower_state_changed(struct slave *slave) > slave_err(bond_dev, slave_dev, "Error: %s\n", errmsg); \ > } while (0) > > +/* ether_setup() resets bond_dev's flags so we always have to restore > + * IFF_MASTER, and only restore IFF_SLAVE if it was set > + */ I would suggest using the kernel pattern for function documentation. At first glance, the name "ether_setup" at the beginning is easy to be confused with the function name (bond_ether_setup). > +static void bond_ether_setup(struct net_device *bond_dev) > +{ > + unsigned int slave_flag = bond_dev->flags & IFF_SLAVE; > + > + ether_setup(bond_dev); > + bond_dev->flags |= IFF_MASTER | slave_flag; > + bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING; > +} > + > /* enslave device <slave> to bond device <master> */ > int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > struct netlink_ext_ack *extack) It seems you never call this newly added helper in the current patch. I think it creates a compilation warning ("defined but not used"). Please add your function in the patch where you actually use it. > -- > 2.39.2 > Thanks, Michal
On 14/03/2023 16:58, Michal Kubiak wrote: > On Tue, Mar 14, 2023 at 01:14:23PM +0200, Nikolay Aleksandrov wrote: >> Add bond_ether_setup helper which will be used in the following patches >> to fix all ether_setup() calls in the bonding driver. It takes care of both >> IFF_MASTER and IFF_SLAVE flags, the former is always restored and the >> latter only if it was set. >> >> Fixes: e36b9d16c6a6d ("bonding: clean muticast addresses when device changes type") >> Fixes: 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure") >> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> >> --- >> drivers/net/bonding/bond_main.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 00646aa315c3..d41024ad2c18 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -1775,6 +1775,18 @@ void bond_lower_state_changed(struct slave *slave) >> slave_err(bond_dev, slave_dev, "Error: %s\n", errmsg); \ >> } while (0) >> >> +/* ether_setup() resets bond_dev's flags so we always have to restore >> + * IFF_MASTER, and only restore IFF_SLAVE if it was set >> + */ > > I would suggest using the kernel pattern for function documentation. > At first glance, the name "ether_setup" at the beginning is easy to be > confused with the function name (bond_ether_setup). > This is an internal helper, I don't think it needs a full kernel doc. >> +static void bond_ether_setup(struct net_device *bond_dev) >> +{ >> + unsigned int slave_flag = bond_dev->flags & IFF_SLAVE; >> + >> + ether_setup(bond_dev); >> + bond_dev->flags |= IFF_MASTER | slave_flag; >> + bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING; >> +} >> + >> /* enslave device <slave> to bond device <master> */ >> int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, >> struct netlink_ext_ack *extack) > > It seems you never call this newly added helper in the current patch. I > think it creates a compilation warning ("defined but not used"). > Please add your function in the patch where you actually use it. > I'm adding the helper in a separate patch to emphasize it and focus the review. I have written in the commit message that the next two fixes will be using it. IMO, this should be ok. >> -- >> 2.39.2 >> > > > Thanks, > Michal Cheers, Nik
On 14/03/2023 17:08, Nikolay Aleksandrov wrote: > On 14/03/2023 16:58, Michal Kubiak wrote: >> On Tue, Mar 14, 2023 at 01:14:23PM +0200, Nikolay Aleksandrov wrote: >>> Add bond_ether_setup helper which will be used in the following patches >>> to fix all ether_setup() calls in the bonding driver. It takes care of both >>> IFF_MASTER and IFF_SLAVE flags, the former is always restored and the >>> latter only if it was set. >>> >>> Fixes: e36b9d16c6a6d ("bonding: clean muticast addresses when device changes type") >>> Fixes: 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure") >>> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> >>> --- >>> drivers/net/bonding/bond_main.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> [snip] >> Please add your function in the patch where you actually use it. >> > > I'm adding the helper in a separate patch to emphasize it and focus the review. > I have written in the commit message that the next two fixes will be using it. > IMO, this should be ok. > Also since both fixes are for different commits this should be backportable for both, that makes it easier to pick if only one of the two would be ported to a specific version (i.e. the first fix is for a commit from 2009, second 2015). Cheers, Nik
Nikolay Aleksandrov <razor@blackwall.org> wrote: >Add bond_ether_setup helper which will be used in the following patches >to fix all ether_setup() calls in the bonding driver. It takes care of both >IFF_MASTER and IFF_SLAVE flags, the former is always restored and the >latter only if it was set. > >Fixes: e36b9d16c6a6d ("bonding: clean muticast addresses when device changes type") >Fixes: 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure") >Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> >--- > drivers/net/bonding/bond_main.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 00646aa315c3..d41024ad2c18 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -1775,6 +1775,18 @@ void bond_lower_state_changed(struct slave *slave) > slave_err(bond_dev, slave_dev, "Error: %s\n", errmsg); \ > } while (0) > >+/* ether_setup() resets bond_dev's flags so we always have to restore >+ * IFF_MASTER, and only restore IFF_SLAVE if it was set >+ */ >+static void bond_ether_setup(struct net_device *bond_dev) >+{ >+ unsigned int slave_flag = bond_dev->flags & IFF_SLAVE; >+ >+ ether_setup(bond_dev); >+ bond_dev->flags |= IFF_MASTER | slave_flag; >+ bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING; >+} Is setting IFF_MASTER always correct here? I note that patch #2 is replacing code that does not set IFF_MASTER, whereas patch #3 is replacing code that does set IFF_MASTER. Presuming that this is the desired behavior, perhaps mention explicitly in the commentary that bond_ether_setup() is only for use on a bond master device. The nomenclature "bond_dev" does imply that, but it's not explicit. Also, why is the call to ether_setup() from bond_setup() not also being converted to bond_ether_setup()? -J >+ > /* enslave device <slave> to bond device <master> */ > int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > struct netlink_ext_ack *extack) >-- >2.39.2 > --- -Jay Vosburgh, jay.vosburgh@canonical.com
On 14/03/2023 17:34, Jay Vosburgh wrote: > Nikolay Aleksandrov <razor@blackwall.org> wrote: > >> Add bond_ether_setup helper which will be used in the following patches >> to fix all ether_setup() calls in the bonding driver. It takes care of both >> IFF_MASTER and IFF_SLAVE flags, the former is always restored and the >> latter only if it was set. >> >> Fixes: e36b9d16c6a6d ("bonding: clean muticast addresses when device changes type") >> Fixes: 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure") >> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> >> --- >> drivers/net/bonding/bond_main.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 00646aa315c3..d41024ad2c18 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -1775,6 +1775,18 @@ void bond_lower_state_changed(struct slave *slave) >> slave_err(bond_dev, slave_dev, "Error: %s\n", errmsg); \ >> } while (0) >> >> +/* ether_setup() resets bond_dev's flags so we always have to restore >> + * IFF_MASTER, and only restore IFF_SLAVE if it was set >> + */ >> +static void bond_ether_setup(struct net_device *bond_dev) >> +{ >> + unsigned int slave_flag = bond_dev->flags & IFF_SLAVE; >> + >> + ether_setup(bond_dev); >> + bond_dev->flags |= IFF_MASTER | slave_flag; >> + bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING; >> +} > > Is setting IFF_MASTER always correct here? I note that patch #2 > is replacing code that does not set IFF_MASTER, whereas patch #3 is > replacing code that does set IFF_MASTER. > > Presuming that this is the desired behavior, perhaps mention > explicitly in the commentary that bond_ether_setup() is only for use on > a bond master device. The nomenclature "bond_dev" does imply that, but > it's not explicit. > Setting IFF_MASTER is always correct because we're talking about a bond master device. I.e. we're restoring the flags to a bond device itself. The bugs are different because previously I had fixed the error path (partly, missed the IFF_SLAVE), but I just noticed the normal enslave path while fixing the IFF_SLAVE one now. :) So yes, both paths need the same treatment for both flags. > Also, why is the call to ether_setup() from bond_setup() not > also being converted to bond_ether_setup()? That is more of a cleanup, the one there is correct because flags are set after that. In that case only IFF_MASTER is needed, IFF_SLAVE cannot be set. Once these are merged in net-next I'll send a followup to use it there, too. > > -J > Thanks, Nik >> + >> /* enslave device <slave> to bond device <master> */ >> int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, >> struct netlink_ext_ack *extack) >> -- >> 2.39.2 >> > > --- > -Jay Vosburgh, jay.vosburgh@canonical.com
On Tue, 14 Mar 2023 13:14:23 +0200 Nikolay Aleksandrov wrote: > +/* ether_setup() resets bond_dev's flags so we always have to restore > + * IFF_MASTER, and only restore IFF_SLAVE if it was set > + */ > +static void bond_ether_setup(struct net_device *bond_dev) > +{ > + unsigned int slave_flag = bond_dev->flags & IFF_SLAVE; > + > + ether_setup(bond_dev); > + bond_dev->flags |= IFF_MASTER | slave_flag; > + bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING; > +} > + We can't split this from patch 2, it's going to generate a warning under normal build flags, people may have WERROR set these days..
On 15/03/2023 09:55, Jakub Kicinski wrote: > On Tue, 14 Mar 2023 13:14:23 +0200 Nikolay Aleksandrov wrote: >> +/* ether_setup() resets bond_dev's flags so we always have to restore >> + * IFF_MASTER, and only restore IFF_SLAVE if it was set >> + */ >> +static void bond_ether_setup(struct net_device *bond_dev) >> +{ >> + unsigned int slave_flag = bond_dev->flags & IFF_SLAVE; >> + >> + ether_setup(bond_dev); >> + bond_dev->flags |= IFF_MASTER | slave_flag; >> + bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING; >> +} >> + > > We can't split this from patch 2, it's going to generate a warning > under normal build flags, people may have WERROR set these days.. Oh well, that's unfortunate. I'll leave a note in patch 03 that it depends on the helper added in the other fix. Thanks, Nik
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 00646aa315c3..d41024ad2c18 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1775,6 +1775,18 @@ void bond_lower_state_changed(struct slave *slave) slave_err(bond_dev, slave_dev, "Error: %s\n", errmsg); \ } while (0) +/* ether_setup() resets bond_dev's flags so we always have to restore + * IFF_MASTER, and only restore IFF_SLAVE if it was set + */ +static void bond_ether_setup(struct net_device *bond_dev) +{ + unsigned int slave_flag = bond_dev->flags & IFF_SLAVE; + + ether_setup(bond_dev); + bond_dev->flags |= IFF_MASTER | slave_flag; + bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING; +} + /* enslave device <slave> to bond device <master> */ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, struct netlink_ext_ack *extack)
Add bond_ether_setup helper which will be used in the following patches to fix all ether_setup() calls in the bonding driver. It takes care of both IFF_MASTER and IFF_SLAVE flags, the former is always restored and the latter only if it was set. Fixes: e36b9d16c6a6d ("bonding: clean muticast addresses when device changes type") Fixes: 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure") Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> --- drivers/net/bonding/bond_main.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)