Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 23 May 2019 11:56:46 -0700
From:   Jakub Kicinski <jakub.kicinski@netronome.com>
To:     Joergen Andreasen <joergen.andreasen@microchip.com>
Cc:     <netdev@vger.kernel.org>,
        "Microchip Linux Driver Support" <UNGLinuxDriver@microchip.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        "David S. Miller" <davem@davemloft.net>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next v2 1/1] net: mscc: ocelot: Implement port
 policers via tc command
Message-ID: <20190523115630.7710cc49@cakuba.netronome.com>
In-Reply-To: <20190523104939.2721-2-joergen.andreasen@microchip.com>
References: <20190502094029.22526-1-joergen.andreasen@microchip.com>
        <20190523104939.2721-1-joergen.andreasen@microchip.com>
        <20190523104939.2721-2-joergen.andreasen@microchip.com>
Organization: Netronome Systems, Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 23 May 2019 12:49:39 +0200, Joergen Andreasen wrote:
> Hardware offload of matchall classifier and police action are now
> supported via the tc command.
> Supported police parameters are: rate and burst.
> 
> Example:
> 
> Add:
> tc qdisc add dev eth3 handle ffff: ingress
> tc filter add dev eth3 parent ffff: prio 1 handle 2	\
> 	matchall skip_sw				\
> 	action police rate 100Mbit burst 10000
> 
> Show:
> tc -s -d qdisc show dev eth3
> tc -s -d filter show dev eth3 ingress
> 
> Delete:
> tc filter del dev eth3 parent ffff: prio 1
> tc qdisc del dev eth3 handle ffff: ingress
> 
> Signed-off-by: Joergen Andreasen <joergen.andreasen@microchip.com>

> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index d715ef4fc92f..3ec7864d9dc8 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -943,6 +943,7 @@ static const struct net_device_ops ocelot_port_netdev_ops = {
>  	.ndo_vlan_rx_kill_vid		= ocelot_vlan_rx_kill_vid,
>  	.ndo_set_features		= ocelot_set_features,
>  	.ndo_get_port_parent_id		= ocelot_get_port_parent_id,
> +	.ndo_setup_tc			= ocelot_setup_tc,
>  };
>  
>  static void ocelot_get_strings(struct net_device *netdev, u32 sset, u8 *data)
> @@ -1663,8 +1664,9 @@ int ocelot_probe_port(struct ocelot *ocelot, u8 port,
>  	dev->netdev_ops = &ocelot_port_netdev_ops;
>  	dev->ethtool_ops = &ocelot_ethtool_ops;
>  
> -	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_RXFCS;
> -	dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> +	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_RXFCS |
> +		NETIF_F_HW_TC;
> +	dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_TC;
>  
>  	memcpy(dev->dev_addr, ocelot->base_mac, ETH_ALEN);
>  	dev->dev_addr[ETH_ALEN - 1] += port;

You need to add a check in set_features to make sure nobody clears the
NETIF_F_TC flag while something is offloaded, otherwise you will miss
the REMOVE callback (it will bounce from the
tc_cls_can_offload_and_chain0() check).

> diff --git a/drivers/net/ethernet/mscc/ocelot_tc.c b/drivers/net/ethernet/mscc/ocelot_tc.c
> new file mode 100644
> index 000000000000..2412e0dbc267
> --- /dev/null
> +++ b/drivers/net/ethernet/mscc/ocelot_tc.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/* Microsemi Ocelot Switch TC driver
> + *
> + * Copyright (c) 2019 Microsemi Corporation
> + */
> +
> +#include "ocelot_tc.h"
> +#include "ocelot_police.h"
> +#include <net/pkt_cls.h>
> +
> +static int ocelot_setup_tc_cls_matchall(struct ocelot_port *port,
> +					struct tc_cls_matchall_offload *f,
> +					bool ingress)
> +{
> +	struct netlink_ext_ack *extack = f->common.extack;
> +	struct ocelot_policer pol = { 0 };
> +	struct flow_action_entry *action;
> +	int err;
> +
> +	netdev_dbg(port->dev, "%s: port %u command %d cookie %lu\n",
> +		   __func__, port->chip_port, f->command, f->cookie);
> +
> +	if (!ingress) {
> +		NL_SET_ERR_MSG_MOD(extack, "Only ingress is supported");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	switch (f->command) {
> +	case TC_CLSMATCHALL_REPLACE:
> +		if (!flow_offload_has_one_action(&f->rule->action)) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Only one action is supported");
> +			return -EOPNOTSUPP;
> +		}
> +
> +		action = &f->rule->action.entries[0];
> +
> +		if (action->id != FLOW_ACTION_POLICE) {
> +			NL_SET_ERR_MSG_MOD(extack, "Unsupported action");
> +			return -EOPNOTSUPP;
> +		}

Please also reject the offload if block is shared, as HW policer state
cannot be shared between ports, the way it is in SW.  You have to save
whether the block is shared or not at bind time, see:

d6787147e15d ("net/sched: remove block pointer from common offload structure")

> +		if (port->tc.police_id && port->tc.police_id != f->cookie) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Only one policer per port is supported\n");
> +			return -EEXIST;
> +		}
> +
> +		pol.rate = (u32)div_u64(action->police.rate_bytes_ps, 1000) * 8;
> +		pol.burst = (u32)div_u64(action->police.rate_bytes_ps *
> +					 PSCHED_NS2TICKS(action->police.burst),
> +					 PSCHED_TICKS_PER_SEC);
> +
> +		err = ocelot_port_policer_add(port, &pol);
> +		if (err) {
> +			NL_SET_ERR_MSG_MOD(extack, "Could not add policer\n");
> +			return err;
> +		}
> +
> +		port->tc.police_id = f->cookie;
> +		return 0;
> +	case TC_CLSMATCHALL_DESTROY:
> +		if (port->tc.police_id != f->cookie)
> +			return -ENOENT;
> +
> +		err = ocelot_port_policer_del(port);
> +		if (err) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Could not delete policer\n");
> +			return err;
> +		}
> +		port->tc.police_id = 0;
> +		return 0;
> +	case TC_CLSMATCHALL_STATS: /* fall through */
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}