Re: [PATCH net-next v3 3/6] net: ti: icssg-prueth: Add support for HSR frame forward offload
From: Alexander Lobakin <aleksander.lobakin@intel.com>
Date: 2024-08-30 14:07:58
Also in:
linux-arm-kernel, lkml
From: Md Danish Anwar <danishanwar@ti.com> Date: Wed, 28 Aug 2024 14:48:58 +0530
Add support for offloading HSR port-to-port frame forward to hardware. When the slave interfaces are added to the HSR interface, the PRU cores will be stopped and ICSSG HSR firmwares will be loaded to them. Similarly, when HSR interface is deleted, the PRU cores will be stopped and dual EMAC firmware will be loaded to them. This commit also renames some APIs that are common between switch and hsr mode with '_fw_offload' suffix.
[...]
quoted hunk ↗ jump to hunk
@@ -726,6 +744,19 @@ static void emac_ndo_set_rx_mode(struct net_device *ndev) queue_work(emac->cmd_wq, &emac->rx_mode_work); } +static int emac_ndo_set_features(struct net_device *ndev, + netdev_features_t features) +{ + netdev_features_t hsr_feature_present = ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
Maybe you could give this definition and/or this variable shorter names, so that you won't cross 80 cols?
+ netdev_features_t hsr_feature_wanted = features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
(same)
+ bool hsr_change_request = ((hsr_feature_wanted ^ hsr_feature_present) != 0);
You don't need to compare with zero. Just = a ^ b. Type `bool` takes care of this.
+ + if (hsr_change_request) + ndev->features = features;
Does it mean you reject any feature change except HSR?
quoted hunk ↗ jump to hunk
+ + return 0; +} + static const struct net_device_ops emac_netdev_ops = { .ndo_open = emac_ndo_open, .ndo_stop = emac_ndo_stop,@@ -737,6 +768,7 @@ static const struct net_device_ops emac_netdev_ops = { .ndo_eth_ioctl = icssg_ndo_ioctl, .ndo_get_stats64 = icssg_ndo_get_stats64, .ndo_get_phys_port_name = icssg_ndo_get_phys_port_name, + .ndo_set_features = emac_ndo_set_features, }; static int prueth_netdev_init(struct prueth *prueth,@@ -865,6 +897,7 @@ static int prueth_netdev_init(struct prueth *prueth, ndev->ethtool_ops = &icssg_ethtool_ops; ndev->hw_features = NETIF_F_SG; ndev->features = ndev->hw_features; + ndev->hw_features |= NETIF_F_HW_HSR_FWD;
Why not HSR_OFFLOAD right away, so that you wouldn't need to replace this line with the mentioned def a commit later?
netif_napi_add(ndev, &emac->napi_rx, icssg_napi_rx_poll); hrtimer_init(&emac->rx_hrtimer, CLOCK_MONOTONIC,
[...]
+ prueth->hsr_members |= BIT(emac->port_id);
+ if (!prueth->is_switch_mode && !prueth->is_hsr_offload_mode) {
+ if (prueth->hsr_members & BIT(PRUETH_PORT_MII0) &&
+ prueth->hsr_members & BIT(PRUETH_PORT_MII1)) {
+ if (!(emac0->ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES) &&
+ !(emac1->ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES))
+ return -EOPNOTSUPP;
+ prueth->is_hsr_offload_mode = true;
+ prueth->default_vlan = 1;
+ emac0->port_vlan = prueth->default_vlan;
+ emac1->port_vlan = prueth->default_vlan;
+ icssg_change_mode(prueth);
+ dev_dbg(prueth->dev, "Enabling HSR offload mode\n");netdev_dbg()?
+ }
+ }
+
+ return 0;
+}
+
+static void prueth_hsr_port_unlink(struct net_device *ndev)
+{
+ struct prueth_emac *emac = netdev_priv(ndev);
+ struct prueth *prueth = emac->prueth;
+ struct prueth_emac *emac0;
+ struct prueth_emac *emac1;
+
+ emac0 = prueth->emac[PRUETH_MAC0];
+ emac1 = prueth->emac[PRUETH_MAC1];
+
+ prueth->hsr_members &= ~BIT(emac->port_id);
+ if (prueth->is_hsr_offload_mode) {
+ prueth->is_hsr_offload_mode = false;
+ emac0->port_vlan = 0;
+ emac1->port_vlan = 0;
+ prueth->hsr_dev = NULL;
+ prueth_emac_restart(prueth);
+ dev_dbg(prueth->dev, "Enabling Dual EMAC mode\n");(same here and in all the places below)
+ } +}
Thanks, Olek