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

[Anwar, Md Danish]

Hi Roger,

On 9/10/2024 5:42 PM, Roger Quadros wrote:
> Hi,
> 
> On 06/09/2024 14:15, MD Danish Anwar wrote:
>> 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
>> restarted and the last used firmwares will be reloaded. PRUeth
>> interfaces will be back to the last used mode.
>>
>> This commit also renames some APIs that are common between switch and
>> hsr mode with '_fw_offload' suffix.
>>
>> Signed-off-by: MD Danish Anwar <danishanwar@xxxxxx>
>> ---
>>  .../net/ethernet/ti/icssg/icssg_classifier.c  |   1 +
>>  drivers/net/ethernet/ti/icssg/icssg_config.c  |  18 +--
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c  | 132 +++++++++++++++++-
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h  |   6 +
>>  4 files changed, 145 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_classifier.c b/drivers/net/ethernet/ti/icssg/icssg_classifier.c
>> index 9ec504d976d6..833ca86d0b71 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_classifier.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_classifier.c
> 
> <snip>
> 
>> +static void emac_change_hsr_feature(struct net_device *ndev,
>> +				    netdev_features_t features,
>> +				    u64 hsr_feature)
>> +{
>> +	netdev_features_t changed = ndev->features ^ features;
>> +
>> +	if (changed & hsr_feature) {
>> +		if (features & hsr_feature)
>> +			ndev->features |= hsr_feature;
>> +		else
>> +			ndev->features &= ~hsr_feature;
> 
> You are not supposed to change ndev->features here.
> 
> From
> "https://www.kernel.org/doc/Documentation/networking/netdev-features.txt";
> "
>  * ndo_set_features:
> 
> Hardware should be reconfigured to match passed feature set. The set
> should not be altered unless some error condition happens that can't
> be reliably detected in ndo_fix_features. In this case, the callback
> should update netdev->features to match resulting hardware state.
> Errors returned are not (and cannot be) propagated anywhere except dmesg.
> (Note: successful return is zero, >0 means silent error.)"
> 
> This means only in 
> 
>> +	}
>> +}
>> +
>> +static int emac_ndo_set_features(struct net_device *ndev,
>> +				 netdev_features_t features)
>> +{
>> +	emac_change_hsr_feature(ndev, features, NETIF_F_HW_HSR_FWD);
>> +	emac_change_hsr_feature(ndev, features, NETIF_F_HW_HSR_DUP);
>> +	emac_change_hsr_feature(ndev, features, NETIF_F_HW_HSR_TAG_INS);
>> +	emac_change_hsr_feature(ndev, features, NETIF_F_HW_HSR_TAG_RM);
> 
> I don't understand this part. 
> 
> As you are not changing hardware state in ndo_set_features, I'm not sure why
> you even need ndo_set_features callback.
> 

We don't need to do any hardware configuration when the feature is
changed. Instead certain APIs will do certain actions based on which HSR
related feature is set in ndev->features. I agree this makes the whole
API redundant. Previously in v3
(https://lore.kernel.org/all/20240828091901.3120935-4-danishanwar@xxxxxx/)
also this API was essentially just changing ndev->features when hsr
change was requested.

+static int emac_ndo_set_features(struct net_device *ndev,
+				 netdev_features_t features)
+{
...

+
+	if (hsr_change_request)
+		ndev->features = features;
+
+	return 0;
+}

I think it would be better to drop this call back in the driver as no
action is needed in the hardware based on what feature is set here.

> You didn't take my feedback about using ndo_fix_features().
> 

Roger, I have taken your feedback regarding ndo_fix_features() and
implemented it in the next patch (patch 4/5
https://lore.kernel.org/all/20240906111538.1259418-5-danishanwar@xxxxxx/).
Please have a look at that patch also. The features that have
dependencies that can be addressed in ndo_fix_features, are not
introduced in this patch so I have not introduced the function here. It
is getting introduced in the next patch.

> Please read this.
> https://www.kernel.org/doc/Documentation/networking/netdev-features.txt
> "Part II: Controlling enabled features"
> "Part III: Implementation hints"
> 
> Also look at how _netdev_update_features() works and calls ndo_fix_features()
> and ndo_set_features()
> 

Thanks for this. I checked that. Based on how _netdev_update_features()
is implemented, I don't think there is any need of ndo_set_features.
Whatever hardware dependencies are related to these HSR features, can be
taken care by ndo_fix_features.

Please let me know how does this look to you.

> https://elixir.bootlin.com/linux/v6.11-rc7/source/net/core/dev.c#L10023
> 
>> +
>> +	return 0;
>> +}
>> +
>>  static const struct net_device_ops emac_netdev_ops = {
>>  	.ndo_open = emac_ndo_open,
>>  	.ndo_stop = emac_ndo_stop,
>> @@ -737,6 +780,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,
>>  };
> 
> <snip>
> 

-- 
Thanks and Regards,
Md Danish Anwar