Would you mind adding the switchdev MRP support for the DSA driver too,
and move the code to the common ocelot library? I would like to give it
a run. I think that's only fair, since I have to keep in sync the
vsc7514 driver too for features that get added through DSA :)
On Tue, Feb 09, 2021 at 09:21:12PM +0100, Horatiu Vultur wrote:
> Add basic support for MRP. The HW will just trap all MRP frames on the
> ring ports to CPU and allow the SW to process them. In this way it is
> possible to for this node to behave both as MRM and MRC.
>
> Current limitations are:
> - it doesn't support Interconnect roles.
> - it supports only a single ring.
> - the HW should be able to do forwarding of MRP Test frames so the SW
> will not need to do this. So it would be able to have the role MRC
> without SW support.
>
> Signed-off-by: Horatiu Vultur <horatiu.vul...@microchip.com>
> ---
> drivers/net/ethernet/mscc/ocelot_net.c | 154 +++++++++++++++++++++
> drivers/net/ethernet/mscc/ocelot_vsc7514.c | 6 +
> include/soc/mscc/ocelot.h | 6 +
> 3 files changed, 166 insertions(+)
>
> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c
> b/drivers/net/ethernet/mscc/ocelot_net.c
> index 8f12fa45b1b5..65971403e823 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -9,7 +9,10 @@
> */
>
> #include <linux/if_bridge.h>
> +#include <linux/mrp_bridge.h>
> #include <net/pkt_cls.h>
> +#include <soc/mscc/ocelot_vcap.h>
> +#include <uapi/linux/mrp_bridge.h>
> #include "ocelot.h"
> #include "ocelot_vcap.h"
>
> @@ -1069,6 +1072,139 @@ static int ocelot_port_obj_del_mdb(struct net_device
> *dev,
> return ocelot_port_mdb_del(ocelot, port, mdb);
> }
>
> +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> +static int ocelot_mrp_del_vcap(struct ocelot *ocelot, int port)
> +{
> + struct ocelot_vcap_block *block_vcap_is2;
> + struct ocelot_vcap_filter *filter;
> +
> + block_vcap_is2 = &ocelot->block[VCAP_IS2];
> + filter = ocelot_vcap_block_find_filter_by_id(block_vcap_is2, port,
> + false);
> + if (!filter)
> + return 0;
> +
> + return ocelot_vcap_filter_del(ocelot, filter);
> +}
> +
> +static int ocelot_add_mrp(struct net_device *dev,
> + const struct switchdev_obj_mrp *mrp)
> +{
> + struct ocelot_port_private *priv = netdev_priv(dev);
> + struct ocelot_port *ocelot_port = &priv->port;
> + struct ocelot *ocelot = ocelot_port->ocelot;
> +
> + if (mrp->p_port != dev && mrp->s_port != dev)
> + return 0;
> +
> + if (ocelot->mrp_ring_id != 0 &&
> + ocelot->mrp_s_port &&
> + ocelot->mrp_p_port)
> + return -EINVAL;
> +
> + if (mrp->p_port == dev)
> + ocelot->mrp_p_port = dev;
> +
> + if (mrp->s_port == dev)
> + ocelot->mrp_s_port = dev;
> +
> + ocelot->mrp_ring_id = mrp->ring_id;
> +
> + return 0;
> +}
> +
> +static int ocelot_del_mrp(struct net_device *dev,
> + const struct switchdev_obj_mrp *mrp)
> +{
> + struct ocelot_port_private *priv = netdev_priv(dev);
> + struct ocelot_port *ocelot_port = &priv->port;
> + struct ocelot *ocelot = ocelot_port->ocelot;
> +
> + if (ocelot->mrp_p_port != dev && ocelot->mrp_s_port != dev)
> + return 0;
> +
> + if (ocelot->mrp_ring_id == 0 &&
> + !ocelot->mrp_s_port &&
> + !ocelot->mrp_p_port)
> + return -EINVAL;
> +
> + if (ocelot_mrp_del_vcap(ocelot, priv->chip_port))
> + return -EINVAL;
> +
> + if (ocelot->mrp_p_port == dev)
> + ocelot->mrp_p_port = NULL;
> +
> + if (ocelot->mrp_s_port == dev)
> + ocelot->mrp_s_port = NULL;
> +
> + ocelot->mrp_ring_id = 0;
> +
> + return 0;
> +}
> +
> +static int ocelot_add_ring_role(struct net_device *dev,
> + const struct switchdev_obj_ring_role_mrp *mrp)
> +{
> + struct ocelot_port_private *priv = netdev_priv(dev);
> + struct ocelot_port *ocelot_port = &priv->port;
> + struct ocelot *ocelot = ocelot_port->ocelot;
> + struct ocelot_vcap_filter *filter;
> + int err;
> +
> + if (ocelot->mrp_ring_id != mrp->ring_id)
> + return -EINVAL;
> +
> + if (!mrp->sw_backup)
> + return -EOPNOTSUPP;
> +
> + if (ocelot->mrp_p_port != dev && ocelot->mrp_s_port != dev)
> + return 0;
> +
> + filter = kzalloc(sizeof(*filter), GFP_KERNEL);
> + if (!filter)
> + return -ENOMEM;
> +
> + filter->key_type = OCELOT_VCAP_KEY_ETYPE;
> + filter->prio = 1;
> + filter->id.cookie = priv->chip_port;
> + filter->id.tc_offload = false;
> + filter->block_id = VCAP_IS2;
> + filter->type = OCELOT_VCAP_FILTER_OFFLOAD;
> + filter->ingress_port_mask = BIT(priv->chip_port);
> + *(__be16 *)filter->key.etype.etype.value = htons(ETH_P_MRP);
> + *(__be16 *)filter->key.etype.etype.mask = htons(0xffff);
> + filter->action.mask_mode = OCELOT_MASK_MODE_PERMIT_DENY;
> + filter->action.port_mask = 0x0;
> + filter->action.cpu_copy_ena = true;
> + filter->action.cpu_qu_num = 0;
> +
> + err = ocelot_vcap_filter_add(ocelot, filter, NULL);
> + if (err)
> + kfree(filter);
> +
> + return err;
> +}
> +
> +static int ocelot_del_ring_role(struct net_device *dev,
> + const struct switchdev_obj_ring_role_mrp *mrp)
> +{
> + struct ocelot_port_private *priv = netdev_priv(dev);
> + struct ocelot_port *ocelot_port = &priv->port;
> + struct ocelot *ocelot = ocelot_port->ocelot;
> +
> + if (ocelot->mrp_ring_id != mrp->ring_id)
> + return -EINVAL;
> +
> + if (!mrp->sw_backup)
> + return -EOPNOTSUPP;
> +
> + if (ocelot->mrp_p_port != dev && ocelot->mrp_s_port != dev)
> + return 0;
> +
> + return ocelot_mrp_del_vcap(ocelot, priv->chip_port);
> +}
> +#endif
> +
Would it make sense for this chunk of conditionally compiled code to
stay in a separate file like ocelot_mrp.c?
> static int ocelot_port_obj_add(struct net_device *dev,
> const struct switchdev_obj *obj,
> struct netlink_ext_ack *extack)
> @@ -1083,6 +1219,15 @@ static int ocelot_port_obj_add(struct net_device *dev,
> case SWITCHDEV_OBJ_ID_PORT_MDB:
> ret = ocelot_port_obj_add_mdb(dev, SWITCHDEV_OBJ_PORT_MDB(obj));
> break;
> +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> + case SWITCHDEV_OBJ_ID_MRP:
> + ret = ocelot_add_mrp(dev, SWITCHDEV_OBJ_MRP(obj));
> + break;
> + case SWITCHDEV_OBJ_ID_RING_ROLE_MRP:
> + ret = ocelot_add_ring_role(dev,
> + SWITCHDEV_OBJ_RING_ROLE_MRP(obj));
> + break;
> +#endif
I'm not really sure why SWITCHDEV_OBJ_ID_MRP is conditionally defined.
If you look at SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING, that isn't
conditionally defined, even though it depends on CONFIG_BRIDGE_VLAN_FILTERING
at runtime.
> default:
> return -EOPNOTSUPP;
> }
> @@ -1103,6 +1248,15 @@ static int ocelot_port_obj_del(struct net_device *dev,
> case SWITCHDEV_OBJ_ID_PORT_MDB:
> ret = ocelot_port_obj_del_mdb(dev, SWITCHDEV_OBJ_PORT_MDB(obj));
> break;
> +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> + case SWITCHDEV_OBJ_ID_MRP:
> + ret = ocelot_del_mrp(dev, SWITCHDEV_OBJ_MRP(obj));
> + break;
> + case SWITCHDEV_OBJ_ID_RING_ROLE_MRP:
> + ret = ocelot_del_ring_role(dev,
> + SWITCHDEV_OBJ_RING_ROLE_MRP(obj));
> + break;
> +#endif
> default:
> return -EOPNOTSUPP;
> }
> diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> index 6b6eb92149ba..96a9c9f98060 100644
> --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> @@ -698,6 +698,12 @@ static irqreturn_t ocelot_xtr_irq_handler(int irq, void
> *arg)
> skb->offload_fwd_mark = 1;
>
> skb->protocol = eth_type_trans(skb, dev);
> +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> + if (skb->protocol == ntohs(ETH_P_MRP) &&
> + (priv->dev == ocelot->mrp_p_port ||
> + priv->dev == ocelot->mrp_s_port))
> + skb->offload_fwd_mark = 0;
> +#endif
I wonder if you could just reserve a certain CPUQ for trapped traffic,
and just generically check for that, instead of MRP port roles?
> if (!skb_defer_rx_timestamp(skb))
> netif_rx(skb);
> dev->stats.rx_bytes += len;
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index d0d48e9620fb..d95c019ad84e 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -682,6 +682,12 @@ struct ocelot {
> /* Protects the PTP clock */
> spinlock_t ptp_clock_lock;
> struct ptp_pin_desc ptp_pins[OCELOT_PTP_PINS_NUM];
> +
> +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> + u16 mrp_ring_id;
> + struct net_device *mrp_p_port;
> + struct net_device *mrp_s_port;
> +#endif
> };
>
> struct ocelot_policer {
> --
> 2.27.0
>
