Hello developers,
I noticed an inconsistency in i40e_common.c file [1]. I don't think it is
causing any functionality issue, but it looks interesting to me. The function
i40e_aq_get_phy_capabilities() has different behavior for X722 and XL710
MACs. An early call to i40e_aq_get_phy_capabilities(report_init = true)
results in:
* function call i40e_aq_get_phy_capabilities() -> i40e_aq_get_link_info()
if the MAC is XL710
* no call to i40e_aq_get_link_info() if MAC is X722
As a consequence, after the first call to i40e_aq_get_phy_capabilities()
the NIC driver fills fields in i40e->hw.phy.link_info structure,
but only for XL710 (and only if FW version is not too old).
Why the driver doesn't call i40e_aq_get_phy_capabilities() for X722
(if the FW version is not too old)?
It looks quite strange to me, I could not find the reason why
the difference is there.
If there's no underlying issue, I would like to see the code treating X722 and
XL710 equally. Yes I noticed the comments in code that the response to AQ cmd
0x604 is a little bit different on X722, the FW doesn't seem to provide valid
link_type* fields in response, but the code might deal with that. Please see an
example (pseudo-patch):
i40e_aq_get_link_info()
...
if (report_init) {
if (hw->mac.type == I40E_MAC_XL710 &&
hw->aq.api_maj_ver == I40E_FW_API_VERSION_MAJOR &&
hw->aq.api_min_ver >= I40E_MINOR_VER_GET_LINK_INFO_XL710) {
status = i40e_aq_get_link_info(hw, true, NULL, NULL);
- } else {
- hw->phy.phy_types = LE32_TO_CPU(abilities->phy_type);
- hw->phy.phy_types |=
- ((u64)abilities->phy_type_ext << 32);
+ } else if (hw->mac.type == I40E_MAC_X722 &&
+ hw->aq.api_maj_ver == I40E_FW_API_VERSION_MAJOR &&
+ hw->aq.api_min_ver >= I40E_MINOR_VER_GET_LINK_INFO_X722) {
status = i40e_aq_get_link_info(hw, true, NULL, NULL);
}
}
The pseudo-patch doesn't solve the 'hw->phy.phy_types' question.
There are two options what to do with 'hw->phy.phy_types':
a) remove it completely (including declaration), it seems nobody reads it
b) initialize 'hw->phy.phy_types' sometimes in i40e_aq_get_link_info() and
sometimes in i40e_aq_get_phy_capabilities() as the current code does. The
i40e_aq_get_phy_capabilities() could check if 'phy_types' were set by
i40e_aq_get_link_info(), if not, then do the initialization in
i40e_aq_get_phy_capabilities(), e.g. keep the 3 lines that are
removed in the above pseudo-patch, just add them behind some check to
execute them only if the i40e_aq_get_link_info() did not already set
that field.
Thanks,
Vita
[1] https://github.com/DPDK/dpdk/blob/main/drivers/net/i40e/base/i40e_common.c
