Re: [PATCH net-next] net: dsa: mv88e6xxx: Allow dynamic reconfiguration of tag protocol

[Florian Fainelli <f.fainelli@gmail.com>]

On 3/23/2021 7:48 AM, Tobias Waldekranz wrote:
> On Tue, Mar 23, 2021 at 13:35, Vladimir Oltean <olteanv@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 <tobias@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.oltean@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 <tobias@waldekranz.com>
>> Signed-off-by: Vladimir Oltean <vladimir.oltean@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!

This is unfortunately not new, the stmmac choked on me with Broadcom
tags as well:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8cad443eacf661796a740903a75cb8944c675b4e
-- 
Florian