Re: [PATCH net-next v7 3/3] net: airoha: defer GDM3/GDM4 WAN mode and GDM2 loopback to QoS offload

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 12784 bytes --]

On Jul 01, Lorenzo Bianconi wrote:
> GDM3 and GDM4 ports require GDM2 loopback to be enabled for hardware
> QoS offload to function. Without it, HTB and ETS offload on these ports
> do not work.
> Previously, GDM3/GDM4 ports were automatically configured as WAN with
> GDM2 loopback enabled during ndo_init(). Add the capability to configure
> GDM3/GDM4 as WAN/LAN on demand when QoS offload is created or destroyed.
> Hook airoha_enable_qos_for_gdm34() into TC_HTB_CREATE so that requesting
> HTB offload on a GDM3/GDM4 LAN port switches it to WAN mode and enables
> GDM2 loopback, with proper rollback on failure. Introduce the
> AIROHA_DEV_F_QOS flag to track whether a device has an active HTB
> qdisc; clear it on TC_HTB_DESTROY. The device keeps its WAN role after
> qdisc teardown so that its configuration is preserved until another
> device explicitly needs the WAN role for QoS offload.
> If another GDM3/GDM4 device already holds the WAN role without an active
> QoS qdisc, demote it to LAN before promoting the requesting device. Skip
> the demotion when the requesting device is itself already the WAN device.
> Since airoha_dev_set_qdma() can now be called on a running device to
> migrate between QDMA blocks, make dev->qdma an RCU pointer so the TX
> path can safely dereference it without holding RTNL.
> Hold flow_offload_mutex in airoha_enable_qos_for_gdm34() and
> airoha_disable_qos_for_gdm34() around the dev->flags update,
> airoha_dev_set_qdma() and GDM2 loopback configuration, serializing
> against concurrent airoha_ppe_hw_init() in the TC_SETUP_CLSFLOWER
> offload path.
> Introduce airoha_qdma_deref() helper that wraps rcu_dereference_protected()
> with a lockdep condition accepting either rtnl_lock or flow_offload_mutex,
> and use it across all control-path dereferences of the RCU-protected
> dev->qdma pointer.
> Add airoha_disable_gdm2_loopback() to disable GDM2 hw loopback.
> 
> Tested-by: Madhur Agrawal <madhur.agrawal@airoha.com>
> Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Commenting on Sashiko's report:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260701-airoha-ethtool-priv_flags-v7-0-b4153bd44428%40kernel.org

> ---
>  drivers/net/ethernet/airoha/airoha_eth.c  | 219 ++++++++++++++++++++++++++----
>  drivers/net/ethernet/airoha/airoha_eth.h  |  13 +-
>  drivers/net/ethernet/airoha/airoha_ppe.c  |   9 +-
>  drivers/net/ethernet/airoha/airoha_regs.h |   1 +
>  4 files changed, 214 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 2c9ceb9f16f8..609a5ea67fb7 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c

[...]

> +static void airoha_disable_qos_for_gdm34(struct net_device *netdev)
> +{
> +	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> +	struct airoha_gdm_port *port = dev->port;
> +	int err;
> +
> +	if (port->id != AIROHA_GDM3_IDX &&
> +	    port->id != AIROHA_GDM4_IDX)
> +		return;
> +
> +	err = airoha_disable_gdm2_loopback(dev);
> +	if (err)
> +		netdev_warn(netdev,
> +			    "failed disabling GDM2 loopback: %d\n", err);
> +
> +	dev->flags &= ~AIROHA_DEV_F_WAN;
> +	airoha_dev_set_qdma(dev);
> +
> +	airoha_set_macaddr(dev, netdev->dev_addr);

- Should the return value of airoha_set_macaddr() be checked here?
  airoha_set_macaddr() can return -EINVAL when the device MAC MSBs do
  not match other same-role sibling devices:
  	if (!is_zero_ether_addr(ref_addr) && memcmp(ref_addr, addr, 3)) {
  		...
  		return -EINVAL;
  	}
  Since AIROHA_DEV_F_WAN has just been cleared and dev->qdma has been
  switched to QDMA0, the scan now iterates LAN-role peers.  If the
  ex-WAN MAC's top three bytes disagree with the LAN peers, the call
  returns -EINVAL and the HW MAC registers are left unwritten while
  the software state has already been flipped to LAN.
  - The asymmetry is intentional. airoha_disable_qos_for_gdm34() is a
    teardown path that returns void, the demotion has to happen regardless.
    Moreover, airoha_set_macaddr() can fail just if the device is
    misconfigured.


> +	if (netif_running(netdev))
> +		airoha_set_gdm_port_fwd_cfg(dev->eth,
> +					    REG_GDM_FWD_CFG(port->id),
> +					    FE_PSE_PORT_PPE1);
> +}
> +
> +static int airoha_enable_qos_for_gdm34(struct net_device *netdev,
> +				       struct netlink_ext_ack *extack)
> +{
> +	struct airoha_gdm_dev *wan_dev, *dev = netdev_priv(netdev);
> +	struct airoha_gdm_port *port = dev->port;
> +	struct airoha_eth *eth = dev->eth;
> +	int err = -EBUSY;
> +
> +	if (port->id != AIROHA_GDM3_IDX &&
> +	    port->id != AIROHA_GDM4_IDX) {
> +		/* HW QoS is always supported by GDM1 and GDM2 */
> +		return 0;
> +	}
> +
> +	if (!airoha_is_lan_gdm_dev(dev)) /* Already enabled */
> +		return 0;
> +
> +	mutex_lock(&flow_offload_mutex);
> +
> +	wan_dev = airoha_get_wan_gdm_dev(eth);
> +	if (wan_dev) {
> +		if ((wan_dev->flags & AIROHA_DEV_F_QOS) ||
> +		    wan_dev->port->id == AIROHA_GDM2_IDX) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "QoS configured for WAN device");
> +			goto error_unlock;
> +		}
> +		airoha_disable_qos_for_gdm34(netdev_from_priv(wan_dev));
> +	}
> +
> +	dev->flags |= AIROHA_DEV_F_WAN;
> +	airoha_dev_set_qdma(dev);
> +	err = airoha_enable_gdm2_loopback(dev);
> +	if (err)
> +		goto error_disable_wan;
> +
> +	err = airoha_set_macaddr(dev, netdev->dev_addr);
> +	if (err)
> +		goto error_disable_loopback;
> +
> +	if (netif_running(netdev)) {
> +		u32 pse_port;
> +
> +		pse_port = airoha_ppe_is_enabled(eth, 1) ? FE_PSE_PORT_PPE2
> +							 : FE_PSE_PORT_PPE1;
> +		airoha_set_gdm_port_fwd_cfg(eth, REG_GDM_FWD_CFG(port->id),
> +					    pse_port);
> +	}
> +
> +	mutex_unlock(&flow_offload_mutex);
> +
> +	return 0;
> +
> +error_disable_loopback:
> +	airoha_disable_gdm2_loopback(dev);
> +error_disable_wan:
> +	dev->flags &= ~AIROHA_DEV_F_WAN;
> +	airoha_dev_set_qdma(dev);
> +error_unlock:
> +	mutex_unlock(&flow_offload_mutex);
> +
> +	return err;

- Does this failure path fully implement the "proper rollback on failure"
  described in the commit message?
  Earlier in the function, if a peer wan_dev exists it is demoted via:
  	airoha_disable_qos_for_gdm34(netdev_from_priv(wan_dev));
  That helper clears AIROHA_DEV_F_WAN on wan_dev, swaps its RCU qdma
  pointer to QDMA0, disables GDM2 loopback, rewrites its MAC to the LAN
  registers, and (if wan_dev was running) sets its REG_GDM_FWD_CFG to
  FE_PSE_PORT_PPE1.
  If a later step for the requesting device fails, the error labels
  error_disable_loopback / error_disable_wan / error_unlock only unwind
  the requesting device.  The demoted wan_dev is never re-promoted.
  For example, airoha_set_macaddr(dev, netdev->dev_addr) can return
  -EINVAL when the requesting device's MAC MSBs disagree with the other
  WAN-role peers:
  	if (!is_zero_ether_addr(ref_addr) && memcmp(ref_addr, addr, 3)) {
  		...
  		return -EINVAL;
  	}
  In that case the failed tc qdisc add returns to userspace, the
  requesting device is rolled back to LAN, and no GDM3/GDM4 device holds
  the WAN role anymore.  The previously working WAN device silently loses
  its WAN role and QDMA1 binding.
  Should the error paths re-promote wan_dev, or should the preconditions
  that can fail (MAC MSB check, GDM2 loopback setup) be validated before
  mutating the peer's state?
  - This issue has been already reported in the past. If the configuration
    fails, I think there is no point to move back the previous device as WAN.
    The user will be able to log again and re-apply the configuration.
    Moreover, airoha_enable_qos_for_gdm34() can fail just if the device is
    misconfigured (failures in airoha_enable_gdm2_loopback() or
    airoha_set_macaddr()).

Regards,
Lorenzo

> +}
> +
>  static int airoha_tc_htb_destroy(struct net_device *netdev)
>  {
>  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> @@ -3057,6 +3217,8 @@ static int airoha_tc_htb_destroy(struct net_device *netdev)
>  	for_each_set_bit(q, dev->qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS)
>  		airoha_tc_remove_htb_queue(netdev, q);
>  
> +	dev->flags &= ~AIROHA_DEV_F_QOS;
> +
>  	return 0;
>  }
>  
> @@ -3076,24 +3238,33 @@ static int airoha_tc_get_htb_get_leaf_queue(struct net_device *netdev,
>  	return 0;
>  }
>  
> -static int airoha_tc_setup_qdisc_htb(struct net_device *dev,
> +static int airoha_tc_setup_qdisc_htb(struct net_device *netdev,
>  				     struct tc_htb_qopt_offload *opt)
>  {
>  	switch (opt->command) {
> -	case TC_HTB_CREATE:
> +	case TC_HTB_CREATE: {
> +		struct airoha_gdm_dev *dev = netdev_priv(netdev);
> +		int err;
> +
> +		err = airoha_enable_qos_for_gdm34(netdev, opt->extack);
> +		if (err)
> +			return err;
> +
> +		dev->flags |= AIROHA_DEV_F_QOS;
>  		break;
> +	}
>  	case TC_HTB_DESTROY:
> -		return airoha_tc_htb_destroy(dev);
> +		return airoha_tc_htb_destroy(netdev);
>  	case TC_HTB_NODE_MODIFY:
> -		return airoha_tc_htb_modify_queue(dev, opt);
> +		return airoha_tc_htb_modify_queue(netdev, opt);
>  	case TC_HTB_LEAF_ALLOC_QUEUE:
> -		return airoha_tc_htb_alloc_leaf_queue(dev, opt);
> +		return airoha_tc_htb_alloc_leaf_queue(netdev, opt);
>  	case TC_HTB_LEAF_DEL:
>  	case TC_HTB_LEAF_DEL_LAST:
>  	case TC_HTB_LEAF_DEL_LAST_FORCE:
> -		return airoha_tc_htb_delete_leaf_queue(dev, opt);
> +		return airoha_tc_htb_delete_leaf_queue(netdev, opt);
>  	case TC_HTB_LEAF_QUERY_QUEUE:
> -		return airoha_tc_get_htb_get_leaf_queue(dev, opt);
> +		return airoha_tc_get_htb_get_leaf_queue(netdev, opt);
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index ac5f571f3e53..a314330fcd48 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -537,11 +537,12 @@ struct airoha_qdma {
>  
>  enum airoha_dev_flags {
>  	AIROHA_DEV_F_WAN = BIT(0),
> +	AIROHA_DEV_F_QOS = BIT(1),
>  };
>  
>  struct airoha_gdm_dev {
> +	struct airoha_qdma __rcu *qdma;
>  	struct airoha_gdm_port *port;
> -	struct airoha_qdma *qdma;
>  	struct airoha_eth *eth;
>  
>  	DECLARE_BITMAP(qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS);
> @@ -677,6 +678,16 @@ int airoha_get_fe_port(struct airoha_gdm_dev *dev);
>  bool airoha_is_valid_gdm_dev(struct airoha_eth *eth,
>  			     struct airoha_gdm_dev *dev);
>  
> +extern struct mutex flow_offload_mutex;
> +
> +static inline struct airoha_qdma *
> +airoha_qdma_deref(struct airoha_gdm_dev *dev)
> +{
> +	return rcu_dereference_protected(dev->qdma,
> +					 lockdep_rtnl_is_held() ||
> +					 lockdep_is_held(&flow_offload_mutex));
> +}
> +
>  void airoha_ppe_set_cpu_port(struct airoha_gdm_dev *dev, u8 ppe_id, u8 fport);
>  bool airoha_ppe_is_enabled(struct airoha_eth *eth, int index);
>  void airoha_ppe_check_skb(struct airoha_ppe_dev *dev, struct sk_buff *skb,
> diff --git a/drivers/net/ethernet/airoha/airoha_ppe.c b/drivers/net/ethernet/airoha/airoha_ppe.c
> index 42f4b0f21d17..0f260c50ac3c 100644
> --- a/drivers/net/ethernet/airoha/airoha_ppe.c
> +++ b/drivers/net/ethernet/airoha/airoha_ppe.c
> @@ -15,7 +15,10 @@
>  #include "airoha_regs.h"
>  #include "airoha_eth.h"
>  
> -static DEFINE_MUTEX(flow_offload_mutex);
> +/* Serialize airoha_gdm_dev flags, QDMA pointer and PPE CPU port
> + * configuration.
> + */
> +DEFINE_MUTEX(flow_offload_mutex);
>  static DEFINE_SPINLOCK(ppe_lock);
>  
>  static const struct rhashtable_params airoha_flow_table_params = {
> @@ -86,8 +89,8 @@ static u32 airoha_ppe_get_timestamp(struct airoha_ppe *ppe)
>  
>  void airoha_ppe_set_cpu_port(struct airoha_gdm_dev *dev, u8 ppe_id, u8 fport)
>  {
> -	struct airoha_qdma *qdma = dev->qdma;
> -	struct airoha_eth *eth = qdma->eth;
> +	struct airoha_qdma *qdma = airoha_qdma_deref(dev);
> +	struct airoha_eth *eth = dev->eth;
>  	u8 qdma_id = qdma - &eth->qdma[0];
>  	u32 fe_cpu_port;
>  
> diff --git a/drivers/net/ethernet/airoha/airoha_regs.h b/drivers/net/ethernet/airoha/airoha_regs.h
> index 436f3c8779c1..4e17dfbcf2b8 100644
> --- a/drivers/net/ethernet/airoha/airoha_regs.h
> +++ b/drivers/net/ethernet/airoha/airoha_regs.h
> @@ -376,6 +376,7 @@
>  
>  #define REG_SRC_PORT_FC_MAP6		0x2298
>  #define FC_ID_OF_SRC_PORT_MASK(_n)	GENMASK(4 + ((_n) << 3), ((_n) << 3))
> +#define FC_MAP6_DEF_VALUE		0x1b1a1918
>  
>  #define REG_CDM5_RX_OQ1_DROP_CNT	0x29d4
>  
> 
> -- 
> 2.54.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]