Skip to content

ENRT/ConfigMixins/DevNfcRxFlowHashConfigMixin.py: add support for missing keys#425

Closed
jtluka wants to merge 3 commits intoLNST-project:masterfrom
jtluka:fix-nfcrx
Closed

ENRT/ConfigMixins/DevNfcRxFlowHashConfigMixin.py: add support for missing keys#425
jtluka wants to merge 3 commits intoLNST-project:masterfrom
jtluka:fix-nfcrx

Conversation

@jtluka
Copy link
Collaborator

@jtluka jtluka commented Jan 15, 2026

Description

Resolves the issue we've seen in downstream (job id 12174708) when the ethtool's output for nfc-rx-flow-hash reported an unsupported key.

The fix is to add support for the missing keys.

Tests

Job id 12184448 Job id v2 12186672

Tests newly added settings:

  • "L2DA": "m",
  • "L3 proto": "t",
  • "VLAN tag": "v",

Reviews

@olichtne

…unsupported keys

Signed-off-by: Jan Tluka <jtluka@redhat.com>
@jtluka jtluka requested a review from olichtne January 15, 2026 11:04
@jtluka jtluka self-assigned this Jan 15, 2026
@jtluka
Copy link
Collaborator Author

jtluka commented Jan 15, 2026

Downstream job contains an issue, this will need additional analysis and fix.

@jtluka
Copy link
Collaborator Author

jtluka commented Jan 15, 2026

So it seems that the ethtool output completely changed the format.

On the left the new format breaking LNST parsing, on the right the old format:

 '_res': {'passed': True,                                                                      | '_res': {'passed': True,                                                                      
          'res_data': {'stderr': '',                                                           |          'res_data': {'stderr': '',                                            
                       'stdout': 'UDP over IPV4 flows use these fields for '                   |                       'stdout': 'UDP over IPV4 flows use these fields for '                   
                                 'computing Hash flow key:\n'                                  |                                 'computing Hash flow key:\n'                                  
                                 'L2DA\n'                                                      |                                 'IP SA\n'                                                     
                                 'L3 proto\n'                                                  |                                 'IP DA\n'                                                     
                                 '\n'},                                                        |                                 'L4 bytes 0 & 1 [TCP/UDP src port]\n'                         
          'type': 'result'},                                                                   |                                 'L4 bytes 2 & 3 [TCP/UDP dst port]\n'                         
 '_what': 'ethtool -n ens4f0np0 rx-flow-hash udp4'}                                            |                                 '\n'},                                                        
{'_desc': None,                                                                                |          'type': 'result'},                                                                   

…ields

Signed-off-by: Jan Tluka <jtluka@redhat.com>
…e setting list is empty

Signed-off-by: Jan Tluka <jtluka@redhat.com>
@jtluka
Copy link
Collaborator Author

jtluka commented Jan 15, 2026

So it seems that the ethtool output completely changed the format.

On the left the new format breaking LNST parsing, on the right the old format:

 '_res': {'passed': True,                                                                      | '_res': {'passed': True,                                                                      
          'res_data': {'stderr': '',                                                           |          'res_data': {'stderr': '',                                            
                       'stdout': 'UDP over IPV4 flows use these fields for '                   |                       'stdout': 'UDP over IPV4 flows use these fields for '                   
                                 'computing Hash flow key:\n'                                  |                                 'computing Hash flow key:\n'                                  
                                 'L2DA\n'                                                      |                                 'IP SA\n'                                                     
                                 'L3 proto\n'                                                  |                                 'IP DA\n'                                                     
                                 '\n'},                                                        |                                 'L4 bytes 0 & 1 [TCP/UDP src port]\n'                         
          'type': 'result'},                                                                   |                                 'L4 bytes 2 & 3 [TCP/UDP dst port]\n'                         
 '_what': 'ethtool -n ens4f0np0 rx-flow-hash udp4'}                                            |                                 '\n'},                                                        
{'_desc': None,                                                                                |          'type': 'result'},                                                                   

I've checked the ethtool source code and the L2DA and L3 proto are supported. It could be that the NICs for some reason have different values configured.

We should add support for all possible settings.

@jtluka jtluka changed the title ENRT/ConfigMixins/DevNfcRxFlowHashConfigMixin.py: don't error out on unsupported keys ENRT/ConfigMixins/DevNfcRxFlowHashConfigMixin.py: add support for missing keys Jan 15, 2026
@jtluka jtluka marked this pull request as draft January 15, 2026 15:31
@jtluka
Copy link
Collaborator Author

jtluka commented Jan 20, 2026

So I checked the Intel ice and Nvidia mlx5_core NIC whether it is possible to set any of the "m", "t", or "v" and none of these can be set with ethtool reporting back with "Invalid argument error.

With that I somehow doubt that this MR is actually needed.

@jtluka
Copy link
Collaborator Author

jtluka commented Jan 27, 2026

So I checked the Intel ice and Nvidia mlx5_core NIC whether it is possible to set any of the "m", "t", or "v" and none of these can be set with ethtool reporting back with "Invalid argument error.

With that I somehow doubt that this MR is actually needed.

bnxt_en should support "v" and "m". I will do a test run on that NIC to check the MR correctness.

@jtluka
Copy link
Collaborator Author

jtluka commented Jan 27, 2026

So I checked the Intel ice and Nvidia mlx5_core NIC whether it is possible to set any of the "m", "t", or "v" and none of these can be set with ethtool reporting back with "Invalid argument error.
With that I somehow doubt that this MR is actually needed.

bnxt_en should support "v" and "m". I will do a test run on that NIC to check the MR correctness.

testing in J:12248420

@jtluka
Copy link
Collaborator Author

jtluka commented Jan 27, 2026

So I checked the Intel ice and Nvidia mlx5_core NIC whether it is possible to set any of the "m", "t", or "v" and none of these can be set with ethtool reporting back with "Invalid argument error.
With that I somehow doubt that this MR is actually needed.

bnxt_en should support "v" and "m". I will do a test run on that NIC to check the MR correctness.

testing in J:12248420

I had to update the test configurations however none of m/v/t cannot be configured on the bnxt_en NICs we have. Since these values are not commonly used, I will remove support for these options as they're not really needed and only include the patch to not raise error but instead just log an error message.

@jtluka
Copy link
Collaborator Author

jtluka commented Jan 27, 2026

I will completely drop this MR. The patches were kind of workaround for a broken ethtool kernel code, so there's no real need of it ...

@jtluka jtluka closed this Jan 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant