* [PATCH net-next 01/15] ice: add support for unmanaged DPLL on E830 NIC
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
@ 2026-08-05 21:35 ` Tony Nguyen
2026-08-11 0:33 ` Jakub Kicinski
2026-08-05 21:35 ` [PATCH net-next 02/15] ice: always do GCS if hardware supports it Tony Nguyen
` (14 subsequent siblings)
15 siblings, 1 reply; 27+ messages in thread
From: Tony Nguyen @ 2026-08-05 21:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Arkadiusz Kubalewski, anthony.l.nguyen, przemyslaw.kitszel,
grzegorz.nitka, zoltan.fodor, vgrinber, corbet, skhan, linux-doc,
horms, jiri, shshitrit, richardcochran, Aleksandr Loktionov,
Paul Menzel, Sunitha Mekala
From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Hardware variants of E830 may support an unmanaged DPLL where the
configuration is hardcoded within the hardware and firmware, meaning
users cannot modify settings. However, users are able to check the DPLL
lock status and obtain configuration information through the Linux DPLL
and devlink health subsystem.
Availability of 'loss of lock' health status code determines if such
support is available, if true, register single DPLL device with 1 input
and 1 output and provide hardcoded/read only properties of a pin and
DPLL device. User is only allowed to check DPLL device status and receive
notifications on DPLL lock status change.
When present, the DPLL device locks to an external signal provided
through the PCIe/OCP pin. The expected input signal is 1PPS
(1 Pulse Per Second) embedded on a 10MHz reference clock.
The DPLL produces output:
- for MAC (Media Access Control) & PHY (Physical Layer) clocks,
- 1PPS for synchronization of onboard PHC (Precision Hardware Clock) timer.
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
.../device_drivers/ethernet/intel/ice.rst | 80 +++++
.../net/ethernet/intel/ice/devlink/health.c | 6 +
.../net/ethernet/intel/ice/ice_adminq_cmd.h | 12 +
drivers/net/ethernet/intel/ice/ice_common.c | 136 ++++++++
drivers/net/ethernet/intel/ice/ice_common.h | 8 +
drivers/net/ethernet/intel/ice/ice_dpll.c | 315 ++++++++++++++++--
drivers/net/ethernet/intel/ice/ice_dpll.h | 10 +
drivers/net/ethernet/intel/ice/ice_main.c | 11 +-
drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 46 +++
drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 1 +
10 files changed, 602 insertions(+), 23 deletions(-)
diff --git a/Documentation/networking/device_drivers/ethernet/intel/ice.rst b/Documentation/networking/device_drivers/ethernet/intel/ice.rst
index 0bca293cf9cb..82b3ccc7027e 100644
--- a/Documentation/networking/device_drivers/ethernet/intel/ice.rst
+++ b/Documentation/networking/device_drivers/ethernet/intel/ice.rst
@@ -941,6 +941,86 @@ To see input signal on those PTP pins, you need to configure DPLL properly.
Output signal is only visible on DPLL and to send it to the board SMA/U.FL pins,
DPLL output pins have to be manually configured.
+Unmanaged DPLL Support
+----------------------
+Hardware variants of E830 may support an unmanaged DPLL:
+
+- Intel(R) Ethernet Network Adapter E830-XXVDA8F for OCP 3.0,
+
+- Intel(R) Ethernet Network Adapter E830-XXVDA4F.
+
+In the case of the unmanaged DPLL, the configuration is hardcoded within the
+hardware and firmware, meaning users cannot modify settings. However,
+users can check the DPLL lock status and obtain configuration information
+through the Linux DPLL subsystem.
+
+When present, the DPLL device locks to an external signal provided through the
+PCIe/OCP pin. The expected input signal is 1PPS (1 Pulse Per Second) embedded
+on a 10MHz reference clock.
+The DPLL produces output:
+
+- for MAC (Media Access Control) & PHY (Physical Layer) clocks,
+
+- 1PPS for synchronization of onboard PHC (Precision Hardware Clock) timer.
+
+Example output of querying the Linux DPLL subsystem can be found below.
+
+.. code-block:: console
+ :caption: Dumping the DPLL pins
+
+ $ <ynl> --spec Documentation/netlink/specs/dpll.yaml --dump pin-get
+ [{'board-label': '1588-TIME_SYNC',
+ 'capabilities': set(),
+ 'clock-id': 282574471561216,
+ 'esync-frequency': 1,
+ 'esync-frequency-supported': [{'frequency-max': 1, 'frequency-min': 1}],
+ 'esync-pulse': 25,
+ 'frequency': 10000000,
+ 'id': 13,
+ 'module-name': 'ice',
+ 'parent-device': [{'direction': 'input',
+ 'parent-id': 6,
+ 'state': 'connected'}],
+ 'phase-adjust-max': 0,
+ 'phase-adjust-min': 0,
+ 'type': 'ext'},
+ {'board-label': 'MAC-PHY-CLK',
+ 'capabilities': set(),
+ 'clock-id': 282574471561216,
+ 'frequency': 156250000,
+ 'id': 14,
+ 'module-name': 'ice',
+ 'parent-device': [{'direction': 'output',
+ 'parent-id': 6,
+ 'state': 'connected'}],
+ 'phase-adjust-max': 0,
+ 'phase-adjust-min': 0,
+ 'type': 'synce-eth-port'},
+ {'board-label': '1588-TIME_REF',
+ 'capabilities': set(),
+ 'clock-id': 282574471561216,
+ 'frequency': 1,
+ 'id': 15,
+ 'module-name': 'ice',
+ 'parent-device': [{'direction': 'output',
+ 'parent-id': 6,
+ 'state': 'connected'}],
+ 'phase-adjust-max': 0,
+ 'phase-adjust-min': 0,
+ 'type': 'int-oscillator'}]
+
+.. code-block:: console
+ :caption: Dumping the DPLL devices
+
+ $ <ynl> --spec Documentation/netlink/specs/dpll.yaml --dump device-get
+ [{'clock-id': 282574471561216,
+ 'id': 6,
+ 'lock-status': 'locked',
+ 'mode': 'manual',
+ 'mode-supported': ['manual'],
+ 'module-name': 'ice',
+ 'type': 'pps'}]
+
GNSS module
-----------
Requires kernel compiled with CONFIG_GNSS=y or CONFIG_GNSS=m.
diff --git a/drivers/net/ethernet/intel/ice/devlink/health.c b/drivers/net/ethernet/intel/ice/devlink/health.c
index 8e9a8a8178d4..a83eb9f104c8 100644
--- a/drivers/net/ethernet/intel/ice/devlink/health.c
+++ b/drivers/net/ethernet/intel/ice/devlink/health.c
@@ -101,6 +101,8 @@ static const struct ice_health_status ice_health_status_lookup[] = {
"Supplied MIB file is invalid. DCB reverted to default configuration.",
"Disable FW-LLDP and check DCBx system configuration.",
{ice_port_number_label, "MIB ID"}},
+ {ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK, "Local DPLL lock status",
+ NULL,},
};
static int ice_health_status_lookup_compare(const void *a, const void *b)
@@ -242,6 +244,10 @@ void ice_process_health_status_event(struct ice_pf *pf, struct ice_rq_event_info
pf->health_reporters.fw_status = *health_info;
devlink_health_report(pf->health_reporters.fw,
"FW syndrome reported", NULL);
+ if (status_code == ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK &&
+ test_bit(ICE_FLAG_DPLL, pf->flags) &&
+ pf->dplls.unmanaged)
+ ice_dpll_lock_state_set_unmanaged(pf, health_info, true);
break;
case ICE_AQC_HEALTH_STATUS_PF:
case ICE_AQC_HEALTH_STATUS_PORT:
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 42878abac9eb..848ac5834297 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1500,6 +1500,7 @@ struct ice_aqc_get_link_topo {
#define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575 0x21
#define ICE_AQC_GET_LINK_TOPO_NODE_NR_ZL30632_80032 0x24
#define ICE_AQC_GET_LINK_TOPO_NODE_NR_SI5383_5384 0x25
+#define ICE_AQC_GET_LINK_TOPO_NODE_NR_ZL80640 0x27
#define ICE_AQC_GET_LINK_TOPO_NODE_NR_E822_PHY 0x30
#define ICE_AQC_GET_LINK_TOPO_NODE_NR_C827 0x31
#define ICE_AQC_GET_LINK_TOPO_NODE_NR_GEN_CLK_MUX 0x47
@@ -2483,11 +2484,14 @@ enum ice_aqc_health_status {
ICE_AQC_HEALTH_STATUS_ERR_BMC_RESET = 0x50B,
ICE_AQC_HEALTH_STATUS_ERR_LAST_MNG_FAIL = 0x50C,
ICE_AQC_HEALTH_STATUS_ERR_RESOURCE_ALLOC_FAIL = 0x50D,
+ ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK = 0x601,
ICE_AQC_HEALTH_STATUS_ERR_FW_LOOP = 0x1000,
ICE_AQC_HEALTH_STATUS_ERR_FW_PFR_FAIL = 0x1001,
ICE_AQC_HEALTH_STATUS_ERR_LAST_FAIL_AQ = 0x1002,
};
+#define ICE_AQC_HEALTH_STATUS_CODE_NUM 64
+
/* Get Health Status (indirect 0xFF22) */
struct ice_aqc_get_health_status {
__le16 health_status_count;
@@ -2514,6 +2518,13 @@ struct ice_aqc_health_status_elem {
__le32 internal_data2;
};
+/* Get Health Status response buffer entry, (0xFF21)
+ * repeated per reported health status
+ */
+struct ice_aqc_health_status_supp_elem {
+ __le16 health_status_code;
+};
+
/* Admin Queue command opcodes */
enum ice_adminq_opc {
/* AQ commands */
@@ -2677,6 +2688,7 @@ enum ice_adminq_opc {
/* System Diagnostic commands */
ice_aqc_opc_set_health_status_cfg = 0xFF20,
+ ice_aqc_opc_get_supported_health_status_codes = 0xFF21,
ice_aqc_opc_get_health_status = 0xFF22,
/* FW Logging Commands */
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index ef1ce106f81b..2c72ed18695c 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -3048,6 +3048,29 @@ bool ice_is_cgu_in_netlist(struct ice_hw *hw)
return false;
}
+/**
+ * ice_is_unmanaged_cgu_in_netlist - check for unmanaged CGU presence
+ * @hw: pointer to the hw struct
+ *
+ * Check if the unmanaged Clock Generation Unit (CGU) device is present in the netlist.
+ * Save the CGU part number in the hw structure for later use.
+ * Return:
+ * * true - unmanaged cgu is present
+ * * false - unmanaged cgu is not present
+ */
+bool ice_is_unmanaged_cgu_in_netlist(struct ice_hw *hw)
+{
+ if (!ice_find_netlist_node(hw, ICE_AQC_LINK_TOPO_NODE_TYPE_CLK_CTRL,
+ ICE_AQC_LINK_TOPO_NODE_CTX_GLOBAL,
+ ICE_AQC_GET_LINK_TOPO_NODE_NR_ZL80640,
+ NULL)) {
+ hw->cgu_part_number = ICE_AQC_GET_LINK_TOPO_NODE_NR_ZL80640;
+ return true;
+ }
+
+ return false;
+}
+
/**
* ice_is_gps_in_netlist
* @hw: pointer to the hw struct
@@ -6312,6 +6335,119 @@ bool ice_is_fw_health_report_supported(struct ice_hw *hw)
ICE_FW_API_HEALTH_REPORT_PATCH);
}
+/**
+ * ice_aq_get_health_status_supported - get supported health status codes
+ * @hw: pointer to the HW struct
+ * @buff: pointer to buffer where health status elements will be stored
+ * @num: number of health status elements buffer can hold
+ *
+ * Return:
+ * * 0 - success,
+ * * negative - AQ error code.
+ */
+static int
+ice_aq_get_health_status_supported(struct ice_hw *hw,
+ struct ice_aqc_health_status_supp_elem *buff,
+ int num)
+{
+ u16 code = ice_aqc_opc_get_supported_health_status_codes;
+ struct libie_aq_desc desc;
+
+ ice_fill_dflt_direct_cmd_desc(&desc, code);
+
+ return ice_aq_send_cmd(hw, &desc, buff, num * sizeof(*buff), NULL);
+}
+
+/**
+ * ice_aq_get_health_status - get current health status array from the firmware
+ * @hw: pointer to the HW struct
+ * @buff: pointer to buffer where health status elements will be stored
+ * @num: number of health status elements buffer can hold
+ *
+ * Return:
+ * * 0 - success,
+ * * negative - AQ error code.
+ */
+int ice_aq_get_health_status(struct ice_hw *hw,
+ struct ice_aqc_health_status_elem *buff, int num)
+{
+ struct libie_aq_desc desc;
+
+ ice_fill_dflt_direct_cmd_desc(&desc,
+ ice_aqc_opc_get_health_status);
+
+ return ice_aq_send_cmd(hw, &desc, buff, num * sizeof(*buff), NULL);
+}
+
+/**
+ * ice_is_health_status_code_supported - check if health status code is supported
+ * @hw: pointer to the hardware structure
+ * @code: health status code to check
+ * @supported: pointer to boolean result
+ *
+ * Return: 0 on success, negative error code otherwise
+ */
+int ice_is_health_status_code_supported(struct ice_hw *hw, u16 code,
+ bool *supported)
+{
+ const int BUFF_SIZE = ICE_AQC_HEALTH_STATUS_CODE_NUM;
+ struct ice_aqc_health_status_supp_elem *buff;
+ int ret;
+
+ *supported = false;
+ buff = kzalloc_objs(*buff, BUFF_SIZE);
+ if (!buff)
+ return -ENOMEM;
+ ret = ice_aq_get_health_status_supported(hw, buff, BUFF_SIZE);
+ if (ret)
+ goto free_buff;
+ for (int i = 0; i < BUFF_SIZE && buff[i].health_status_code; i++)
+ if (le16_to_cpu(buff[i].health_status_code) == code) {
+ *supported = true;
+ break;
+ }
+
+free_buff:
+ kfree(buff);
+ return ret;
+}
+
+/**
+ * ice_get_last_health_status_code - get last health status for given code
+ * @hw: pointer to the hardware structure
+ * @out: pointer to the health status struct to be filled
+ * @code: health status code to check
+ *
+ * Return: 0 on success, negative error code otherwise
+ */
+int ice_get_last_health_status_code(struct ice_hw *hw,
+ struct ice_aqc_health_status_elem *out,
+ u16 code)
+{
+ const int BUFF_SIZE = ICE_AQC_HEALTH_STATUS_CODE_NUM;
+ struct ice_aqc_health_status_elem *buff;
+ int ret, last_status = -1;
+
+ buff = kzalloc_objs(*buff, BUFF_SIZE);
+ if (!buff)
+ return -ENOMEM;
+ ret = ice_aq_get_health_status(hw, buff, BUFF_SIZE);
+ if (ret)
+ goto free_buff;
+ for (int i = 0; i < BUFF_SIZE && buff[i].health_status_code; i++)
+ if (le16_to_cpu(buff[i].health_status_code) == code)
+ last_status = i;
+
+ if (last_status >= 0)
+ memcpy(out, &buff[last_status], sizeof(*out));
+ else
+ memset(out, 0, sizeof(*out));
+
+free_buff:
+ kfree(buff);
+ return ret;
+}
+
/**
* ice_aq_set_health_status_cfg - Configure FW health events
* @hw: pointer to the HW struct
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index 9f5344212195..51036188cd39 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -162,6 +162,7 @@ ice_aq_get_phy_caps(struct ice_port_info *pi, bool qual_mods, u8 report_mode,
bool ice_is_phy_rclk_in_netlist(struct ice_hw *hw);
bool ice_is_clock_mux_in_netlist(struct ice_hw *hw);
bool ice_is_cgu_in_netlist(struct ice_hw *hw);
+bool ice_is_unmanaged_cgu_in_netlist(struct ice_hw *hw);
bool ice_is_gps_in_netlist(struct ice_hw *hw);
int
ice_aq_get_netlist_node(struct ice_hw *hw, struct ice_aqc_get_link_topo *cmd,
@@ -188,6 +189,13 @@ ice_get_link_default_override(struct ice_link_default_override_tlv *ldo,
struct ice_port_info *pi);
bool ice_is_phy_caps_an_enabled(struct ice_aqc_get_phy_caps_data *caps);
bool ice_is_fw_health_report_supported(struct ice_hw *hw);
+int ice_aq_get_health_status(struct ice_hw *hw,
+ struct ice_aqc_health_status_elem *buff, int num);
+int ice_is_health_status_code_supported(struct ice_hw *hw, u16 code,
+ bool *supported);
+int ice_get_last_health_status_code(struct ice_hw *hw,
+ struct ice_aqc_health_status_elem *out,
+ u16 code);
int ice_aq_set_health_status_cfg(struct ice_hw *hw, u8 event_source);
int ice_aq_get_phy_equalization(struct ice_hw *hw, u16 data_in, u16 op_code,
u8 serdes_num, int *output);
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
index 85a74cd6ea1f..1ca137f67dd4 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
@@ -19,6 +19,8 @@
#define ICE_DPLL_SW_PIN_INPUT_BASE_SFP 4
#define ICE_DPLL_SW_PIN_INPUT_BASE_QSFP 6
#define ICE_DPLL_SW_PIN_OUTPUT_BASE 0
+#define ICE_DPLL_HEALTH_STATUS_LOCKED 1
+#define ICE_DPLL_HEALTH_STATUS_UNLOCKED 0
#define E825_RCLK_PARENT_0_PIN_IDX 0
#define E825_RCLK_PARENT_1_PIN_IDX 1
@@ -89,6 +91,10 @@ static const struct dpll_pin_frequency ice_esync_range[] = {
DPLL_PIN_FREQUENCY_RANGE(0, DPLL_PIN_FREQUENCY_1_HZ),
};
+static const struct dpll_pin_frequency ice_esync_range_unmanaged[] = {
+ DPLL_PIN_FREQUENCY_1PPS,
+};
+
/**
* ice_dpll_is_sw_pin - check if given pin shall be controlled by SW
* @pf: private board structure
@@ -1098,9 +1104,11 @@ ice_dpll_pin_state_get(const struct dpll_pin *pin, void *pin_priv,
return -EBUSY;
mutex_lock(&pf->dplls.lock);
- ret = ice_dpll_pin_state_update(pf, p, pin_type, extack);
- if (ret)
- goto unlock;
+ if (!pf->dplls.unmanaged) {
+ ret = ice_dpll_pin_state_update(pf, p, pin_type, extack);
+ if (ret)
+ goto unlock;
+ }
if (pin_type == ICE_DPLL_PIN_TYPE_INPUT ||
pin_type == ICE_DPLL_PIN_TYPE_OUTPUT)
*state = p->state[d->dpll_idx];
@@ -2243,9 +2251,14 @@ ice_dpll_input_esync_get(const struct dpll_pin *pin, void *pin_priv,
mutex_unlock(&pf->dplls.lock);
return -EOPNOTSUPP;
}
- esync->range = ice_esync_range;
- esync->range_num = ARRAY_SIZE(ice_esync_range);
- if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN) {
+ if (pf->dplls.unmanaged) {
+ esync->range = ice_esync_range_unmanaged;
+ esync->range_num = ARRAY_SIZE(ice_esync_range_unmanaged);
+ } else {
+ esync->range = ice_esync_range;
+ esync->range_num = ARRAY_SIZE(ice_esync_range);
+ }
+ if (p->flags[0] & ICE_DPLL_IN_ESYNC_ENABLED) {
esync->freq = DPLL_PIN_FREQUENCY_1_HZ;
esync->pulse = ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT;
} else {
@@ -2876,6 +2889,19 @@ static const struct dpll_pin_ops ice_dpll_output_ops = {
.esync_get = ice_dpll_output_esync_get,
};
+static const struct dpll_pin_ops ice_dpll_input_unmanaged_ops = {
+ .frequency_get = ice_dpll_input_frequency_get,
+ .direction_get = ice_dpll_input_direction,
+ .state_on_dpll_get = ice_dpll_input_state_get,
+ .esync_get = ice_dpll_input_esync_get,
+};
+
+static const struct dpll_pin_ops ice_dpll_output_unmanaged_ops = {
+ .frequency_get = ice_dpll_output_frequency_get,
+ .direction_get = ice_dpll_output_direction,
+ .state_on_dpll_get = ice_dpll_output_state_get,
+};
+
static const struct dpll_device_ops ice_dpll_ops = {
.lock_status_get = ice_dpll_lock_status_get,
.mode_get = ice_dpll_mode_get,
@@ -3439,8 +3465,10 @@ ice_dpll_deinit_direct_pins(struct ice_pf *pf, bool cgu,
struct dpll_device *second)
{
if (cgu) {
- ice_dpll_unregister_pins(first, pins, ops, count);
- ice_dpll_unregister_pins(second, pins, ops, count);
+ if (first)
+ ice_dpll_unregister_pins(first, pins, ops, count);
+ if (second)
+ ice_dpll_unregister_pins(second, pins, ops, count);
}
ice_dpll_release_pins(pins, count);
}
@@ -3472,12 +3500,15 @@ ice_dpll_init_direct_pins(struct ice_pf *pf, bool cgu,
int ret;
ret = ice_dpll_get_pins(pf, pins, start_idx, count, pf->dplls.clock_id);
- if (ret)
+ if (!cgu || ret)
return ret;
- if (cgu) {
+
+ if (first) {
ret = ice_dpll_register_pins(first, pins, ops, count);
if (ret)
goto release_pins;
+ }
+ if (second) {
ret = ice_dpll_register_pins(second, pins, ops, count);
if (ret)
goto unregister_first;
@@ -3486,7 +3517,8 @@ ice_dpll_init_direct_pins(struct ice_pf *pf, bool cgu,
return 0;
unregister_first:
- ice_dpll_unregister_pins(first, pins, ops, count);
+ if (first)
+ ice_dpll_unregister_pins(first, pins, ops, count);
release_pins:
ice_dpll_release_pins(pins, count);
return ret;
@@ -3824,6 +3856,18 @@ static void ice_dpll_deinit_pins(struct ice_pf *pf, bool cgu)
struct ice_dpll *de = &d->eec;
struct ice_dpll *dp = &d->pps;
+ if (d->unmanaged) {
+ ice_dpll_unregister_pins(dp->dpll, inputs,
+ &ice_dpll_input_unmanaged_ops,
+ num_inputs);
+ ice_dpll_unregister_pins(dp->dpll, outputs,
+ &ice_dpll_output_unmanaged_ops,
+ num_outputs);
+ ice_dpll_release_pins(inputs, num_inputs);
+ ice_dpll_release_pins(outputs, num_outputs);
+ return;
+ }
+
ice_dpll_deinit_rclk_pin(pf);
if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825) {
ice_dpll_deinit_txclk_pins(pf);
@@ -4082,23 +4126,29 @@ static int ice_dpll_init_pins(struct ice_pf *pf, bool cgu)
const struct dpll_pin_ops *input_ops;
int ret, count;
- input_ops = &ice_dpll_input_ops;
- output_ops = &ice_dpll_output_ops;
+ if (!pf->dplls.unmanaged) {
+ input_ops = &ice_dpll_input_ops;
+ output_ops = &ice_dpll_output_ops;
+ } else {
+ input_ops = &ice_dpll_input_unmanaged_ops;
+ output_ops = &ice_dpll_output_unmanaged_ops;
+ }
ret = ice_dpll_init_direct_pins(pf, cgu, pf->dplls.inputs, 0,
pf->dplls.num_inputs, input_ops,
- pf->dplls.eec.dpll,
- pf->dplls.pps.dpll);
+ pf->dplls.eec.dpll, pf->dplls.pps.dpll);
if (ret)
return ret;
count = pf->dplls.num_inputs;
- if (cgu) {
+ if (cgu || pf->dplls.unmanaged) {
ret = ice_dpll_init_direct_pins(pf, cgu, pf->dplls.outputs,
count, pf->dplls.num_outputs,
output_ops, pf->dplls.eec.dpll,
pf->dplls.pps.dpll);
if (ret)
goto deinit_inputs;
+ if (pf->dplls.unmanaged)
+ return 0;
count += pf->dplls.num_outputs;
if (!pf->dplls.generic) {
ret = ice_dpll_init_direct_pins(pf, cgu, pf->dplls.sma,
@@ -4206,12 +4256,13 @@ ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu,
return ret;
}
d->pf = pf;
- if (cgu || pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825) {
+ if (cgu || pf->dplls.unmanaged ||
+ pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825) {
const struct dpll_device_ops *ops = &ice_dpll_ops;
if (type == DPLL_TYPE_PPS && ice_dpll_is_pps_phase_monitor(pf))
ops = &ice_dpll_pom_ops;
- if (cgu)
+ if (cgu && !pf->dplls.unmanaged)
ice_dpll_update_state(pf, d, true);
ret = dpll_device_register(d->dpll, type, ops, d);
if (ret) {
@@ -4239,6 +4290,33 @@ static void ice_dpll_deinit_worker(struct ice_pf *pf)
kthread_destroy_worker(d->kworker);
}
+/**
+ * ice_dpll_pin_freq_info - find pin frequency from supported ones
+ * @hw: pointer to the hardware structure
+ * @pin_idx: pin index
+ * @input: if input pin
+ *
+ * This function searches through the array of supported frequencies for a
+ * DPLL pin and returns single frequency pin is capable, if pin support only
+ * one frequency. Shall be used only for dpll with driver hardcoded frequency.
+ *
+ * Return:
+ * * 0 - failure, pin uses multiple frequencies,
+ * * frequency - success.
+ */
+static u64 ice_dpll_pin_freq_info(struct ice_hw *hw, u8 pin_idx, bool input)
+{
+ struct dpll_pin_frequency *freqs;
+ u8 freq_num;
+
+ /* Get supported frequencies for this pin */
+ freqs = ice_cgu_get_pin_freq_supp(hw, pin_idx, input, &freq_num);
+ if (!freqs || freq_num != 1 || freqs[0].min != freqs[0].max)
+ return 0;
+
+ return freqs[0].min;
+}
+
/**
* ice_dpll_init_worker - Initialize DPLLs periodic worker
* @pf: board private structure
@@ -4398,6 +4476,19 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
pins[i].prop.board_label = ice_cgu_get_pin_name(hw, i, input);
pins[i].prop.type = ice_cgu_get_pin_type(hw, i, input);
if (input) {
+ if (pf->dplls.unmanaged) {
+ pins[i].freq = ice_dpll_pin_freq_info(hw, i,
+ input);
+ pins[i].state[0] = DPLL_PIN_STATE_CONNECTED;
+ pins[i].status =
+ ICE_AQC_GET_CGU_IN_CFG_STATUS_ESYNC_CAP;
+ pins[i].flags[0] = ICE_DPLL_IN_ESYNC_ENABLED;
+ /* skip priority, capabilities, phase range,
+ * pin state AQ query and freq_supported -
+ * not available for unmanaged DPLL
+ */
+ continue;
+ }
ret = ice_aq_get_cgu_ref_prio(hw, de->dpll_idx, i,
&de->input_prio[i]);
if (ret)
@@ -4411,6 +4502,16 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
if (ice_dpll_is_sw_pin(pf, i, true))
pins[i].hidden = true;
} else {
+ if (pf->dplls.unmanaged) {
+ pins[i].freq = ice_dpll_pin_freq_info(hw, i,
+ input);
+ pins[i].state[0] = DPLL_PIN_STATE_CONNECTED;
+ /* skip output state caps, phase range,
+ * pin state AQ query and freq_supported -
+ * not available for unmanaged DPLL
+ */
+ continue;
+ }
ret = ice_cgu_get_output_pin_state_caps(hw, i, &caps);
if (ret)
return ret;
@@ -4428,10 +4529,13 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
pins[i].prop.freq_supported_num = freq_supp_num;
pins[i].pf = pf;
}
- if (input)
+ if (input && !pf->dplls.unmanaged) {
ret = ice_dpll_init_ref_sync_inputs(pf);
+ if (ret)
+ return ret;
+ }
- return ret;
+ return 0;
}
/**
@@ -4712,6 +4816,82 @@ static int ice_dpll_init_info_e825c(struct ice_pf *pf)
return ret;
}
+/**
+ * ice_dpll_lock_state_init_unmanaged - initialize lock state for unmanaged dpll
+ * @pf: board private structure
+ *
+ * Initialize the lock state for unmanaged DPLL by checking health status.
+ * For unmanaged DPLL, we rely on hardware autonomous operation.
+ *
+ * Return:
+ * * 0 - success
+ * * negative - init failure reason
+ */
+static int ice_dpll_lock_state_init_unmanaged(struct ice_pf *pf)
+{
+ u16 code = ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK;
+ struct ice_aqc_health_status_elem buff;
+ int ret;
+
+ ret = ice_get_last_health_status_code(&pf->hw, &buff, code);
+ if (ret)
+ return ret;
+ ice_dpll_lock_state_set_unmanaged(pf, &buff, false);
+
+ return ret;
+}
+
+/**
+ * ice_dpll_init_info_unmanaged - init dpll information for unmanaged dpll
+ * @pf: board private structure
+ *
+ * Acquire (from HW) and set basic dpll information (on pf->dplls struct).
+ * For unmanaged dpll mode.
+ *
+ * Return:
+ * * 0 - success
+ * * negative - init failure reason
+ */
+static int ice_dpll_init_info_unmanaged(struct ice_pf *pf)
+{
+ struct ice_dplls *d = &pf->dplls;
+ int ret;
+
+ d->clock_id = ice_generate_clock_id(pf);
+ d->num_inputs = ice_cgu_get_pin_num(&pf->hw, true);
+ d->num_outputs = ice_cgu_get_pin_num(&pf->hw, false);
+ ret = ice_dpll_lock_state_init_unmanaged(pf);
+ if (ret)
+ return ret;
+ d->inputs = kzalloc_objs(*d->inputs, d->num_inputs);
+ if (!d->inputs)
+ return -ENOMEM;
+
+ ret = ice_dpll_init_pins_info(pf, ICE_DPLL_PIN_TYPE_INPUT);
+ if (ret)
+ goto deinit_info;
+
+ d->outputs = kzalloc_objs(*d->outputs, d->num_outputs);
+ if (!d->outputs) {
+ ret = -ENOMEM;
+ goto deinit_info;
+ }
+
+ ret = ice_dpll_init_pins_info(pf, ICE_DPLL_PIN_TYPE_OUTPUT);
+ if (ret)
+ goto deinit_info;
+
+ d->pps.mode = DPLL_MODE_MANUAL;
+ dev_dbg(ice_pf_to_dev(pf), "%s - success, inputs:%u, outputs:%u\n",
+ __func__, d->num_inputs, d->num_outputs);
+ return 0;
+deinit_info:
+ dev_err(ice_pf_to_dev(pf), "%s - fail: d->inputs:%p, d->outputs:%p\n",
+ __func__, d->inputs, d->outputs);
+ ice_dpll_deinit_info(pf);
+ return ret;
+}
+
/**
* ice_dpll_init_info - prepare pf's dpll information structure
* @pf: board private structure
@@ -4815,6 +4995,42 @@ static int ice_dpll_init_info(struct ice_pf *pf, bool cgu)
return ret;
}
+/**
+ * ice_dpll_lock_state_set_unmanaged - determine lock state from health status
+ * @pf: board private structure
+ * @buff: health status buffer
+ * @notify: if true, notify dpll device
+ *
+ * Set unmanaged dpll lock state based on health status code and internal data.
+ * Context: Acquires and releases pf->dplls.lock (must release before notify
+ * if called).
+ */
+void ice_dpll_lock_state_set_unmanaged(struct ice_pf *pf,
+ const struct ice_aqc_health_status_elem *buff,
+ bool notify)
+{
+ u32 internal_data = le32_to_cpu(buff->internal_data1);
+ struct ice_dpll *d = &pf->dplls.pps;
+
+ if (!ice_pf_src_tmr_owned(pf))
+ return;
+
+ mutex_lock(&pf->dplls.lock);
+ if (buff->health_status_code == 0 ||
+ internal_data == ICE_DPLL_HEALTH_STATUS_LOCKED)
+ d->dpll_state = DPLL_LOCK_STATUS_LOCKED;
+ else
+ d->dpll_state = DPLL_LOCK_STATUS_UNLOCKED;
+
+ if (d->prev_dpll_state == d->dpll_state)
+ notify = false;
+ else
+ d->prev_dpll_state = d->dpll_state;
+ mutex_unlock(&pf->dplls.lock);
+ if (notify && d->dpll)
+ dpll_device_change_ntf(d->dpll);
+}
+
/**
* ice_dpll_deinit - Disable the driver/HW support for dpll subsystem
* the dpll device.
@@ -4854,9 +5070,10 @@ void ice_dpll_deinit(struct ice_pf *pf)
if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825)
cancel_work_sync(&pf->dplls.txclk_work);
- ice_dpll_deinit_pins(pf, cgu);
+ ice_dpll_deinit_pins(pf, cgu || pf->dplls.unmanaged);
if (!IS_ERR_OR_NULL(pf->dplls.pps.dpll))
- ice_dpll_deinit_dpll(pf, &pf->dplls.pps, cgu);
+ ice_dpll_deinit_dpll(pf, &pf->dplls.pps,
+ cgu || pf->dplls.unmanaged);
if (!IS_ERR_OR_NULL(pf->dplls.eec.dpll))
ice_dpll_deinit_dpll(pf, &pf->dplls.eec, cgu);
if (!IS_ERR_OR_NULL(pf->dplls.txc.dpll))
@@ -4866,6 +5083,45 @@ void ice_dpll_deinit(struct ice_pf *pf)
mutex_destroy(&pf->dplls.lock);
}
+/**
+ * ice_dpll_init_unmanaged - initialize support for unmanaged dpll subsystem
+ * @pf: board private structure
+ *
+ * Set up the device dplls for unmanaged mode, register them and pins connected
+ * within Linux dpll subsystem. Allow userspace to obtain state of DPLL.
+ *
+ * Context: Initializes pf->dplls.lock mutex.
+ */
+static void ice_dpll_init_unmanaged(struct ice_pf *pf)
+{
+ struct ice_dplls *d = &pf->dplls;
+ int err;
+
+ if (!ice_pf_src_tmr_owned(pf))
+ return;
+ mutex_init(&d->lock);
+ err = ice_dpll_init_info_unmanaged(pf);
+ if (err)
+ goto err_exit;
+ err = ice_dpll_init_dpll(pf, &pf->dplls.pps, true, DPLL_TYPE_PPS);
+ if (err)
+ goto deinit_info;
+ err = ice_dpll_init_pins(pf, true);
+ if (err)
+ goto deinit_pps;
+ set_bit(ICE_FLAG_DPLL, pf->flags);
+
+ return;
+
+deinit_pps:
+ ice_dpll_deinit_dpll(pf, &pf->dplls.pps, true);
+deinit_info:
+ ice_dpll_deinit_info(pf);
+err_exit:
+ mutex_destroy(&d->lock);
+ dev_warn(ice_pf_to_dev(pf), "DPLLs init failure err:%d\n", err);
+}
+
/**
* ice_dpll_init_e825 - initialize support for dpll subsystem
* @pf: board private structure
@@ -4975,8 +5231,23 @@ static void ice_dpll_init_e810(struct ice_pf *pf)
dev_warn(ice_pf_to_dev(pf), "DPLLs init failure err:%d\n", err);
}
+/**
+ * ice_dpll_init - initialize support for dpll subsystem
+ * @pf: board private structure
+ *
+ * Set up the device dplls, register them and pins connected within Linux dpll
+ * subsystem. Allow userspace to obtain state of DPLL and handling of DPLL
+ * configuration requests.
+ *
+ * Context: Initializes pf->dplls.lock mutex.
+ */
void ice_dpll_init(struct ice_pf *pf)
{
+ if (pf->dplls.unmanaged) {
+ ice_dpll_init_unmanaged(pf);
+ return;
+ }
+
switch (pf->hw.mac_type) {
case ICE_MAC_GENERIC_3K_E825:
ice_dpll_init_e825(pf);
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h
index 103ba3e49068..c59d746a8567 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.h
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
@@ -26,6 +26,8 @@
#define ICE_CGU_R11_SYNCE_S_BYP_CLK GENMASK(6, 1)
#define ICE_CGU_BYPASS_MUX_OFFSET_E825C 3
+#define ICE_DPLL_UNMANAGED_PIN_NUM 4
+#define ICE_DPLL_IN_ESYNC_ENABLED ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN
/**
* enum ice_dpll_pin_sw - enumerate ice software pin indices:
@@ -193,6 +195,7 @@ struct ice_dplls {
s32 output_phase_adj_max;
u32 periodic_counter;
bool generic;
+ bool unmanaged;
struct work_struct txclk_work;
bool txclk_switch_requested;
struct rw_semaphore txclk_notify_rwsem;
@@ -201,9 +204,16 @@ struct ice_dplls {
#if IS_ENABLED(CONFIG_PTP_1588_CLOCK)
void ice_dpll_init(struct ice_pf *pf);
void ice_dpll_deinit(struct ice_pf *pf);
+void ice_dpll_lock_state_set_unmanaged(struct ice_pf *pf,
+ const struct ice_aqc_health_status_elem *buff,
+ bool notify);
#else
static inline void ice_dpll_init(struct ice_pf *pf) { }
static inline void ice_dpll_deinit(struct ice_pf *pf) { }
+static inline void
+ice_dpll_lock_state_set_unmanaged(struct ice_pf *pf,
+ const struct ice_aqc_health_status_elem *buff,
+ bool notify) { }
#endif
#endif
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index d88835482d3a..cc30d8c73a41 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4720,7 +4720,9 @@ void ice_deinit_dev(struct ice_pf *pf)
static void ice_init_features(struct ice_pf *pf)
{
+ u16 code = ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK;
struct device *dev = ice_pf_to_dev(pf);
+ int err;
if (ice_is_safe_mode(pf))
return;
@@ -4732,8 +4734,15 @@ static void ice_init_features(struct ice_pf *pf)
if (ice_is_feature_supported(pf, ICE_F_GNSS))
ice_gnss_init(pf);
+ /* Initialize unmanaged DPLL detection */
+ err = ice_is_health_status_code_supported(&pf->hw, code,
+ &pf->dplls.unmanaged);
+ if (err || !ice_is_unmanaged_cgu_in_netlist(&pf->hw))
+ pf->dplls.unmanaged = false;
+
if (ice_is_feature_supported(pf, ICE_F_CGU) ||
- ice_is_feature_supported(pf, ICE_F_PHY_RCLK))
+ ice_is_feature_supported(pf, ICE_F_PHY_RCLK) ||
+ pf->dplls.unmanaged)
ice_dpll_init(pf);
/* Note: Flow director init failure is non-fatal to load */
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 8e5f97835954..f905104f95d8 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -20,6 +20,10 @@ static struct dpll_pin_frequency ice_cgu_pin_freq_10_mhz[] = {
DPLL_PIN_FREQUENCY_10MHZ,
};
+static struct dpll_pin_frequency ice_cgu_pin_freq_156_25mhz[] = {
+ DPLL_PIN_FREQUENCY_RANGE(156250000, 156250000),
+};
+
static const struct ice_cgu_pin_desc ice_e810t_sfp_cgu_inputs[] = {
{ "CVL-SDP22", ZL_REF0P, DPLL_PIN_TYPE_INT_OSCILLATOR,
ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
@@ -131,6 +135,18 @@ static const struct ice_cgu_pin_desc ice_e823_zl_cgu_outputs[] = {
{ "NONE", ZL_OUT5, 0, 0 },
};
+static const struct ice_cgu_pin_desc ice_e830_unmanaged_inputs[] = {
+ { "1588-TIME_SYNC", 0, DPLL_PIN_TYPE_EXT,
+ ARRAY_SIZE(ice_cgu_pin_freq_10_mhz), ice_cgu_pin_freq_10_mhz },
+};
+
+static const struct ice_cgu_pin_desc ice_e830_unmanaged_outputs[] = {
+ { "MAC-PHY-CLK", 0, DPLL_PIN_TYPE_SYNCE_ETH_PORT,
+ ARRAY_SIZE(ice_cgu_pin_freq_156_25mhz), ice_cgu_pin_freq_156_25mhz },
+ { "1588-TIME_REF", 1, DPLL_PIN_TYPE_INT_OSCILLATOR,
+ ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz},
+};
+
/* Low level functions for interacting with and managing the device clock used
* for the Precision Time Protocol.
*
@@ -5979,6 +5995,24 @@ ice_cgu_get_pin_desc(struct ice_hw *hw, bool input, int *size)
case ICE_DEV_ID_E823C_SGMII:
t = ice_cgu_get_pin_desc_e823(hw, input, size);
break;
+ case ICE_DEV_ID_E830CC_BACKPLANE:
+ case ICE_DEV_ID_E830CC_QSFP56:
+ case ICE_DEV_ID_E830CC_SFP:
+ case ICE_DEV_ID_E830CC_SFP_DD:
+ case ICE_DEV_ID_E830C_BACKPLANE:
+ case ICE_DEV_ID_E830C_QSFP:
+ case ICE_DEV_ID_E830C_SFP:
+ case ICE_DEV_ID_E830_XXV_BACKPLANE:
+ case ICE_DEV_ID_E830_XXV_QSFP:
+ case ICE_DEV_ID_E830_XXV_SFP:
+ if (input) {
+ t = ice_e830_unmanaged_inputs;
+ *size = ARRAY_SIZE(ice_e830_unmanaged_inputs);
+ } else {
+ t = ice_e830_unmanaged_outputs;
+ *size = ARRAY_SIZE(ice_e830_unmanaged_outputs);
+ }
+ break;
default:
break;
}
@@ -6005,6 +6039,18 @@ int ice_cgu_get_num_pins(struct ice_hw *hw, bool input)
return 0;
}
+/**
+ * ice_cgu_get_pin_num - get pin description array size
+ * @hw: pointer to the hw struct
+ * @input: if request is done against input or output pins
+ *
+ * Return: size of pin description array for given hw.
+ */
+int ice_cgu_get_pin_num(struct ice_hw *hw, bool input)
+{
+ return ice_cgu_get_num_pins(hw, input);
+}
+
/**
* ice_cgu_get_pin_type - get pin's type
* @hw: pointer to the hw struct
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index 16b1988e993d..fa1e908cebac 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -364,6 +364,7 @@ int ice_read_sma_ctrl(struct ice_hw *hw, u8 *data);
int ice_write_sma_ctrl(struct ice_hw *hw, u8 data);
int ice_ptp_read_sdp_ac(struct ice_hw *hw, __le16 *entries, uint *num_entries);
int ice_cgu_get_num_pins(struct ice_hw *hw, bool input);
+int ice_cgu_get_pin_num(struct ice_hw *hw, bool input);
enum dpll_pin_type ice_cgu_get_pin_type(struct ice_hw *hw, u8 pin, bool input);
struct dpll_pin_frequency *
ice_cgu_get_pin_freq_supp(struct ice_hw *hw, u8 pin, bool input, u8 *num);
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next 01/15] ice: add support for unmanaged DPLL on E830 NIC
2026-08-05 21:35 ` [PATCH net-next 01/15] ice: add support for unmanaged DPLL on E830 NIC Tony Nguyen
@ 2026-08-11 0:33 ` Jakub Kicinski
0 siblings, 0 replies; 27+ messages in thread
From: Jakub Kicinski @ 2026-08-11 0:33 UTC (permalink / raw)
To: anthony.l.nguyen
Cc: Jakub Kicinski, davem, pabeni, edumazet, andrew+netdev, netdev,
tactii, dima.ruinskiy, aleksandr.loktionov, pmenzel,
michalx.cohen
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: add support for unmanaged DPLL on E830 NIC
This adds support for E830 variants with an unmanaged DPLL, where the
configuration lives in hardware/firmware and userspace can only read the
lock status through the DPLL and devlink health subsystems. Detection is
based on firmware advertising the 'loss of lock' health status code plus a
ZL80640 clock-control node in the netlist, after which a single PPS DPLL
device with read-only pins is registered.
A few questions below about the new detection, notification and teardown
paths.
The commit message says:
"Availability of 'loss of lock' health status code determines if such
support is available, if true, register single DPLL device with 1 input
and 1 output"
[Severity: Low]
Should this say 1 input and 2 outputs? ice_e830_unmanaged_outputs[] added
in ice_ptp_hw.c contains both "MAC-PHY-CLK" and "1588-TIME_REF", and
ice_dpll_init_info_unmanaged() takes d->num_outputs straight from
ice_cgu_get_pin_num(&pf->hw, false), i.e. ARRAY_SIZE() = 2. The ice.rst
dump added by the same patch also lists three pins (ids 13, 14, 15), so
the documentation and the code agree with each other but not with the
changelog.
> diff --git a/drivers/net/ethernet/intel/ice/devlink/health.c b/drivers/net/ethernet/intel/ice/devlink/health.c
> index 8e9a8a8178d48..a83eb9f104c83 100644
> --- a/drivers/net/ethernet/intel/ice/devlink/health.c
> +++ b/drivers/net/ethernet/intel/ice/devlink/health.c
[ ... ]
> @@ -242,6 +244,10 @@ void ice_process_health_status_event(struct ice_pf *pf, struct ice_rq_event_info
> pf->health_reporters.fw_status = *health_info;
> devlink_health_report(pf->health_reporters.fw,
> "FW syndrome reported", NULL);
> + if (status_code == ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK &&
> + test_bit(ICE_FLAG_DPLL, pf->flags) &&
> + pf->dplls.unmanaged)
> + ice_dpll_lock_state_set_unmanaged(pf, health_info, true);
> break;
[Severity: High]
Can this race with ice_dpll_deinit()?
ICE_FLAG_DPLL and pf->dplls.unmanaged are tested here with no lock held
and without taking any reference that keeps the DPLL objects alive.
ice_process_health_status_event() runs from the service task
(__ice_clean_ctrlq()), and ice_remove() -> ice_unload() ->
ice_deinit_features() -> ice_dpll_deinit() runs before
ice_deinit_dev() -> ice_service_task_stop(), so the service task is still
live while the DPLL objects are being torn down:
CPU0 (service task) CPU1 (ice_remove)
ice_process_health_status_event()
test_bit(ICE_FLAG_DPLL) -> true
pf->dplls.unmanaged -> true
ice_dpll_lock_state_set_unmanaged()
mutex_lock(&pf->dplls.lock) <- blocks
ice_dpll_deinit()
clear_bit(ICE_FLAG_DPLL)
ice_dpll_deinit_dpll(pps)
dpll_device_put()
mutex_destroy(&dplls.lock)
... writes d->dpll_state
mutex_unlock()
dpll_device_change_ntf(d->dpll)
Two things look problematic in that window: pf->dplls.lock is taken (and
then destroyed underneath), and d->dpll is dereferenced after the device
was put. ice_dpll_deinit_dpll() calls dpll_device_put() but never sets
d->dpll = NULL, so the "if (notify && d->dpll)" guard in
ice_dpll_lock_state_set_unmanaged() cannot detect the freed device.
The driver already has the pattern for the analogous txclk notifier: the
flag is re-checked under pf->dplls.lock and in-flight notifiers are drained
with down_write()/up_write() on pf->dplls.txclk_notify_rwsem in
ice_dpll_deinit(). Note that drain is gated on
pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825, so it would not cover the E830
unmanaged case even if the new path used the rwsem. Would re-checking
ICE_FLAG_DPLL under pf->dplls.lock, clearing d->dpll in
ice_dpll_deinit_dpll(), and adding an unconditional drain be enough here?
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index 42878abac9eb1..848ac58342979 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
[ ... ]
> @@ -2483,11 +2484,14 @@ enum ice_aqc_health_status {
> ICE_AQC_HEALTH_STATUS_ERR_BMC_RESET = 0x50B,
> ICE_AQC_HEALTH_STATUS_ERR_LAST_MNG_FAIL = 0x50C,
> ICE_AQC_HEALTH_STATUS_ERR_RESOURCE_ALLOC_FAIL = 0x50D,
> + ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK = 0x601,
> ICE_AQC_HEALTH_STATUS_ERR_FW_LOOP = 0x1000,
> ICE_AQC_HEALTH_STATUS_ERR_FW_PFR_FAIL = 0x1001,
> ICE_AQC_HEALTH_STATUS_ERR_LAST_FAIL_AQ = 0x1002,
> };
>
> +#define ICE_AQC_HEALTH_STATUS_CODE_NUM 64
> +
> /* Get Health Status (indirect 0xFF22) */
> struct ice_aqc_get_health_status {
> __le16 health_status_count;
[ ... ]
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index ef1ce106f81b5..2c72ed18695c5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -3048,6 +3048,29 @@ bool ice_is_cgu_in_netlist(struct ice_hw *hw)
> return false;
> }
>
> +/**
> + * ice_is_unmanaged_cgu_in_netlist - check for unmanaged CGU presence
> + * @hw: pointer to the hw struct
> + *
> + * Check if the unmanaged Clock Generation Unit (CGU) device is present in the netlist.
> + * Save the CGU part number in the hw structure for later use.
> + * Return:
> + * * true - unmanaged cgu is present
> + * * false - unmanaged cgu is not present
> + */
> +bool ice_is_unmanaged_cgu_in_netlist(struct ice_hw *hw)
> +{
> + if (!ice_find_netlist_node(hw, ICE_AQC_LINK_TOPO_NODE_TYPE_CLK_CTRL,
> + ICE_AQC_LINK_TOPO_NODE_CTX_GLOBAL,
> + ICE_AQC_GET_LINK_TOPO_NODE_NR_ZL80640,
> + NULL)) {
> + hw->cgu_part_number = ICE_AQC_GET_LINK_TOPO_NODE_NR_ZL80640;
> + return true;
> + }
> +
> + return false;
> +}
> +
> /**
> * ice_is_gps_in_netlist
> * @hw: pointer to the hw struct
[ ... ]
> @@ -6312,6 +6335,119 @@ bool ice_is_fw_health_report_supported(struct ice_hw *hw)
[ ... ]
> +int ice_is_health_status_code_supported(struct ice_hw *hw, u16 code,
> + bool *supported)
> +{
> + const int BUFF_SIZE = ICE_AQC_HEALTH_STATUS_CODE_NUM;
> + struct ice_aqc_health_status_supp_elem *buff;
> + int ret;
> +
> + *supported = false;
> + buff = kzalloc_objs(*buff, BUFF_SIZE);
> + if (!buff)
> + return -ENOMEM;
> + ret = ice_aq_get_health_status_supported(hw, buff, BUFF_SIZE);
> + if (ret)
> + goto free_buff;
> + for (int i = 0; i < BUFF_SIZE && buff[i].health_status_code; i++)
> + if (le16_to_cpu(buff[i].health_status_code) == code) {
> + *supported = true;
> + break;
> + }
> +
> +free_buff:
> + kfree(buff);
> + return ret;
> +}
[Severity: Low]
Both new helpers scan a fixed 64-element buffer up to the first zero
health_status_code and never look at the element count firmware returns in
struct ice_aqc_get_health_status.health_status_count, so truncation cannot
be distinguished from a complete read. The driver's own event path treats
that count as authoritative:
drivers/net/ethernet/intel/ice/devlink/health.c:
count = le16_to_cpu(cmd->health_status_count);
If firmware ever reports more than ICE_AQC_HEALTH_STATUS_CODE_NUM supported
codes, would ice_is_health_status_code_supported() miss 0x601 and silently
disable the whole unmanaged DPLL feature? Would using the returned count,
or at least detecting a full buffer, be better here?
> +
> +/**
> + * ice_get_last_health_status_code - get last health status for given code
> + * @hw: pointer to the hardware structure
> + * @out: pointer to the health status struct to be filled
> + * @code: health status code to check
> + *
> + * Return: 0 on success, negative error code otherwise
> + */
> +int ice_get_last_health_status_code(struct ice_hw *hw,
> + struct ice_aqc_health_status_elem *out,
> + u16 code)
> +{
> + const int BUFF_SIZE = ICE_AQC_HEALTH_STATUS_CODE_NUM;
> + struct ice_aqc_health_status_elem *buff;
> + int ret, last_status = -1;
> +
> + buff = kzalloc_objs(*buff, BUFF_SIZE);
> + if (!buff)
> + return -ENOMEM;
> + ret = ice_aq_get_health_status(hw, buff, BUFF_SIZE);
> + if (ret)
> + goto free_buff;
> + for (int i = 0; i < BUFF_SIZE && buff[i].health_status_code; i++)
> + if (le16_to_cpu(buff[i].health_status_code) == code)
> + last_status = i;
> +
> + if (last_status >= 0)
> + memcpy(out, &buff[last_status], sizeof(*out));
> + else
> + memset(out, 0, sizeof(*out));
> +
> +free_buff:
> + kfree(buff);
> + return ret;
> +}
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
> index 9f53442121955..51036188cd396 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.h
> +++ b/drivers/net/ethernet/intel/ice/ice_common.h
[ ... ]
> @@ -188,6 +189,13 @@ ice_get_link_default_override(struct ice_link_default_override_tlv *ldo,
> struct ice_port_info *pi);
> bool ice_is_phy_caps_an_enabled(struct ice_aqc_get_phy_caps_data *caps);
> bool ice_is_fw_health_report_supported(struct ice_hw *hw);
> +int ice_aq_get_health_status(struct ice_hw *hw,
> + struct ice_aqc_health_status_elem *buff, int num);
> +int ice_is_health_status_code_supported(struct ice_hw *hw, u16 code,
> + bool *supported);
> +int ice_get_last_health_status_code(struct ice_hw *hw,
> + struct ice_aqc_health_status_elem *out,
> + u16 code);
> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
> index 85a74cd6ea1f3..1ca137f67dd48 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
> @@ -19,6 +19,8 @@
> #define ICE_DPLL_SW_PIN_INPUT_BASE_SFP 4
> #define ICE_DPLL_SW_PIN_INPUT_BASE_QSFP 6
> #define ICE_DPLL_SW_PIN_OUTPUT_BASE 0
> +#define ICE_DPLL_HEALTH_STATUS_LOCKED 1
> +#define ICE_DPLL_HEALTH_STATUS_UNLOCKED 0
[ ... ]
> @@ -4206,12 +4256,13 @@ ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu,
> return ret;
> }
> d->pf = pf;
> - if (cgu || pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825) {
> + if (cgu || pf->dplls.unmanaged ||
> + pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825) {
> const struct dpll_device_ops *ops = &ice_dpll_ops;
>
> if (type == DPLL_TYPE_PPS && ice_dpll_is_pps_phase_monitor(pf))
> ops = &ice_dpll_pom_ops;
> - if (cgu)
> + if (cgu && !pf->dplls.unmanaged)
> ice_dpll_update_state(pf, d, true);
[Severity: Medium]
Should the pom_ops selection above also exclude the unmanaged case, the
way the ice_dpll_update_state() call immediately below it does?
ice_dpll_init_unmanaged() calls
ice_dpll_init_dpll(pf, &pf->dplls.pps, true, DPLL_TYPE_PPS), so the
DPLL_TYPE_PPS branch is reachable, and ice_dpll_is_pps_phase_monitor()
returns true for every admin queue failure except LIBIE_AQ_RC_ESRCH:
if (ret && pf->hw.adminq.sq_last_status == LIBIE_AQ_RC_ESRCH)
return false;
return true;
If E830 firmware rejects ice_aq_get_cgu_input_pin_measure() with any other
error code, the device is registered with the settable ice_dpll_pom_ops,
exposing phase-offset-monitor set on a device the changelog and the new
ice.rst section describe as non-configurable. Also,
ice_dpll_phase_offset_monitor_set() only stores
d->phase_offset_monitor_period, and ice_dpll_init_unmanaged() never calls
ice_dpll_init_worker(), so nothing would ever consume it. Would adding
"&& !pf->dplls.unmanaged" to the pom_ops condition be correct here?
> @@ -4428,10 +4529,13 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
> pins[i].prop.freq_supported_num = freq_supp_num;
> pins[i].pf = pf;
> }
> - if (input)
> + if (input && !pf->dplls.unmanaged) {
> ret = ice_dpll_init_ref_sync_inputs(pf);
> + if (ret)
> + return ret;
> + }
>
> - return ret;
> + return 0;
> }
[ ... ]
> @@ -4712,6 +4816,82 @@ static int ice_dpll_init_info_e825c(struct ice_pf *pf)
> return ret;
> }
>
> +/**
> + * ice_dpll_lock_state_init_unmanaged - initialize lock state for unmanaged dpll
> + * @pf: board private structure
> + *
> + * Initialize the lock state for unmanaged DPLL by checking health status.
> + * For unmanaged DPLL, we rely on hardware autonomous operation.
> + *
> + * Return:
> + * * 0 - success
> + * * negative - init failure reason
> + */
> +static int ice_dpll_lock_state_init_unmanaged(struct ice_pf *pf)
> +{
> + u16 code = ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK;
> + struct ice_aqc_health_status_elem buff;
> + int ret;
> +
> + ret = ice_get_last_health_status_code(&pf->hw, &buff, code);
> + if (ret)
> + return ret;
> + ice_dpll_lock_state_set_unmanaged(pf, &buff, false);
> +
> + return ret;
> +}
[ ... ]
> +static int ice_dpll_init_info_unmanaged(struct ice_pf *pf)
> +{
> + struct ice_dplls *d = &pf->dplls;
> + int ret;
> +
> + d->clock_id = ice_generate_clock_id(pf);
> + d->num_inputs = ice_cgu_get_pin_num(&pf->hw, true);
> + d->num_outputs = ice_cgu_get_pin_num(&pf->hw, false);
> + ret = ice_dpll_lock_state_init_unmanaged(pf);
> + if (ret)
> + return ret;
> + d->inputs = kzalloc_objs(*d->inputs, d->num_inputs);
> + if (!d->inputs)
> + return -ENOMEM;
[Severity: Medium]
Should the pin counts be validated here? ice_cgu_get_pin_num() resolves to
ice_cgu_get_pin_desc(), whose new cases only cover E830 device IDs, so on
any other device that reaches this function both counts are 0.
kzalloc_objs(*d->inputs, 0) returns ZERO_SIZE_PTR, which is non-NULL, so
the check above does not fire, and with the
ice_dpll_init_info_direct_pins() tail now returning an unconditional 0
(pre-patch it returned the initial ret = -EINVAL when the pin loop never
executed) a pin-less DPLL device would be registered instead of the init
failing.
Related: pf->dplls.unmanaged is derived only from the firmware health-code
probe and the ZL80640 netlist search, neither of which looks at
pf->hw.mac_type or device_id, and ice_dpll_init() then returns before the
mac_type switch (see below). Would gating the detection on E830, or
rejecting num_inputs == 0, be worth adding?
[ ... ]
> @@ -4815,6 +4995,42 @@ static int ice_dpll_init_info(struct ice_pf *pf, bool cgu)
> return ret;
> }
>
> +/**
> + * ice_dpll_lock_state_set_unmanaged - determine lock state from health status
> + * @pf: board private structure
> + * @buff: health status buffer
> + * @notify: if true, notify dpll device
> + *
> + * Set unmanaged dpll lock state based on health status code and internal data.
> + * Context: Acquires and releases pf->dplls.lock (must release before notify
> + * if called).
> + */
> +void ice_dpll_lock_state_set_unmanaged(struct ice_pf *pf,
> + const struct ice_aqc_health_status_elem *buff,
> + bool notify)
> +{
> + u32 internal_data = le32_to_cpu(buff->internal_data1);
> + struct ice_dpll *d = &pf->dplls.pps;
> +
> + if (!ice_pf_src_tmr_owned(pf))
> + return;
> +
> + mutex_lock(&pf->dplls.lock);
> + if (buff->health_status_code == 0 ||
> + internal_data == ICE_DPLL_HEALTH_STATUS_LOCKED)
> + d->dpll_state = DPLL_LOCK_STATUS_LOCKED;
> + else
> + d->dpll_state = DPLL_LOCK_STATUS_UNLOCKED;
[Severity: Medium]
Is treating health_status_code == 0 as DPLL_LOCK_STATUS_LOCKED intended?
ice_get_last_health_status_code() signals "no matching record found" by
zeroing the element and still returning 0:
if (last_status >= 0)
memcpy(out, &buff[last_status], sizeof(*out));
else
memset(out, 0, sizeof(*out));
so the caller cannot tell "firmware reported nothing" apart from
"firmware reported locked". The 0x601 record can legitimately be absent
(firmware has not posted it yet, the log was cleared by a preceding reset,
or it fell outside the 64-element window), and absence of the record is
not evidence of lock. Would reporting DPLL_LOCK_STATUS_UNLOCKED (or
returning -ENOENT from ice_get_last_health_status_code()) be safer for a
clock device?
> +
> + if (d->prev_dpll_state == d->dpll_state)
> + notify = false;
> + else
> + d->prev_dpll_state = d->dpll_state;
> + mutex_unlock(&pf->dplls.lock);
> + if (notify && d->dpll)
> + dpll_device_change_ntf(d->dpll);
> +}
> +
> /**
> * ice_dpll_deinit - Disable the driver/HW support for dpll subsystem
> * the dpll device.
> @@ -4854,9 +5070,10 @@ void ice_dpll_deinit(struct ice_pf *pf)
> if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825)
> cancel_work_sync(&pf->dplls.txclk_work);
>
> - ice_dpll_deinit_pins(pf, cgu);
> + ice_dpll_deinit_pins(pf, cgu || pf->dplls.unmanaged);
> if (!IS_ERR_OR_NULL(pf->dplls.pps.dpll))
> - ice_dpll_deinit_dpll(pf, &pf->dplls.pps, cgu);
> + ice_dpll_deinit_dpll(pf, &pf->dplls.pps,
> + cgu || pf->dplls.unmanaged);
> if (!IS_ERR_OR_NULL(pf->dplls.eec.dpll))
> ice_dpll_deinit_dpll(pf, &pf->dplls.eec, cgu);
> if (!IS_ERR_OR_NULL(pf->dplls.txc.dpll))
> @@ -4866,6 +5083,45 @@ void ice_dpll_deinit(struct ice_pf *pf)
> mutex_destroy(&pf->dplls.lock);
> }
>
> +/**
> + * ice_dpll_init_unmanaged - initialize support for unmanaged dpll subsystem
> + * @pf: board private structure
> + *
> + * Set up the device dplls for unmanaged mode, register them and pins connected
> + * within Linux dpll subsystem. Allow userspace to obtain state of DPLL.
> + *
> + * Context: Initializes pf->dplls.lock mutex.
> + */
> +static void ice_dpll_init_unmanaged(struct ice_pf *pf)
> +{
> + struct ice_dplls *d = &pf->dplls;
> + int err;
> +
> + if (!ice_pf_src_tmr_owned(pf))
> + return;
> + mutex_init(&d->lock);
> + err = ice_dpll_init_info_unmanaged(pf);
> + if (err)
> + goto err_exit;
> + err = ice_dpll_init_dpll(pf, &pf->dplls.pps, true, DPLL_TYPE_PPS);
> + if (err)
> + goto deinit_info;
> + err = ice_dpll_init_pins(pf, true);
> + if (err)
> + goto deinit_pps;
> + set_bit(ICE_FLAG_DPLL, pf->flags);
[Severity: Medium]
How is the reported lock status resynchronized with hardware after init?
For the unmanaged path d->dpll_state is a pure software cache: it is seeded
once here via ice_dpll_lock_state_init_unmanaged() and afterwards only
updated by asynchronous 0x601 health events. No periodic worker is started
(unlike the managed paths), ice_dpll_lock_status_get() just returns the
cached value:
mutex_lock(&pf->dplls.lock);
*status = d->dpll_state;
mutex_unlock(&pf->dplls.lock);
and ice_dpll_init() is only called from ice_init_features(), i.e. probe,
with no re-seed after PFR/CORER/GLOBR when the control queues are torn
down and events can be missed. Would a single lost or reset-window event
leave the netlink lock-status permanently diverged from the hardware for
the rest of the driver load?
> +
> + return;
> +
> +deinit_pps:
> + ice_dpll_deinit_dpll(pf, &pf->dplls.pps, true);
> +deinit_info:
> + ice_dpll_deinit_info(pf);
> +err_exit:
> + mutex_destroy(&d->lock);
> + dev_warn(ice_pf_to_dev(pf), "DPLLs init failure err:%d\n", err);
> +}
> +
[ ... ]
> @@ -4975,8 +5231,23 @@ static void ice_dpll_init_e810(struct ice_pf *pf)
> dev_warn(ice_pf_to_dev(pf), "DPLLs init failure err:%d\n", err);
> }
>
> +/**
> + * ice_dpll_init - initialize support for dpll subsystem
> + * @pf: board private structure
> + *
> + * Set up the device dplls, register them and pins connected within Linux dpll
> + * subsystem. Allow userspace to obtain state of DPLL and handling of DPLL
> + * configuration requests.
> + *
> + * Context: Initializes pf->dplls.lock mutex.
> + */
> void ice_dpll_init(struct ice_pf *pf)
> {
> + if (pf->dplls.unmanaged) {
> + ice_dpll_init_unmanaged(pf);
> + return;
> + }
> +
> switch (pf->hw.mac_type) {
> case ICE_MAC_GENERIC_3K_E825:
> ice_dpll_init_e825(pf);
[Severity: Medium]
Continuing the earlier point: this returns before the mac_type switch, so
if pf->dplls.unmanaged is ever true on an E825 part, ice_dpll_init_e825()
and its INIT_WORK(&d->txclk_work), init_rwsem(&d->txclk_notify_rwsem) and
init_completion(&d->dpll_init) are skipped, while ice_dpll_deinit() still
branches on mac_type:
if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825) {
down_write(&pf->dplls.txclk_notify_rwsem);
up_write(&pf->dplls.txclk_notify_rwsem);
}
...
if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825)
cancel_work_sync(&pf->dplls.txclk_work);
Would that operate on a never-initialized rw_semaphore and work_struct?
Since pf->dplls.unmanaged is computed purely from the firmware health-code
probe and the generic ZL80640 netlist search, nothing structurally
prevents that combination today.
> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h
> index 103ba3e490682..c59d746a85671 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.h
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
> @@ -26,6 +26,8 @@
> #define ICE_CGU_R11_SYNCE_S_BYP_CLK GENMASK(6, 1)
>
> #define ICE_CGU_BYPASS_MUX_OFFSET_E825C 3
> +#define ICE_DPLL_UNMANAGED_PIN_NUM 4
> +#define ICE_DPLL_IN_ESYNC_ENABLED ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN
[Severity: Low]
These aren't bugs, but a few of the new definitions look unused or
duplicated:
ICE_DPLL_UNMANAGED_PIN_NUM has no reference anywhere in the tree, and its
value 4 also disagrees with the 3 pins the new tables define (1 input plus
2 outputs).
ICE_DPLL_HEALTH_STATUS_UNLOCKED in ice_dpll.c is never compared against;
ice_dpll_lock_state_set_unmanaged() only tests
ICE_DPLL_HEALTH_STATUS_LOCKED.
ice_cgu_get_pin_num() in ice_ptp_hw.c is a verbatim
"return ice_cgu_get_num_pins(hw, input);" wrapper with a near-identical
name, and both are now declared in ice_ptp_hw.h and used interchangeably
in ice_dpll.c (ice_cgu_get_num_pins() in
ice_dpll_init_info_direct_pins(), ice_cgu_get_pin_num() in
ice_dpll_init_info_unmanaged()) for counts that must stay consistent.
ice_aq_get_health_status() is declared in ice_common.h although its only
caller, ice_get_last_health_status_code(), is in the same C file, so it
could be static.
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index d88835482d3aa..cc30d8c73a41c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -4732,8 +4734,15 @@ static void ice_init_features(struct ice_pf *pf)
> if (ice_is_feature_supported(pf, ICE_F_GNSS))
> ice_gnss_init(pf);
>
> + /* Initialize unmanaged DPLL detection */
> + err = ice_is_health_status_code_supported(&pf->hw, code,
> + &pf->dplls.unmanaged);
> + if (err || !ice_is_unmanaged_cgu_in_netlist(&pf->hw))
> + pf->dplls.unmanaged = false;
> +
> if (ice_is_feature_supported(pf, ICE_F_CGU) ||
> - ice_is_feature_supported(pf, ICE_F_PHY_RCLK))
> + ice_is_feature_supported(pf, ICE_F_PHY_RCLK) ||
> + pf->dplls.unmanaged)
> ice_dpll_init(pf);
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 8e5f97835954c..f905104f95d88 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
[ ... ]
> @@ -131,6 +135,18 @@ static const struct ice_cgu_pin_desc ice_e823_zl_cgu_outputs[] = {
> { "NONE", ZL_OUT5, 0, 0 },
> };
>
> +static const struct ice_cgu_pin_desc ice_e830_unmanaged_inputs[] = {
> + { "1588-TIME_SYNC", 0, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_10_mhz), ice_cgu_pin_freq_10_mhz },
> +};
> +
> +static const struct ice_cgu_pin_desc ice_e830_unmanaged_outputs[] = {
> + { "MAC-PHY-CLK", 0, DPLL_PIN_TYPE_SYNCE_ETH_PORT,
> + ARRAY_SIZE(ice_cgu_pin_freq_156_25mhz), ice_cgu_pin_freq_156_25mhz },
> + { "1588-TIME_REF", 1, DPLL_PIN_TYPE_INT_OSCILLATOR,
> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz},
> +};
[ ... ]
> @@ -6005,6 +6039,18 @@ int ice_cgu_get_num_pins(struct ice_hw *hw, bool input)
> return 0;
> }
>
> +/**
> + * ice_cgu_get_pin_num - get pin description array size
> + * @hw: pointer to the hw struct
> + * @input: if request is done against input or output pins
> + *
> + * Return: size of pin description array for given hw.
> + */
> +int ice_cgu_get_pin_num(struct ice_hw *hw, bool input)
> +{
> + return ice_cgu_get_num_pins(hw, input);
> +}
> +
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> index 16b1988e993d2..fa1e908cebac6 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> @@ -364,6 +364,7 @@ int ice_read_sma_ctrl(struct ice_hw *hw, u8 *data);
> int ice_write_sma_ctrl(struct ice_hw *hw, u8 data);
> int ice_ptp_read_sdp_ac(struct ice_hw *hw, __le16 *entries, uint *num_entries);
> int ice_cgu_get_num_pins(struct ice_hw *hw, bool input);
> +int ice_cgu_get_pin_num(struct ice_hw *hw, bool input);
--
pw-bot: cr
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next 02/15] ice: always do GCS if hardware supports it
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
2026-08-05 21:35 ` [PATCH net-next 01/15] ice: add support for unmanaged DPLL on E830 NIC Tony Nguyen
@ 2026-08-05 21:35 ` Tony Nguyen
2026-08-11 0:35 ` Jakub Kicinski
2026-08-05 21:35 ` [PATCH net-next 03/15] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 Tony Nguyen
` (13 subsequent siblings)
15 siblings, 1 reply; 27+ messages in thread
From: Tony Nguyen @ 2026-08-05 21:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Michal Swiatkowski, anthony.l.nguyen, aleksandr.loktionov,
przemyslaw.kitszel, jramaseu, Alexander Nowlin
From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
There is no need to check for NETIF_HW_CSUM. If the code reach
calculating checksum it means that correct checksum flags are set,
because kernel is checking that when setting ip->summed.
Instead of netdev feature flag use Tx ring flag to check if the hardware
can use special descriptor for checksum calculating.
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ice/ice_lib.c | 4 ++++
drivers/net/ethernet/intel/ice/ice_txrx.c | 2 +-
drivers/net/ethernet/intel/ice/ice_txrx.h | 1 +
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 8cdc4fda89e9..fc9d4e0fa755 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -1415,6 +1415,10 @@ static int ice_vsi_alloc_rings(struct ice_vsi *vsi)
set_bit(ICE_TX_RING_FLAGS_VLAN_L2TAG2, ring->flags);
else
set_bit(ICE_TX_RING_FLAGS_VLAN_L2TAG1, ring->flags);
+
+ if (ice_is_feature_supported(pf, ICE_F_GCS))
+ set_bit(ICE_TX_RING_FLAGS_GCS, ring->flags);
+
WRITE_ONCE(vsi->tx_rings[i], ring);
}
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
index 31303ab5be17..477c86d8c53b 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
@@ -1739,7 +1739,7 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
l3_len = l4.hdr - ip.hdr;
offset |= (l3_len / 4) << ICE_TX_DESC_LEN_IPLEN_S;
- if ((tx_ring->netdev->features & NETIF_F_HW_CSUM) &&
+ if (test_bit(ICE_TX_RING_FLAGS_GCS, tx_ring->flags) &&
!(first->tx_flags & ICE_TX_FLAGS_TSO) &&
!skb_csum_is_sctp(skb)) {
/* Set GCS */
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
index 5e517f219379..15dbd5100912 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.h
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
@@ -217,6 +217,7 @@ enum ice_tx_ring_flags {
ICE_TX_RING_FLAGS_VLAN_L2TAG1,
ICE_TX_RING_FLAGS_VLAN_L2TAG2,
ICE_TX_RING_FLAGS_TXTIME,
+ ICE_TX_RING_FLAGS_GCS,
ICE_TX_RING_FLAGS_NBITS,
};
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next 02/15] ice: always do GCS if hardware supports it
2026-08-05 21:35 ` [PATCH net-next 02/15] ice: always do GCS if hardware supports it Tony Nguyen
@ 2026-08-11 0:35 ` Jakub Kicinski
0 siblings, 0 replies; 27+ messages in thread
From: Jakub Kicinski @ 2026-08-11 0:35 UTC (permalink / raw)
To: anthony.l.nguyen
Cc: Jakub Kicinski, davem, pabeni, edumazet, andrew+netdev, netdev,
tactii, dima.ruinskiy, aleksandr.loktionov, pmenzel,
michalx.cohen
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: always do GCS if hardware supports it
This replaces the NETIF_F_HW_CSUM test in ice_tx_csum() with a new
per-Tx-ring flag, ICE_TX_RING_FLAGS_GCS, which is set in
ice_vsi_alloc_rings() whenever ICE_F_GCS is supported. The E830 generic
checksum (GCS) context descriptor is therefore emitted based on hardware
capability instead of the netdev feature bit. There are no Link: tags in
the commit header.
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index 8cdc4fda89e97..fc9d4e0fa7558 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -1415,6 +1415,10 @@ static int ice_vsi_alloc_rings(struct ice_vsi *vsi)
> set_bit(ICE_TX_RING_FLAGS_VLAN_L2TAG2, ring->flags);
> else
> set_bit(ICE_TX_RING_FLAGS_VLAN_L2TAG1, ring->flags);
> +
> + if (ice_is_feature_supported(pf, ICE_F_GCS))
> + set_bit(ICE_TX_RING_FLAGS_GCS, ring->flags);
> +
> WRITE_ONCE(vsi->tx_rings[i], ring);
> }
[Severity: Medium]
The changelog says "There is no need to check for NETIF_HW_CSUM." Was
that check really redundant? In ice_set_netdev_features() the bit is
added to hw_features only, and that happens after netdev->features has
already been assigned:
netdev->features |= netdev->hw_features;
...
/* Mutual exclusivity for TSO and GCS is enforced by the set features
* ndo callback.
*/
if (ice_is_feature_supported(pf, ICE_F_GCS))
netdev->hw_features |= NETIF_F_HW_CSUM;
So NETIF_F_HW_CSUM was advertised as changeable but was off by default,
which makes the removed test the actual on/off switch for the GCS branch
rather than a redundant condition.
Since ICE_TX_RING_FLAGS_GCS is set here for every Tx ring whenever
ICE_F_GCS is supported (set for every ICE_MAC_E830 PF in
ice_init_feature_support(), and never cleared, and never resynchronized
from ndo_set_features), do GCS context descriptors now get emitted while
TSO is simultaneously enabled on the same netdev? That is the
combination ice_set_features() still refuses at this commit:
/* Due to E830 hardware limitations, TSO (NETIF_F_ALL_TSO) with GCS
* (NETIF_F_HW_CSUM) is not supported.
*/
if (ice_is_feature_supported(pf, ICE_F_GCS) &&
((features & NETIF_F_HW_CSUM) && (features & NETIF_F_ALL_TSO))) {
...
return -EIO;
commit 905d1a220e8d ("ice: Add E830 checksum offload support") states the
limitation at feature granularity: inner packet header modification is
not supported and MSS is limited to 1023 bytes when both GCS and TSO are
enabled.
If that limitation is device or configuration level, does a default E830
port (MTU 1500, TSO on) now run in the unsupported mode after this patch?
If it is per-packet only, then the -EIO check and the comment claiming the
ndo callback enforces exclusivity are stale, and could be retired in this
same change with that reasoning recorded.
For completeness: the follow-up patch "ice: use NETIF_F_HW_CSUM instead of
IP/IPV6" removes the -EIO check and the stale comment and moves
NETIF_F_HW_CSUM into csumo_features, but GCS being active with TSO enabled
persists to the end of the series.
Two effects not mentioned in the changelog: every checksummed packet now
also consumes a Tx context descriptor, and at this commit
tx-checksum-ip-generic no longer controls GCS at all, since clearing it
leaves NETIF_F_IP_CSUM/NETIF_F_IPV6_CSUM set and CHECKSUM_PARTIAL skbs
still take the GCS branch. Could the changelog mention both?
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> index 31303ab5be175..477c86d8c53bc 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> @@ -1739,7 +1739,7 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
> l3_len = l4.hdr - ip.hdr;
> offset |= (l3_len / 4) << ICE_TX_DESC_LEN_IPLEN_S;
>
> - if ((tx_ring->netdev->features & NETIF_F_HW_CSUM) &&
> + if (test_bit(ICE_TX_RING_FLAGS_GCS, tx_ring->flags) &&
> !(first->tx_flags & ICE_TX_FLAGS_TSO) &&
> !skb_csum_is_sctp(skb)) {
> /* Set GCS */
[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
making the GCS branch the default widens the exposure of a missing bounds
check in the block right below this hunk in ice_tx_csum():
u16 csum_start = (skb->csum_start - skb->mac_header) / 2;
u16 csum_offset = skb->csum_offset / 2;
u16 gcs_params;
gcs_params = FIELD_PREP(ICE_TX_GCS_DESC_START_M, csum_start) |
FIELD_PREP(ICE_TX_GCS_DESC_OFFSET_M, csum_offset) |
FIELD_PREP(ICE_TX_GCS_DESC_TYPE_M,
ICE_TX_GCS_DESC_CSUM_PSH);
In ice_lan_tx_rx.h these fields are narrow:
#define ICE_TX_GCS_DESC_START_M GENMASK(7, 0)
#define ICE_TX_GCS_DESC_OFFSET_M GENMASK(11, 8)
Both are in 2-byte units, so only csum_start up to 510 bytes past the MAC
header and csum_offset up to 30 can be expressed. FIELD_PREP rejects only
out-of-range compile-time constants, so larger runtime values are silently
masked and the descriptor is still committed:
off->cd_qw1 |= ICE_TX_DESC_DTYPE_CTX;
off->cd_gcs_params = gcs_params;
off->td_offset |= offset;
off->td_cmd |= cmd;
return 1;
There is no range check, no fall back to the legacy L4T checksum path and
no skb_checksum_help() call. Can the device then compute the sum over the
wrong span and write the two result bytes over payload, leaving the real
checksum field holding the pseudo-header partial sum?
Are these offsets trustworthy at this point? skb_partial_csum_set()
bounds them only by skb_headlen():
net/core/skbuff.c:skb_partial_csum_set() {
...
if (unlikely(csum_start >= U16_MAX || csum_end > skb_headlen(skb))) {
...
}
so a tun/tap guest using virtio_net_hdr, or an AF_PACKET sender with
CAP_NET_RAW, can pick csum_start beyond 510 or csum_offset beyond 30, and
skb_csum_hwoffload_help() returns 0 without fixing anything up once
NETIF_F_HW_CSUM is advertised. Long IPv6 extension header chains and
MPLS/tunnel stacks can also push csum_start past 510.
Would it make sense to validate csum_start and csum_offset against the
descriptor field widths here and fall through to the legacy checksum path
when they do not fit?
[ ... ]
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next 03/15] ice: use NETIF_F_HW_CSUM instead of IP/IPV6
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
2026-08-05 21:35 ` [PATCH net-next 01/15] ice: add support for unmanaged DPLL on E830 NIC Tony Nguyen
2026-08-05 21:35 ` [PATCH net-next 02/15] ice: always do GCS if hardware supports it Tony Nguyen
@ 2026-08-05 21:35 ` Tony Nguyen
2026-08-11 0:35 ` Jakub Kicinski
2026-08-05 21:35 ` [PATCH net-next 04/15] virtchnl: add VIRTCHNL_VLAN_ETHERTYPE_88E7 support Tony Nguyen
` (12 subsequent siblings)
15 siblings, 1 reply; 27+ messages in thread
From: Tony Nguyen @ 2026-08-05 21:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Michal Swiatkowski, anthony.l.nguyen, aleksandr.loktionov,
przemyslaw.kitszel, benoit.monin, willemb, jramaseu,
Alexander Nowlin
From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
The hardware is capable of calculating checksum for IPV6 packets with
extension header. To not drop such packets switch from IP/IPV6 checksum
to HW_CSUM.
HW_CSUM is also used in previous generation (i40e).
Previously HW_CSUM was used to indicate that hardware supports general
checksum. Drop it assuming that if the hardware supports it, it is used.
Disabling offload for E830 in case of TSO isn't needed anymore as the
check for TSO is done in Tx path just before preparation of the special
GCS descriptor.
Suggested-by: Jakub Ramaseuski <jramaseu@redhat.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ice/ice_main.c | 21 +--------------------
1 file changed, 1 insertion(+), 20 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index cc30d8c73a41..a5e9c3114706 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3491,9 +3491,8 @@ void ice_set_netdev_features(struct net_device *netdev)
NETIF_F_RXHASH;
csumo_features = NETIF_F_RXCSUM |
- NETIF_F_IP_CSUM |
NETIF_F_SCTP_CRC |
- NETIF_F_IPV6_CSUM;
+ NETIF_F_HW_CSUM;
vlano_features = NETIF_F_HW_VLAN_CTAG_FILTER |
NETIF_F_HW_VLAN_CTAG_TX |
@@ -3555,12 +3554,6 @@ void ice_set_netdev_features(struct net_device *netdev)
/* Allow core to manage IRQs affinity */
netif_set_affinity_auto(netdev);
- /* Mutual exclusivity for TSO and GCS is enforced by the set features
- * ndo callback.
- */
- if (ice_is_feature_supported(pf, ICE_F_GCS))
- netdev->hw_features |= NETIF_F_HW_CSUM;
-
netif_set_tso_max_size(netdev, ICE_MAX_TSO_SIZE);
}
@@ -6519,18 +6512,6 @@ ice_set_features(struct net_device *netdev, netdev_features_t features)
if (changed & NETIF_F_LOOPBACK)
ret = ice_set_loopback(vsi, !!(features & NETIF_F_LOOPBACK));
- /* Due to E830 hardware limitations, TSO (NETIF_F_ALL_TSO) with GCS
- * (NETIF_F_HW_CSUM) is not supported.
- */
- if (ice_is_feature_supported(pf, ICE_F_GCS) &&
- ((features & NETIF_F_HW_CSUM) && (features & NETIF_F_ALL_TSO))) {
- if (netdev->features & NETIF_F_HW_CSUM)
- dev_err(ice_pf_to_dev(pf), "To enable TSO, you must first disable HW checksum.\n");
- else
- dev_err(ice_pf_to_dev(pf), "To enable HW checksum, you must first disable TSO.\n");
- return -EIO;
- }
-
return ret;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next 03/15] ice: use NETIF_F_HW_CSUM instead of IP/IPV6
2026-08-05 21:35 ` [PATCH net-next 03/15] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 Tony Nguyen
@ 2026-08-11 0:35 ` Jakub Kicinski
0 siblings, 0 replies; 27+ messages in thread
From: Jakub Kicinski @ 2026-08-11 0:35 UTC (permalink / raw)
To: anthony.l.nguyen
Cc: Jakub Kicinski, davem, pabeni, edumazet, andrew+netdev, netdev,
tactii, dima.ruinskiy, aleksandr.loktionov, pmenzel,
michalx.cohen
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: use NETIF_F_HW_CSUM instead of IP/IPV6
This switches the ice netdev checksum offload advertisement from
NETIF_F_IP_CSUM/NETIF_F_IPV6_CSUM to NETIF_F_HW_CSUM, so IPv6 packets
carrying extension headers are no longer pushed down the software checksum
path. It also removes the conditional NETIF_F_HW_CSUM advertisement for
GCS-capable parts and the TSO/GCS mutual exclusion check in
ice_set_features().
A few questions below.
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index cc30d8c73a41c..a5e9c3114706f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -3491,9 +3491,8 @@ void ice_set_netdev_features(struct net_device *netdev)
> NETIF_F_RXHASH;
>
> csumo_features = NETIF_F_RXCSUM |
> - NETIF_F_IP_CSUM |
> NETIF_F_SCTP_CRC |
> - NETIF_F_IPV6_CSUM;
> + NETIF_F_HW_CSUM;
[Severity: Medium]
Should this hunk carry a Fixes: tag and be split out from the GCS/TSO
cleanup?
As far as I can tell this is not just an offload-advertisement cleanup, it
fixes a user-visible drop. With only NETIF_F_IP_CSUM/NETIF_F_IPV6_CSUM set,
skb_csum_hwoffload_help() in net/core/dev.c forces the software path:
if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) {
if (vlan_get_protocol(skb) == htons(ETH_P_IPV6) &&
skb_network_header_len(skb) != sizeof(struct ipv6hdr))
goto sw_checksum;
and skb_checksum_help() refuses GSO skbs:
if (unlikely(skb_is_gso(skb))) {
skb_warn_bad_offload(skb);
return -EINVAL;
}
Since ice advertises NETIF_F_TSO6, netif_needs_gso() is false and the skb
arrives still marked GSO, so validate_xmit_skb() takes the
out_kfree_skb path and the frame is dropped rather than de-offloaded.
That means TSO'd IPv6 traffic with an extension header (SRv6, HOPOPTS,
DSTOPTS) is dropped on every ice device on stable kernels containing the
skb_csum_hwoffload_help() change (04c20a9356f2, v6.13+). The changelog only
hints at this with "To not drop such packets", and there is no Fixes: tag,
so it is hard to identify for backport. Would it make sense to make this
one-liner a standalone patch with a Fixes: tag, and keep the GCS/TSO
mutual-exclusion removal (which depends on the earlier "ice: always do GCS
if hardware supports it" patch in this series) separate?
[Severity: Medium]
On non-GCS parts, is NETIF_F_HW_CSUM an accurate description of what the
legacy path in ice_tx_csum() (drivers/net/ethernet/intel/ice/ice_txrx.c)
implements?
NETIF_F_HW_CSUM promises the driver honours skb->csum_start /
skb->csum_offset for any protocol, and once it is set
skb_csum_hwoffload_help() returns 0 unconditionally, so the core no longer
checks that skb->csum_offset is offsetof(struct tcphdr, check) or
offsetof(struct udphdr, check).
The legacy branch of ice_tx_csum() derives the insertion point purely from
its own header parse and never looks at skb->csum_offset:
switch (l4_proto) {
case IPPROTO_TCP:
cmd |= ICE_TX_DESC_CMD_L4T_EOFT_TCP;
l4_len = l4.tcp->doff;
offset |= l4_len << ICE_TX_DESC_LEN_L4_LEN_S;
break;
...
default:
goto checksum_sw_fb;
}
So for a CHECKSUM_PARTIAL frame that parses as IPv4/IPv6 + TCP/UDP but asks
for a different csum_offset (virtio_net_hdr/tap or AF_PACKET with a vnet
header; skb_partial_csum_set() only checks that start/off lie inside the
linear area), hardware writes the checksum at the standard L4 offset while
the field the stack asked to be filled keeps the pseudo-header seed. The
checksum_sw_fb fallback does not catch this, since it is only reached when
the network header is neither IPv4 nor IPv6 or when l4_proto hits the switch
default.
i40e has the same loose interpretation (it never references
skb->csum_offset either), so this may well be intended precedent - is that
the reasoning here?
>
> vlano_features = NETIF_F_HW_VLAN_CTAG_FILTER |
> NETIF_F_HW_VLAN_CTAG_TX |
> @@ -3555,12 +3554,6 @@ void ice_set_netdev_features(struct net_device *netdev)
> /* Allow core to manage IRQs affinity */
> netif_set_affinity_auto(netdev);
>
> - /* Mutual exclusivity for TSO and GCS is enforced by the set features
> - * ndo callback.
> - */
> - if (ice_is_feature_supported(pf, ICE_F_GCS))
> - netdev->hw_features |= NETIF_F_HW_CSUM;
> -
[Severity: High]
With NETIF_F_HW_CSUM now in csumo_features it is enabled by default
(netdev->features |= netdev->hw_features) and also propagated into
hw_enc_features and vlan_features. On E830 that makes the GCS context
descriptor path in ice_tx_csum() the default Tx checksum path. Can the
values programmed there be truncated?
u16 csum_start = (skb->csum_start - skb->mac_header) / 2;
u16 csum_offset = skb->csum_offset / 2;
u16 gcs_params;
gcs_params = FIELD_PREP(ICE_TX_GCS_DESC_START_M, csum_start) |
FIELD_PREP(ICE_TX_GCS_DESC_OFFSET_M, csum_offset) |
The descriptor fields in ice_lan_tx_rx.h are narrow:
#define ICE_TX_GCS_DESC_START_M GENMASK(7, 0)
#define ICE_TX_GCS_DESC_OFFSET_M GENMASK(11, 8)
i.e. at most 510 bytes of L2+L3 header and at most a 30-byte checksum
offset. Neither value is range-checked, and FIELD_PREP()'s
__BF_FIELD_CHECK only rejects out-of-range constants, so a runtime value is
silently masked.
The driver's ndo_features_check, ice_features_check(), only bounds the two
lengths individually:
len = skb_network_offset(skb);
if (len > ICE_TXD_MACLEN_MAX || len & 0x1)
goto out_rm_features;
len = skb_network_header_len(skb);
if (len > ICE_TXD_IPLEN_MAX || len & 0x1)
goto out_rm_features;
That permits 254 + 508 = 762 combined bytes, well past the 510 bytes the
GCS start field can express. An IPv6 frame with roughly 498..508 bytes of
header plus extension headers (a long HOPOPTS/DSTOPTS chain, or an SRv6 SRH
with many SIDs over a VLAN) passes both checks, yet
(csum_start - mac_header) / 2 = 259 becomes 3 after masking, so the NIC
sums the wrong byte range and inserts the result inside the MAC/IPv6
header.
The same masking applies to skb->csum_offset > 30, which
virtio_net_hdr_to_skb()/skb_partial_csum_set() allow (they only bound
start/off against skb_headlen()), and which the core no longer filters now
that NETIF_F_HW_CSUM is advertised. Would a FIELD_MAX() check on both
values before taking the GCS branch, or an added combined-length check in
ice_features_check(), be appropriate here?
> netif_set_tso_max_size(netdev, ICE_MAX_TSO_SIZE);
> }
>
> @@ -6519,18 +6512,6 @@ ice_set_features(struct net_device *netdev, netdev_features_t features)
> if (changed & NETIF_F_LOOPBACK)
> ret = ice_set_loopback(vsi, !!(features & NETIF_F_LOOPBACK));
>
> - /* Due to E830 hardware limitations, TSO (NETIF_F_ALL_TSO) with GCS
> - * (NETIF_F_HW_CSUM) is not supported.
> - */
> - if (ice_is_feature_supported(pf, ICE_F_GCS) &&
> - ((features & NETIF_F_HW_CSUM) && (features & NETIF_F_ALL_TSO))) {
> - if (netdev->features & NETIF_F_HW_CSUM)
> - dev_err(ice_pf_to_dev(pf), "To enable TSO, you must first disable HW checksum.\n");
> - else
> - dev_err(ice_pf_to_dev(pf), "To enable HW checksum, you must first disable TSO.\n");
> - return -EIO;
> - }
> -
> return ret;
> }
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next 04/15] virtchnl: add VIRTCHNL_VLAN_ETHERTYPE_88E7 support
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
` (2 preceding siblings ...)
2026-08-05 21:35 ` [PATCH net-next 03/15] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 Tony Nguyen
@ 2026-08-05 21:35 ` Tony Nguyen
2026-08-11 0:35 ` Jakub Kicinski
2026-08-05 21:35 ` [PATCH net-next 05/15] ice: add 0x88E7 handling to SW validation paths Tony Nguyen
` (11 subsequent siblings)
15 siblings, 1 reply; 27+ messages in thread
From: Tony Nguyen @ 2026-08-05 21:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Aleksandr Loktionov, anthony.l.nguyen, przemyslaw.kitszel
From: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
IEEE 802.1ah (Provider Backbone Bridging) uses ethertype 0x88E7 as the
Backbone Service Tag (B-TAG). Some deployments stack 802.1ah on top of
existing QinQ or single-tagged VLAN infrastructure, creating scenarios
where a VF needs to negotiate support for the 0x88E7 ethertype with the
PF in order to install correct VLAN filters.
Add VIRTCHNL_VLAN_ETHERTYPE_88E7 = BIT(3) to the virtchnl_vlan_support
capability bitmask so that VF drivers can advertise and negotiate 0x88E7
VLAN ethertype support through the existing VIRTCHNL_OP_GET_OFFLOAD_VLAN_V2_CAPS
handshake.
The new bit follows the established BIT(N) pattern of the enum and does
not conflict with any existing flag.
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
include/linux/avf/virtchnl.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index 11bdab5522fd..511c8827c640 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -636,6 +636,7 @@ enum virtchnl_vlan_support {
VIRTCHNL_VLAN_ETHERTYPE_8100 = BIT(0),
VIRTCHNL_VLAN_ETHERTYPE_88A8 = BIT(1),
VIRTCHNL_VLAN_ETHERTYPE_9100 = BIT(2),
+ VIRTCHNL_VLAN_ETHERTYPE_88E7 = BIT(3),
VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1 = BIT(8),
VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2 = BIT(9),
VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2_2 = BIT(10),
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next 04/15] virtchnl: add VIRTCHNL_VLAN_ETHERTYPE_88E7 support
2026-08-05 21:35 ` [PATCH net-next 04/15] virtchnl: add VIRTCHNL_VLAN_ETHERTYPE_88E7 support Tony Nguyen
@ 2026-08-11 0:35 ` Jakub Kicinski
0 siblings, 0 replies; 27+ messages in thread
From: Jakub Kicinski @ 2026-08-11 0:35 UTC (permalink / raw)
To: anthony.l.nguyen
Cc: Jakub Kicinski, davem, pabeni, edumazet, andrew+netdev, netdev,
tactii, dima.ruinskiy, aleksandr.loktionov, pmenzel,
michalx.cohen
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
virtchnl: add VIRTCHNL_VLAN_ETHERTYPE_88E7 support
This adds VIRTCHNL_VLAN_ETHERTYPE_88E7 = BIT(3) to enum
virtchnl_vlan_support so that the 0x88E7 VLAN ethertype can be negotiated
over VIRTCHNL_OP_GET_OFFLOAD_VLAN_V2_CAPS. The change touches only the
shared virtchnl header.
> diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
> index 11bdab5522fd5..511c8827c640f 100644
> --- a/include/linux/avf/virtchnl.h
> +++ b/include/linux/avf/virtchnl.h
> @@ -636,6 +636,7 @@ enum virtchnl_vlan_support {
> VIRTCHNL_VLAN_ETHERTYPE_8100 = BIT(0),
> VIRTCHNL_VLAN_ETHERTYPE_88A8 = BIT(1),
> VIRTCHNL_VLAN_ETHERTYPE_9100 = BIT(2),
> + VIRTCHNL_VLAN_ETHERTYPE_88E7 = BIT(3),
[Severity: Medium]
Is there anything in the tree that can actually negotiate this bit?
The commit message says the bit is added "so that VF drivers can advertise
and negotiate 0x88E7 VLAN ethertype support through the existing
VIRTCHNL_OP_GET_OFFLOAD_VLAN_V2_CAPS handshake", but on the PF side
ice_vc_set_dvm_caps() and ice_vc_set_svm_caps() in
drivers/net/ethernet/intel/ice/virt/virtchnl.c only ever fill in:
supported_caps->outer = VIRTCHNL_VLAN_ETHERTYPE_8100 |
VIRTCHNL_VLAN_ETHERTYPE_88A8 |
VIRTCHNL_VLAN_ETHERTYPE_9100 |
VIRTCHNL_VLAN_ETHERTYPE_AND;
caps->filtering.ethertype_init = VIRTCHNL_VLAN_ETHERTYPE_8100 |
VIRTCHNL_VLAN_ETHERTYPE_88A8 |
VIRTCHNL_VLAN_ETHERTYPE_9100;
so BIT(3) never appears in the caps message stored in vf->vlan_v2_caps.
On the VF side, iavf_tpid_to_vc_ethertype() in
drivers/net/ethernet/intel/iavf/iavf_virtchnl.c maps only ETH_P_8021Q and
ETH_P_8021AD, and iavf_get_netdev_vlan_hw_features() /
iavf_get_netdev_vlan_features() in iavf_main.c test only the _8100 and
_88A8 bits, so a PF that did advertise BIT(3) would be silently ignored.
As a consequence, the ETH_P_8021AH / VIRTCHNL_VLAN_ETHERTYPE_88E7 cases
added by the follow-up patch in this series ("ice: add 0x88E7 handling to
SW validation paths") look unreachable. ice_vc_validate_vlan_tpid() ends
with:
if (!(filtering_caps & vlan_ethertype))
return false;
and filtering_caps never contains BIT(3), so 0x88E7 filters are still
rejected. Likewise ice_vc_get_tpid() is only reached after
ice_vc_valid_vlan_setting() has already done:
if (ethertype_setting && !(negotiated_settings & ethertype_setting))
return false;
Should the advertisement side land in the same series, or is the intent
that only an out-of-tree/firmware PF sets this bit? If the latter, would
it be worth dropping the claim in the follow-up commit that "the PF
correctly accepts and translates 0x88E7 (ETH_P_8021AH) VLAN filters
requested by VFs", since that is not the case at the end of the series?
[Severity: Medium]
Is the terminology in the commit message right? It says:
IEEE 802.1ah (Provider Backbone Bridging) uses ethertype 0x88E7 as the
Backbone Service Tag (B-TAG).
802.1ah uses 0x88A8 for the Backbone VLAN Tag (B-TAG), which is already
covered by VIRTCHNL_VLAN_ETHERTYPE_88A8, and 0x88E7 for the Backbone
Service Instance Tag (I-TAG). The ice parser documentation in
drivers/net/ethernet/intel/ice/ice_protocol_type.h agrees:
* K = STAG or BVLAN - Outer L2 header has STAG (ethernet type 0x88a8) or
* BVLAN (ethernet type 0x88a8).
* L = ITAG - Outer L2 header has ITAG *ethernet type 0x88e7)
Related to that, how is a 0x88E7 tag meant to be expressed through this
ABI? struct virtchnl_vlan in this same header carries only:
u16 tci; /* tci[15:13] = PCP and tci[11:0] = VID */
u16 tci_mask;
u16 tpid; /* 0x8100, 0x88a8, etc. ... */
which has no room for an 802.1ah 24-bit I-SID.
And on the hardware side, ICE_AQ_VSI_OUTER_TAG_TYPE_M in
drivers/net/ethernet/intel/ice/ice_adminq_cmd.h is a two-bit field with
only NONE/STAG/VLAN_8100/VLAN_9100 encodings, and
tpid_to_vsi_outer_vlan_type() in ice_vsi_vlan_lib.c ends with:
default:
*tag_type = 0;
return -EINVAL;
So a peer that did negotiate BIT(3) and then sent
VIRTCHNL_OP_ENABLE_VLAN_STRIPPING_V2 would reach
ice_vc_ena_vlan_offload() -> ice_vsi_ena_outer_stripping(vsi,
ETH_P_8021AH) and get -EINVAL, while its own feature flags claim support.
Could the commit message state which 802.1ah tag is meant and how it maps
onto struct virtchnl_vlan and the hardware outer tag types?
[Severity: Low]
Should the documentation block above this enum gain a matching line? It
currently describes each ethertype bit:
* VIRTCHNL_VLAN_ETHERTYPE_8100 - This field supports 0x8100 ethertype.
* VIRTCHNL_VLAN_ETHERTYPE_88A8 - This field supports 0x88A8 ethertype.
* VIRTCHNL_VLAN_ETHERTYPE_9100 - This field supports 0x9100 ethertype.
but there is no entry for the new bit, and the AND/XOR wording in the same
block is written in terms of the enumerated ethertypes. Since 0x88E7 is
described as ETH_P_8021AH "802.1ah Backbone Service Tag" in
include/uapi/linux/if_ether.h but as ITAG in ice_protocol_type.h, a
third-party implementer reading this header cannot tell whether the bit
means B-TAG or I-TAG handling. Adding the documentation line, and
referencing ETH_P_8021AH, would pin that down.
> VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1 = BIT(8),
> VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2 = BIT(9),
> VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2_2 = BIT(10),
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next 05/15] ice: add 0x88E7 handling to SW validation paths
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
` (3 preceding siblings ...)
2026-08-05 21:35 ` [PATCH net-next 04/15] virtchnl: add VIRTCHNL_VLAN_ETHERTYPE_88E7 support Tony Nguyen
@ 2026-08-05 21:35 ` Tony Nguyen
2026-08-11 0:35 ` Jakub Kicinski
2026-08-05 21:35 ` [PATCH net-next 06/15] ice: reduce loglevel to debug for 'Can't delete DSCP' message Tony Nguyen
` (10 subsequent siblings)
15 siblings, 1 reply; 27+ messages in thread
From: Tony Nguyen @ 2026-08-05 21:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Aleksandr Loktionov, anthony.l.nguyen, przemyslaw.kitszel,
Rafal Romanowski
From: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
The virtchnl v2 VLAN capability handshake now includes the new
VIRTCHNL_VLAN_ETHERTYPE_88E7 flag for IEEE 802.1ah B-TAG support.
Wire up the corresponding software-path handling in ice so the PF
correctly accepts and translates 0x88E7 (ETH_P_8021AH) VLAN filters
requested by VFs.
Three software-only changes, no hardware offload path affected:
- ice_check_supported_vlan_tpid() (ice_tc_lib.c): accept ETH_P_8021AH
in the TC VLAN TPID validation switch so 0x88E7-tagged flower filters
are not rejected early.
- validate_vlan() (ice_vsi_vlan_lib.c): allow ETH_P_8021AH as a valid
TPID when adding VLAN filters to a VSI, consistent with the other
accepted dot1q/dot1ad/QinQ1 TPIDs.
- ice_vc_validate_vlan_tpid() / ice_vc_get_tpid() (virt/virtchnl.c):
bidirectional translation between ETH_P_8021AH and
VIRTCHNL_VLAN_ETHERTYPE_88E7 in the virtchnl VLAN v2 filter path.
This does not add 0x88E7 hardware offload capability, does not change
outer-tag programming, and does not alter any datapath.
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ice/ice_tc_lib.c | 1 +
drivers/net/ethernet/intel/ice/ice_vsi_vlan_lib.c | 3 ++-
drivers/net/ethernet/intel/ice/virt/virtchnl.c | 6 ++++++
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
index d20357c04127..4560e55c8f9c 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
@@ -174,6 +174,7 @@ static u16 ice_check_supported_vlan_tpid(u16 vlan_tpid)
case ETH_P_8021Q:
case ETH_P_8021AD:
case ETH_P_QINQ1:
+ case ETH_P_8021AH:
return vlan_tpid;
default:
return 0;
diff --git a/drivers/net/ethernet/intel/ice/ice_vsi_vlan_lib.c b/drivers/net/ethernet/intel/ice/ice_vsi_vlan_lib.c
index 54984966851d..b67272e2f603 100644
--- a/drivers/net/ethernet/intel/ice/ice_vsi_vlan_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vsi_vlan_lib.c
@@ -24,7 +24,8 @@ static void print_invalid_tpid(struct ice_vsi *vsi, u16 tpid)
static bool validate_vlan(struct ice_vsi *vsi, struct ice_vlan *vlan)
{
if (vlan->tpid != ETH_P_8021Q && vlan->tpid != ETH_P_8021AD &&
- vlan->tpid != ETH_P_QINQ1 && (vlan->tpid || vlan->vid)) {
+ vlan->tpid != ETH_P_QINQ1 && vlan->tpid != ETH_P_8021AH &&
+ (vlan->tpid || vlan->vid)) {
print_invalid_tpid(vsi, vlan->tpid);
return false;
}
diff --git a/drivers/net/ethernet/intel/ice/virt/virtchnl.c b/drivers/net/ethernet/intel/ice/virt/virtchnl.c
index ca8018e3dd42..06d2f9be93ae 100644
--- a/drivers/net/ethernet/intel/ice/virt/virtchnl.c
+++ b/drivers/net/ethernet/intel/ice/virt/virtchnl.c
@@ -1702,6 +1702,9 @@ static bool ice_vc_validate_vlan_tpid(u16 filtering_caps, u16 tpid)
case ETH_P_QINQ1:
vlan_ethertype = VIRTCHNL_VLAN_ETHERTYPE_9100;
break;
+ case ETH_P_8021AH:
+ vlan_ethertype = VIRTCHNL_VLAN_ETHERTYPE_88E7;
+ break;
}
if (!(filtering_caps & vlan_ethertype))
@@ -2136,6 +2139,9 @@ static int ice_vc_get_tpid(u32 ethertype_setting, u16 *tpid)
case VIRTCHNL_VLAN_ETHERTYPE_9100:
*tpid = ETH_P_QINQ1;
break;
+ case VIRTCHNL_VLAN_ETHERTYPE_88E7:
+ *tpid = ETH_P_8021AH;
+ break;
default:
*tpid = 0;
return -EINVAL;
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next 05/15] ice: add 0x88E7 handling to SW validation paths
2026-08-05 21:35 ` [PATCH net-next 05/15] ice: add 0x88E7 handling to SW validation paths Tony Nguyen
@ 2026-08-11 0:35 ` Jakub Kicinski
0 siblings, 0 replies; 27+ messages in thread
From: Jakub Kicinski @ 2026-08-11 0:35 UTC (permalink / raw)
To: anthony.l.nguyen
Cc: Jakub Kicinski, davem, pabeni, edumazet, andrew+netdev, netdev,
tactii, dima.ruinskiy, aleksandr.loktionov, pmenzel,
michalx.cohen
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: add 0x88E7 handling to SW validation paths
This adds ETH_P_8021AH (0x88E7) to three software validation paths in ice:
the TC flower TPID check, the per-VSI VLAN filter TPID check, and the
virtchnl VLAN v2 TPID translation helpers. A few questions below about
the effect of these hunks and about the commit message.
> diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> index d20357c041273..4560e55c8f9c9 100644
> --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> @@ -174,6 +174,7 @@ static u16 ice_check_supported_vlan_tpid(u16 vlan_tpid)
> case ETH_P_8021Q:
> case ETH_P_8021AD:
> case ETH_P_QINQ1:
> + case ETH_P_8021AH:
> return vlan_tpid;
> default:
> return 0;
[Severity: Medium]
The commit message says this hunk exists "so 0x88E7-tagged flower filters
are not rejected early". Where was the early rejection? The only caller
of ice_check_supported_vlan_tpid() is ice_tc_fill_rules():
rule_info->vlan_type = ice_check_supported_vlan_tpid(vlan_tpid);
There is no check for a 0 return and no -EINVAL or extack emitted, so an
unsupported TPID previously just resulted in vlan_type == 0.
The same paragraph says "no hardware offload path affected" and "does not
alter any datapath", but rule_info->vlan_type reaches hardware rule
programming through ice_add_adv_rule() -> ice_fill_adv_packet_vlan(),
which stamps it into the dummy packet used to program the recipe:
hdr = (struct ice_vlan_hdr *)&pkt[offset];
hdr->type = cpu_to_be16(vlan_type);
and it also participates in rule identity in ice_rules_equal().
For a flower rule created with num_of_vlans >= 1 and protocol 0x88E7,
cls_flower's is_vlan_key()/fl_set_key_vlan() copy any ethertype into
key->vlan.vlan_tpid with a full mask, ice_parse_cls_flower() copies it to
headers->vlan_hdr.vlan_tpid, and the programmed rule now carries TPID
0x88E7 where it previously carried the dummy packet default. Is that
silent change of which frames the offloaded rule matches intended, and
should the commit message describe it rather than state the opposite?
> diff --git a/drivers/net/ethernet/intel/ice/ice_vsi_vlan_lib.c b/drivers/net/ethernet/intel/ice/ice_vsi_vlan_lib.c
> index 54984966851d5..b67272e2f603c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_vsi_vlan_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_vsi_vlan_lib.c
> @@ -24,7 +24,8 @@ static void print_invalid_tpid(struct ice_vsi *vsi, u16 tpid)
> static bool validate_vlan(struct ice_vsi *vsi, struct ice_vlan *vlan)
> {
> if (vlan->tpid != ETH_P_8021Q && vlan->tpid != ETH_P_8021AD &&
> - vlan->tpid != ETH_P_QINQ1 && (vlan->tpid || vlan->vid)) {
> + vlan->tpid != ETH_P_QINQ1 && vlan->tpid != ETH_P_8021AH &&
> + (vlan->tpid || vlan->vid)) {
> print_invalid_tpid(vsi, vlan->tpid);
> return false;
> }
[Severity: Medium]
The commit message introduces 0x88E7 as "IEEE 802.1ah B-TAG support". Is
that the right mapping? ice's own in-tree documentation in
ice_protocol_type.h says otherwise:
* K = STAG or BVLAN - Outer L2 header has STAG (ethernet type 0x88a8) or
* BVLAN (ethernet type 0x88a8).
* L = ITAG - Outer L2 header has ITAG *ethernet type 0x88e7)
So the 802.1ah B-TAG is 0x88a8, which the existing ETH_P_8021AD case
already covers, and 0x88E7 is the I-TAG.
That distinction matters for the filter layout. The I-TAG is a 4-byte
I-TCI (I-PCP/DEI/UCA/reserved plus a 24-bit I-SID) followed by 12 bytes of
encapsulated C-DA/C-SA, but ice_fill_sw_rule() programs the requested vid
at the 802.1Q TCI offset next to the TPID:
if (!(vlan_id > ICE_MAX_VLAN_ID)) {
off = (__force __be16 *)(eth_hdr + ICE_ETH_VLAN_TCI_OFFSET);
*off = cpu_to_be16(vlan_id);
off = (__force __be16 *)(eth_hdr + ICE_ETH_ETHTYPE_OFFSET);
*off = cpu_to_be16(vlan_tpid);
}
For a real 0x88E7 frame those 16 bits are I-PCP/DEI/UCA/reserved plus the
top byte of the I-SID, not a VID. Can a "vid N, tpid 0x88E7" filter key on
anything meaningful here?
There is also an internal inconsistency: validate_vlan() now accepts
ETH_P_8021AH while tpid_to_vsi_outer_vlan_type() in the same file still
returns -EINVAL for it:
switch (tpid) {
case ETH_P_8021Q:
...
case ETH_P_QINQ1:
*tag_type = ICE_AQ_VSI_OUTER_TAG_VLAN_9100;
break;
default:
return -EINVAL;
Should a TPID that can be recorded as a filter also be programmable as an
outer tag type, or is accepting it in validate_vlan() premature?
> diff --git a/drivers/net/ethernet/intel/ice/virt/virtchnl.c b/drivers/net/ethernet/intel/ice/virt/virtchnl.c
> index ca8018e3dd428..06d2f9be93ae4 100644
> --- a/drivers/net/ethernet/intel/ice/virt/virtchnl.c
> +++ b/drivers/net/ethernet/intel/ice/virt/virtchnl.c
> @@ -1702,6 +1702,9 @@ static bool ice_vc_validate_vlan_tpid(u16 filtering_caps, u16 tpid)
> case ETH_P_QINQ1:
> vlan_ethertype = VIRTCHNL_VLAN_ETHERTYPE_9100;
> break;
> + case ETH_P_8021AH:
> + vlan_ethertype = VIRTCHNL_VLAN_ETHERTYPE_88E7;
> + break;
> }
>
> if (!(filtering_caps & vlan_ethertype))
[Severity: Medium]
Can this new case ever change the result? filtering_caps comes from
vf->vlan_v2_caps.filtering.filtering_support, and the only producers are
ice_vc_set_dvm_caps():
supported_caps->outer = VIRTCHNL_VLAN_ETHERTYPE_8100 |
VIRTCHNL_VLAN_ETHERTYPE_88A8 |
VIRTCHNL_VLAN_ETHERTYPE_9100 |
VIRTCHNL_VLAN_ETHERTYPE_AND;
caps->filtering.ethertype_init = VIRTCHNL_VLAN_ETHERTYPE_8100 |
VIRTCHNL_VLAN_ETHERTYPE_88A8 |
VIRTCHNL_VLAN_ETHERTYPE_9100;
and ice_vc_set_svm_caps():
supported_caps->inner = VIRTCHNL_VLAN_ETHERTYPE_8100;
supported_caps->outer = VIRTCHNL_VLAN_UNSUPPORTED;
caps->filtering.ethertype_init = VIRTCHNL_VLAN_ETHERTYPE_8100;
Neither sets VIRTCHNL_VLAN_ETHERTYPE_88E7, so a VF sending
VIRTCHNL_OP_ADD_VLAN_V2 with tpid 0x88E7 still fails the
!(filtering_caps & vlan_ethertype) test and
ice_vc_add_vlan_v2_msg()/ice_vc_remove_vlan_v2_msg() keep returning
VIRTCHNL_STATUS_ERR_PARAM, exactly as before the patch. Grepping the tree
at this commit shows VIRTCHNL_VLAN_ETHERTYPE_88E7 appearing in ice only at
these two new sites, with no producer of the capability bit added
elsewhere in the series.
Given that, does the commit message claim that "the PF correctly accepts
and translates 0x88E7 (ETH_P_8021AH) VLAN filters requested by VFs" hold?
The validate_vlan() relaxation in ice_vsi_vlan_lib.c is likewise
unreachable from the VF path while this gate stays closed. Should the
capability advertisement come in the same patch, or the message be reworded
to say the translation is added ahead of the capability?
> @@ -2136,6 +2139,9 @@ static int ice_vc_get_tpid(u32 ethertype_setting, u16 *tpid)
> case VIRTCHNL_VLAN_ETHERTYPE_9100:
> *tpid = ETH_P_QINQ1;
> break;
> + case VIRTCHNL_VLAN_ETHERTYPE_88E7:
> + *tpid = ETH_P_8021AH;
> + break;
> default:
> *tpid = 0;
> return -EINVAL;
[Severity: Low]
This makes ice the first in-tree interpreter of VIRTCHNL_VLAN_ETHERTYPE_88E7,
but that bit is still undocumented in include/linux/avf/virtchnl.h. The
comment block above enum virtchnl_vlan_support lists only:
* VIRTCHNL_VLAN_ETHERTYPE_8100 - This field supports 0x8100 ethertype.
* VIRTCHNL_VLAN_ETHERTYPE_88A8 - This field supports 0x88A8 ethertype.
* VIRTCHNL_VLAN_ETHERTYPE_9100 - This field supports 0x9100 ethertype.
while the enum itself now has:
VIRTCHNL_VLAN_ETHERTYPE_88E7 = BIT(3),
The AND/XOR usage examples in the same block also do not mention 88E7.
Could the documentation be extended to say which fields may carry the bit
and how it combines with VIRTCHNL_VLAN_ETHERTYPE_AND/XOR?
Also worth noting: this arm is reached only via
ice_vc_valid_vlan_setting_msg(), which checks stripping_support and
insertion_support, neither of which contains the 88E7 bit, and the returned
tpid would then be rejected by tpid_to_vsi_outer_vlan_type().
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next 06/15] ice: reduce loglevel to debug for 'Can't delete DSCP' message
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
` (4 preceding siblings ...)
2026-08-05 21:35 ` [PATCH net-next 05/15] ice: add 0x88E7 handling to SW validation paths Tony Nguyen
@ 2026-08-05 21:35 ` Tony Nguyen
2026-08-05 21:35 ` [PATCH net-next 07/15] ice: use ice_fill_eth_hdr() in ice_fill_sw_rule() Tony Nguyen
` (9 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Tony Nguyen @ 2026-08-05 21:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Grzegorz Nitka, anthony.l.nguyen, przemyslaw.kitszel,
Aleksandr Loktionov, Simon Horman, Arpana Arland
From: Grzegorz Nitka <grzegorz.nitka@intel.com>
Reduce netdev message "Can't delete DSCP netlink app ..." from error
to debug level when in FW (firmware) DCB mode.
This is not a real error and a kind of expected behavior. The device
is fully operational and error message might be wrongly interpreted by
the user.
Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Tested-by: Arpana Arland <arpanax.arland@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ice/ice_dcb_nl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_nl.c b/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
index a10c1c8d8697..bb5cc0a2ba69 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
@@ -872,7 +872,7 @@ static int ice_dcbnl_delapp(struct net_device *netdev, struct dcb_app *app)
int ret = 0;
if (pf->dcbx_cap & DCB_CAP_DCBX_LLD_MANAGED) {
- netdev_err(netdev, "can't delete DSCP netlink app when FW DCB agent is active\n");
+ netdev_dbg(netdev, "can't delete DSCP netlink app when FW DCB agent is active\n");
return -EINVAL;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH net-next 07/15] ice: use ice_fill_eth_hdr() in ice_fill_sw_rule()
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
` (5 preceding siblings ...)
2026-08-05 21:35 ` [PATCH net-next 06/15] ice: reduce loglevel to debug for 'Can't delete DSCP' message Tony Nguyen
@ 2026-08-05 21:35 ` Tony Nguyen
2026-08-05 21:35 ` [PATCH net-next 08/15] ice: increase OICR interrupt moderation rate to 20K interrupts/sec Tony Nguyen
` (8 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Tony Nguyen @ 2026-08-05 21:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Marcin Szycik, anthony.l.nguyen, przemyslaw.kitszel,
Martyna Szapar-Mudlaw, Aleksandr Loktionov, Simon Horman,
Rinitha S
From: Marcin Szycik <marcin.szycik@intel.com>
Use the already existing helper function to fill Ethernet header. Also
replace sizeof with a (also existing) macro to reduce the number of
variables.
Suggested-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@intel.com>
Signed-off-by: Marcin Szycik <marcin.szycik@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ice/ice_switch.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index 6a5875bd9c6b..4daee252e2a6 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -2612,7 +2612,6 @@ ice_fill_sw_rule(struct ice_hw *hw, struct ice_fltr_info *f_info,
u16 vlan_id = ICE_MAX_VLAN_ID + 1;
u16 vlan_tpid = ETH_P_8021Q;
void *daddr = NULL;
- u16 eth_hdr_sz;
u8 *eth_hdr;
u32 act = 0;
__be16 *off;
@@ -2625,11 +2624,10 @@ ice_fill_sw_rule(struct ice_hw *hw, struct ice_fltr_info *f_info,
return;
}
- eth_hdr_sz = sizeof(dummy_eth_header);
eth_hdr = s_rule->hdr_data;
/* initialize the ether header with a dummy header */
- memcpy(eth_hdr, dummy_eth_header, eth_hdr_sz);
+ ice_fill_eth_hdr(eth_hdr);
ice_fill_sw_info(hw, f_info);
switch (f_info->fltr_act) {
@@ -2730,7 +2728,7 @@ ice_fill_sw_rule(struct ice_hw *hw, struct ice_fltr_info *f_info,
/* Create the switch rule with the final dummy Ethernet header */
if (opc != ice_aqc_opc_update_sw_rules)
- s_rule->hdr_len = cpu_to_le16(eth_hdr_sz);
+ s_rule->hdr_len = cpu_to_le16(DUMMY_ETH_HDR_LEN);
}
/**
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH net-next 08/15] ice: increase OICR interrupt moderation rate to 20K interrupts/sec
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
` (6 preceding siblings ...)
2026-08-05 21:35 ` [PATCH net-next 07/15] ice: use ice_fill_eth_hdr() in ice_fill_sw_rule() Tony Nguyen
@ 2026-08-05 21:35 ` Tony Nguyen
2026-08-05 21:35 ` [PATCH net-next 09/15] ice: add rx timestamp tracepoint for debugging Tony Nguyen
` (7 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Tony Nguyen @ 2026-08-05 21:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Aleksandr Loktionov, anthony.l.nguyen, przemyslaw.kitszel,
richardcochran, Jacob Keller, Simon Horman, Alexander Nowlin
From: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
The miscellaneous interrupt cause (OICR) is throttled to 8K
interrupts per second (124 us minimum spacing). This interrupt
handles VF mailbox messages and Tx timestamps, so the low rate
imposes a minimum latency floor on both use-cases.
Raise the rate to 20K interrupts per second (50 us minimum
spacing) to allow lower latency handling for Tx timestamp
bursts and high VF message rates.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ice/ice_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index a5e9c3114706..fefbc51c7190 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3428,7 +3428,7 @@ static int ice_req_irq_msix_misc(struct ice_pf *pf)
((pf->ll_ts_irq.index + pf_intr_start_offset) &
PFINT_SB_CTL_MSIX_INDX_M) | PFINT_SB_CTL_CAUSE_ENA_M);
wr32(hw, GLINT_ITR(ICE_RX_ITR, pf->oicr_irq.index),
- ITR_REG_ALIGN(ICE_ITR_8K) >> ICE_ITR_GRAN_S);
+ ITR_REG_ALIGN(ICE_ITR_20K) >> ICE_ITR_GRAN_S);
ice_flush(hw);
ice_irq_dynamic_ena(hw, NULL, NULL);
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH net-next 09/15] ice: add rx timestamp tracepoint for debugging
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
` (7 preceding siblings ...)
2026-08-05 21:35 ` [PATCH net-next 08/15] ice: increase OICR interrupt moderation rate to 20K interrupts/sec Tony Nguyen
@ 2026-08-05 21:35 ` Tony Nguyen
2026-08-05 21:35 ` [PATCH net-next 10/15] i40e: prepare for XDP metadata ops support Tony Nguyen
` (6 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Tony Nguyen @ 2026-08-05 21:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Przemyslaw Korba, anthony.l.nguyen, przemyslaw.kitszel,
richardcochran, Aleksandr Loktionov, Rinitha S
From: Przemyslaw Korba <przemyslaw.korba@intel.com>
Add ice_rx_hwtstamp trace event to help debug PTP rx timestamp issues.
The trace captures cached_time, ts_high, and the extended ts_ns value in
ice_ptp_get_rx_hwts().
Enable with:
echo 1 > /sys/kernel/debug/tracing/events/ice/ice_rx_hwtstamp/enable
cat /sys/kernel/debug/tracing/trace_pipe
Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ice/ice_ptp.c | 2 ++
drivers/net/ethernet/intel/ice/ice_trace.h | 18 ++++++++++++++++++
2 files changed, 20 insertions(+)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index eaec36ab6ae3..e8392ec4f134 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -2327,6 +2327,8 @@ u64 ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
ts_high = le32_to_cpu(rx_desc->wb.flex_ts.ts_high);
ts_ns = ice_ptp_extend_32b_ts(cached_time, ts_high);
+ ice_trace(rx_hwtstamp, cached_time, ts_high, ts_ns);
+
return ts_ns;
}
diff --git a/drivers/net/ethernet/intel/ice/ice_trace.h b/drivers/net/ethernet/intel/ice/ice_trace.h
index 4f35ef8d6b29..3cb8c6ea03cf 100644
--- a/drivers/net/ethernet/intel/ice/ice_trace.h
+++ b/drivers/net/ethernet/intel/ice/ice_trace.h
@@ -241,6 +241,24 @@ DEFINE_TX_TSTAMP_OP_EVENT(ice_tx_tstamp_fw_req);
DEFINE_TX_TSTAMP_OP_EVENT(ice_tx_tstamp_fw_done);
DEFINE_TX_TSTAMP_OP_EVENT(ice_tx_tstamp_complete);
+/* Trace event for Rx timestamp */
+TRACE_EVENT(ice_rx_hwtstamp,
+ TP_PROTO(u64 cached_time, u32 ts_high, u64 ts_ns),
+
+ TP_ARGS(cached_time, ts_high, ts_ns),
+
+ TP_STRUCT__entry(__field(u64, cached_time)
+ __field(u32, ts_high)
+ __field(u64, ts_ns)),
+
+ TP_fast_assign(__entry->cached_time = cached_time;
+ __entry->ts_high = ts_high;
+ __entry->ts_ns = ts_ns;),
+
+ TP_printk("cached_time=%llu ts_high=%u ts_ns=%llu",
+ __entry->cached_time, __entry->ts_high, __entry->ts_ns)
+);
+
DECLARE_EVENT_CLASS(ice_esw_br_fdb_template,
TP_PROTO(struct ice_esw_br_fdb_entry *fdb),
TP_ARGS(fdb),
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH net-next 10/15] i40e: prepare for XDP metadata ops support
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
` (8 preceding siblings ...)
2026-08-05 21:35 ` [PATCH net-next 09/15] ice: add rx timestamp tracepoint for debugging Tony Nguyen
@ 2026-08-05 21:35 ` Tony Nguyen
2026-08-06 21:36 ` sashiko-bot
2026-08-11 0:35 ` Jakub Kicinski
2026-08-05 21:35 ` [PATCH net-next 11/15] i40e: add support for bpf_xdp_metadata_rx_hash() Tony Nguyen
` (5 subsequent siblings)
15 siblings, 2 replies; 27+ messages in thread
From: Tony Nguyen @ 2026-08-05 21:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Kohei Enju, anthony.l.nguyen, kohei.enju, przemyslaw.kitszel,
maciej.fijalkowski, magnus.karlsson, ast, daniel, hawk,
john.fastabend, sdf, bpf, Aleksandr Loktionov, Patryk Holda
From: Kohei Enju <kohei@enjuk.jp>
Prepare 'struct i40e_xdp_buff' that contains an xdp_buff and a pointer
to i40e_rx_desc in order to pass the RX descriptor to the XDP kfuncs.
Also in ZC path, use XSK_CHECK_PRIV_TYPE() to ensure i40e_xdp_buff
doesn't exceed the offset of cb in xdp_buff_xsk.
No functional changes.
Signed-off-by: Kohei Enju <kohei@enjuk.jp>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Patryk Holda <patryk.holda@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 2 +-
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 5 ++++-
drivers/net/ethernet/intel/i40e/i40e_txrx.h | 7 ++++++-
drivers/net/ethernet/intel/i40e/i40e_xsk.c | 12 ++++++++++++
4 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 0cd0e5597c90..4b11ae9ed8d5 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -3622,7 +3622,7 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring)
}
skip:
- xdp_init_buff(&ring->xdp, xdp_frame_sz, &ring->xdp_rxq);
+ xdp_init_buff(&ring->xdp_ctx.xdp, xdp_frame_sz, &ring->xdp_rxq);
rx_ctx.dbuff = DIV_ROUND_UP(ring->rx_buf_len,
BIT_ULL(I40E_RXQ_CTX_DBUFF_SHIFT));
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index ef5e657816f0..7f68adb5109b 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -2438,10 +2438,11 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget,
unsigned int *rx_cleaned)
{
unsigned int total_rx_bytes = 0, total_rx_packets = 0;
+ struct i40e_xdp_buff *xdp_ctx = &rx_ring->xdp_ctx;
u16 cleaned_count = I40E_DESC_UNUSED(rx_ring);
u16 clean_threshold = rx_ring->count / 2;
unsigned int offset = rx_ring->rx_offset;
- struct xdp_buff *xdp = &rx_ring->xdp;
+ struct xdp_buff *xdp = &xdp_ctx->xdp;
unsigned int xdp_xmit = 0;
struct bpf_prog *xdp_prog;
bool failure = false;
@@ -2530,6 +2531,8 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget,
if (neop)
continue;
+ xdp_ctx->desc = rx_desc;
+
xdp_res = i40e_run_xdp(rx_ring, xdp, xdp_prog);
if (xdp_res) {
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
index 1e5fd63d47f4..bb741ff3e5f2 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
@@ -283,6 +283,11 @@ struct i40e_rx_buffer {
__u32 page_count;
};
+struct i40e_xdp_buff {
+ struct xdp_buff xdp;
+ const union i40e_rx_desc *desc;
+};
+
struct i40e_queue_stats {
u64 packets;
u64 bytes;
@@ -345,7 +350,7 @@ struct i40e_ring {
* and to resume packet building for this ring in the next call to
* i40e_clean_rx_ring_irq().
*/
- struct xdp_buff xdp;
+ struct i40e_xdp_buff xdp_ctx;
/* Next descriptor to be processed; next_to_clean is updated only on
* processing EOP descriptor
diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 9f47388eaba5..51a05ce4c7ce 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -246,6 +246,8 @@ bool i40e_alloc_rx_buffers_zc(struct i40e_ring *rx_ring, u16 count)
u32 nb_buffs, i;
dma_addr_t dma;
+ XSK_CHECK_PRIV_TYPE(struct i40e_xdp_buff);
+
rx_desc = I40E_RX_DESC(rx_ring, ntu);
xdp = i40e_rx_bi(rx_ring, ntu);
@@ -396,6 +398,14 @@ static void i40e_handle_xdp_result_zc(struct i40e_ring *rx_ring,
WARN_ON_ONCE(1);
}
+static struct i40e_xdp_buff *xsk_buff_to_i40e_ctx(struct xdp_buff *xdp)
+{
+ /* xdp_buff pointer used by ZC code path is allocated as xdp_buff_xsk.
+ * i40e_xdp_buff private fields overlap with xdp_buff_xsk->cb.
+ */
+ return (struct i40e_xdp_buff *)xdp;
+}
+
/**
* i40e_clean_rx_irq_zc - Consumes Rx packets from the hardware ring
* @rx_ring: Rx ring
@@ -472,6 +482,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
if (i40e_is_non_eop(rx_ring, rx_desc))
continue;
+ xsk_buff_to_i40e_ctx(first)->desc = rx_desc;
+
xdp_res = i40e_run_xdp_zc(rx_ring, first, xdp_prog);
i40e_handle_xdp_result_zc(rx_ring, first, rx_desc, &rx_packets,
&rx_bytes, xdp_res, &failure);
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next 10/15] i40e: prepare for XDP metadata ops support
2026-08-05 21:35 ` [PATCH net-next 10/15] i40e: prepare for XDP metadata ops support Tony Nguyen
@ 2026-08-06 21:36 ` sashiko-bot
2026-08-07 19:00 ` Kohei Enju
2026-08-11 0:35 ` Jakub Kicinski
1 sibling, 1 reply; 27+ messages in thread
From: sashiko-bot @ 2026-08-06 21:36 UTC (permalink / raw)
To: Tony Nguyen; +Cc: bpf
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Use-After-Free in the AF_XDP Zero-Copy receive path due to stagnant index variable on error.
--
--- Patch [10]: [PATCH net-next 10/15] i40e: prepare for XDP metadata ops support ---
commit cf380b8d06da010c94723db1eb6af6df8fb0fdc8
Author: Kohei Enju <kohei@enjuk.jp>
i40e: prepare for XDP metadata ops support
This commit introduces struct i40e_xdp_buff to wrap an xdp_buff alongside
a pointer to an i40e_rx_desc. It passes the RX descriptor to XDP kfuncs
and adds bounds checking in the AF_XDP Zero-Copy path using
XSK_CHECK_PRIV_TYPE.
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> index 9f47388eaba53..51a05ce4c7ce8 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
[ ... ]
> @@ -472,6 +482,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
> if (i40e_is_non_eop(rx_ring, rx_desc))
> continue;
>
> + xsk_buff_to_i40e_ctx(first)->desc = rx_desc;
> +
[Severity: Critical]
This is a pre-existing issue, but does this function have a use-after-free
in its error handling path?
If xsk_buff_add_frag() fails earlier in the loop, it calls
xsk_buff_free(first) and breaks out of the loop without advancing
next_to_clean:
if (!first)
first = bi;
else if (!xsk_buff_add_frag(first, bi)) {
xsk_buff_free(first);
break;
}
On the subsequent NAPI poll, because next_to_process != next_to_clean,
the code loads the freed buffer pointer from the stale next_to_clean
index:
if (next_to_process != next_to_clean)
first = *i40e_rx_bi(rx_ring, next_to_clean);
Could this cause the next packet to be built using a freed buffer?
> xdp_res = i40e_run_xdp_zc(rx_ring, first, xdp_prog);
> i40e_handle_xdp_result_zc(rx_ring, first, rx_desc, &rx_packets,
> &rx_bytes, xdp_res, &failure);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805213541.2281287-1-anthony.l.nguyen@intel.com?part=10
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next 10/15] i40e: prepare for XDP metadata ops support
2026-08-06 21:36 ` sashiko-bot
@ 2026-08-07 19:00 ` Kohei Enju
0 siblings, 0 replies; 27+ messages in thread
From: Kohei Enju @ 2026-08-07 19:00 UTC (permalink / raw)
To: sashiko-reviews; +Cc: Tony Nguyen, bpf
On 08/06 21:36, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Use-After-Free in the AF_XDP Zero-Copy receive path due to stagnant index variable on error.
> --
>
> --- Patch [10]: [PATCH net-next 10/15] i40e: prepare for XDP metadata ops support ---
> commit cf380b8d06da010c94723db1eb6af6df8fb0fdc8
> Author: Kohei Enju <kohei@enjuk.jp>
>
> i40e: prepare for XDP metadata ops support
>
> This commit introduces struct i40e_xdp_buff to wrap an xdp_buff alongside
> a pointer to an i40e_rx_desc. It passes the RX descriptor to XDP kfuncs
> and adds bounds checking in the AF_XDP Zero-Copy path using
> XSK_CHECK_PRIV_TYPE.
>
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> > index 9f47388eaba53..51a05ce4c7ce8 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
>
> [ ... ]
>
> > @@ -472,6 +482,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
> > if (i40e_is_non_eop(rx_ring, rx_desc))
> > continue;
> >
> > + xsk_buff_to_i40e_ctx(first)->desc = rx_desc;
> > +
>
> [Severity: Critical]
> This is a pre-existing issue, but does this function have a use-after-free
> in its error handling path?
>
> If xsk_buff_add_frag() fails earlier in the loop, it calls
IIUC, I40E_MAX_CHAINED_RX_BUFFERS (5) limits a packet to one head and at
most four frags. Since xsk_buff_add_frag() fails only when nr_frags ==
MAX_SKB_FRAGS, which is at least 17, this path is unreachable for a
valid i40e RX chain.
However, the UAF scenario looks valid if it does fail, so the
pre-existing error may still be worth fixing.
> xsk_buff_free(first) and breaks out of the loop without advancing
> next_to_clean:
>
> if (!first)
> first = bi;
> else if (!xsk_buff_add_frag(first, bi)) {
> xsk_buff_free(first);
> break;
> }
>
> On the subsequent NAPI poll, because next_to_process != next_to_clean,
> the code loads the freed buffer pointer from the stale next_to_clean
> index:
>
> if (next_to_process != next_to_clean)
> first = *i40e_rx_bi(rx_ring, next_to_clean);
>
> Could this cause the next packet to be built using a freed buffer?
>
> > xdp_res = i40e_run_xdp_zc(rx_ring, first, xdp_prog);
> > i40e_handle_xdp_result_zc(rx_ring, first, rx_desc, &rx_packets,
> > &rx_bytes, xdp_res, &failure);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260805213541.2281287-1-anthony.l.nguyen@intel.com?part=10
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next 10/15] i40e: prepare for XDP metadata ops support
2026-08-05 21:35 ` [PATCH net-next 10/15] i40e: prepare for XDP metadata ops support Tony Nguyen
2026-08-06 21:36 ` sashiko-bot
@ 2026-08-11 0:35 ` Jakub Kicinski
1 sibling, 0 replies; 27+ messages in thread
From: Jakub Kicinski @ 2026-08-11 0:35 UTC (permalink / raw)
To: anthony.l.nguyen
Cc: Jakub Kicinski, davem, pabeni, edumazet, andrew+netdev, netdev,
tactii, dima.ruinskiy, aleksandr.loktionov, pmenzel,
michalx.cohen
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
i40e: prepare for XDP metadata ops support
This adds struct i40e_xdp_buff, which wraps the xdp_buff stored on the Rx
ring together with a pointer to the Rx descriptor, and sets that pointer
before running the XDP program in both the normal and the AF_XDP zero-copy
receive paths. The ZC path also gains XSK_CHECK_PRIV_TYPE() to verify the
new struct fits in the xdp_buff_xsk cb area.
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 0cd0e5597c909..4b11ae9ed8d53 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -3622,7 +3622,7 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring)
> }
>
> skip:
> - xdp_init_buff(&ring->xdp, xdp_frame_sz, &ring->xdp_rxq);
> + xdp_init_buff(&ring->xdp_ctx.xdp, xdp_frame_sz, &ring->xdp_rxq);
[Severity: High]
This isn't a bug introduced by this patch, but can the state persisted in
rx_ring->xdp_ctx outlive the pages it points at?
i40e_clean_rx_ring() unmaps and drains every Rx page and resets the
next_to_* indices, but leaves the saved buffer alone:
drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_clean_rx_ring() {
...
__page_frag_cache_drain(rx_bi->page, rx_bi->pagecnt_bias);
rx_bi->page = NULL;
rx_bi->page_offset = 0;
}
skip_free:
...
rx_ring->next_to_alloc = 0;
rx_ring->next_to_clean = 0;
rx_ring->next_to_process = 0;
rx_ring->next_to_use = 0;
}
On bring-up the only re-initialization is the xdp_init_buff() call above,
and that touches rxq, frame_sz and flags only:
include/net/xdp.h:xdp_init_buff() {
xdp->rxq = rxq;
...
xdp->frame_sz_flags_init = frame_sz;
...
}
So data, data_hard_start and the frag state survive an ifdown/ifup, MTU
change, ring resize or PF reset. If NAPI returned in the middle of a
multi-buffer packet (the state the comment above the member says is kept on
purpose), does the first iteration after the ring comes back up then take
the fragment-append branch on stale data?
drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_clean_rx_irq() {
...
if (!xdp->data) {
...
xdp_prepare_buff(xdp, hard_start, offset, size, true);
} else if (i40e_add_xdp_frag(xdp, &nfrags, rx_buffer, size) &&
!neop) {
...
}
i40e_add_xdp_frag() derives sinfo from data_hard_start and writes nr_frags,
xdp_frags_size and a frag entry, which would land in a page already handed
back by __page_frag_cache_drain(), and the following i40e_build_skb() /
i40e_process_rx_buffs() would then operate on and drop a reference to that
page.
ice does clear the stash in this situation: ice_clean_rx_ring() calls
libeth_xdp_return_stash(&rx_ring->xdp), described as freeing an incomplete
buffer stash when the queue is about to be destroyed. Would clearing
xdp_ctx (data = NULL and the new desc = NULL) in i40e_clean_rx_ring() be
appropriate here?
The new desc pointer inherits the same lifetime problem: it is a raw
pointer into the descriptor ring that i40e_free_rx_resources() releases via
dma_free_coherent(), and it is only assigned on the EOP path, never
cleared.
>
> rx_ctx.dbuff = DIV_ROUND_UP(ring->rx_buf_len,
> BIT_ULL(I40E_RXQ_CTX_DBUFF_SHIFT));
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index ef5e657816f00..7f68adb5109b0 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
[ ... ]
> @@ -2530,6 +2531,8 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget,
> if (neop)
> continue;
>
> + xdp_ctx->desc = rx_desc;
> +
> xdp_res = i40e_run_xdp(rx_ring, xdp, xdp_prog);
>
> if (xdp_res) {
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> index 1e5fd63d47f47..bb741ff3e5f2c 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> @@ -283,6 +283,11 @@ struct i40e_rx_buffer {
> __u32 page_count;
> };
>
> +struct i40e_xdp_buff {
> + struct xdp_buff xdp;
> + const union i40e_rx_desc *desc;
> +};
> +
> struct i40e_queue_stats {
> u64 packets;
> u64 bytes;
> @@ -345,7 +350,7 @@ struct i40e_ring {
> * and to resume packet building for this ring in the next call to
> * i40e_clean_rx_ring_irq().
> */
> - struct xdp_buff xdp;
> + struct i40e_xdp_buff xdp_ctx;
[Severity: Low]
This is a pre-existing issue, not something this patch introduced, but the
comment just above this member names i40e_clean_rx_ring_irq() twice and
that function does not exist; git grep for it matches only these two
comment lines. The function that saves and resumes this state is
i40e_clean_rx_irq(). Since the hunk already touches the member, would it
make sense to fix the name, and also the "Storing xdp_buff on ring" wording
now that the member is a struct i40e_xdp_buff?
While there, could the lifetime of the new desc field be described? It is
set only on the EOP path in i40e_clean_rx_irq() and is not initialized by
the xdp_init_buff() call in i40e_configure_rx_ring().
>
> /* Next descriptor to be processed; next_to_clean is updated only on
> * processing EOP descriptor
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> index 9f47388eaba53..51a05ce4c7ce8 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
[ ... ]
> @@ -472,6 +482,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
> if (i40e_is_non_eop(rx_ring, rx_desc))
> continue;
>
> + xsk_buff_to_i40e_ctx(first)->desc = rx_desc;
> +
> xdp_res = i40e_run_xdp_zc(rx_ring, first, xdp_prog);
> i40e_handle_xdp_result_zc(rx_ring, first, rx_desc, &rx_packets,
> &rx_bytes, xdp_res, &failure);
[Severity: High]
This isn't a bug introduced by this patch either, but can "first" here
already be a freed xsk buffer, so that this new store writes into the cb of
a buffer that is back on the pool free list?
Two places in i40e_clean_rx_irq_zc() free a buffer without advancing
next_to_clean and without clearing the rx_bi_zc slot. The programming
status branch:
if (i40e_rx_is_programming_status(qword)) {
...
bi = *i40e_rx_bi(rx_ring, next_to_process);
xsk_buff_free(bi);
if (++next_to_process == count)
next_to_process = 0;
continue;
}
and the multi-buffer error path:
if (!first)
first = bi;
else if (!xsk_buff_add_frag(first, bi)) {
xsk_buff_free(first);
break;
}
If the loop then exits at:
size = FIELD_GET(I40E_RXD_QW1_LENGTH_PBUF_MASK, qword);
if (!size)
break;
the function writes back rx_ring->next_to_clean unchanged while
next_to_process has moved on, and i40e_alloc_rx_buffers_zc() cannot refill
that slot because I40E_DESC_UNUSED stops at next_to_clean - 1. So does
rx_bi_zc[next_to_clean] keep pointing at a freed buffer that the next poll
picks up?
if (next_to_process != next_to_clean)
first = *i40e_rx_bi(rx_ring, next_to_clean);
That buffer is then passed to xsk_buff_set_size(), xsk_buff_add_frag() and
i40e_run_xdp_zc(), and with this patch also written through by
xsk_buff_to_i40e_ctx(first)->desc = rx_desc.
On teardown, does the same stale pointer get freed a second time?
i40e_xsk_clean_rx_ring() walks [next_to_clean, next_to_use):
while (ntc != ntu) {
struct xdp_buff *rx_bi = *i40e_rx_bi(rx_ring, ntc);
xsk_buff_free(rx_bi);
and it runs before i40e_clear_rx_bi_zc() zeroes the array.
ice_clean_rx_irq_zc() frees, clears first, and still advances ntc:
} else if (likely(size) && !xsk_buff_add_frag(first, xdp)) {
xsk_buff_free(first);
first = NULL;
}
if (++ntc == cnt)
ntc = 0;
Would the same handling be right for i40e?
The multi-buffer error leg looks hard to reach on this hardware, since
xsk_buff_add_frag() only fails at MAX_SKB_FRAGS while rx_ctx.rxmax is
programmed from I40E_MAX_CHAINED_RX_BUFFERS, but the programming status leg
needs no oversized frame.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next 11/15] i40e: add support for bpf_xdp_metadata_rx_hash()
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
` (9 preceding siblings ...)
2026-08-05 21:35 ` [PATCH net-next 10/15] i40e: prepare for XDP metadata ops support Tony Nguyen
@ 2026-08-05 21:35 ` Tony Nguyen
2026-08-05 21:35 ` [PATCH net-next 12/15] i40e: add support for bpf_xdp_metadata_rx_vlan_tag() Tony Nguyen
` (4 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Tony Nguyen @ 2026-08-05 21:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Kohei Enju, anthony.l.nguyen, kohei.enju, przemyslaw.kitszel,
maciej.fijalkowski, magnus.karlsson, ast, daniel, hawk,
john.fastabend, sdf, bpf, Aleksandr Loktionov, Patryk Holda
From: Kohei Enju <kohei@enjuk.jp>
Introduce i40e_xdp_rx_hash() which takes the same approach as
i40e_rx_hash() to extract the hash from RX descriptors.
Tested with X710 adapter using xdp_hw_metadata, and verified hash
consistency between bpf_xdp_metadata_rx_hash() and skb->hash.
Signed-off-by: Kohei Enju <kohei@enjuk.jp>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Patryk Holda <patryk.holda@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 30 +++++++++++++++++++++
drivers/net/ethernet/intel/i40e/i40e_type.h | 5 ++++
2 files changed, 35 insertions(+)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 4b11ae9ed8d5..e487d697a634 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -4,6 +4,7 @@
#include <generated/utsrelease.h>
#include <linux/crash_dump.h>
#include <linux/net/intel/libie/pctype.h>
+#include <linux/net/intel/libie/rx.h>
#include <linux/if_bridge.h>
#include <linux/if_macvlan.h>
#include <linux/module.h>
@@ -13582,6 +13583,34 @@ static int i40e_xdp(struct net_device *dev,
}
}
+static int i40e_xdp_rx_hash(const struct xdp_md *_ctx, u32 *hash,
+ enum xdp_rss_hash_type *rss_type)
+{
+ const struct i40e_xdp_buff *ctx = (const void *)_ctx;
+ const union i40e_rx_desc *desc = ctx->desc;
+ struct libeth_rx_pt rx_ptype;
+ u8 raw_rx_ptype;
+ u64 status;
+
+ status = le64_to_cpu(desc->wb.qword1.status_error_len);
+ raw_rx_ptype = FIELD_GET(I40E_RXD_QW1_PTYPE_MASK, status);
+ rx_ptype = libie_rx_pt_parse(raw_rx_ptype);
+
+ if (!libeth_rx_pt_has_hash(ctx->xdp.rxq->dev, rx_ptype) ||
+ FIELD_GET(I40E_RX_DESC_STATUS_FLTSTAT_MASK, status) !=
+ I40E_RX_DESC_FLTSTAT_RSS_HASH)
+ return -ENODATA;
+
+ *hash = le32_to_cpu(desc->wb.qword0.hi_dword.rss);
+ *rss_type = rx_ptype.hash_type;
+
+ return 0;
+}
+
+static const struct xdp_metadata_ops i40e_xdp_metadata_ops = {
+ .xmo_rx_hash = i40e_xdp_rx_hash,
+};
+
static const struct net_device_ops i40e_netdev_ops = {
.ndo_open = i40e_open,
.ndo_stop = i40e_close,
@@ -13784,6 +13813,7 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
i40e_vsi_config_netdev_tc(vsi, vsi->tc_config.enabled_tc);
netdev->netdev_ops = &i40e_netdev_ops;
+ netdev->xdp_metadata_ops = &i40e_xdp_metadata_ops;
netdev->watchdog_timeo = 5 * HZ;
i40e_set_ethtool_ops(netdev);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_type.h b/drivers/net/ethernet/intel/i40e/i40e_type.h
index ed8bbdb586da..16a65c6e5153 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_type.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_type.h
@@ -4,6 +4,7 @@
#ifndef _I40E_TYPE_H_
#define _I40E_TYPE_H_
+#include <linux/bits.h>
#include <uapi/linux/if_ether.h>
#include "i40e_adminq.h"
#include "i40e_hmc.h"
@@ -699,6 +700,10 @@ enum i40e_rx_desc_status_bits {
I40E_RX_DESC_STATUS_LAST /* this entry must be last!!! */
};
+#define I40E_RX_DESC_STATUS_FLTSTAT_MASK \
+ GENMASK_ULL(I40E_RX_DESC_STATUS_FLTSTAT_SHIFT + 1, \
+ I40E_RX_DESC_STATUS_FLTSTAT_SHIFT)
+
#define I40E_RXD_QW1_STATUS_SHIFT 0
#define I40E_RXD_QW1_STATUS_MASK ((BIT(I40E_RX_DESC_STATUS_LAST) - 1) \
<< I40E_RXD_QW1_STATUS_SHIFT)
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH net-next 12/15] i40e: add support for bpf_xdp_metadata_rx_vlan_tag()
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
` (10 preceding siblings ...)
2026-08-05 21:35 ` [PATCH net-next 11/15] i40e: add support for bpf_xdp_metadata_rx_hash() Tony Nguyen
@ 2026-08-05 21:35 ` Tony Nguyen
2026-08-05 21:35 ` [PATCH net-next 13/15] i40e: Avoid repeating RX filter warning Tony Nguyen
` (3 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Tony Nguyen @ 2026-08-05 21:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Kohei Enju, anthony.l.nguyen, kohei.enju, przemyslaw.kitszel,
maciej.fijalkowski, magnus.karlsson, ast, daniel, hawk,
john.fastabend, sdf, bpf, Aleksandr Loktionov, Patryk Holda
From: Kohei Enju <kohei@enjuk.jp>
Introduce i40e_xdp_rx_vlan_tag() which takes the same approach as
i40e_process_skb_fields() to extract the VLAN tag from the RX
descriptor.
Tested with X710 adapter using xdp_hw_metadata, and confirmed that VLAN
tags match between bpf_xdp_metadata_rx_vlan_tag() and
skb->vlan_proto/vlan_tci.
Signed-off-by: Kohei Enju <kohei@enjuk.jp>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Patryk Holda <patryk.holda@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index e487d697a634..719f45b5cb40 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -13607,8 +13607,27 @@ static int i40e_xdp_rx_hash(const struct xdp_md *_ctx, u32 *hash,
return 0;
}
+static int i40e_xdp_rx_vlan_tag(const struct xdp_md *_ctx, __be16 *vlan_proto,
+ u16 *vlan_tci)
+{
+ const struct i40e_xdp_buff *ctx = (const void *)_ctx;
+ const union i40e_rx_desc *desc = ctx->desc;
+ u64 status;
+
+ status = le64_to_cpu(desc->wb.qword1.status_error_len);
+
+ if (!(status & BIT(I40E_RX_DESC_STATUS_L2TAG1P_SHIFT)))
+ return -ENODATA;
+
+ *vlan_proto = cpu_to_be16(ETH_P_8021Q);
+ *vlan_tci = le16_to_cpu(desc->wb.qword0.lo_dword.l2tag1);
+
+ return 0;
+}
+
static const struct xdp_metadata_ops i40e_xdp_metadata_ops = {
.xmo_rx_hash = i40e_xdp_rx_hash,
+ .xmo_rx_vlan_tag = i40e_xdp_rx_vlan_tag,
};
static const struct net_device_ops i40e_netdev_ops = {
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH net-next 13/15] i40e: Avoid repeating RX filter warning
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
` (11 preceding siblings ...)
2026-08-05 21:35 ` [PATCH net-next 12/15] i40e: add support for bpf_xdp_metadata_rx_vlan_tag() Tony Nguyen
@ 2026-08-05 21:35 ` Tony Nguyen
2026-08-05 21:35 ` [PATCH net-next 14/15] igc: Support ACPI-based MAC pass-through Tony Nguyen
` (2 subsequent siblings)
15 siblings, 0 replies; 27+ messages in thread
From: Tony Nguyen @ 2026-08-05 21:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Chris Packham, anthony.l.nguyen, przemyslaw.kitszel, blair.steven,
carl.smith, Aleksandr Loktionov, Simon Horman, Rinitha S
From: Chris Packham <chris.packham@alliedtelesis.co.nz>
When the i40e runs out of space for RX filters the driver switches to
promiscuous mode and warns that it has done so. In scenarios with a
large number of these filters this can generate a lot of warnings. For
example:
$ dmesg -c > /dev/null
$ ip link add dev br0 type bridge vlan_filtering 1 vlan_default_pvid 1
$ ip link set dev eth7 master br0
$ bridge vlan add vid 1 dev eth7 pvid untagged self
$ bridge vlan add vid 2-4094 dev eth7 tagged
$ dmesg
[ 25.601705] i40e 0000:01:00.1: Error LIBIE_AQ_RC_ENOSPC, forcing overflow promiscuous on PF
[ 25.601833] i40e 0000:01:00.1: Error LIBIE_AQ_RC_ENOSPC, forcing overflow promiscuous on PF
[ 25.601961] i40e 0000:01:00.1: Error LIBIE_AQ_RC_ENOSPC, forcing overflow promiscuous on PF
[ 25.602088] i40e 0000:01:00.1: Error LIBIE_AQ_RC_ENOSPC, forcing overflow promiscuous on PF
[ 25.602216] i40e 0000:01:00.1: Error LIBIE_AQ_RC_ENOSPC, forcing overflow promiscuous on PF
[ 25.602344] i40e 0000:01:00.1: Error LIBIE_AQ_RC_ENOSPC, forcing overflow promiscuous on PF
...
Use test_and_set_bit() so that the warning is only issued when the
driver enables promiscuous mode and not on the addition of subsequent RX
filters.
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 719f45b5cb40..abbc71e815ae 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -2412,10 +2412,12 @@ void i40e_aqc_add_filters(struct i40e_vsi *vsi, const char *vsi_name,
if (fcnt != num_add) {
if (vsi->type == I40E_VSI_MAIN) {
- set_bit(__I40E_VSI_OVERFLOW_PROMISC, vsi->state);
- dev_warn(&vsi->back->pdev->dev,
- "Error %s adding RX filters on %s, promiscuous mode forced on\n",
- libie_aq_str(aq_status), vsi_name);
+ if (!test_and_set_bit(__I40E_VSI_OVERFLOW_PROMISC,
+ vsi->state)) {
+ dev_warn(&vsi->back->pdev->dev,
+ "Error %s adding RX filters on %s, promiscuous mode forced on\n",
+ libie_aq_str(aq_status), vsi_name);
+ }
} else if (vsi->type == I40E_VSI_SRIOV ||
vsi->type == I40E_VSI_VMDQ1 ||
vsi->type == I40E_VSI_VMDQ2) {
@@ -2465,10 +2467,11 @@ i40e_aqc_broadcast_filter(struct i40e_vsi *vsi, const char *vsi_name,
}
if (aq_ret) {
- set_bit(__I40E_VSI_OVERFLOW_PROMISC, vsi->state);
- dev_warn(&vsi->back->pdev->dev,
- "Error %s, forcing overflow promiscuous on %s\n",
- libie_aq_str(hw->aq.asq_last_status), vsi_name);
+ if (!test_and_set_bit(__I40E_VSI_OVERFLOW_PROMISC, vsi->state)) {
+ dev_warn(&vsi->back->pdev->dev,
+ "Error %s, forcing overflow promiscuous on %s\n",
+ libie_aq_str(hw->aq.asq_last_status), vsi_name);
+ }
}
return aq_ret;
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH net-next 14/15] igc: Support ACPI-based MAC pass-through
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
` (12 preceding siblings ...)
2026-08-05 21:35 ` [PATCH net-next 13/15] i40e: Avoid repeating RX filter warning Tony Nguyen
@ 2026-08-05 21:35 ` Tony Nguyen
2026-08-11 0:35 ` Jakub Kicinski
2026-08-05 21:35 ` [PATCH net-next 15/15] e1000e: Avoid DMA re-mapping on RX copybreak Tony Nguyen
2026-08-11 0:35 ` [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Jakub Kicinski
15 siblings, 1 reply; 27+ messages in thread
From: Tony Nguyen @ 2026-08-05 21:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Dima Ruinskiy, anthony.l.nguyen, allen.lan, Chia-Lin Kao (AceLan)
From: Dima Ruinskiy <dima.ruinskiy@intel.com>
Some systems implement a system MAC address object in the ACPI table,
using either \\_SB.AMAC or \\MACA object names. This system MAC address,
when enabled, is intended to override the permanent MAC address of the
network controller.
Implement lookup of the relevant ACPI object names and use them to
initialize the MAC address.
On systems where the feature is disabled or unsupported, the ACPI objects
do not exist or do not contain a valid Ethernet MAC, causing a fallback
to the existing MAC address initialization path.
Assisted-by: GitHub-Copilot:claude-opus-4.7
Signed-off-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
Tested-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/igc/igc_main.c | 64 ++++++++++++++++++++++-
1 file changed, 62 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 21d2fe38a41a..424c12c0394a 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -11,6 +11,8 @@
#include <net/pkt_sched.h>
#include <linux/bpf_trace.h>
#include <net/xdp_sock_drv.h>
+#include <linux/acpi.h>
+#include <linux/hex.h>
#include <linux/pci.h>
#include <linux/mdio.h>
@@ -7105,6 +7107,57 @@ static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer *timer)
return HRTIMER_NORESTART;
}
+static bool igc_get_acpi_mac_passthru(u8 *mac)
+{
+ static const struct {
+ const char *name;
+ acpi_object_type type;
+ u32 length;
+ } sources[] = {
+ { "\\_SB.AMAC", ACPI_TYPE_BUFFER, 23 },
+ { "\\MACA", ACPI_TYPE_STRING, 22 },
+ };
+ struct acpi_buffer buffer;
+ union acpi_object *obj;
+ bool mac_found = false;
+ acpi_status status;
+ u8 buf[ETH_ALEN];
+ int i;
+
+ if (!IS_ENABLED(CONFIG_ACPI))
+ return false;
+
+ for (i = 0; i < ARRAY_SIZE(sources) && !mac_found; i++) {
+ buffer.length = ACPI_ALLOCATE_BUFFER;
+ buffer.pointer = NULL;
+
+ status = acpi_evaluate_object(NULL, (char *)sources[i].name,
+ NULL, &buffer);
+ if (ACPI_FAILURE(status))
+ continue;
+
+ obj = buffer.pointer;
+ if (!obj || obj->type != sources[i].type ||
+ obj->string.length != sources[i].length)
+ goto free_obj;
+
+ if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) ||
+ obj->string.pointer[21] != '#')
+ goto free_obj;
+
+ if (hex2bin(buf, obj->string.pointer + 9, ETH_ALEN) ||
+ !is_valid_ether_addr(buf))
+ goto free_obj;
+
+ ether_addr_copy(mac, buf);
+ mac_found = true;
+free_obj:
+ kfree(obj);
+ }
+
+ return mac_found;
+}
+
/**
* igc_probe - Device Initialization Routine
* @pdev: PCI device information struct
@@ -7268,9 +7321,16 @@ static int igc_probe(struct pci_dev *pdev,
}
if (eth_platform_get_mac_address(&pdev->dev, hw->mac.addr)) {
- /* copy the MAC address out of the NVM */
- if (hw->mac.ops.read_mac_addr(hw))
+ /* Look for a system-provided MAC in the ACPI table before
+ * falling back to reading the address from the NVM.
+ */
+ if (igc_get_acpi_mac_passthru(hw->mac.addr)) {
+ netdev->addr_assign_type = NET_ADDR_STOLEN;
+ dev_info(&pdev->dev, "Using ACPI pass-through MAC addr %pM\n",
+ hw->mac.addr);
+ } else if (hw->mac.ops.read_mac_addr(hw)) {
dev_err(&pdev->dev, "NVM Read Error\n");
+ }
}
eth_hw_addr_set(netdev, hw->mac.addr);
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next 14/15] igc: Support ACPI-based MAC pass-through
2026-08-05 21:35 ` [PATCH net-next 14/15] igc: Support ACPI-based MAC pass-through Tony Nguyen
@ 2026-08-11 0:35 ` Jakub Kicinski
0 siblings, 0 replies; 27+ messages in thread
From: Jakub Kicinski @ 2026-08-11 0:35 UTC (permalink / raw)
To: anthony.l.nguyen
Cc: Jakub Kicinski, davem, pabeni, edumazet, andrew+netdev, netdev,
tactii, dima.ruinskiy, aleksandr.loktionov, pmenzel,
michalx.cohen
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
igc: Support ACPI-based MAC pass-through
This adds igc_get_acpi_mac_passthru(), which evaluates the ACPI objects
\_SB.AMAC and \MACA and parses the _AUXMAC_#AABBCCDDEEFF# payload into an
Ethernet address. igc_probe() now uses that address in preference to the
NVM address whenever eth_platform_get_mac_address() fails.
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 21d2fe38a41a6..424c12c0394a3 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
[ ... ]
> @@ -7105,6 +7107,57 @@ static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer *timer)
> return HRTIMER_NORESTART;
> }
>
> +static bool igc_get_acpi_mac_passthru(u8 *mac)
> +{
> + static const struct {
> + const char *name;
> + acpi_object_type type;
> + u32 length;
> + } sources[] = {
> + { "\\_SB.AMAC", ACPI_TYPE_BUFFER, 23 },
> + { "\\MACA", ACPI_TYPE_STRING, 22 },
> + };
> + struct acpi_buffer buffer;
> + union acpi_object *obj;
> + bool mac_found = false;
> + acpi_status status;
> + u8 buf[ETH_ALEN];
> + int i;
> +
> + if (!IS_ENABLED(CONFIG_ACPI))
> + return false;
> +
> + for (i = 0; i < ARRAY_SIZE(sources) && !mac_found; i++) {
> + buffer.length = ACPI_ALLOCATE_BUFFER;
> + buffer.pointer = NULL;
> +
> + status = acpi_evaluate_object(NULL, (char *)sources[i].name,
> + NULL, &buffer);
> + if (ACPI_FAILURE(status))
> + continue;
> +
> + obj = buffer.pointer;
> + if (!obj || obj->type != sources[i].type ||
> + obj->string.length != sources[i].length)
> + goto free_obj;
> +
> + if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) ||
> + obj->string.pointer[21] != '#')
> + goto free_obj;
> +
> + if (hex2bin(buf, obj->string.pointer + 9, ETH_ALEN) ||
> + !is_valid_ether_addr(buf))
> + goto free_obj;
> +
> + ether_addr_copy(mac, buf);
> + mac_found = true;
> +free_obj:
> + kfree(obj);
> + }
> +
> + return mac_found;
> +}
> +
> /**
> * igc_probe - Device Initialization Routine
> * @pdev: PCI device information struct
> @@ -7268,9 +7321,16 @@ static int igc_probe(struct pci_dev *pdev,
> }
>
> if (eth_platform_get_mac_address(&pdev->dev, hw->mac.addr)) {
> - /* copy the MAC address out of the NVM */
> - if (hw->mac.ops.read_mac_addr(hw))
> + /* Look for a system-provided MAC in the ACPI table before
> + * falling back to reading the address from the NVM.
> + */
> + if (igc_get_acpi_mac_passthru(hw->mac.addr)) {
> + netdev->addr_assign_type = NET_ADDR_STOLEN;
> + dev_info(&pdev->dev, "Using ACPI pass-through MAC addr %pM\n",
> + hw->mac.addr);
[Severity: High]
Can two igc ports end up with the same MAC address here?
igc_get_acpi_mac_passthru() takes only an output pointer and evaluates the
absolute paths \_SB.AMAC and \MACA with a NULL handle:
status = acpi_evaluate_object(NULL, (char *)sources[i].name,
NULL, &buffer);
There is no reference to ACPI_HANDLE(&pdev->dev), no PCI slot/function or
hw->device_id check, no NVM/EFUSE pass-through enable bit and no DMI match,
so every invocation returns the same six bytes. igc_probe() runs once per
igc PCI function, so on a dual/quad-port i225/i226 board, or with an onboard
igc plus an i226 add-in or Thunderbolt adapter, each probe takes the same
system address.
Is the address only a software label at that point, or does it reach the
hardware filter? From igc_probe() the path looks like:
igc_probe()
igc_reset()
hw->mac.ops.init_hw() /* igc_init_hw_base() */
igc_init_rx_addrs()
hw->mac.ops.rar_set(hw, hw->mac.addr, 0);
so the duplicated address is programmed into RAR0 as each controller's real
unicast receive filter (and magic-packet WoL filter). The existing check
if (!is_valid_ether_addr(netdev->dev_addr)) {
cannot detect this, since a duplicated address is still valid.
These same objects are the Dell/Lenovo dock pass-through ABI already consumed
by drivers/net/usb/r8152.c:vendor_mac_passthru_addr_read(), which uses the
identical names, types, lengths 0x16/0x17, '#' at offset 0x15 and hex2bin at
+9, but gates the lookup on device identity or capability first:
if (tp->lenovo_macpassthru) {
mac_obj_name = "\\MACA";
...
} else {
/* test for -AD variant of RTL8153 */
...
if ((ocp_data & PASS_THRU_MASK) != 1) {
... return -ENODEV;
On a machine with such a dock, would an igc port now claim the address that
firmware designated for the dock interface, colliding with it and changing
the identity used for 802.1X MAB, DHCP reservations and MAC ACLs? Should
this lookup be gated on a designated port, a capability bit, or a platform
match?
Relatedly, the commit message says the system MAC address "is intended to
override the permanent MAC address of the network controller", in the
singular. If applying it to every igc controller in the system is intended,
could the message say so and explain why the duplicate-address case is
acceptable?
One more thing on this path: read_mac_addr() is what normally populates
hw->mac.perm_addr, and it is now skipped in the ACPI case, so perm_addr is
left unset compared to the NVM path. Is that intentional?
> + } else if (hw->mac.ops.read_mac_addr(hw)) {
> dev_err(&pdev->dev, "NVM Read Error\n");
> + }
> }
>
> eth_hw_addr_set(netdev, hw->mac.addr);
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next 15/15] e1000e: Avoid DMA re-mapping on RX copybreak
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
` (13 preceding siblings ...)
2026-08-05 21:35 ` [PATCH net-next 14/15] igc: Support ACPI-based MAC pass-through Tony Nguyen
@ 2026-08-05 21:35 ` Tony Nguyen
2026-08-11 0:35 ` Jakub Kicinski
2026-08-11 0:35 ` [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Jakub Kicinski
15 siblings, 1 reply; 27+ messages in thread
From: Tony Nguyen @ 2026-08-05 21:35 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Matt Vollrath, anthony.l.nguyen, dima.ruinskiy,
Aleksandr Loktionov, Paul Menzel, Michal Cohen
From: Matt Vollrath <tactii@gmail.com>
This patch factors out DMA re-mapping for skbs which were recycled in
the RX path due to copybreak or errors. There is only one path out of
the e1000_clean_rx_irq() loop where the skb is consumed and DMA needs
to be re-mapped, so don't unmap it before checking the conditions.
The buffer allocation loop is adjusted to not assume that DMA is
unmapped, handling mapping errors gracefully.
On systems with IOMMU enabled, the cost of re-mapping DMA is greater
than the cost of copying data out of the ring buffer. When I use this
patch and configure e1000e with copybreak=2048, my system with IOMMU
completes RX roughly twice as fast under load.
Informal performance comparisons were based on Asus Gryphon Z97 which
includes an I218-V and with a Xeon E3-1240 v3 in the socket. ktime_get()
measurement was injected into e1000e_poll() wrapping the
adapter->clean_rx() call. The total time spent in clean_rx() was divided
by work_done to print the average time spent per buffer. iperf3 -R was
used to saturate the RX path and awk was used for statistics. Control
revision was set to 7.1-rc1 because iwl-next hadn't been updated yet.
rev | iommu | copybreak | samples | mean (ns) | stdev
7.1-rc1 | off | 0 | 4748 | 453.72 | 155.82
7.1-rc1 | off | 2048 | 4743 | 554.83 | 103.67
7.1-rc1 | on | 0 | 4751 | 1139.22 | 150.56
* 7.1-rc1 | on | 2048 | 4737 | 1267.02 | 184.62
+patch | off | 0 | 4739 | 456.30 | 146.33
+patch | off | 2048 | 4739 | 538.56 | 132.97
+patch | on | 0 | 4769 | 1165.97 | 140.19
* +patch | on | 2048 | 4745 | 562.25 | 171.80
No surprises here, IOMMU DMA ops are known to be expensive. For most
users the kernel default is iommu=on and driver default is
copybreak=256, so unless the workload is small packets, some tuning of
either knob would be needed to see the full benefit of this change.
The kludge of unconditional unmapping has existed since this driver was
introduced in 2007[1], inherited from the e1000 driver which has since
factored it out[2]. IOMMU tech was new at the time.
[1] Commit bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver (currently for ICH9 devices only)")
[2] Commit 2b294b18689c ("e1000: perform copybreak ahead of DMA unmap")
Assisted-by: Claude:claude-4-7-opus
Signed-off-by: Matt Vollrath <tactii@gmail.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Tested-by: Michal Cohen <michalx.cohen@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 32 ++++++++++++++--------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 844f31ab37ad..b08bcbdb19d9 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -675,6 +675,8 @@ static void e1000_alloc_rx_buffers(struct e1000_ring *rx_ring,
skb = buffer_info->skb;
if (skb) {
skb_trim(skb, 0);
+ if (likely(buffer_info->dma))
+ goto write_desc;
goto map_skb;
}
@@ -692,10 +694,12 @@ static void e1000_alloc_rx_buffers(struct e1000_ring *rx_ring,
DMA_FROM_DEVICE);
if (dma_mapping_error(&pdev->dev, buffer_info->dma)) {
dev_err(&pdev->dev, "Rx DMA map failed\n");
+ buffer_info->dma = 0;
adapter->rx_dma_failed++;
break;
}
+write_desc:
rx_desc = E1000_RX_DESC_EXT(*rx_ring, i);
rx_desc->read.buffer_addr = cpu_to_le64(buffer_info->dma);
@@ -953,7 +957,6 @@ static bool e1000_clean_rx_irq(struct e1000_ring *rx_ring, int *work_done,
dma_rmb(); /* read descriptor and rx_buffer_info after status DD */
skb = buffer_info->skb;
- buffer_info->skb = NULL;
prefetch(skb->data - NET_IP_ALIGN);
@@ -967,9 +970,6 @@ static bool e1000_clean_rx_irq(struct e1000_ring *rx_ring, int *work_done,
cleaned = true;
cleaned_count++;
- dma_unmap_single(&pdev->dev, buffer_info->dma,
- adapter->rx_buffer_len, DMA_FROM_DEVICE);
- buffer_info->dma = 0;
length = le16_to_cpu(rx_desc->wb.upper.length);
@@ -985,8 +985,6 @@ static bool e1000_clean_rx_irq(struct e1000_ring *rx_ring, int *work_done,
if (adapter->flags2 & FLAG2_IS_DISCARDING) {
/* All receives must fit into a single buffer */
e_dbg("Receive packet consumed multiple buffers\n");
- /* recycle */
- buffer_info->skb = skb;
if (staterr & E1000_RXD_STAT_EOP)
adapter->flags2 &= ~FLAG2_IS_DISCARDING;
goto next_desc;
@@ -994,8 +992,6 @@ static bool e1000_clean_rx_irq(struct e1000_ring *rx_ring, int *work_done,
if (unlikely((staterr & E1000_RXDEXT_ERR_FRAME_ERR_MASK) &&
!(netdev->features & NETIF_F_RXALL))) {
- /* recycle */
- buffer_info->skb = skb;
goto next_desc;
}
@@ -1022,19 +1018,33 @@ static bool e1000_clean_rx_irq(struct e1000_ring *rx_ring, int *work_done,
struct sk_buff *new_skb =
napi_alloc_skb(&adapter->napi, length);
if (new_skb) {
+ dma_sync_single_for_cpu(&pdev->dev,
+ buffer_info->dma,
+ adapter->rx_buffer_len,
+ DMA_FROM_DEVICE);
skb_copy_to_linear_data_offset(new_skb,
-NET_IP_ALIGN,
(skb->data -
NET_IP_ALIGN),
(length +
NET_IP_ALIGN));
- /* save the skb in buffer_info as good */
- buffer_info->skb = skb;
+ dma_sync_single_for_device(&pdev->dev,
+ buffer_info->dma,
+ adapter->rx_buffer_len,
+ DMA_FROM_DEVICE);
skb = new_skb;
+ goto copybreak_done;
}
/* else just continue with the old one */
}
- /* end copybreak code */
+
+ buffer_info->skb = NULL;
+ dma_unmap_single(&pdev->dev, buffer_info->dma,
+ adapter->rx_buffer_len,
+ DMA_FROM_DEVICE);
+ buffer_info->dma = 0;
+
+copybreak_done:
skb_put(skb, length);
/* Receive Checksum Offload */
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next 15/15] e1000e: Avoid DMA re-mapping on RX copybreak
2026-08-05 21:35 ` [PATCH net-next 15/15] e1000e: Avoid DMA re-mapping on RX copybreak Tony Nguyen
@ 2026-08-11 0:35 ` Jakub Kicinski
0 siblings, 0 replies; 27+ messages in thread
From: Jakub Kicinski @ 2026-08-11 0:35 UTC (permalink / raw)
To: anthony.l.nguyen
Cc: Jakub Kicinski, davem, pabeni, edumazet, andrew+netdev, netdev,
tactii, dima.ruinskiy, aleksandr.loktionov, pmenzel,
michalx.cohen
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
e1000e: Avoid DMA re-mapping on RX copybreak
The patch stops unmapping the RX buffer before the copybreak and error
checks, so recycled skbs keep their DMA mapping and only the consumed path
unmaps. The refill loop is adjusted to skip mapping when buffer_info->dma is
already set, and the copybreak path now uses dma_sync_single_for_cpu() /
dma_sync_single_for_device() around the copy.
No Link: tags are present in the commit header.
> e1000e: Avoid DMA re-mapping on RX copybreak
>
> The buffer allocation loop is adjusted to not assume that DMA is
> unmapped, handling mapping errors gracefully.
[Severity: Low]
Is "handling mapping errors gracefully" the whole story for the new
buffer_info->dma = 0 assignment in e1000_alloc_rx_buffers()?
Before this patch, a failed dma_map_single() there left DMA_MAPPING_ERROR
(~(dma_addr_t)0) in buffer_info->dma and broke out of the loop.
e1000_clean_rx_ring() gates its unmap on a non-zero value:
if (buffer_info->dma) {
if (adapter->clean_rx == e1000_clean_rx_irq)
dma_unmap_single(&pdev->dev, buffer_info->dma,
adapter->rx_buffer_len,
DMA_FROM_DEVICE);
so a down or reset before the next refill unmapped an address that was never
mapped. The new assignment closes that, which makes this hunk a bug fix on
top of the performance work.
Would it make sense to split that one line out with a Fixes: bc7f75fa9788
tag (and possibly Cc: stable), so stable triage can see it separately from
the rest of the change?
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 844f31ab37ad4..b08bcbdb19d97 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -675,6 +675,8 @@ static void e1000_alloc_rx_buffers(struct e1000_ring *rx_ring,
> skb = buffer_info->skb;
> if (skb) {
> skb_trim(skb, 0);
> + if (likely(buffer_info->dma))
> + goto write_desc;
> goto map_skb;
> }
>
> @@ -692,10 +694,12 @@ static void e1000_alloc_rx_buffers(struct e1000_ring *rx_ring,
> DMA_FROM_DEVICE);
> if (dma_mapping_error(&pdev->dev, buffer_info->dma)) {
> dev_err(&pdev->dev, "Rx DMA map failed\n");
> + buffer_info->dma = 0;
> adapter->rx_dma_failed++;
> break;
> }
>
> +write_desc:
> rx_desc = E1000_RX_DESC_EXT(*rx_ring, i);
> rx_desc->read.buffer_addr = cpu_to_le64(buffer_info->dma);
>
[Severity: High]
This isn't a bug introduced by this patch, but should the two sibling
refill routines in the same file be brought in line with the
"buffer_info->dma == 0 means not mapped" contract this hunk makes explicit?
e1000_alloc_rx_buffers_ps() still leaves the error cookie behind:
if (dma_mapping_error(&pdev->dev, buffer_info->dma)) {
dev_err(&pdev->dev, "Rx DMA map failed\n");
adapter->rx_dma_failed++;
/* cleanup skb */
dev_kfree_skb_any(skb);
buffer_info->skb = NULL;
break;
}
buffer_info->skb is cleared but buffer_info->dma keeps DMA_MAPPING_ERROR,
which is non-zero, so e1000_clean_rx_ring() on a subsequent down or reset
calls dma_unmap_single() on an address that was never mapped.
e1000_alloc_jumbo_rx_buffers() already uses the same map-skip idiom this
patch adopts, and also skips the zeroing:
if (!buffer_info->dma) {
buffer_info->dma = dma_map_page(&pdev->dev,
buffer_info->page, 0,
PAGE_SIZE,
DMA_FROM_DEVICE);
if (dma_mapping_error(&pdev->dev, buffer_info->dma)) {
adapter->alloc_rx_buff_failed++;
break;
}
}
After that break, rx_ring->next_to_use points at the failed entry, so the
next refill finds buffer_info->skb and buffer_info->page set, evaluates
!buffer_info->dma as false, skips the mapping and programs the descriptor:
rx_desc = E1000_RX_DESC_EXT(*rx_ring, i);
rx_desc->read.buffer_addr = cpu_to_le64(buffer_info->dma);
Can that arm a hardware RX descriptor with the DMA_MAPPING_ERROR cookie as
the bus address after the tail write, and is the same cookie later passed to
dma_unmap_page() from e1000_clean_rx_ring()?
> @@ -967,9 +970,6 @@ static bool e1000_clean_rx_irq(struct e1000_ring *rx_ring, int *work_done,
>
> cleaned = true;
> cleaned_count++;
> - dma_unmap_single(&pdev->dev, buffer_info->dma,
> - adapter->rx_buffer_len, DMA_FROM_DEVICE);
> - buffer_info->dma = 0;
>
> length = le16_to_cpu(rx_desc->wb.upper.length);
>
[ ... ]
> @@ -994,8 +992,6 @@ static bool e1000_clean_rx_irq(struct e1000_ring *rx_ring, int *work_done,
>
> if (unlikely((staterr & E1000_RXDEXT_ERR_FRAME_ERR_MASK) &&
> !(netdev->features & NETIF_F_RXALL))) {
> - /* recycle */
> - buffer_info->skb = skb;
> goto next_desc;
> }
>
> @@ -1022,19 +1018,33 @@ static bool e1000_clean_rx_irq(struct e1000_ring *rx_ring, int *work_done,
> struct sk_buff *new_skb =
> napi_alloc_skb(&adapter->napi, length);
> if (new_skb) {
> + dma_sync_single_for_cpu(&pdev->dev,
> + buffer_info->dma,
> + adapter->rx_buffer_len,
> + DMA_FROM_DEVICE);
> skb_copy_to_linear_data_offset(new_skb,
> -NET_IP_ALIGN,
> (skb->data -
> NET_IP_ALIGN),
> (length +
> NET_IP_ALIGN));
[Severity: High]
This is a pre-existing issue, but since this hunk rewrites the block, is
this a good place to bound length against adapter->rx_buffer_len?
length comes straight from the descriptor written by the NIC:
length = le16_to_cpu(rx_desc->wb.upper.length);
The new sync window is adapter->rx_buffer_len, while the copy source is the
ring skb and the copy size is length + NET_IP_ALIGN. Can this read past the
end of the RX buffer allocation and hand those bytes to the stack via
e1000_receive_skb() when length exceeds adapter->rx_buffer_len?
The precondition seems to be documented in the driver itself.
e1000_change_mtu() shrinks the allocation to 1522 bytes:
/* adjust allocation if LPE protects us, and we aren't using SBP */
if (max_frame <= (VLAN_ETH_FRAME_LEN + ETH_FCS_LEN))
adapter->rx_buffer_len = VLAN_ETH_FRAME_LEN + ETH_FCS_LEN;
but e1000_setup_rctl() programs the hardware size from the default case:
switch (adapter->rx_buffer_len) {
case 2048:
default:
rctl |= E1000_RCTL_SZ_2048;
rctl &= ~E1000_RCTL_BSEX;
break;
and sets E1000_RCTL_SBP when NETIF_F_RXALL is enabled, which is user
settable through hw_features. With rx-all on, the frame error drop above is
also bypassed, so an oversize single-descriptor frame reaches this copy.
Note the copybreak=2048 setting recommended in the commit message puts a
1523..2048 byte length inside the length < copybreak window.
The sibling e1000 driver took the corresponding fix in commit 9c72a5182ed9
("e1000: fix OOB in e1000_tbi_should_accept()"), which added:
if (unlikely(length > adapter->rx_buffer_len))
return false;
> + dma_sync_single_for_device(&pdev->dev,
> + buffer_info->dma,
> + adapter->rx_buffer_len,
> + DMA_FROM_DEVICE);
> - /* save the skb in buffer_info as good */
> - buffer_info->skb = skb;
> skb = new_skb;
> + goto copybreak_done;
> }
> /* else just continue with the old one */
> }
> - /* end copybreak code */
> +
> + buffer_info->skb = NULL;
> + dma_unmap_single(&pdev->dev, buffer_info->dma,
> + adapter->rx_buffer_len,
> + DMA_FROM_DEVICE);
> + buffer_info->dma = 0;
> +
> +copybreak_done:
> skb_put(skb, length);
[Severity: High]
Same unvalidated length on the fall-through path: the ring skb was allocated
with bufsz = adapter->rx_buffer_len, so can skb_put(skb, length) push
skb->tail past skb->end and reach skb_over_panic() for the oversize case
described above?
[ ... ]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e)
2026-08-05 21:35 [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e) Tony Nguyen
` (14 preceding siblings ...)
2026-08-05 21:35 ` [PATCH net-next 15/15] e1000e: Avoid DMA re-mapping on RX copybreak Tony Nguyen
@ 2026-08-11 0:35 ` Jakub Kicinski
15 siblings, 0 replies; 27+ messages in thread
From: Jakub Kicinski @ 2026-08-11 0:35 UTC (permalink / raw)
To: Tony Nguyen; +Cc: davem, pabeni, edumazet, andrew+netdev, netdev
On Wed, 5 Aug 2026 14:35:25 -0700 Tony Nguyen wrote:
> Subject: [PATCH net-next 00/15][pull request] Intel Wired LAN Driver Updates 2026-08-05 (ice, i40e, igc, e1000e)
I only looked at the High severity sashiko reports, they look legit.
Please consider splitting your PRs by driver, the i40e patches are
likely okay. You can have multiple PRs pending for net-next, as long
as sum of all their patches isn't >15.
^ permalink raw reply [flat|nested] 27+ messages in thread