Re: [PATCH net-next v2 1/1] net: mscc: ocelot: Implement port policers via tc command

[Joergen Andreasen <joergen.andreasen@microchip.com>]

Hi Jakub,

The 05/23/2019 11:56, Jakub Kicinski wrote:
> External E-Mail
> 
> 
> 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).

I will add this check in v3

> 
> > 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")

I will fix this in v3.

> 
> > +		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;
> > +	}
> > +}
> 

-- 
Joergen Andreasen, Microchip