Re: [PATCH net-next v3 3/6] net: ti: icssg-prueth: Add support for HSR frame forward offload

[Alexander Lobakin <aleksander.lobakin@intel.com>]

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.

[...]

> @@ -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?

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