Re: [PATCH net-next v4 6/8] net: sched: create tc_can_offload_extack() wrapper

[Jiri Pirko <jiri@resnulli.us>]

Sat, Jan 20, 2018 at 11:12:25AM CET, jakub.kicinski@netronome.com wrote:
>On Sat, Jan 20, 2018 at 12:59 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Sat, Jan 20, 2018 at 02:44:48AM CET, jakub.kicinski@netronome.com wrote:
>>>From: Quentin Monnet <quentin.monnet@netronome.com>
>>>
>>>Create a wrapper around tc_can_offload() that takes an additional
>>>extack pointer argument in order to output an error message if TC
>>>offload is disabled on the device.
>>>
>>>In this way, the error message is handled by the core and can be the
>>>same for all drivers.
>>>
>>>Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
>>>Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>>---
>>> include/net/pkt_cls.h | 11 +++++++++++
>>> 1 file changed, 11 insertions(+)
>>>
>>>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>>>index f497f622580b..2f8f16a4d88e 100644
>>>--- a/include/net/pkt_cls.h
>>>+++ b/include/net/pkt_cls.h
>>>@@ -656,6 +656,17 @@ static inline bool tc_can_offload(const struct net_device *dev)
>>>       return dev->features & NETIF_F_HW_TC;
>>> }
>>>
>>>+static inline bool tc_can_offload_extack(const struct net_device *dev,
>>>+                                       struct netlink_ext_ack *extack)
>>
>> I don't like to add tc_can_offload variant for this. It makes sense
>> the original tc_can_offload to be extended and set the extack message
>> always.
>>
>> It would require some more work in drivers (5), sure, but we endup with
>> nicer and consistent code.
>
>$ git grep tc_can_offload
>drivers/net/ethernet/broadcom/bnxt/bnxt.c:      if
>(!bnxt_tc_flower_enabled(bp) || !tc_can_offload(bp->dev))
>drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c:  if
>(!bnxt_tc_flower_enabled(vf_rep->bp) || !tc_can_offload(bp->dev))
>drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c:        if
>(!tc_can_offload(dev))
>drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:  if
>(!tc_can_offload(adapter->netdev))
>drivers/net/ethernet/mellanox/mlx5/core/en_main.c:      if
>(!tc_can_offload(priv->netdev))
>drivers/net/ethernet/mellanox/mlx5/core/en_rep.c:       if
>(!tc_can_offload(priv->netdev))
>drivers/net/ethernet/mellanox/mlxsw/spectrum.c:         if
>(!tc_can_offload(mlxsw_sp_port->dev))
>drivers/net/ethernet/netronome/nfp/bpf/main.c:
>!tc_can_offload(nn->dp.netdev) ||
>drivers/net/ethernet/netronome/nfp/flower/offload.c:    if
>(!tc_can_offload(repr->netdev))
>drivers/net/ethernet/netronome/nfp/flower/offload.c:    if
>(!tc_can_offload(repr->netdev))
>drivers/net/netdevsim/bpf.c:        !tc_can_offload(ns->netdev) ||
>include/net/pkt_cls.h:static inline bool tc_can_offload(const struct
>net_device *dev)
>net/dsa/slave.c:        if (!tc_can_offload(dev))
>net/sched/cls_api.c:    if (!tc_can_offload(dev) &&
>tcf_block_offload_in_use(block))
>net/sched/sch_mq.c:     if (!tc_can_offload(dev) ||
>!dev->netdev_ops->ndo_setup_tc)
>net/sched/sch_prio.c:   if (!tc_can_offload(dev) ||
>!dev->netdev_ops->ndo_setup_tc)
>net/sched/sch_prio.c:   if (!tc_can_offload(dev) ||
>!dev->netdev_ops->ndo_setup_tc)
>net/sched/sch_red.c:    if (!tc_can_offload(dev) ||
>!dev->netdev_ops->ndo_setup_tc)
>net/sched/sch_red.c:    if (!tc_can_offload(dev) ||
>!dev->netdev_ops->ndo_setup_tc)
>
>Do you mean the qdisc offloads too?  The whole lot?

Yes.