| Message ID | 20210116005943.219479-2-olteanv@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | LAG offload for Ocelot DSA switches | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to net-next |
| netdev/tree_selection | success | Clearly marked for net-next |
On Sat, 16 Jan 2021 02:59:30 +0200 Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > Commit 7afb3e575e5a ("net: mscc: ocelot: don't handle netdev events for > other netdevs") was too aggressive, and it made ocelot_netdevice_event > react only to network interface events emitted for the ocelot switch > ports. > > In fact, only the PRECHANGEUPPER should have had that check. > > When we ignore all events that are not for us, we miss the fact that the > upper of the LAG changes, and the bonding interface gets enslaved to a > bridge. This is an operation we could offload under certain conditions. I see the commit in question is in net, perhaps worth spelling out why this is not a fix? Perhaps add some "in the future" to the last sentence if it's the case that this will only matter with the following patches applied?
On Sat, Jan 16, 2021 at 05:26:23PM -0800, Jakub Kicinski wrote: > On Sat, 16 Jan 2021 02:59:30 +0200 Vladimir Oltean wrote: > > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > Commit 7afb3e575e5a ("net: mscc: ocelot: don't handle netdev events for > > other netdevs") was too aggressive, and it made ocelot_netdevice_event > > react only to network interface events emitted for the ocelot switch > > ports. > > > > In fact, only the PRECHANGEUPPER should have had that check. > > > > When we ignore all events that are not for us, we miss the fact that the > > upper of the LAG changes, and the bonding interface gets enslaved to a > > bridge. This is an operation we could offload under certain conditions. > > I see the commit in question is in net, perhaps worth spelling out why > this is not a fix? Perhaps add some "in the future" to the last > sentence if it's the case that this will only matter with the following > patches applied? It is a fix. However, so is patch 13/14 "net: mscc: ocelot: rebalance LAGs on link up/down events", but I didn't see an easy way to backport that. Honestly the reasons why I did not attempt to split this series into a part for "net" and one for "net-next" are: (a) It would unnecessarily complicate my work for felix DSA, where this is considered a new feature as opposed to ocelot switchdev where it was supposedly already working (although.. not quite, due to the lack of rebalancing, a link down would throw off the LAG). I don't really think that anybody was seriously using LAG offload on ocelot so far. (b) Even if I were to split this patch, it can only be trivially backported as far as commit 9c90eea310f8 ("net: mscc: ocelot: move net_device related functions to ocelot_net.c") from June 2020 anyway. (c) I cannot test the mscc_ocelot.ko switchdev driver with traffic, since I don't have the hardware (I just have a local patch that I keep rebasing on top of net-next which makes me able to at least probe it and access its registers on a different switch revision, but the traffic I/O procedure there is completely different). So I can not really confirm what is the state I'm leaving the mscc_ocelot driver in, for stable kernels. At least now, I've made the entry points into the control code path very similar to those of DSA, and I've exercised the switchdev driver in blind (without traffic), so I have a bit more confidence that it should work. (d) Had the AUTOSEL guys picked up this patch, I would have probably had no objection (since my belief is that there's nothing to break and nothing to fix in stable kernels). That being said, if we want to engage in a rigid demonstration of procedures, sure we can do that. I have other patches anyway to fill the pipeline until "net" is merged back into "net-next" :)
On Sun, 17 Jan 2021 14:37:44 +0200 Vladimir Oltean wrote: > That being said, if we want to engage in a rigid demonstration of > procedures, sure we can do that. I have other patches anyway to fill the > pipeline until "net" is merged back into "net-next" :) If you don't mind I'd rather apply the fix to net, and the rest on Thu/Fri after the trees get merged.
On Mon, Jan 18, 2021 at 11:04:47AM -0800, Jakub Kicinski wrote: > On Sun, 17 Jan 2021 14:37:44 +0200 Vladimir Oltean wrote: > > That being said, if we want to engage in a rigid demonstration of > > procedures, sure we can do that. I have other patches anyway to fill the > > pipeline until "net" is merged back into "net-next" :) > > If you don't mind I'd rather apply the fix to net, and the rest on > Thu/Fri after the trees get merged. Sure, I already split this patch and sent it to "net": https://patchwork.kernel.org/project/netdevbpf/patch/20210118135210.2666246-1-olteanv@gmail.com/
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c index a520fd485912..467170363ab2 100644 --- a/drivers/net/ethernet/mscc/ocelot_net.c +++ b/drivers/net/ethernet/mscc/ocelot_net.c @@ -1153,10 +1153,8 @@ static int ocelot_netdevice_event(struct notifier_block *unused, struct net_device *dev = netdev_notifier_info_to_dev(ptr); int ret = 0; - if (!ocelot_netdevice_dev_check(dev)) - return 0; - if (event == NETDEV_PRECHANGEUPPER && + ocelot_netdevice_dev_check(dev) && netif_is_lag_master(info->upper_dev)) { struct netdev_lag_upper_info *lag_upper_info = info->upper_info; struct netlink_ext_ack *extack;