Wed, Mar 02, 2016 at 04:53:37PM CET, a...@vadai.me wrote: >On Tue, Mar 01, 2016 at 09:13:25AM -0800, John Fastabend wrote: >> On 16-03-01 09:00 AM, Amir Vadai wrote: >> > On Tue, Mar 01, 2016 at 03:52:08PM +0100, Jiri Pirko wrote: >> >> Tue, Mar 01, 2016 at 03:24:48PM CET, a...@vadai.me wrote: >> >>> Extend ndo_setup_tc() to support ingress tc offloading. Will be used by >> >>> later patches to offload tc flower filter. >> >>> >> >>> Feature is off by default and could be enabled by issuing: >> >>> # ethtool -K eth0 hw-tc-offload on >> >>> >> >>> Offloads flow table is dynamically created when first filter is >> >>> added. >> >>> Rules are saved in a hash table that is maintained by the consumer (for >> >>> example - the flower offload in the next patch). >> >>> When last filter is removed and no filters exist in the hash table, the >> >>> offload flow table is destroyed. >> >> >> >> <snip> >> >> >> >>> @@ -1880,6 +1883,17 @@ static int mlx5e_setup_tc(struct net_device >> >>> *netdev, u8 tc) >> >>> static int mlx5e_ndo_setup_tc(struct net_device *dev, u32 handle, >> >>> __be16 proto, struct tc_to_netdev *tc) >> >>> { >> >>> + struct mlx5e_priv *priv = netdev_priv(dev); >> >>> + >> >>> + if (TC_H_MAJ(handle) != TC_H_MAJ(TC_H_INGRESS)) >> >>> + goto mqprio; >> >>> + >> >>> + switch (tc->type) { >> >>> + default: >> >>> + return -EINVAL; >> >> >> >> -EOPNOTSUPP would be better here perhaps? >> >> >> >> >> >>> + } >> >>> + >> >>> +mqprio: >> >>> if (handle != TC_H_ROOT || tc->type != TC_SETUP_MQPRIO) >> >>> return -EINVAL; >> >>> >> >>> @@ -1963,6 +1977,13 @@ static int mlx5e_set_features(struct net_device >> >>> *netdev, >> >>> mlx5e_disable_vlan_filter(priv); >> >>> } >> >>> >> >>> + if ((changes & NETIF_F_HW_TC) && !(features & NETIF_F_HW_TC) && >> >>> + mlx5e_tc_num_filters(priv)) { >> >>> + netdev_err(netdev, >> >>> + "Active offloaded tc filters, can't turn >> >>> hw_tc_offload off\n"); >> >>> + return -EINVAL; >> >> >> >> This should not fail I believe. Just disable it in hw. I would even toss >> >> away the rules if necessary. >> > It depends on the answer regarding your comment on the previous patch. >> > If we have the rule in both SW and HW, and remove it from the HW it is >> > ok (although, currently I don't understand why would anyone want in both >> > places). >> > If the rule is processed by HW only - turning off this flag, will >> > disable the offloaded rules - it might be misleading, so I prefered not >> > to allow it and print a message. >> >> When we get the HW only mode we will need to also flush the hardware >> representation in software as well as the hardware state. > >Yep, I do think that just failing the operation is the best appraoch. >Will make the design simpler, and from the user point of view, less >surprises. > >Jiri?
I don't feel it is ok, but at the same time, it is probably the best solution for now. Other solutions would be too complicated.