inconsistency in i40e_aq_get_phy_capabilities() [i40e_common.c]

inconsistency in i40e_aq_get_phy_capabilities() [i40e_common.c]

[Vita Batrla]

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