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 <redacted>
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

[...]

+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