On Tue, Mar 23, 2021 at 13:35, Vladimir Oltean <olte...@gmail.com> wrote:
> On Tue, Mar 23, 2021 at 11:23:26AM +0100, Tobias Waldekranz wrote:
>> All devices are capable of using regular DSA tags. Support for
>> Ethertyped DSA tags sort into three categories:
>>
>> 1. No support. Older chips fall into this category.
>>
>> 2. Full support. Datasheet explicitly supports configuring the CPU
>> port to receive FORWARDs with a DSA tag.
>>
>> 3. Undocumented support. Datasheet lists the configuration from
>> category 2 as "reserved for future use", but does empirically
>> behave like a category 2 device.
>>
>> Because there are ethernet controllers that do not handle regular DSA
>> tags in all cases, it is sometimes preferable to rely on the
>> undocumented behavior, as the alternative is a very crippled
>> system. But, in those cases, make sure to log the fact that an
>> undocumented feature has been enabled.
>>
>> Signed-off-by: Tobias Waldekranz <tob...@waldekranz.com>
>> ---
>>
>> In a system using an NXP T1023 SoC connected to a 6390X switch, we
>> noticed that TO_CPU frames where not reaching the CPU. This only
>> happened on hardware port 8. Looking at the DSA master interface
>> (dpaa-ethernet) we could see that an Rx error counter was bumped at
>> the same rate. The logs indicated a parser error.
>>
>> It just so happens that a TO_CPU coming in on device 0, port 8, will
>> result in the first two bytes of the DSA tag being one of:
>>
>> 00 40
>> 00 44
>> 00 46
>>
>> My guess is that since these values look like 802.3 length fields, the
>> controller's parser will signal an error if the frame length does not
>> match what is in the header.
>
> Interesting assumption.
> Could you please try this patch out, just for my amusement? It is only
> compile-tested.
>
> -----------------------------[ cut here ]-----------------------------
> From ab75b63d1bfeccc3032060e6e6dbd2ea8f1d31ed Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.olt...@nxp.com>
> Date: Tue, 23 Mar 2021 13:03:34 +0200
> Subject: [PATCH] fsl/fman: ignore RX parse errors on ports that are DSA
> masters
>
> Tobias reports that when an FMan port receives a Marvell DSA tagged
> frame (normal/legacy tag, not EtherType tag) which is a TO_CPU frame
> coming in from device 0, port 8, that frame will be dropped.
>
> It appears that the first two bytes of this particular DSA tag (which
> overlap with what the FMan parser interprets as an EtherType/Length
> field) look like one of the possible values below:
>
> 00 40
> 00 44
> 00 46
>
> Reported-by: Tobias Waldekranz <tob...@waldekranz.com>
> Signed-off-by: Vladimir Oltean <vladimir.olt...@nxp.com>
> ---
> .../net/ethernet/freescale/dpaa/dpaa_eth.c | 65 ++++++++++---------
> .../net/ethernet/freescale/fman/fman_port.c | 8 ++-
> .../net/ethernet/freescale/fman/fman_port.h | 2 +-
> 3 files changed, 42 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index 720dc99bd1fc..069d38cd63c5 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -55,6 +55,7 @@
> #include <linux/phy_fixed.h>
> #include <linux/bpf.h>
> #include <linux/bpf_trace.h>
> +#include <net/dsa.h>
> #include <soc/fsl/bman.h>
> #include <soc/fsl/qman.h>
> #include "fman.h"
> @@ -2447,34 +2448,6 @@ static inline int dpaa_eth_napi_schedule(struct
> dpaa_percpu_priv *percpu_priv,
> return 0;
> }
>
> -static enum qman_cb_dqrr_result rx_error_dqrr(struct qman_portal *portal,
> - struct qman_fq *fq,
> - const struct qm_dqrr_entry *dq,
> - bool sched_napi)
> -{
> - struct dpaa_fq *dpaa_fq = container_of(fq, struct dpaa_fq, fq_base);
> - struct dpaa_percpu_priv *percpu_priv;
> - struct net_device *net_dev;
> - struct dpaa_bp *dpaa_bp;
> - struct dpaa_priv *priv;
> -
> - net_dev = dpaa_fq->net_dev;
> - priv = netdev_priv(net_dev);
> - dpaa_bp = dpaa_bpid2pool(dq->fd.bpid);
> - if (!dpaa_bp)
> - return qman_cb_dqrr_consume;
> -
> - percpu_priv = this_cpu_ptr(priv->percpu_priv);
> -
> - if (dpaa_eth_napi_schedule(percpu_priv, portal, sched_napi))
> - return qman_cb_dqrr_stop;
> -
> - dpaa_eth_refill_bpools(priv);
> - dpaa_rx_error(net_dev, priv, percpu_priv, &dq->fd, fq->fqid);
> -
> - return qman_cb_dqrr_consume;
> -}
> -
> static int dpaa_xdp_xmit_frame(struct net_device *net_dev,
> struct xdp_frame *xdpf)
> {
> @@ -2699,7 +2672,7 @@ static enum qman_cb_dqrr_result rx_default_dqrr(struct
> qman_portal *portal,
> return qman_cb_dqrr_consume;
> }
>
> - if (unlikely(fd_status & FM_FD_STAT_RX_ERRORS) != 0) {
> + if (!netdev_uses_dsa(net_dev) && unlikely(fd_status &
> FM_FD_STAT_RX_ERRORS) != 0) {
> if (net_ratelimit())
> netif_warn(priv, hw, net_dev, "FD status = 0x%08x\n",
> fd_status & FM_FD_STAT_RX_ERRORS);
> @@ -2802,6 +2775,37 @@ static enum qman_cb_dqrr_result rx_default_dqrr(struct
> qman_portal *portal,
> return qman_cb_dqrr_consume;
> }
>
> +static enum qman_cb_dqrr_result rx_error_dqrr(struct qman_portal *portal,
> + struct qman_fq *fq,
> + const struct qm_dqrr_entry *dq,
> + bool sched_napi)
> +{
> + struct dpaa_fq *dpaa_fq = container_of(fq, struct dpaa_fq, fq_base);
> + struct dpaa_percpu_priv *percpu_priv;
> + struct net_device *net_dev;
> + struct dpaa_bp *dpaa_bp;
> + struct dpaa_priv *priv;
> +
> + net_dev = dpaa_fq->net_dev;
> + if (netdev_uses_dsa(net_dev))
> + return rx_default_dqrr(portal, fq, dq, sched_napi);
> +
> + priv = netdev_priv(net_dev);
> + dpaa_bp = dpaa_bpid2pool(dq->fd.bpid);
> + if (!dpaa_bp)
> + return qman_cb_dqrr_consume;
> +
> + percpu_priv = this_cpu_ptr(priv->percpu_priv);
> +
> + if (dpaa_eth_napi_schedule(percpu_priv, portal, sched_napi))
> + return qman_cb_dqrr_stop;
> +
> + dpaa_eth_refill_bpools(priv);
> + dpaa_rx_error(net_dev, priv, percpu_priv, &dq->fd, fq->fqid);
> +
> + return qman_cb_dqrr_consume;
> +}
> +
> static enum qman_cb_dqrr_result conf_error_dqrr(struct qman_portal *portal,
> struct qman_fq *fq,
> const struct qm_dqrr_entry *dq,
> @@ -2955,6 +2959,7 @@ static int dpaa_phy_init(struct net_device *net_dev)
>
> static int dpaa_open(struct net_device *net_dev)
> {
> + bool ignore_errors = netdev_uses_dsa(net_dev);
> struct mac_device *mac_dev;
> struct dpaa_priv *priv;
> int err, i;
> @@ -2968,7 +2973,7 @@ static int dpaa_open(struct net_device *net_dev)
> goto phy_init_failed;
>
> for (i = 0; i < ARRAY_SIZE(mac_dev->port); i++) {
> - err = fman_port_enable(mac_dev->port[i]);
> + err = fman_port_enable(mac_dev->port[i], ignore_errors);
> if (err)
> goto mac_start_failed;
> }
> diff --git a/drivers/net/ethernet/freescale/fman/fman_port.c
> b/drivers/net/ethernet/freescale/fman/fman_port.c
> index d9baac0dbc7d..763faec11f5c 100644
> --- a/drivers/net/ethernet/freescale/fman/fman_port.c
> +++ b/drivers/net/ethernet/freescale/fman/fman_port.c
> @@ -106,6 +106,7 @@
> #define BMI_EBD_EN 0x80000000
>
> #define BMI_PORT_CFG_EN 0x80000000
> +#define BMI_PORT_CFG_FDOVR 0x02000000
>
> #define BMI_PORT_STATUS_BSY 0x80000000
>
> @@ -1629,7 +1630,7 @@ int fman_port_disable(struct fman_port *port)
> }
>
> /* Disable BMI */
> - tmp = ioread32be(bmi_cfg_reg) & ~BMI_PORT_CFG_EN;
> + tmp = ioread32be(bmi_cfg_reg) & ~(BMI_PORT_CFG_EN | BMI_PORT_CFG_FDOVR);
> iowrite32be(tmp, bmi_cfg_reg);
>
> /* Wait for graceful stop end */
> @@ -1655,6 +1656,7 @@ EXPORT_SYMBOL(fman_port_disable);
> /**
> * fman_port_enable
> * @port: A pointer to a FM Port module.
> + * @ignore_errors: If set, do not discard frames received with errors.
> *
> * A runtime routine provided to allow disable/enable of port.
> *
> @@ -1662,7 +1664,7 @@ EXPORT_SYMBOL(fman_port_disable);
> *
> * Return: 0 on success; Error code otherwise.
> */
> -int fman_port_enable(struct fman_port *port)
> +int fman_port_enable(struct fman_port *port, bool ignore_errors)
> {
> u32 __iomem *bmi_cfg_reg;
> u32 tmp;
> @@ -1692,6 +1694,8 @@ int fman_port_enable(struct fman_port *port)
>
> /* Enable BMI */
> tmp = ioread32be(bmi_cfg_reg) | BMI_PORT_CFG_EN;
> + if (ignore_errors)
> + tmp |= BMI_PORT_CFG_FDOVR;
> iowrite32be(tmp, bmi_cfg_reg);
>
> return 0;
> diff --git a/drivers/net/ethernet/freescale/fman/fman_port.h
> b/drivers/net/ethernet/freescale/fman/fman_port.h
> index 82f12661a46d..0928361b0e73 100644
> --- a/drivers/net/ethernet/freescale/fman/fman_port.h
> +++ b/drivers/net/ethernet/freescale/fman/fman_port.h
> @@ -147,7 +147,7 @@ int fman_port_cfg_buf_prefix_content(struct fman_port
> *port,
>
> int fman_port_disable(struct fman_port *port);
>
> -int fman_port_enable(struct fman_port *port);
> +int fman_port_enable(struct fman_port *port, bool ignore_errors);
>
> u32 fman_port_get_qman_channel_id(struct fman_port *port);
>
> -----------------------------[ cut here ]-----------------------------
>
> The netdev_uses_dsa thing is a bit trashy, I think that a more polished
> version should rather set NETIF_F_RXALL for the DSA master, and have the
> dpaa driver act upon that. But first I'm curious if it works.
It does work. Thank you!
Does setting RXALL mean that the master would accept frames with a bad
FCS as well? If so, would that mean that we would have to verify it in
software?
>>
>> As a workaround, switching to EDSA (thereby always having a proper
>> EtherType in the frame) solves the issue.
>
> So basically every user needs to change the tag protocol manually to be
> able to receive from port 8? Not sure if that's too friendly.
No it is not friendly at all. My goal was to add it as a device-tree
property, but for reasons I will detail in my answer to Andrew, I did
not manage to figure out a good way to do that. Happy to take
suggestions.
>> drivers/net/dsa/mv88e6xxx/chip.c | 41 +++++++++++++++++++++++++++++---
>> drivers/net/dsa/mv88e6xxx/chip.h | 3 +++
>> 2 files changed, 41 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
>> b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 95f07fcd4f85..e7ec883d5f6b 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -2531,10 +2531,10 @@ static int mv88e6xxx_setup_port_mode(struct
>> mv88e6xxx_chip *chip, int port)
>> return mv88e6xxx_set_port_mode_normal(chip, port);
>>
>> /* Setup CPU port mode depending on its supported tag format */
>> - if (chip->info->tag_protocol == DSA_TAG_PROTO_DSA)
>> + if (chip->tag_protocol == DSA_TAG_PROTO_DSA)
>> return mv88e6xxx_set_port_mode_dsa(chip, port);
>>
>> - if (chip->info->tag_protocol == DSA_TAG_PROTO_EDSA)
>> + if (chip->tag_protocol == DSA_TAG_PROTO_EDSA)
>> return mv88e6xxx_set_port_mode_edsa(chip, port);
>>
>> return -EINVAL;
>> @@ -5564,7 +5564,39 @@ static enum dsa_tag_protocol
>> mv88e6xxx_get_tag_protocol(struct dsa_switch *ds,
>> {
>> struct mv88e6xxx_chip *chip = ds->priv;
>>
>> - return chip->info->tag_protocol;
>> + return chip->tag_protocol;
>> +}
>> +
>> +static int mv88e6xxx_change_tag_protocol(struct dsa_switch *ds, int port,
>> + enum dsa_tag_protocol proto)
>> +{
>> + struct mv88e6xxx_chip *chip = ds->priv;
>> + enum dsa_tag_protocol old_protocol;
>> + int err;
>> +
>> + switch (proto) {
>> + case DSA_TAG_PROTO_EDSA:
>> + if (chip->info->tag_protocol != DSA_TAG_PROTO_EDSA)
>> + dev_warn(chip->dev, "Relying on undocumented EDSA
>> tagging behavior\n");
>> +
>> + break;
>> + case DSA_TAG_PROTO_DSA:
>> + break;
>> + default:
>> + return -EPROTONOSUPPORT;
>> + }
>> +
>> + old_protocol = chip->tag_protocol;
>> + chip->tag_protocol = proto;
>> +
>> + mv88e6xxx_reg_lock(chip);
>> + err = mv88e6xxx_setup_port_mode(chip, port);
>> + mv88e6xxx_reg_unlock(chip);
>> +
>> + if (err)
>> + chip->tag_protocol = old_protocol;
>> +
>> + return err;
>> }
>>
>> static int mv88e6xxx_port_mdb_add(struct dsa_switch *ds, int port,
>> @@ -6029,6 +6061,7 @@ static int mv88e6xxx_crosschip_lag_leave(struct
>> dsa_switch *ds, int sw_index,
>>
>> static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
>> .get_tag_protocol = mv88e6xxx_get_tag_protocol,
>> + .change_tag_protocol = mv88e6xxx_change_tag_protocol,
>> .setup = mv88e6xxx_setup,
>> .teardown = mv88e6xxx_teardown,
>> .phylink_validate = mv88e6xxx_validate,
>> @@ -6209,6 +6242,8 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
>> if (err)
>> goto out;
>>
>> + chip->tag_protocol = chip->info->tag_protocol;
>> +
>> mv88e6xxx_phy_init(chip);
>>
>> if (chip->info->ops->get_eeprom) {
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h
>> b/drivers/net/dsa/mv88e6xxx/chip.h
>> index bce6e0dc8535..96b775f3fda2 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.h
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.h
>> @@ -261,6 +261,9 @@ struct mv88e6xxx_region_priv {
>> struct mv88e6xxx_chip {
>> const struct mv88e6xxx_info *info;
>>
>> + /* Currently configured tagging protocol */
>> + enum dsa_tag_protocol tag_protocol;
>> +
>> /* The dsa_switch this private structure is related to */
>> struct dsa_switch *ds;
>>
>> --
>> 2.25.1
>>
